diff --git a/NEXT_RELEASE_CHANGELOG.md b/NEXT_RELEASE_CHANGELOG.md index 8392b63c60..7798894cd1 100644 --- a/NEXT_RELEASE_CHANGELOG.md +++ b/NEXT_RELEASE_CHANGELOG.md @@ -21,6 +21,7 @@ * Prevent mapper generation from a type with a generic super bound to a type with a generic extends bound (#3994) * Fix location for Javadoc when generating the distribution zip +* Stop binding a property-level `@Condition` method to an unrelated source parameter when its type matches by accident (#4037) ### Documentation diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java b/processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java index 8500b28d49..e01b045ec3 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java @@ -7,11 +7,14 @@ import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.stream.Collectors; import org.mapstruct.ap.internal.gem.ConditionStrategyGem; import org.mapstruct.ap.internal.model.common.Parameter; +import org.mapstruct.ap.internal.model.common.ParameterBinding; import org.mapstruct.ap.internal.model.common.PresenceCheck; +import org.mapstruct.ap.internal.model.common.SourceRHS; import org.mapstruct.ap.internal.model.presence.NullPresenceCheck; import org.mapstruct.ap.internal.model.presence.OptionalPresenceCheck; import org.mapstruct.ap.internal.model.source.Method; @@ -47,6 +50,24 @@ public static PresenceCheck getPresenceCheck( ctx ); + // For a property-level @Condition, the candidate parameter bindings include every source + // parameter of the mapping method alongside the property's source value (SourceRHS). When + // the SourceRHS type does not match any parameter of the @Condition method but an unrelated + // sibling source parameter happens to match by type, the type selector used to silently + // bind that sibling and the generated check ran on the wrong value. A sibling source + // parameter is only a valid binding when it is the root of the property's source path + // (e.g. @Mapping(source = "rootParam.nested.value") may bind a method parameter to + // rootParam itself), otherwise reject the match and let the default null check take over. + // See #4037. + SourceRHS sourceRHS = selectionParameters == null ? null : selectionParameters.getSourceRHS(); + if ( sourceRHS != null && !sourceRHS.isSourceReferenceParameter() ) { + String rootSourceParameterName = sourceRHS.getSourceParameterName(); + matchingMethods.removeIf( selected -> bindsUnrelatedSourceParameter( + selected.getParameterBindings(), + rootSourceParameterName + ) ); + } + if ( matchingMethods.isEmpty() ) { return null; } @@ -182,6 +203,23 @@ private static MethodReference getPresenceCheckMethodReference( } } + private static boolean bindsUnrelatedSourceParameter(List bindings, + String rootSourceParameterName) { + for ( ParameterBinding binding : bindings ) { + if ( !binding.isSourceParameter() ) { + continue; + } + if ( binding.isMappingTarget() || binding.isTargetType() || binding.isMappingContext() + || binding.isSourcePropertyName() || binding.isTargetPropertyName() ) { + continue; + } + if ( !Objects.equals( binding.getVariableName(), rootSourceParameterName ) ) { + return true; + } + } + return false; + } + private static List getAllAvailableMethods(Method method, List sourceModelMethods, SelectionCriteria selectionCriteria) { ParameterProvidedMethods contextProvidedMethods = method.getContextProvidedMethods(); diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_4037/Issue4037Mapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_4037/Issue4037Mapper.java new file mode 100644 index 0000000000..9e53debad8 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_4037/Issue4037Mapper.java @@ -0,0 +1,106 @@ +/* + * 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._4037; + +import org.mapstruct.Condition; +import org.mapstruct.Mapper; +import org.mapstruct.Mapping; +import org.mapstruct.NullValueCheckStrategy; +import org.mapstruct.factory.Mappers; + +@Mapper( + nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS, + uses = Issue4037Mapper.PrechecksSupport.class +) +public interface Issue4037Mapper { + + Issue4037Mapper INSTANCE = Mappers.getMapper( Issue4037Mapper.class ); + + @Mapping(target = "browserId", source = "requestBean.deviceInfo.browserId") + @Mapping(target = "osId", source = "requestBean.deviceInfo.osId") + @Mapping(target = "deviceName", source = "requestBean.deviceInfo.deviceName") + AudienceProfileRequest map(RequestBean requestBean, String slotId); + + class RequestBean { + private DeviceInfo deviceInfo; + + public DeviceInfo getDeviceInfo() { + return deviceInfo; + } + + public void setDeviceInfo(DeviceInfo deviceInfo) { + this.deviceInfo = deviceInfo; + } + } + + class DeviceInfo { + private Integer browserId; + private Integer osId; + private String deviceName; + + public Integer getBrowserId() { + return browserId; + } + + public void setBrowserId(Integer browserId) { + this.browserId = browserId; + } + + public Integer getOsId() { + return osId; + } + + public void setOsId(Integer osId) { + this.osId = osId; + } + + public String getDeviceName() { + return deviceName; + } + + public void setDeviceName(String deviceName) { + this.deviceName = deviceName; + } + } + + class AudienceProfileRequest { + private Integer browserId; + private Integer osId; + private String deviceName; + + public Integer getBrowserId() { + return browserId; + } + + public void setBrowserId(Integer browserId) { + this.browserId = browserId; + } + + public Integer getOsId() { + return osId; + } + + public void setOsId(Integer osId) { + this.osId = osId; + } + + public String getDeviceName() { + return deviceName; + } + + public void setDeviceName(String deviceName) { + this.deviceName = deviceName; + } + } + + class PrechecksSupport { + + @Condition + public boolean isNotBlank(String value) { + return value != null && !value.trim().isEmpty(); + } + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_4037/Issue4037Test.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_4037/Issue4037Test.java new file mode 100644 index 0000000000..cf0ad1e2f2 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_4037/Issue4037Test.java @@ -0,0 +1,65 @@ +/* + * 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._4037; + +import org.mapstruct.ap.testutil.IssueKey; +import org.mapstruct.ap.testutil.ProcessorTest; +import org.mapstruct.ap.testutil.WithClasses; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Reproducer for #4037. + *

+ * When the mapping method declares more than one source parameter and a {@code @Condition} method + * exists whose parameter type matches one of those sibling parameters, the type selector used to + * silently bind the unrelated sibling to the {@code @Condition} call. The presence check was + * therefore generated against the wrong value (for example {@code isNotBlank(slotId)} as the guard + * for an {@code Integer} property) and could discard valid property values whenever the unrelated + * source happened to be blank. + */ +@IssueKey("4037") +@WithClasses(Issue4037Mapper.class) +public class Issue4037Test { + + @ProcessorTest + public void presenceCheckOnIntegerPropertyDoesNotConsumeUnrelatedStringParameter() { + Issue4037Mapper.RequestBean requestBean = new Issue4037Mapper.RequestBean(); + Issue4037Mapper.DeviceInfo deviceInfo = new Issue4037Mapper.DeviceInfo(); + deviceInfo.setBrowserId( 123 ); + deviceInfo.setOsId( 456 ); + deviceInfo.setDeviceName( "Pixel" ); + requestBean.setDeviceInfo( deviceInfo ); + + // The sibling slotId parameter is blank. Before the fix, isNotBlank(slotId) was generated as + // the guard for every property, so blank slotId silently nulled out browserId and osId. + Issue4037Mapper.AudienceProfileRequest profile = Issue4037Mapper.INSTANCE.map( requestBean, "" ); + + assertThat( profile.getBrowserId() ).isEqualTo( 123 ); + assertThat( profile.getOsId() ).isEqualTo( 456 ); + // String property of matching type still gets the @Condition. + assertThat( profile.getDeviceName() ).isEqualTo( "Pixel" ); + } + + @ProcessorTest + public void presenceCheckOnStringPropertyStillApplies() { + // The @Condition is still applied to property sources of matching type. A blank deviceName + // should be dropped while non-blank ones (and unrelated numeric properties) pass through. + Issue4037Mapper.RequestBean requestBean = new Issue4037Mapper.RequestBean(); + Issue4037Mapper.DeviceInfo deviceInfo = new Issue4037Mapper.DeviceInfo(); + deviceInfo.setBrowserId( 7 ); + deviceInfo.setOsId( null ); + deviceInfo.setDeviceName( " " ); + requestBean.setDeviceInfo( deviceInfo ); + + Issue4037Mapper.AudienceProfileRequest profile = + Issue4037Mapper.INSTANCE.map( requestBean, "anything" ); + + assertThat( profile.getBrowserId() ).isEqualTo( 7 ); + assertThat( profile.getOsId() ).isNull(); + assertThat( profile.getDeviceName() ).isNull(); + } +}