Skip to content

#4037 Stop binding property-level @Condition to unrelated source parameter#4069

Open
seonwooj0810 wants to merge 1 commit into
mapstruct:mainfrom
seonwooj0810:fix/issue-4037-condition-wrong-source-param
Open

#4037 Stop binding property-level @Condition to unrelated source parameter#4069
seonwooj0810 wants to merge 1 commit into
mapstruct:mainfrom
seonwooj0810:fix/issue-4037-condition-wrong-source-param

Conversation

@seonwooj0810

Copy link
Copy Markdown
Contributor

Fixes #4037.

Why this happens

For a property-level @Condition, PresenceCheckMethodResolver asks TypeSelector to bind every parameter of the @Condition method to an available ParameterBinding. The bindings published for a property mapping include both the property's source value (SourceRHS) and every other source parameter of the mapping method.

TypeSelector does not know which binding is the value being checked. It enumerates permutations and keeps the ones whose types are assignable. When SourceRHS does not match the @Condition parameter's type but an unrelated sibling source parameter happens to match, that sibling is silently selected — even though it has nothing to do with the property being mapped.

In the reporter's example:

@Mapping(target = "browserId", source = "requestBean.deviceInfo.browserId")
AudienceProfileRequest map(RequestBean requestBean, String slotId);

class PrechecksSupport {
    @Condition boolean isNotBlank(String value) { ... }
}

browserId is an Integer, so SourceRHS does not match isNotBlank(String). The sibling slotId parameter does, so MapStruct generates

Integer browserId = requestBeanDeviceInfoBrowserId( requestBean );
if ( prechecksSupport.isNotBlank( slotId ) ) {
    audienceProfileRequest.setBrowserId( browserId );
}

A blank slotId would then silently null out every Integer property of the result even when the actual source values are present.

What changes

After PresenceCheckMethodResolver.getPresenceCheck collects the matching methods, discard any whose chosen binding consumes a sibling source parameter that is not the root of the property's source path. The root is SourceRHS#getSourceParameterName() — e.g. requestBean for source = "requestBean.deviceInfo.browserId". All other binding kinds (the SourceRHS itself, @MappingContext, @TargetType, @SourcePropertyName, @TargetPropertyName, @MappingTarget) remain valid candidates.

This keeps every existing pattern working:

  • @Condition isNotBlank(String value) on a String property — value binds to SourceRHS.
  • @Condition shouldMap(EmployeeDto source) on a property of employeesource binds to the mapper parameter, which is the root of the property's source path.
  • @Condition isAmericanCitizen(EmployeeDto employerDto) referenced via conditionQualifiedByName — same as above.
  • @Condition isPresent(EmployeeDto source, String value)source binds to the parameter root, value binds to SourceRHS.

When the unrelated-sibling case is the only match left, the resolver returns null and the property falls back to the default null check (or no check, depending on nullValueCheckStrategy).

Tests

  • org.mapstruct.ap.test.bugs._4037.Issue4037Test — multi-source-parameter mapper with nullValueCheckStrategy = ALWAYS and a @Condition isNotBlank(String). Confirms the Integer properties no longer use isNotBlank(slotId) as their guard while the String property still gets the @Condition.
  • The full processor module test suite runs green (3633 tests). The conditional/qualifier/expression and presence-check suites in particular continue to pass without changes.

Verification done

  • ./mvnw -pl processor -Dtest='Issue4037Test' test — 4/4 pass.
  • ./mvnw -pl processor -Dtest='*Conditional*Test,*Condition*Test,Issue4037Test' test — 58/58 pass.
  • ./mvnw -pl processor -Dtest='*NullCheck*Test,*NullValueCheckStrategy*Test,*Selection*Test,*Presence*Test,*PresenceCheck*Test,*Selector*Test' test — 136/136 pass.
  • ./mvnw -pl processor -Dtest='org.mapstruct.ap.test.bugs.**,*MultiSource*Test,*SourcePresence*Test' test — 696/696 pass.
  • ./mvnw -pl processor test — 3633/3633 pass.
  • ./mvnw -pl processor checkstyle:check — 0 violations.
  • Inspected the generated source for the reproducer (Issue4037MapperImpl.java) and confirmed the previously buggy if ( prechecksSupport.isNotBlank( slotId ) ) guard is replaced with the default if ( browserId != null ) / if ( osId != null ) check, while the matching String property still emits if ( prechecksSupport.isNotBlank( deviceName ) ).

…urce parameter

For property-level @condition resolution the TypeSelector permutes every
candidate ParameterBinding, which for a mapping method declaring multiple
source parameters includes those siblings alongside the property's own
SourceRHS. When the SourceRHS type does not match any parameter of the
@condition method but a sibling source parameter happens to match by
type, the selector silently bound the sibling and the check ran on an
unrelated value (e.g. isNotBlank(slotId) was generated as the guard for
an Integer property).

After the matching methods are found, drop any whose chosen binding
consumes a sibling source parameter that is not the root of the
property's source path. Bindings to the SourceRHS itself, to
context/target/source-property-name parameters, and to the source
parameter whose property is being mapped (rootParam in
@mapping(source = "rootParam.nested.value")) remain valid; with no
acceptable binding the resolver falls back to the default null check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@Condition applied on wrong source parameter while mapping

1 participant