From 3b5b028ffa8db8d1f5407dafbdaabe007949bb2a Mon Sep 17 00:00:00 2001 From: thunderhook <8238759+thunderhook@users.noreply.github.com> Date: Sun, 25 Aug 2024 20:44:23 +0200 Subject: [PATCH] #3678 improve removal of duplicate lifecycle callback methods --- .../ap/internal/model/BeanMappingMethod.java | 21 +------- .../LifecycleCallbackMethodReference.java | 33 ++++++++++++ .../ap/internal/model/MethodReference.java | 6 +-- .../source/selector/SelectionCriteria.java | 3 +- .../creation/MappingResolverImpl.java | 11 ++-- .../ap/test/bugs/_3678/Issue3678Mapper.java | 52 +++++++++++++------ .../ap/test/bugs/_3678/Issue3678Test.java | 33 +++++++++--- 7 files changed, 106 insertions(+), 53 deletions(-) diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java b/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java index a265408be..aec5405e1 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java @@ -407,10 +407,8 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { ) ); // remove methods that are already being invoked - removeMappingReferencesWithoutSourceParameters( beforeMappingReferencesWithFinalizedReturnType ); - removeMappingReferencesWithoutSourceParameters( afterMappingReferencesWithFinalizedReturnType ); - removeMappingReferencesWithSingleSourceParameter( beforeMappingReferencesWithFinalizedReturnType ); - removeMappingReferencesWithSingleSourceParameter( afterMappingReferencesWithFinalizedReturnType ); + beforeMappingReferencesWithFinalizedReturnType.removeAll( beforeMappingMethods ); + afterMappingReferencesWithFinalizedReturnType.removeAll( afterMappingMethods ); } Map presenceChecksByParameter = new LinkedHashMap<>(); @@ -453,21 +451,6 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { ); } - private void removeMappingReferencesWithoutSourceParameters(List references) { - references.removeIf( r -> r.getSourceParameters().isEmpty() && r.getReturnType().isVoid() ); - } - - private void removeMappingReferencesWithSingleSourceParameter( - List references) { - references.removeIf( Builder::isSingleSourceParameter ); - } - - private static boolean isSingleSourceParameter(LifecycleCallbackMethodReference reference) { - return reference.getParameterBindings().size() == 1 - && reference.getParameterBindings().stream().allMatch( ParameterBinding::isSourceParameter ) - && reference.getReturnType().isVoid(); - } - private boolean doesNotAllowAbstractReturnTypeAndCanBeConstructed(Type returnTypeImpl) { return !isAbstractReturnTypeAllowed() && canReturnTypeBeConstructed( returnTypeImpl ); diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/LifecycleCallbackMethodReference.java b/processor/src/main/java/org/mapstruct/ap/internal/model/LifecycleCallbackMethodReference.java index c47fd258c..522678c76 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/LifecycleCallbackMethodReference.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/LifecycleCallbackMethodReference.java @@ -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; @@ -118,4 +119,36 @@ public class LifecycleCallbackMethodReference extends MethodReference { 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 ); + } } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/MethodReference.java b/processor/src/main/java/org/mapstruct/ap/internal/model/MethodReference.java index e2e518c55..b26fdcde8 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/MethodReference.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/MethodReference.java @@ -387,9 +387,6 @@ public class MethodReference extends ModelElement implements Assignment { if ( this == obj ) { return true; } - if ( !super.equals( obj ) ) { - return false; - } if ( getClass() != obj.getClass() ) { return false; } @@ -400,6 +397,9 @@ public class MethodReference extends ModelElement implements Assignment { if ( !Objects.equals( providingParameter, other.providingParameter ) ) { return false; } + if ( !Objects.equals( name, other.name ) ) { + return false; + } return true; } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/SelectionCriteria.java b/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/SelectionCriteria.java index ecc9ac9e0..876c31a13 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/SelectionCriteria.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/SelectionCriteria.java @@ -7,7 +7,6 @@ package org.mapstruct.ap.internal.model.source.selector; import java.util.Collections; import java.util.List; - import javax.lang.model.type.TypeMirror; import org.mapstruct.ap.internal.model.common.SourceRHS; @@ -116,7 +115,7 @@ public class SelectionCriteria { this.type = preferUpdateMapping ? Type.PREFER_UPDATE_MAPPING : null; } - public boolean hasQualfiers() { + public boolean hasQualifiers() { return !qualifyingInfo.qualifiedByNames().isEmpty() || !qualifyingInfo.qualifiers().isEmpty(); } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java b/processor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java index d84ba974d..a4e552f51 100755 --- a/processor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java @@ -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; @@ -20,7 +16,6 @@ import java.util.Set; 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; @@ -67,6 +62,10 @@ import org.mapstruct.ap.internal.util.NativeTypes; 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 @@ -335,7 +334,7 @@ public class MappingResolverImpl implements MappingResolver { } private boolean hasQualfiers() { - return selectionCriteria != null && selectionCriteria.hasQualfiers(); + return selectionCriteria != null && selectionCriteria.hasQualifiers(); } private void printQualifierMessage(SelectionCriteria selectionCriteria ) { diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java index de52264bb..0060693f6 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Mapper.java @@ -11,54 +11,76 @@ import java.util.List; import org.mapstruct.AfterMapping; import org.mapstruct.BeforeMapping; import org.mapstruct.Mapper; -import org.mapstruct.ReportingPolicy; +import org.mapstruct.Mapping; -@Mapper(unmappedSourcePolicy = ReportingPolicy.ERROR) +@Mapper public abstract class Issue3678Mapper { - abstract SimpleDestination sourceToDestination(SimpleSource source); + @Mapping( target = "name", source = "sourceA.name") + @Mapping( target = "description", source = "sourceB.description") + abstract Target mapTwoSources(SourceA sourceA, SourceB sourceB); + + @Mapping( target = "description", constant = "some description") + abstract Target mapSingleSource(SourceA sourceA); List invocations = new ArrayList<>(); @BeforeMapping - void beforeMapping(SimpleSource simpleSource) { - invocations.add( "beforeMapping" ); + void beforeMappingSourceA(SourceA sourceA) { + invocations.add( "beforeMappingSourceA" ); } @AfterMapping - void afterMapping(SimpleSource simpleSource) { - invocations.add( "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 getInvocations() { return invocations; } - public static class SimpleSource { + public static class SourceA { private final String name; - private final String description; - public SimpleSource(String name, String description) { + public SourceA(String name) { this.name = name; - this.description = description; } 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 SimpleDestination { + public static final class Target { private final String name; private final String description; - private SimpleDestination(String name, String description) { + private Target(String name, String description) { this.name = name; this.description = description; } @@ -93,8 +115,8 @@ public abstract class Issue3678Mapper { return this; } - public SimpleDestination build() { - return new SimpleDestination( this.name, this.description ); + public Target build() { + return new Target( this.name, this.description ); } } } diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Test.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Test.java index bb35f1434..4ad6642f2 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Test.java +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3678/Issue3678Test.java @@ -14,21 +14,38 @@ import org.mapstruct.factory.Mappers; import static org.assertj.core.api.Assertions.assertThat; @IssueKey("3678") +@WithClasses(Issue3678Mapper.class) public class Issue3678Test { - @WithClasses({ - Issue3678Mapper.class, - }) @ProcessorTest - void ignoreMappingsWithoutSourceShouldBeInvertible() { + void beforeAndAfterMappingOnlyCalledOnceForTwoSources() { Issue3678Mapper mapper = Mappers.getMapper( Issue3678Mapper.class ); - Issue3678Mapper.SimpleSource source = new Issue3678Mapper.SimpleSource( "name", "description" ); - Issue3678Mapper.SimpleDestination simpleDestination = mapper.sourceToDestination( source ); + 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( "beforeMapping", "afterMapping" ) - .doesNotHaveDuplicates(); + .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" + ); } }