Presence check method used only once when multiple source parameters are provided

[thunderhook]

Expected behavior

Feel free to change the title...

Given the following mapper having a mapping method with two parameters:

import java.util.List;

import org.mapstruct.Condition;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.factory.Mappers;

@Mapper
interface WrongConditionMapper {

    @Mapping(target = "currentId", source = "source.uuid")
    @Mapping(target = "targetIds", source = "sourceIds")
    Target map(Source source, List<String> sourceIds);

    @Condition
    default boolean isNotEmpty(List<String> elements) {
        return elements != null && !elements.isEmpty();
    }

    class Source {
        public String uuid;
    }

    class Target {
        public String currentId;
        public List<String> targetIds;
    }

}

The isNotEmpty presence check method should only be applied when mapping sourceIds to targetIds but not source.uuid
to currentId.

Actual behavior

class WrongConditionMapperImpl implements WrongConditionMapper {

    @Override
    public Target map(Source source, List<String> sourceIds) {
        if ( source == null && sourceIds == null ) {
            return null;
        }

        Target target = new Target();

        if ( source != null ) {
            // this isNotEmpty check has nothing to do with source.uuid or target.currentId
            if ( isNotEmpty( sourceIds ) ) {
                target.currentId = source.uuid;
            }
        }
        if ( isNotEmpty( sourceIds ) ) {
            List<String> list = sourceIds;
            target.targetIds = new ArrayList<String>( list );
        }

        return target;
    }
}

We didn't even find a workaround for this and implemented the method ourself.

I tried to find the culprit but I'm not experienced enough to fix this, but it is because it is picked up with the PresenceCheckMethodResolver here:

• mapstruct/processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java

  Lines 125 to 128 in 0a2a0aa

  	return selectors.getMatchingMethods(
  	getAllAvailableMethods( method, ctx.getSourceModel(), selectionContext.getSelectionCriteria() ),
  	selectionContext
  	);

Steps to reproduce the problem

Here is a test mapper and a corresponding unit test:

import java.util.List;

import org.mapstruct.Condition;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.factory.Mappers;

@Mapper
interface WrongConditionMapper {

    @Mapping(target = "currentId", source = "source.uuid")
    @Mapping(target = "targetIds", source = "sourceIds")
    Target map(Source source, List<String> sourceIds);

    @Condition
    default boolean isNotEmpty(List<String> elements) {
        return elements != null && !elements.isEmpty();
    }

    @Condition
    default boolean stringCondition(String str) {
        return str != null;
    }

    class Source {
        public String uuid;
    }

    class Target {
        public String currentId;
        public List<String> targetIds;
    }

}

import java.util.ArrayList;

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;

@WithClasses(WrongConditionMapper.class)
@IssueKey("3601")
class Issue3601Test {

    @ProcessorTest
    void shouldMapCurrentId() {

        Issue3601Mapper mapper = Mappers.getMapper( Issue3601Mapper.class );
        Issue3601Mapper.Source source = new Issue3601Mapper.Source();
        source.uuid = "some-uuid";

        Issue3601Mapper.Target target = mapper.map( source, null );

        assertThat( target ).isNotNull();
        assertThat( target.currentId ).isEqualTo( "some-uuid" );
        assertThat( target.targetIds ).isNull();

        target = mapper.map( source, Collections.emptyList() );

        assertThat( target ).isNotNull();
        assertThat( target.currentId ).isEqualTo( "some-uuid" );
        assertThat( target.targetIds ).isNull();

        ArrayList<String> sourceIds = new ArrayList<>();
        sourceIds.add( "other-uuid" );
        target = mapper.map( source, sourceIds );

        assertThat( target ).isNotNull();
        assertThat( target.currentId ).isEqualTo( "some-uuid" );
        assertThat( target.targetIds ).containsExactly( "other-uuid" );
    }
}

MapStruct Version

1.5.5, 1.6.0.Beta1

[filiphr]

@thunderhook have you tried this with 1.6.0.Beta2? I think that if you change the @Condition to be @SourceParameterCondition then it'll work as expected.

[thunderhook]

Yeah i tested this on main. Thanks for the hint, I totally forgot about the new feature. Test is green, however it is is then not picked up when the property mapping takes place:

// GENERATED
class Issue3601MapperImpl implements Issue3601Mapper {

    @Override
    public Target map(Source source, List<String> sourceIds) {
        if ( source == null && !isNotEmpty( sourceIds ) ) {
            return null;
        }

        Target target = new Target();

        if ( source != null ) {
            if ( stringCondition( source.uuid ) ) {
                target.currentId = source.uuid;
            }
        }
        // if ( isNotEmpty( sourceIds ) ) { ... } is missing here
        List<String> list = sourceIds;
        if ( list != null ) {
            target.targetIds = new ArrayList<String>( list );
        }

        return target;
    }
}

I adapated the test case above and added a mapping with an empty list and now it fails again.

[filiphr]

Good catch @thunderhook. This is indeed a bug in the way we are rechecking the presence of the source parameters.

Shall we rename this issue to tackle that bug?

[thunderhook]

@filiphr Feel free to rename it as you like, I found no better way to describe it. 👍 Thanks in advance!

[filiphr]

This is trickier than I thought. The null check I believe comes from the SetterWrapperForCollectionsAndMapsWithNullCheck.

This would work with something like:

@Mapper
public interface Issue3601Mapper {

    Issue3601Mapper INSTANCE = Mappers.getMapper( Issue3601Mapper.class );

    @Mapping(target = "currentId", source = "source.uuid")
    @Mapping(target = "targetIds", source = "sourceIds", conditionQualifiedByName = "isNotEmpty")
    Target map(Source source, List<String> sourceIds);

    @Condition
    @Named( "isNotEmpty" )
    default boolean isNotEmpty(List<String> elements) {
        return elements != null && !elements.isEmpty();
    }

    @SourceParameterCondition
    default boolean isNotEmptyParameter(List<String> elements) {
        return elements != null && !elements.isEmpty();
    }

    class Source {
        public final String uuid;

        public Source(String uuid) {
            this.uuid = uuid;
        }
    }

    class Target {
        public String currentId;
        public List<String> targetIds;
    }
}

but I'm not sure how to fix it without that. Using @SourceParameterCondition applies the presence check on the source parameter and the @Condition applies it when mapping the property. I need to see if in the second part it looks for a source parameter condition instead of a property one

[filiphr]

I actually found a solution for this. Have a look at PR #3620

[thunderhook]

@filiphr Thanks for your work on this!
The test in the pull request is green, however the generated code does not look correct.

I think there are still several problems when it comes to multiple parameters.

Let's try to summarise the recent changes about @Condition and see if I have understood them correctly.

• @SourceParameterCondition is about presence check and is generally at the top of a method with an early exit (see Javadoc).
• @Condition (equivalent to @Condition( appliesTo = ConditionStrategy.PROPERTIES ) is about a property mappig check inside a mapping method (see JavaDoc).
• special case @Condition( appliesTo = { ConditionStrategy.PROPERTIES, ConditionStrategy.SOURCE_PARAMETERS }) should be used for both positions. (I will call them MultiConditions here)

I created a branch with some tests and my expectations what should be generated (see 4af76ec).
In our case the @SourceParameterCondition will not have an effect since it is about a String. Nevertheless, I've added all combinations that can occur when multiple conditions are present and the test results are as following (empty is green):

Condition on String	Condition on List	Tests on main	Tests on filiphr:3601
PropertyCondition	SourceParameterCondition		fail
PropertyCondition	PropertyCondition		fail
SourceParameterCondition	PropertyCondition	fail	fail
SourceParameterCondition	SourceParameterCondition		fail
PropertyCondition	MultiCondition
SourceParameterCondition	MultiCondition	fail	fail
MultiCondition	PropertyCondition		fail
MultiCondition	SourceParameterCondition		fail
MultiCondition	MultiCondition
PropertyCondition	-
SourceParameterCondition	-
MultiCondition	-
-	PropertyCondition	fail	fail
-	SourceParameterCondition		fail
-	MultiCondition	fail	fail

I tried those tests on your branch and more failures occur.

For me it seems that those problems arise only when multiple are present.
And your merge request seems to have made it worse. 😅

Could you copy the tests from my branch and let me know what you think? Thanks in advance!