#1454 Support for lifecycle methods on type being built with builders #3267

thunderhook · 2023-05-08T19:21:23Z

Hi,

this is my first PR for mapstruct. Disclaimer: Because of the IntelliJ Community Edition I do not have any highlighting or formatting for .ftl files, so bear with me. 😅

I tried to be as consistent as possible with the naming of the lifecycle method reference collections (beforeMappingReferencesWithFinalizedReturnType & afterMappingReferencesWithFinalizedReturnType) but if you have better suggestions, please tell me.

There is one edge-case i stumbled accross. With supporting @AfterMapping on the targets builder and finalized object, the following combination can be done by using methods both returning the result:

    // snippet of processor/src/test/java/org/mapstruct/ap/test/builder/lifecycle/MappingContext.java
    @AfterMapping
    public Order afterWithBuilderTargetReturningTarget(@MappingTarget Order.Builder orderBuilder) {
        invokedMethods.add( "afterWithBuilderTargetReturningTarget" );

        return orderBuilder.create();
    }

    @AfterMapping
    public Order afterWithTargetReturningTarget(@MappingTarget Order order) {
        invokedMethods.add( "afterWithTargetReturningTarget" );

        return order;
    }

Which results in the following mapper:

public class OrderMapperImpl implements OrderMapper {

    @Override
    public Order map(OrderDto source, MappingContext context) {

        // ... removed for brevity ...

        context.afterWithBuilderTarget( source, order );
        Order target = context.afterWithBuilderTargetReturningTarget( order );
        if ( target != null ) {
            return target; // in this case it will return here
        }

        // this code will not be reached
        Order orderResult = order.create();

        context.afterWithTargetType( source, Order.class );
        context.afterWithTarget( source, orderResult );
        Order target1 = context.afterWithTargetReturningTarget( orderResult );
        if ( target1 != null ) {
            return target1;
        }

        return orderResult;
    }

This makes sense but might be confusing. I was wondering if MapStruct should throw an error in that case?
For me, I had to adapt the test, that context#afterWithBuilderTargetReturningTarget returns null so that he other @AfterMappings get called.

Fixes #1454

documentation still has to be adapted here

filiphr

This looks really good @thunderhook. I've left some comments in line. Regarding your remarks:

There is one edge-case i stumbled accross. With supporting @AfterMapping on the targets builder and finalized object, the following combination can be done by using methods both returning the result:

I think that it is fine to have it as is now. We have clearly documented how things work when something is returned from a lifecycle method, so it is completely valid to use it like that if someone wants to do it. Keep in mind that afterWithBuilderTargetReturningTarget in our example always returns an instance, but in an actual implementation you might return an instance based on certain logic in your own method. Adapting the test for us is fine.

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

thunderhook · 2023-05-20T21:08:11Z

Checkstyle failed because of the 2000 line limit in BeanMappingMethod . Therefore I joined some lines. If you have another idea what could be extracted, please let me know (I thought of the inner Builder class but it needs too many internals (private fields/classes) of this class)

filiphr

Checkstyle failed because of the 2000 line limit in BeanMappingMethod

merging some the lines looks OK. Perhaps we need to refactor some things there (or increase this limit of 2000).

I don't like the wrappings in forEach and Optional.ofNullable though. It creates extra classes. Let's increase the line limit (I am also adding some new things to that class now for something else, so it would fail anyways)

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

thunderhook · 2023-05-21T14:55:58Z

Checkstyle failed because of the 2000 line limit in BeanMappingMethod

merging some the lines looks OK. Perhaps we need to refactor some things there (or increase this limit of 2000).

I don't like the wrappings in forEach and Optional.ofNullable though. It creates extra classes. Let's increase the line limit (I am also adding some new things to that class now for something else, so it would fail anyways)

Yeah, you're right. I will revert the changes to make life easier when you rebase

thunderhook · 2023-05-21T17:06:27Z

I increased the FileLength limit to 2500 to get the pipeline green. Feel free to adapt it as you wish afterwards.

filiphr

Looks really good @thunderhook.

Two last things that I just remembered about. One I commented inline and the other on is the documetation. In the Mapping customization with before-mapping and after-mapping methods we have a not mentioning the fact that @AfterMapping / @BeforeMapping methods are not being invoked.

filiphr · 2023-05-21T17:14:32Z

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

        }

+        private void removeMappingReferencesWithoutSourceParameters(List<LifecycleCallbackMethodReference> references) {
+            references.removeIf( r -> r.getSourceParameters().isEmpty() && r.getReturnType().isVoid() );


Let's add a comment why we are doing this. The reason why this is done is because lifecycle methods without any parameters are going to be already invoked. Mostly for future us, when we scratch our head why this is being done 😄

Thank you for explaining why I did this. I already already forgot the reason 2 weeks after implementing it... 😆 I added a comment.

… builders

…ng with builders

thunderhook · 2023-05-21T19:17:52Z

... the other on is the documetation. In the Mapping customization with before-mapping and after-mapping methods we have a not mentioning the fact that @AfterMapping / @BeforeMapping methods are not being invoked.

I'm not sure if I understood you correctly. I tried to tackle the documentation part and replaced the Important: with a NOTE-block going into detail with builders. Have a look at the MR.

Documenting and formulating is definitely not my forte, I tried my best. 😅

filiphr

The docs are perfect. Thanks a lot. I'll merge it once the builds are done

filiphr reviewed May 20, 2023

View reviewed changes

thunderhook force-pushed the 1454 branch from 693e1ab to d314856 Compare May 20, 2023 21:04

filiphr requested changes May 21, 2023

View reviewed changes

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java Outdated Show resolved Hide resolved

thunderhook force-pushed the 1454 branch from d314856 to 182644a Compare May 21, 2023 17:04

filiphr reviewed May 21, 2023

View reviewed changes

thunderhook added 2 commits May 21, 2023 21:12

mapstruct#1454 Support for lifecycle methods on type being built with…

79791b2

… builders

mapstruct#1454 Improve documentation about @BeforeMapping/@AfterMappi…

e9e7825

…ng with builders

thunderhook force-pushed the 1454 branch from 182644a to e9e7825 Compare May 21, 2023 19:13

filiphr approved these changes May 21, 2023

View reviewed changes

filiphr merged commit 6d205e5 into mapstruct:main May 21, 2023

This was referenced May 27, 2023

@AfterMapping to render mapping calls after the build() methods for result type value. #3287

Closed

@AfterMapping method with @MappingTarget original not correctly identified when use Lombok @Builder #3176

Closed

thunderhook mentioned this pull request Aug 25, 2024

#3678 Prevent duplicate @BeforeMapping and @AfterMapping calls on classes that use the Builder pattern. #3680

Closed

thunderhook mentioned this pull request Jan 24, 2025

Not all @AfterMapping methods executed when using record #3805

Closed

Uh oh!

#1454 Support for lifecycle methods on type being built with builders #3267

#1454 Support for lifecycle methods on type being built with builders #3267

Uh oh!

Conversation

thunderhook commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #1454

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thunderhook commented May 20, 2023

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thunderhook commented May 21, 2023

Uh oh!

thunderhook commented May 21, 2023

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

filiphr May 21, 2023

Choose a reason for hiding this comment

Uh oh!

thunderhook May 21, 2023

Choose a reason for hiding this comment

Uh oh!

thunderhook commented May 21, 2023

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thunderhook commented May 8, 2023 •

edited

Loading