From 66f4288842460f27b88ba4580cd5384551127d61 Mon Sep 17 00:00:00 2001 From: Filip Hrisafov Date: Sat, 20 Jul 2024 13:53:39 +0200 Subject: [PATCH] #3601 Always use SourceParameterCondition when checking source parameter This is a breaking change, with this change whenever a source parameter is used as a source for a target property the condition has to apply to source parameters and not properties --- NEXT_RELEASE_CHANGELOG.md | 68 +++++++++ .../source/selector/SelectionCriteria.java | 129 ++++++++++++------ .../ap/test/bugs/_3601/Issue3601Mapper.java | 50 +++++++ .../ap/test/bugs/_3601/Issue3601Test.java | 47 +++++++ ...itionalMethodWithSourceToTargetMapper.java | 6 +- 5 files changed, 252 insertions(+), 48 deletions(-) create mode 100644 processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Mapper.java create mode 100644 processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Test.java diff --git a/NEXT_RELEASE_CHANGELOG.md b/NEXT_RELEASE_CHANGELOG.md index a6c424b08..305bdda94 100644 --- a/NEXT_RELEASE_CHANGELOG.md +++ b/NEXT_RELEASE_CHANGELOG.md @@ -8,7 +8,75 @@ Source properties are ignored anyway, the `BeanMapping#unmappedSourcePolicy` sho ### Bugs +* Breaking change: Presence check method used only once when multiple source parameters are provided (#3601) + ### Documentation ### Build +## Breaking changes + +### Presence checks for source parameters + +In 1.6, support for presence checks on source parameters has been added. +This means that even if you want to map a source parameter directly to some target property the new `@SourceParameterCondition` or `@Condition(appliesTo = ConditionStrategy.SOURCE_PARAMETERS)` should be used. + +e.g. + +If we had the following in 1.5: +```java +@Mapper +public interface OrderMapper { + + @Mapping(source = "dto", target = "customer", conditionQualifiedByName = "mapCustomerFromOrder") + Order map(OrderDTO dto); + + @Condition + @Named("mapCustomerFromOrder") + default boolean mapCustomerFromOrder(OrderDTO dto) { + return dto != null && dto.getCustomerName() != null; + } + +} +``` + +Them MapStruct would generate + +```java +public class OrderMapperImpl implements OrderMapper { + + @Override + public Order map(OrderDTO dto) { + if ( dto == null ) { + return null; + } + + Order order = new Order(); + + if ( mapCustomerFromOrder( dto ) ) { + order.setCustomer( orderDtoToCustomer( orderDTO ) ); + } + + return order; + } +} +``` + +In order for the same to be generated in 1.6, the mapper needs to look like this: + +```java +@Mapper +public interface OrderMapper { + + @Mapping(source = "dto", target = "customer", conditionQualifiedByName = "mapCustomerFromOrder") + Order map(OrderDTO dto); + + @SourceParameterCondition + @Named("mapCustomerFromOrder") + default boolean mapCustomerFromOrder(OrderDTO dto) { + return dto != null && dto.getCustomerName() != null; + } + +} +``` + 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 fa5e1c29c..ecc9ac9e0 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 @@ -5,7 +5,6 @@ */ package org.mapstruct.ap.internal.model.source.selector; -import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -22,50 +21,31 @@ import org.mapstruct.ap.internal.model.source.SelectionParameters; */ public class SelectionCriteria { - private final List qualifiers = new ArrayList<>(); - private final List qualifiedByNames = new ArrayList<>(); + private final QualifyingInfo qualifyingInfo; private final String targetPropertyName; - private final TypeMirror qualifyingResultType; private final SourceRHS sourceRHS; private boolean ignoreQualifiers = false; private Type type; - private final boolean allowDirect; - private final boolean allowConversion; - private final boolean allowMappingMethod; - private final boolean allow2Steps; + private final MappingControl mappingControl; public SelectionCriteria(SelectionParameters selectionParameters, MappingControl mappingControl, String targetPropertyName, Type type) { - if ( selectionParameters != null ) { - if ( type == Type.PRESENCE_CHECK ) { - qualifiers.addAll( selectionParameters.getConditionQualifiers() ); - qualifiedByNames.addAll( selectionParameters.getConditionQualifyingNames() ); - } - else { - qualifiers.addAll( selectionParameters.getQualifiers() ); - qualifiedByNames.addAll( selectionParameters.getQualifyingNames() ); - } - qualifyingResultType = selectionParameters.getResultType(); - sourceRHS = selectionParameters.getSourceRHS(); - } - else { - this.qualifyingResultType = null; - sourceRHS = null; - } - if ( mappingControl != null ) { - this.allowDirect = mappingControl.allowDirect(); - this.allowConversion = mappingControl.allowTypeConversion(); - this.allowMappingMethod = mappingControl.allowMappingMethod(); - this.allow2Steps = mappingControl.allowBy2Steps(); - } - else { - this.allowDirect = true; - this.allowConversion = true; - this.allowMappingMethod = true; - this.allow2Steps = true; - } + this( + QualifyingInfo.fromSelectionParameters( selectionParameters ), + selectionParameters != null ? selectionParameters.getSourceRHS() : null, + mappingControl, + targetPropertyName, + type + ); + } + + private SelectionCriteria(QualifyingInfo qualifyingInfo, SourceRHS sourceRHS, MappingControl mappingControl, + String targetPropertyName, Type type) { + this.qualifyingInfo = qualifyingInfo; this.targetPropertyName = targetPropertyName; + this.sourceRHS = sourceRHS; this.type = type; + this.mappingControl = mappingControl; } /** @@ -109,11 +89,11 @@ public class SelectionCriteria { } public List getQualifiers() { - return ignoreQualifiers ? Collections.emptyList() : qualifiers; + return ignoreQualifiers ? Collections.emptyList() : qualifyingInfo.qualifiers(); } public List getQualifiedByNames() { - return ignoreQualifiers ? Collections.emptyList() : qualifiedByNames; + return ignoreQualifiers ? Collections.emptyList() : qualifyingInfo.qualifiedByNames(); } public String getTargetPropertyName() { @@ -121,7 +101,7 @@ public class SelectionCriteria { } public TypeMirror getQualifyingResultType() { - return qualifyingResultType; + return qualifyingInfo.qualifyingResultType(); } public boolean isPreferUpdateMapping() { @@ -137,23 +117,23 @@ public class SelectionCriteria { } public boolean hasQualfiers() { - return !qualifiedByNames.isEmpty() || !qualifiers.isEmpty(); + return !qualifyingInfo.qualifiedByNames().isEmpty() || !qualifyingInfo.qualifiers().isEmpty(); } public boolean isAllowDirect() { - return allowDirect; + return mappingControl == null || mappingControl.allowDirect(); } public boolean isAllowConversion() { - return allowConversion; + return mappingControl == null || mappingControl.allowTypeConversion(); } public boolean isAllowMappingMethod() { - return allowMappingMethod; + return mappingControl == null || mappingControl.allowMappingMethod(); } public boolean isAllow2Steps() { - return allow2Steps; + return mappingControl == null || mappingControl.allowBy2Steps(); } public boolean isSelfAllowed() { @@ -181,7 +161,22 @@ public class SelectionCriteria { } public static SelectionCriteria forPresenceCheckMethods(SelectionParameters selectionParameters) { - return new SelectionCriteria( selectionParameters, null, null, Type.PRESENCE_CHECK ); + SourceRHS sourceRHS = selectionParameters.getSourceRHS(); + Type type; + QualifyingInfo qualifyingInfo = new QualifyingInfo( + selectionParameters.getConditionQualifiers(), + selectionParameters.getConditionQualifyingNames(), + selectionParameters.getResultType() + ); + if ( sourceRHS != null && sourceRHS.isSourceReferenceParameter() ) { + // If the source reference is for a source parameter, + // then the presence check should be for the source parameter + type = Type.SOURCE_PARAMETER_CHECK; + } + else { + type = Type.PRESENCE_CHECK; + } + return new SelectionCriteria( qualifyingInfo, sourceRHS, null, null, type ); } public static SelectionCriteria forSourceParameterCheckMethods(SelectionParameters selectionParameters) { @@ -193,6 +188,50 @@ public class SelectionCriteria { return new SelectionCriteria( selectionParameters, mappingControl, null, Type.SELF_NOT_ALLOWED ); } + private static class QualifyingInfo { + + private static final QualifyingInfo EMPTY = new QualifyingInfo( + Collections.emptyList(), + Collections.emptyList(), + null + ); + + private final List qualifiers; + private final List qualifiedByNames; + private final TypeMirror qualifyingResultType; + + private QualifyingInfo(List qualifiers, List qualifiedByNames, + TypeMirror qualifyingResultType) { + this.qualifiers = qualifiers; + this.qualifiedByNames = qualifiedByNames; + this.qualifyingResultType = qualifyingResultType; + } + + public List qualifiers() { + return qualifiers; + } + + public List qualifiedByNames() { + return qualifiedByNames; + } + + public TypeMirror qualifyingResultType() { + return qualifyingResultType; + } + + private static QualifyingInfo fromSelectionParameters(SelectionParameters selectionParameters) { + if ( selectionParameters == null ) { + return EMPTY; + } + return new QualifyingInfo( + selectionParameters.getQualifiers(), + selectionParameters.getQualifyingNames(), + selectionParameters.getResultType() + ); + } + } + + public enum Type { PREFER_UPDATE_MAPPING, OBJECT_FACTORY, diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Mapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Mapper.java new file mode 100644 index 000000000..851e353de --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Mapper.java @@ -0,0 +1,50 @@ +/* + * 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._3601; + +import java.util.List; + +import org.mapstruct.Mapper; +import org.mapstruct.Mapping; +import org.mapstruct.SourceParameterCondition; +import org.mapstruct.factory.Mappers; + +/** + * @author Filip Hrisafov + */ +@Mapper +public interface Issue3601Mapper { + + Issue3601Mapper INSTANCE = Mappers.getMapper( Issue3601Mapper.class ); + + @Mapping(target = "currentId", source = "source.uuid") + @Mapping(target = "targetIds", source = "sourceIds") + Target map(Source source, List sourceIds); + + @SourceParameterCondition + default boolean isNotEmpty(List elements) { + return elements != null && !elements.isEmpty(); + } + + class Source { + private final String uuid; + + public Source(String uuid) { + this.uuid = uuid; + } + + public String getUuid() { + return uuid; + } + } + + class Target { + //CHECKSTYLE:OFF + public String currentId; + public List targetIds; + //CHECKSTYLE:ON + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Test.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Test.java new file mode 100644 index 000000000..b9b68fb58 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_3601/Issue3601Test.java @@ -0,0 +1,47 @@ +/* + * 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._3601; + +import java.util.Collections; + +import org.junit.jupiter.api.extension.RegisterExtension; +import org.mapstruct.ap.testutil.IssueKey; +import org.mapstruct.ap.testutil.ProcessorTest; +import org.mapstruct.ap.testutil.WithClasses; +import org.mapstruct.ap.testutil.runner.GeneratedSource; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Filip Hrisafov + */ +@IssueKey("3601") +class Issue3601Test { + + @RegisterExtension + final GeneratedSource generatedSource = new GeneratedSource(); + + @ProcessorTest + @WithClasses( Issue3601Mapper.class ) + void shouldUseSourceParameterPresenceCheckCorrectly() { + Issue3601Mapper.Target target = Issue3601Mapper.INSTANCE.map( + new Issue3601Mapper.Source( "test1" ), + Collections.emptyList() + ); + + assertThat( target ).isNotNull(); + assertThat( target.currentId ).isEqualTo( "test1" ); + assertThat( target.targetIds ).isNull(); + + target = Issue3601Mapper.INSTANCE.map( + null, + Collections.emptyList() + ); + + assertThat( target ).isNull(); + } + +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/conditional/qualifier/ConditionalMethodWithSourceToTargetMapper.java b/processor/src/test/java/org/mapstruct/ap/test/conditional/qualifier/ConditionalMethodWithSourceToTargetMapper.java index 35864fe62..6577a6fd9 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/conditional/qualifier/ConditionalMethodWithSourceToTargetMapper.java +++ b/processor/src/test/java/org/mapstruct/ap/test/conditional/qualifier/ConditionalMethodWithSourceToTargetMapper.java @@ -5,10 +5,10 @@ */ package org.mapstruct.ap.test.conditional.qualifier; -import org.mapstruct.Condition; import org.mapstruct.Mapper; import org.mapstruct.Mapping; import org.mapstruct.Named; +import org.mapstruct.SourceParameterCondition; import org.mapstruct.factory.Mappers; /** @@ -29,13 +29,13 @@ public interface ConditionalMethodWithSourceToTargetMapper { Address convertToAddress(OrderDTO orderDTO); - @Condition + @SourceParameterCondition @Named("mapCustomerFromOrder") default boolean mapCustomerFromOrder(OrderDTO orderDTO) { return orderDTO != null && ( orderDTO.getCustomerName() != null || mapAddressFromOrder( orderDTO ) ); } - @Condition + @SourceParameterCondition @Named("mapAddressFromOrder") default boolean mapAddressFromOrder(OrderDTO orderDTO) { return orderDTO != null && ( orderDTO.getLine1() != null || orderDTO.getLine2() != null );