Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thunderhook wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b74bde5
#3678 Prevent duplicate @BeforeMapping and @AfterMapping calls on cla…
thunderhook Aug 21, 2024
b5b94bd
#3678 write a separate test
thunderhook Aug 22, 2024
af7b02c
#3678 add before mapping to test
thunderhook Aug 22, 2024
3b5b028
#3678 improve removal of duplicate lifecycle callback methods
thunderhook Aug 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,9 @@ else if ( !method.isUpdateMethod() ) {
existingVariableNames
) );

// remove methods without parameters as they are already being invoked
removeMappingReferencesWithoutSourceParameters( beforeMappingReferencesWithFinalizedReturnType );
removeMappingReferencesWithoutSourceParameters( afterMappingReferencesWithFinalizedReturnType );
// remove methods that are already being invoked
beforeMappingReferencesWithFinalizedReturnType.removeAll( beforeMappingMethods );
afterMappingReferencesWithFinalizedReturnType.removeAll( afterMappingMethods );
}

Map<String, PresenceCheck> presenceChecksByParameter = new LinkedHashMap<>();
Expand Down Expand Up @@ -451,10 +451,6 @@ else if ( !sourceParameter.getType().isPrimitive() ) {
);
}

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

private boolean doesNotAllowAbstractReturnTypeAndCanBeConstructed(Type returnTypeImpl) {
return !isAbstractReturnTypeAllowed()
&& canReturnTypeBeConstructed( returnTypeImpl );
Expand Down
33 changes: 33 additions & 0 deletions ...essor/src/main/java/org/mapstruct/ap/internal/model/LifecycleCallbackMethodReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.mapstruct.ap.internal.model;

import java.util.Objects;
import java.util.Set;

import org.mapstruct.ap.internal.model.common.Parameter;
Expand Down Expand Up @@ -118,4 +119,36 @@ public static LifecycleCallbackMethodReference forMethodReference(SelectedMethod
containingMethod,
existingVariableNames );
}

@Override
public boolean equals(Object obj) {
if ( this == obj ) {
return true;
}
if ( !super.equals( obj ) ) {
return false;
}
if ( getClass() != obj.getClass() ) {
return false;
}
LifecycleCallbackMethodReference other = (LifecycleCallbackMethodReference) obj;
if ( !Objects.equals( declaringType, other.declaringType )) {
return false;
}
if ( !Objects.equals( methodReturnType, other.methodReturnType )) {
return false;
}
if ( !Objects.equals( methodResultType, other.methodResultType )) {
return false;
}
if ( !Objects.equals( targetVariableName, other.targetVariableName )) {
return false;
}
return true;
}

@Override
public int hashCode() {
return Objects.hash( super.hashCode(), declaringType, methodReturnType, methodResultType, targetVariableName );
}
}
6 changes: 3 additions & 3 deletions processor/src/main/java/org/mapstruct/ap/internal/model/MethodReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,6 @@ public boolean equals(Object obj) {
if ( this == obj ) {
return true;
}
if ( !super.equals( obj ) ) {
return false;
}
Comment on lines -390 to -392
Copy link
Contributor Author

@thunderhook thunderhook Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ( getClass() != obj.getClass() ) {
return false;
}
Expand All @@ -400,6 +397,9 @@ public boolean equals(Object obj) {
if ( !Objects.equals( providingParameter, other.providingParameter ) ) {
return false;
}
if ( !Objects.equals( name, other.name ) ) {
return false;
}
Comment on lines +400 to +402
Copy link
Contributor Author

@thunderhook thunderhook Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return true;
}
Expand Down
3 changes: 1 addition & 2 deletions ...ssor/src/main/java/org/mapstruct/ap/internal/model/source/selector/SelectionCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import java.util.Collections;
import java.util.List;

import javax.lang.model.type.TypeMirror;

import org.mapstruct.ap.internal.model.common.SourceRHS;
Expand Down Expand Up @@ -116,7 +115,7 @@ public void setPreferUpdateMapping(boolean preferUpdateMapping) {
this.type = preferUpdateMapping ? Type.PREFER_UPDATE_MAPPING : null;
}

public boolean hasQualfiers() {
public boolean hasQualifiers() {
return !qualifyingInfo.qualifiedByNames().isEmpty() || !qualifyingInfo.qualifiers().isEmpty();
}

Expand Down
11 changes: 5 additions & 6 deletions ...essor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
*/
package org.mapstruct.ap.internal.processor.creation;

import static org.mapstruct.ap.internal.util.Collections.first;
import static org.mapstruct.ap.internal.util.Collections.firstKey;
import static org.mapstruct.ap.internal.util.Collections.firstValue;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.Iterator;
Expand All @@ -20,7 +16,6 @@
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.Element;
import javax.lang.model.element.ExecutableElement;
Expand Down Expand Up @@ -67,6 +62,10 @@
import org.mapstruct.ap.internal.util.Strings;
import org.mapstruct.ap.internal.util.TypeUtils;

import static org.mapstruct.ap.internal.util.Collections.first;
import static org.mapstruct.ap.internal.util.Collections.firstKey;
import static org.mapstruct.ap.internal.util.Collections.firstValue;

/**
* The one and only implementation of {@link MappingResolver}. The class has been split into an interface an
* implementation for the sake of avoiding package dependencies. Specifically, this implementation refers to classes
Expand Down Expand Up @@ -335,7 +334,7 @@ else if ( allowMappingMethod() ) {
}

private boolean hasQualfiers() {
return selectionCriteria != null && selectionCriteria.hasQualfiers();
return selectionCriteria != null && selectionCriteria.hasQualifiers();
}

private void printQualifierMessage(SelectionCriteria selectionCriteria ) {
Expand Down
124 changes: 124 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/
package org.mapstruct.ap.test.bugs._3678;

import java.util.ArrayList;
import java.util.List;

import org.mapstruct.AfterMapping;
import org.mapstruct.BeforeMapping;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;

@Mapper
public abstract class Issue3678Mapper {

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

@thunderhook thunderhook Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a specific test with two sources to be sure.


@Mapping( target = "description", constant = "some description")
abstract Target mapSingleSource(SourceA sourceA);

List<String> invocations = new ArrayList<>();

@BeforeMapping
void beforeMappingSourceA(SourceA sourceA) {
invocations.add( "beforeMappingSourceA" );
}

@AfterMapping
void afterMappingSourceB(SourceA sourceA) {
invocations.add( "afterMappingSourceA" );
}

@BeforeMapping
void beforeMappingSourceB(SourceB sourceB) {
invocations.add( "beforeMappingSourceB" );
}

@AfterMapping
void afterMappingSourceB(SourceB sourceB) {
invocations.add( "afterMappingSourceB" );
}

public List<String> getInvocations() {
return invocations;
}

public static class SourceA {

private final String name;

public SourceA(String name) {
this.name = name;
}

public String getName() {
return name;
}
}

public static class SourceB {

private final String description;

public SourceB(String description) {
this.description = description;
}

public String getDescription() {
return description;
}
}

public static final class Target {

private final String name;
private final String description;

private Target(String name, String description) {
this.name = name;
this.description = description;
}

public String getName() {
return name;
}

public String getDescription() {
return description;
}

public static Builder builder() {
return new Builder();
}

public static class Builder {

private String name;
private String description;

public Builder() {
}

public Builder name(String name) {
this.name = name;
return this;
}

public Builder description(String description) {
this.description = description;
return this;
}

public Target build() {
return new Target( this.name, this.description );
}
}
}

}
51 changes: 51 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/

package org.mapstruct.ap.test.bugs._3678;

import org.mapstruct.ap.testutil.IssueKey;
import org.mapstruct.ap.testutil.ProcessorTest;
import org.mapstruct.ap.testutil.WithClasses;
import org.mapstruct.factory.Mappers;

import static org.assertj.core.api.Assertions.assertThat;

@IssueKey("3678")
@WithClasses(Issue3678Mapper.class)
public class Issue3678Test {

@ProcessorTest
void beforeAndAfterMappingOnlyCalledOnceForTwoSources() {

Issue3678Mapper mapper = Mappers.getMapper( Issue3678Mapper.class );
Issue3678Mapper.SourceA sourceA = new Issue3678Mapper.SourceA( "name" );
Issue3678Mapper.SourceB sourceB = new Issue3678Mapper.SourceB( "description" );
Issue3678Mapper.Target target = mapper.mapTwoSources( sourceA, sourceB );

assertThat( mapper.getInvocations() )
.containsExactly(
"beforeMappingSourceA",
"beforeMappingSourceB",
"afterMappingSourceA",
"afterMappingSourceB"
);
}

@ProcessorTest
void beforeAndAfterMappingOnlyCalledOnceForSingleSource() {

Issue3678Mapper mapper = Mappers.getMapper( Issue3678Mapper.class );
Issue3678Mapper.SourceA sourceA = new Issue3678Mapper.SourceA( "name" );
Issue3678Mapper.Target target = mapper.mapSingleSource( sourceA );

assertThat( mapper.getInvocations() )
.containsExactly(
"beforeMappingSourceA",
"afterMappingSourceA"
);
}

}
2 changes: 2 additions & 0 deletions .../src/test/java/org/mapstruct/ap/test/builder/lifecycle/BuilderLifecycleCallbacksTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ public void lifecycleMethodsShouldBeInvoked() {
assertThat( context.getInvokedMethods() )
.contains(
Copy link
Contributor Author

@thunderhook thunderhook Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@thunderhook thunderhook Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@filiphr filiphr Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@thunderhook thunderhook Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"beforeWithoutParameters",
"beforeWithSource",
"beforeWithTargetType",
"beforeWithBuilderTargetType",
"beforeWithBuilderTarget",
"afterWithoutParameters",
"afterWithBuilderTargetType",
"afterWithBuilderTarget",
"afterWithBuilderTargetReturningTarget",
"afterWithSource",
"afterWithTargetType",
"afterWithTarget",
"afterWithTargetReturningTarget"
Expand Down
10 changes: 10 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/builder/lifecycle/MappingContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ public void beforeWithoutParameters() {
invokedMethods.add( "beforeWithoutParameters" );
}

@BeforeMapping
public void beforeWithSource(OrderDto source) {
invokedMethods.add( "beforeWithSource" );
}

@BeforeMapping
public void beforeWithTargetType(OrderDto source, @TargetType Class<Order> orderClass) {
invokedMethods.add( "beforeWithTargetType" );
Expand All @@ -50,6 +55,11 @@ public void afterWithoutParameters() {
invokedMethods.add( "afterWithoutParameters" );
}

@AfterMapping
public void afterWithSource(OrderDto source) {
invokedMethods.add( "afterWithSource" );
}

@AfterMapping
public void afterWithTargetType(OrderDto source, @TargetType Class<Order> orderClass) {
invokedMethods.add( "afterWithTargetType" );
Expand Down