#3678 Prevent duplicate @BeforeMapping and @AfterMapping calls on classes that use the Builder pattern. by thunderhook · Pull Request #3680 · mapstruct/mapstruct

thunderhook · 2024-08-21T21:48:30Z

…sses that use the Builder pattern.

thunderhook · 2024-08-21T21:55:39Z

...sor/src/test/java/org/mapstruct/ap/test/builder/lifecycle/BuilderLifecycleCallbacksTest.java

@@ -43,13 +48,15 @@ public void lifecycleMethodsShouldBeInvoked() {
        assertThat( context.getInvokedMethods() )
            .contains(


@filiphr
At first I tried to validate the fix by using containsExactly(...). But it soon became too complex and hard to understand due to the nested nature of this test.

So I added an additional fixture test that covers this.

Do you think this should be in a separate Issue3678Mapper or is it ok to test it here?

I removed the fixture, added the before/afterMapping only with source for the sake of completeness here.

But i provided a separate test that explicitly checks if before and after mapping methods are not called more than once.

If we have a dedicated test do we even need changes in this particular test here?

Not necessariliy. It was however easier to debug the internals of the method resolution due to multiple lifecycle callback methods. I will remove it after implementing the fix.

filiphr

I'm not sure the fix is correct.

How will this behave for something like:

@Mapper
public interface CustomMapper {

    Target map(SourceA sourceA, SourceB sourceB);

    @BeforeMapping
    default void beforeWithA(SourceA sourceA) {
        // ...
    }


    @BeforeMapping
    default void beforeWithB(SourceB sourceB) {
        // ...
    }

}

filiphr · 2024-08-24T09:38:42Z

processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java

+import org.mapstruct.Mapper;
+import org.mapstruct.ReportingPolicy;
+
+@Mapper(unmappedSourcePolicy = ReportingPolicy.ERROR)


This is not needed for our tests as it'll anyways fails if there is a compiler warning

filiphr · 2024-08-24T09:41:25Z

...sor/src/test/java/org/mapstruct/ap/test/builder/lifecycle/BuilderLifecycleCallbacksTest.java

@@ -43,13 +48,15 @@ public void lifecycleMethodsShouldBeInvoked() {
        assertThat( context.getInvokedMethods() )
            .contains(


If we have a dedicated test do we even need changes in this particular test here?

filiphr · 2024-08-24T09:41:55Z

processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Test.java

+            .containsExactly( "beforeMapping", "afterMapping" )
+            .doesNotHaveDuplicates();


doesNotHaveDuplicates is obsolete because we are using containsExactly.

Yes, I just wanted to express the intent of this test more clearly. We can of course remove this.

I think containsExactly expresses the intent clearly. It says I'm expecting exactly this and nothing more. If there are duplicates it would clearly fail

filiphr · 2024-08-24T09:47:10Z

Looking at the issue I feel like the fix should be happening on a different level, perhaps when getting the lifecycle methods. Most likely the selection criteria needs to be changed

thunderhook · 2024-08-24T10:08:22Z

I'm not sure the fix is correct.

How will this behave for something like:

@Mapper
public interface CustomMapper {

    Target map(SourceA sourceA, SourceB sourceB);

    @BeforeMapping
    default void beforeWithA(SourceA sourceA) {
        // ...
    }


    @BeforeMapping
    default void beforeWithB(SourceB sourceB) {
        // ...
    }

}

Good catch. I will have a look at that again.

thunderhook · 2024-08-25T18:50:05Z

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

-        if ( !super.equals( obj ) ) {
-            return false;
-        }


This evaluated against Object#equals and therefore was always false. This might be a relic from legacy inheritance structures or never worked as intended.

thunderhook · 2024-08-25T18:51:15Z

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

+        if ( !Objects.equals( name, other.name ) ) {
+            return false;
+        }


Not 100% sure if the name (and already been there: declaringMapper + providingParameter) is enough to differ between multiple method references. All unit tests are green.

thunderhook · 2024-08-25T18:55:48Z

So, i tried to adapt the method retrieval process. It became confusing very quickly and hard to differ between callback lifecycle methods that belong to the builder object and the finalized object.

The initial idea to remove the already invoked methods came from here:

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

So, I had an idea how to improve this. Why not remove the methods via removeAll(...)? I had to generate equals/hashCode but now this looks more clean. There was a bug in the equals method anyway, I've fixed that (see the other comment)

@filiphr WDYT? Is this good enough?

Problems that arised during the attempt rewriting the selectors and criteria:

Should there be a different SelectionCriteria.Type.LIFECYCLE_CALLBACK_WHAT_SO_EVER?
The chaining from BeanMappingMethod > LifecycleMethodResolver > SelectionContext was somewhat cumbersome
Should it be a blacklist like collect everything except the methods from Set<...>
- If so, what model should be used for that blacklist? MethodReference or SelectedMethod?

thunderhook · 2024-08-25T18:57:32Z

processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java

+
+    @Mapping( target = "name", source = "sourceA.name")
+    @Mapping( target = "description", source = "sourceB.description")
+    abstract Target mapTwoSources(SourceA sourceA, SourceB sourceB);


Added a specific test with two sources to be sure.

filiphr · 2024-08-25T19:29:21Z

Thanks @thunderhook. I'll need some time to think about this. I'm not too fond of the removals. I would prefer if we can select the right types when we need them. The SelectionContext already has mappingTargetType and returnType, so if we can somehow support multiple target types here we would be able to collect all in one go

thunderhook · 2024-09-01T08:02:48Z

Thanks @thunderhook. I'll need some time to think about this. I'm not too fond of the removals. I would prefer if we can select the right types when we need them. The SelectionContext already has mappingTargetType and returnType, so if we can somehow support multiple target types here we would be able to collect all in one go

Don't spend time on this yet, I think I get what you mean. I'll revisit this and hopefully come up with the solution you suggested. If there are any problems we can further discuss this here. 👍

thunderhook · 2024-09-03T20:13:38Z

@filiphr Short heads up: Unfortunately, I won't be able to do this any time soon. What do you think about merging the fix with this variant and creating a refactoring issue? Or is it too dangerous for you that the PR could have side effects?

filiphr · 2024-09-13T23:04:42Z

That's fine @thunderhook. I'll have a go at it. I'm not sure what kind of side effects this might have and I would rather spend the time on finding the right solution then rush into something

filiphr · 2024-09-15T08:32:14Z

@thunderhook I found a different solution for this. Have a look at #3716. I'll close this PR in favour of that one

#3678 Prevent duplicate @BeforeMapping and @AfterMapping calls on cla…

b74bde5

…sses that use the Builder pattern.

thunderhook mentioned this pull request Aug 21, 2024

@AfterMapping methods are called twice when target uses Lombok @Builder annotation #3678

Closed

thunderhook commented Aug 21, 2024

View reviewed changes

thunderhook added 2 commits August 22, 2024 22:10

#3678 write a separate test

b5b94bd

#3678 add before mapping to test

af7b02c

filiphr requested changes Aug 24, 2024

View reviewed changes

#3678 improve removal of duplicate lifecycle callback methods

3b5b028

thunderhook commented Aug 25, 2024

View reviewed changes

thunderhook mentioned this pull request Sep 10, 2024

Compile error when using AfterMapping method with Builder and TargetObject #3703

Closed

filiphr closed this Sep 15, 2024

		@@ -43,13 +48,15 @@ public void lifecycleMethodsShouldBeInvoked() {
		assertThat( context.getInvokedMethods() )
		.contains(

		.containsExactly( "beforeMapping", "afterMapping" )
		.doesNotHaveDuplicates();

Uh oh!

Conversation

thunderhook commented Aug 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr commented Aug 24, 2024

Uh oh!

thunderhook commented Aug 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thunderhook commented Aug 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr commented Aug 25, 2024

Uh oh!

thunderhook commented Sep 1, 2024

Uh oh!

thunderhook commented Sep 3, 2024

Uh oh!

filiphr commented Sep 13, 2024

Uh oh!

filiphr commented Sep 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants