#2274, #2023 Fix problems with property mapping using source parameters

Fixes problems when property mapping is using source parameter and has default value / expression or is doing an update

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

@@ -1619,12 +1619,47 @@ public class BeanMappingMethod extends NormalTypeMappingMethod {
                             .collect( Collectors.toList() );
     }
 
-    public List<Parameter> getSourcePrimitiveParameters() {
+    public List<Parameter> getSourceParametersNeedingNullCheck() {
         return getSourceParameters().stream()
-                            .filter( parameter -> parameter.getType().isPrimitive() )
+                            .filter( this::needsNullCheck )
                             .collect( Collectors.toList() );
     }
 
+    public List<Parameter> getSourceParametersNotNeedingNullCheck() {
+        return getSourceParameters().stream()
+                            .filter( parameter -> !needsNullCheck( parameter ) )
+                            .collect( Collectors.toList() );
+    }
+
+    private boolean needsNullCheck(Parameter parameter) {
+        if ( parameter.getType().isPrimitive() ) {
+            return false;
+        }
+
+        List<PropertyMapping> mappings = propertyMappingsByParameter( parameter );
+        if ( mappings.size() == 1 && doesNotNeedNullCheckForSourceParameter( mappings.get( 0 ) ) ) {
+            return false;
+        }
+
+        mappings = constructorPropertyMappingsByParameter( parameter );
+
+        if ( mappings.size() == 1 && doesNotNeedNullCheckForSourceParameter( mappings.get( 0 ) ) ) {
+            return false;
+        }
+
+        return true;
+    }
+
+    private boolean doesNotNeedNullCheckForSourceParameter(PropertyMapping mapping) {
+        if ( mapping.getAssignment().isCallingUpdateMethod() ) {
+            // If the mapping assignment is calling an update method then we should do a null check
+            // in the bean mapping
+            return false;
+        }
+
+        return mapping.getAssignment().isSourceReferenceParameter();
+    }
+
     @Override
     public int hashCode() {
         //Needed for Checkstyle, otherwise it fails due to EqualsHashCode rule

processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java

@@ -46,6 +46,8 @@ import org.mapstruct.ap.internal.util.ValueProvider;
 import org.mapstruct.ap.internal.util.accessor.Accessor;
 import org.mapstruct.ap.internal.util.accessor.AccessorType;
 
+import static org.mapstruct.ap.internal.gem.NullValueCheckStrategyGem.ALWAYS;
+import static org.mapstruct.ap.internal.gem.NullValuePropertyMappingStrategyGem.IGNORE;
 import static org.mapstruct.ap.internal.model.ForgedMethod.forElementMapping;
 import static org.mapstruct.ap.internal.model.ForgedMethod.forParameterMapping;
 import static org.mapstruct.ap.internal.model.ForgedMethod.forPropertyMapping;
@@ -374,11 +376,6 @@ public class PropertyMapping extends ModelElement {
             return null;
         }
 
-        private boolean hasDefaultValueAssignment(Assignment rhs) {
-            return ( defaultValue != null || defaultJavaExpression != null ) &&
-                ( !rhs.getSourceType().isPrimitive() || rhs.getSourcePresenceCheckerReference() != null);
-        }
-
         private Assignment assignToPlain(Type targetType, AccessorType targetAccessorType,
                                          Assignment rightHandSide) {
 
@@ -421,8 +418,7 @@ public class PropertyMapping extends ModelElement {
             }
             else {
                 // If the property mapping has a default value assignment then we have to do a null value check
-                boolean includeSourceNullCheck = SetterWrapper.doSourceNullCheck( rhs, nvcs, nvpms, targetType )
+                boolean includeSourceNullCheck = setterWrapperNeedsSourceNullCheck( rhs, targetType );
-                    || hasDefaultValueAssignment( rhs );
                 if ( !includeSourceNullCheck ) {
                     // solution for #834 introduced a local var and null check for nested properties always.
                     // however, a local var is not needed if there's no need to check for null.
@@ -438,6 +434,49 @@ public class PropertyMapping extends ModelElement {
             }
         }
 
+        /**
+         * Checks whether the setter wrapper should include a null / presence check or not
+         *
+         * @param rhs the source right hand side
+         * @param targetType the target type
+         *
+         * @return whether to include a null / presence check or not
+         */
+        private boolean setterWrapperNeedsSourceNullCheck(Assignment rhs, Type targetType) {
+            if ( rhs.getSourceType().isPrimitive() && rhs.getSourcePresenceCheckerReference() == null ) {
+                // If the source type is primitive or it doesn't have a presence checker then
+                // we shouldn't do a null check
+                return false;
+            }
+
+            if ( nvcs == ALWAYS ) {
+                // NullValueCheckStrategy is ALWAYS -> do a null check
+                return true;
+            }
+
+            if ( nvpms == SET_TO_DEFAULT || nvpms == IGNORE ) {
+                // NullValuePropertyMapping is SET_TO_DEFAULT or IGNORE -> do a null check
+                return true;
+            }
+
+            if ( rhs.getType().isConverted() ) {
+                // A type conversion is applied, so a null check is required
+                return true;
+            }
+
+            if ( rhs.getType().isDirect() && targetType.isPrimitive() ) {
+                // If the type is direct and the target type is primitive (i.e. we are unboxing) then check is needed
+                return true;
+            }
+
+            if ( defaultValue != null || defaultJavaExpression != null ) {
+                // If there is default value defined then a check is needed
+                return true;
+            }
+
+            return false;
+        }
+
         private Assignment assignToPlainViaAdder( Assignment rightHandSide) {
 
             Assignment result = rightHandSide;

processor/src/main/java/org/mapstruct/ap/internal/model/assignment/SetterWrapper.java

@@ -10,12 +10,6 @@ import java.util.List;
 
 import org.mapstruct.ap.internal.model.common.Assignment;
 import org.mapstruct.ap.internal.model.common.Type;
-import org.mapstruct.ap.internal.gem.NullValueCheckStrategyGem;
-import org.mapstruct.ap.internal.gem.NullValuePropertyMappingStrategyGem;
-
-import static org.mapstruct.ap.internal.gem.NullValueCheckStrategyGem.ALWAYS;
-import static org.mapstruct.ap.internal.gem.NullValuePropertyMappingStrategyGem.IGNORE;
-import static org.mapstruct.ap.internal.gem.NullValuePropertyMappingStrategyGem.SET_TO_DEFAULT;
 
 /**
  * Wraps the assignment in a target setter.
@@ -77,28 +71,4 @@ public class SetterWrapper extends AssignmentWrapper {
         return includeSourceNullCheck;
     }
 
-    /**
-     * Wraps the assignment in a target setter. include a null check when
-     *
-     * - Not if source is the parameter iso property, because the null check is than handled by the bean mapping
-     * - Not when source is primitive, you can't null check a primitive
-     * - The source property is fed to a conversion somehow before its assigned to the target
-     * - The user decided to ALLWAYS include a null check
-     *
-     * @param rhs the source righthand side
-     * @param nvcs null value check strategy
-     * @param nvpms null value property mapping strategy
-     * @param targetType the target type
-     *
-     * @return include a null check
-     */
-    public static boolean doSourceNullCheck(Assignment rhs, NullValueCheckStrategyGem nvcs,
-                                            NullValuePropertyMappingStrategyGem nvpms, Type targetType) {
-        return !rhs.isSourceReferenceParameter()
-            && !rhs.getSourceType().isPrimitive()
-            && (ALWAYS == nvcs
-            || SET_TO_DEFAULT == nvpms || IGNORE == nvpms
-            || rhs.getType().isConverted()
-            || (rhs.getType().isDirect() && targetType.isPrimitive()));
-    }
 }

processor/src/main/resources/org/mapstruct/ap/internal/model/BeanMappingMethod.ftl

@@ -27,7 +27,7 @@
     <#if !existingInstanceMapping>
         <#if hasConstructorMappings()>
             <#if (sourceParameters?size > 1)>
-                <#list sourceParametersExcludingPrimitives as sourceParam>
+                <#list sourceParametersNeedingNullCheck as sourceParam>
                     <#if (constructorPropertyMappingsByParameter(sourceParam)?size > 0)>
                         <#list constructorPropertyMappingsByParameter(sourceParam) as propertyMapping>
                             <@includeModel object=propertyMapping.targetType /> ${propertyMapping.targetWriteAccessorName} = ${propertyMapping.targetType.null};
@@ -39,7 +39,7 @@
                         }
                     </#if>
                 </#list>
-                <#list sourcePrimitiveParameters as sourceParam>
+                <#list sourceParametersNotNeedingNullCheck as sourceParam>
                     <#if (constructorPropertyMappingsByParameter(sourceParam)?size > 0)>
                         <#list constructorPropertyMappingsByParameter(sourceParam) as propertyMapping>
                             <@includeModel object=propertyMapping.targetType /> ${propertyMapping.targetWriteAccessorName} = ${propertyMapping.targetType.null};
@@ -80,7 +80,7 @@
     	</#if>
     </#list>
     <#if (sourceParameters?size > 1)>
-        <#list sourceParametersExcludingPrimitives as sourceParam>
+        <#list sourceParametersNeedingNullCheck as sourceParam>
             <#if (propertyMappingsByParameter(sourceParam)?size > 0)>
                 if ( ${sourceParam.name} != null ) {
                     <#list propertyMappingsByParameter(sourceParam) as propertyMapping>
@@ -89,7 +89,7 @@
                 }
             </#if>
         </#list>
-        <#list sourcePrimitiveParameters as sourceParam>
+        <#list sourceParametersNotNeedingNullCheck as sourceParam>
             <#if (propertyMappingsByParameter(sourceParam)?size > 0)>
                 <#list propertyMappingsByParameter(sourceParam) as propertyMapping>
                     <@includeModel object=propertyMapping targetBeanName=resultName existingInstanceMapping=existingInstanceMapping defaultValueAssignment=propertyMapping.defaultValueAssignment/>

processor/src/test/java/org/mapstruct/ap/test/bugs/_2023/Issue2023Mapper.java

@@ -0,0 +1,24 @@
+/*
+ * 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._2023;
+
+import java.util.UUID;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+import org.mapstruct.factory.Mappers;
+
+/**
+ * @author Filip Hrisafov
+ */
+@Mapper
+public interface Issue2023Mapper {
+
+    Issue2023Mapper INSTANCE = Mappers.getMapper( Issue2023Mapper.class );
+
+    @Mapping(target = "correlationId", source = "correlationId", defaultExpression = "java(UUID.randomUUID())")
+    NewPersonRequest createRequest(PersonDto person, UUID correlationId);
+}

processor/src/test/java/org/mapstruct/ap/test/bugs/_2023/Issue2023Test.java

@@ -0,0 +1,51 @@
+/*
+ * 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._2023;
+
+import java.util.UUID;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mapstruct.ap.testutil.IssueKey;
+import org.mapstruct.ap.testutil.WithClasses;
+import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author Filip Hrisafov
+ */
+@IssueKey("2023")
+@RunWith(AnnotationProcessorTestRunner.class)
+@WithClasses({
+    Issue2023Mapper.class,
+    NewPersonRequest.class,
+    PersonDto.class,
+})
+public class Issue2023Test {
+
+    @Test
+    public void shouldUseDefaultExpressionCorrectly() {
+        PersonDto person = new PersonDto();
+        person.setName( "John" );
+        person.setEmail( "john@doe.com" );
+
+        NewPersonRequest request = Issue2023Mapper.INSTANCE.createRequest( person, null );
+
+        assertThat( request ).isNotNull();
+        assertThat( request.getName() ).isEqualTo( "John" );
+        assertThat( request.getEmail() ).isEqualTo( "john@doe.com" );
+        assertThat( request.getCorrelationId() ).isNotNull();
+
+        UUID correlationId = UUID.randomUUID();
+        request = Issue2023Mapper.INSTANCE.createRequest( person, correlationId );
+
+        assertThat( request ).isNotNull();
+        assertThat( request.getName() ).isEqualTo( "John" );
+        assertThat( request.getEmail() ).isEqualTo( "john@doe.com" );
+        assertThat( request.getCorrelationId() ).isEqualTo( correlationId );
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/bugs/_2023/NewPersonRequest.java

@@ -0,0 +1,42 @@
+/*
+ * 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._2023;
+
+import java.util.UUID;
+
+/**
+ * @author Filip Hrisafov
+ */
+public class NewPersonRequest {
+
+    private String name;
+    private String email;
+    private UUID correlationId;
+
+    public String getName() {
+        return name;
+    }
+
+    public void setName(String name) {
+        this.name = name;
+    }
+
+    public String getEmail() {
+        return email;
+    }
+
+    public void setEmail(String email) {
+        this.email = email;
+    }
+
+    public UUID getCorrelationId() {
+        return correlationId;
+    }
+
+    public void setCorrelationId(UUID correlationId) {
+        this.correlationId = correlationId;
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/bugs/_2023/PersonDto.java

@@ -0,0 +1,31 @@
+/*
+ * 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._2023;
+
+/**
+ * @author Filip Hrisafov
+ */
+public class PersonDto {
+
+    private String name;
+    private String email;
+
+    public String getName() {
+        return name;
+    }
+
+    public void setName(String name) {
+        this.name = name;
+    }
+
+    public String getEmail() {
+        return email;
+    }
+
+    public void setEmail(String email) {
+        this.email = email;
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/constructor/manysourcearguments/ManySourceArgumentsConstructorMapper.java

@@ -0,0 +1,25 @@
+/*
+ * 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.constructor.manysourcearguments;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+import org.mapstruct.ReportingPolicy;
+import org.mapstruct.ap.test.constructor.Person;
+import org.mapstruct.factory.Mappers;
+
+/**
+ * @author Filip Hrisafov
+ */
+@Mapper(unmappedTargetPolicy = ReportingPolicy.IGNORE)
+public interface ManySourceArgumentsConstructorMapper {
+
+    ManySourceArgumentsConstructorMapper INSTANCE = Mappers.getMapper( ManySourceArgumentsConstructorMapper.class );
+
+    @Mapping(target = "name", defaultValue = "Tester")
+    @Mapping(target = "city", defaultExpression = "java(\"Zurich\")")
+    Person map(String name, String city);
+}

processor/src/test/java/org/mapstruct/ap/test/constructor/manysourcearguments/ManySourceArgumentsConstructorMappingTest.java

@@ -0,0 +1,40 @@
+/*
+ * 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.constructor.manysourcearguments;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mapstruct.ap.test.constructor.Person;
+import org.mapstruct.ap.testutil.WithClasses;
+import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author Filip Hrisafov
+ */
+@RunWith( AnnotationProcessorTestRunner.class )
+@WithClasses( {
+    ManySourceArgumentsConstructorMapper.class,
+    Person.class,
+} )
+public class ManySourceArgumentsConstructorMappingTest {
+
+    @Test
+    public void shouldCorrectlyUseDefaultValueForSourceParameters() {
+        Person person = ManySourceArgumentsConstructorMapper.INSTANCE.map( null, "Test Valley" );
+
+        assertThat( person ).isNotNull();
+        assertThat( person.getName() ).isEqualTo( "Tester" );
+        assertThat( person.getCity() ).isEqualTo( "Test Valley" );
+
+        person = ManySourceArgumentsConstructorMapper.INSTANCE.map( "Other Tester", null );
+
+        assertThat( person ).isNotNull();
+        assertThat( person.getName() ).isEqualTo( "Other Tester" );
+        assertThat( person.getCity() ).isEqualTo( "Zurich" );
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/updatemethods/manysourcearguments/ExampleMapper.java

@@ -0,0 +1,19 @@
+/*
+ * 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.updatemethods.manysourcearguments;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.MappingTarget;
+import org.mapstruct.factory.Mappers;
+
+@Mapper
+public interface ExampleMapper {
+
+    ExampleMapper INSTANCE = Mappers.getMapper( ExampleMapper.class );
+
+    void update(@MappingTarget ExampleTarget target, ExampleSource source, ExampleMember member);
+
+}

processor/src/test/java/org/mapstruct/ap/test/updatemethods/manysourcearguments/ExampleMember.java

@@ -0,0 +1,19 @@
+/*
+ * 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.updatemethods.manysourcearguments;
+
+public class ExampleMember {
+
+    private final String name;
+
+    public ExampleMember(String name) {
+        this.name = name;
+    }
+
+    public String getName() {
+        return name;
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/updatemethods/manysourcearguments/ExampleSource.java

@@ -0,0 +1,21 @@
+/*
+ * 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.updatemethods.manysourcearguments;
+
+import java.time.LocalDate;
+
+public class ExampleSource {
+
+    private final LocalDate birthday;
+
+    public ExampleSource(LocalDate birthday) {
+        this.birthday = birthday;
+    }
+
+    public LocalDate getBirthday() {
+        return birthday;
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/updatemethods/manysourcearguments/ExampleTarget.java

@@ -0,0 +1,31 @@
+/*
+ * 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.updatemethods.manysourcearguments;
+
+import java.time.LocalDate;
+
+public class ExampleTarget {
+
+    private LocalDate birthday;
+
+    private ExampleMember member;
+
+    public LocalDate getBirthday() {
+        return birthday;
+    }
+
+    public void setBirthday(LocalDate birthday) {
+        this.birthday = birthday;
+    }
+
+    public ExampleMember getMember() {
+        return member;
+    }
+
+    public void setMember(ExampleMember member) {
+        this.member = member;
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/updatemethods/manysourcearguments/ManyArgumentsUpdateMappingTest.java

@@ -0,0 +1,56 @@
+/*
+ * 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.updatemethods.manysourcearguments;
+
+import java.time.LocalDate;
+import java.time.Month;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mapstruct.ap.testutil.IssueKey;
+import org.mapstruct.ap.testutil.WithClasses;
+import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author Filip Hrisafov
+ */
+@IssueKey("2274")
+@RunWith(AnnotationProcessorTestRunner.class)
+@WithClasses({
+    ExampleMapper.class,
+    ExampleMember.class,
+    ExampleSource.class,
+    ExampleTarget.class,
+})
+public class ManyArgumentsUpdateMappingTest {
+
+    @Test
+    public void shouldUpdateToNullIfSourceParametersAreNull() {
+        ExampleTarget target = new ExampleTarget();
+        target.setBirthday( LocalDate.of( 2020, Month.NOVEMBER, 15 ) );
+        target.setMember( new ExampleMember( "test" ) );
+        ExampleMapper.INSTANCE.update(
+            target,
+            new ExampleSource( LocalDate.of( 2020, Month.AUGUST, 30 ) ),
+            null
+        );
+
+        assertThat( target.getBirthday() ).isEqualTo( LocalDate.of( 2020, Month.AUGUST, 30 ) );
+        assertThat( target.getMember() ).isNull();
+
+        ExampleMapper.INSTANCE.update(
+            target,
+            new ExampleSource( null ),
+            new ExampleMember( "second test" )
+        );
+
+        assertThat( target.getBirthday() ).isNull();
+        assertThat( target.getMember() ).isNotNull();
+        assertThat( target.getMember().getName() ).isEqualTo( "second test" );
+    }
+}