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!

[filiphr]

Thanks for the detailed analysis @thunderhook. I had a look at your tests, I'm not sure that I can say that the tests that are failing on top of main have a correct fixture. The reason why they are failing is due to the fact that we support passing passing source parameters in condition presence checks. This means that the presence checks are being invoked because they have a source parameter in there. If you change the list in the isNotEmpty with the other source parameter Source then they'll get invoked as well.

The test in the pull request is green, however the generated code does not look correct.

What is wrong with the generated code for the test in the PR? Which part of it is not correct?

Regarding the tests that are failing on top of my branch.

Condition on String	Condition on List	Tests on main	Tests on filiphr:3601	My Comment
PropertyCondition	SourceParameterCondition		fail	This is the scenario that we are fixing. The code generated on main is incorrect. The second check should be the source property isNotEmpty and not a not null check.
PropertyCondition	PropertyCondition		fail	This the the scenario that we are fixing. The code generated on main is incorrect. The second check should be a non null check and not an isNotEmpty
SourceParameterCondition	PropertyCondition	fail	fail	This is exactly like the second scenario (PropertyCondition / PropertyCondition)
SourceParameterCondition	SourceParameterCondition		fail	This is exactly like the first scenario (PropertyCondition / SourceParameterCondition)
PropertyCondition	MultiCondition
SourceParameterCondition	MultiCondition	fail	fail
MultiCondition	PropertyCondition		fail	This is exactly like the second scenario (PropertyCondition / PropertyCondition)
MultiCondition	SourceParameterCondition		fail	This is exactly like the first scenario (PropertyCondition / SourceParameterCondition)
MultiCondition	MultiCondition
PropertyCondition	-
SourceParameterCondition	-
MultiCondition	-
-	PropertyCondition	fail	fail	This is exactly like the second scenario (PropertyCondition / PropertyCondition)
-	SourceParameterCondition		fail	This is exactly like the first scenario (PropertyCondition / SourceParameterCondition)
-	MultiCondition	fail	fail

I have also copied the fixtures into the PR and pushed it. I'm not sure that I'd like to keep all of them, as they are all more or less the same. There are only 2 difference scenarios in the end (as you can see from my comments in the table above)

[thunderhook]

Thanks for revisiting this. I hope I have not added any more confusion by providing the permuted tests.

What is wrong with the generated code for the test in the PR? Which part of it is not correct?

Unfortunately, I have documented it very poorly here. I had a fresh look at it on your branch.

Irrelevant presence check method of List<String> called when mapping String to String (uuid).

Happens in StringSourceParameterConditionListMultiConditionMapper and OnlyListMultiConditionMapper, see comment in the generated code.

@Mapper
public interface StringSourceParameterConditionListMultiConditionMapper {

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

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

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

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

}

// GENERATED CODE
public class StringSourceParameterConditionListMultiConditionMapperImpl implements StringSourceParameterConditionListMultiConditionMapper {

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

        Target target = new Target();

        if ( source != null ) {
            // why is this here? this is the problem in the original description of the issue
            if ( isNotEmpty( sourceIds ) ) {
                target.currentId = source.getUuid();
            }
        }
        if ( isNotEmpty( sourceIds ) ) {
            List<String> list = sourceIds;
            target.targetIds = new ArrayList<String>( list );
        }

        return target;
    }
}

Appearance of the !isNotEmpty( sourceIds ) at the beginning of a method, when it shouldn't.

Keep these two definitions in mind:

• @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).

Let's just take the OnlyListSourceParameterConditionMapper. Based on the definition above, the source parameter condition should be used at the beginning of the method (1). Since it is not a property condition, it should not be used as a check inside the mapping method (2):

@Mapper
public interface OnlyListSourceParameterConditionMapper {

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

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

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

}

// GENERATED CODE
public class OnlyListSourceParameterConditionMapperImpl implements OnlyListSourceParameterConditionMapper {

    @Override
    public Target map(Source source, List<String> sourceIds) {
        if ( source == null && !isNotEmpty( sourceIds ) ) { // (1) - seems okay to me
            return null;
        }

        Target target = new Target();

        if ( source != null ) {
            target.currentId = source.getUuid();
        }
        // why is this here? the condition is not a property condition
        if ( isNotEmpty( sourceIds ) ) {
            List<String> list = sourceIds;
            target.targetIds = new ArrayList<String>( list );
        }

        return target;
    }
}

Perhaps I am wrong in my definitions/assumptions of the two types of conditions. Perhaps this is just a confusion on my part that I would like to have cleared up. Thanks in advance!

I'm not sure that I'd like to keep all of them, as they are all more or less the same. There are only 2 difference scenarios in the end (as you can see from my comments in the table above)

I agree, I would suggest just before merging that we delete some of them to make sure we don't miss an edge case right now.