#153 Use clear() / addAll(..) for collection properties in @MappingTarget mappings

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

@@ -43,6 +43,7 @@ public class PropertyMapping extends ModelElement {
     private final String targetAccessorName;
     private final Type targetType;
     private final boolean isTargetAccessorSetter;
+    private final String targetReadAccessorName;
 
     private final MethodReference mappingMethod;
     private final TypeConversion conversion;
@@ -60,6 +61,8 @@ public class PropertyMapping extends ModelElement {
         this.targetAccessorName = targetAccessorName;
         this.targetType = targetType;
         this.isTargetAccessorSetter = targetAccessorName.startsWith( "set" );
+        this.targetReadAccessorName =
+            this.isTargetAccessorSetter ? "get" + targetAccessorName.substring( 3 ) : targetAccessorName;
 
         this.mappingMethod = mappingMethod;
         this.conversion = conversion;
@@ -112,6 +115,13 @@ public class PropertyMapping extends ModelElement {
         return isTargetAccessorSetter;
     }
 
+    /**
+     * @return the read-accessor for the target property (i.e. the getter method)
+     */
+    public String getTargetReadAccessorName() {
+        return targetReadAccessorName;
+    }
+
     @Override
     public Set<Type> getImportTypes() {
         Set<Type> importTypes = new HashSet<Type>();

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

@@ -29,13 +29,13 @@
         <#list sourceParameters as sourceParam>
     if ( ${sourceParam.name} != null ) {
             <#list propertyMappingsByParameter[sourceParam.name] as propertyMapping>
-                <@includeModel object=propertyMapping targetBeanName=resultName/>
+                <@includeModel object=propertyMapping targetBeanName=resultName existingInstanceMapping=existingInstanceMapping/>
             </#list>
     }
         </#list>
     <#else>
         <#list propertyMappingsByParameter[sourceParameters[0].name] as propertyMapping>
-            <@includeModel object=propertyMapping targetBeanName=resultName/>
+            <@includeModel object=propertyMapping targetBeanName=resultName existingInstanceMapping=existingInstanceMapping/>
         </#list>
     </#if>
     <#if returnType.name != "void">

processor/src/main/resources/org.mapstruct.ap.model.PropertyMapping.ftl

@@ -20,13 +20,17 @@
 -->
 <#-- a) invoke mapping method -->
 <#if mappingMethod??>
-<#if targetAccessorSetter>
+    <@assignResult
-${ext.targetBeanName}.${targetAccessorName}( <@includeModel object=mappingMethod input="${sourceBeanName}.${sourceAccessorName}()" targetType="${targetType.name}"/> );
+        existingInstanceMapping=ext.existingInstanceMapping
-<#elseif targetType.collectionType>
+        targetAccessorSetter=targetAccessorSetter
-if ( ${sourceBeanName}.${sourceAccessorName}() != null && ${ext.targetBeanName}.${targetAccessorName}() != null ) {
+        targetType=targetType
-    ${ext.targetBeanName}.${targetAccessorName}().addAll( <@includeModel object=mappingMethod input="${sourceBeanName}.${sourceAccessorName}()" targetType="${targetType.name}"/> );
+        targetBeanName=ext.targetBeanName
-}
+        targetReadAccessorName=targetReadAccessorName
-</#if>
+        targetAccessorName=targetAccessorName
+        sourceBeanName=sourceBeanName
+        sourceAccessorName=sourceAccessorName><#compress>
+            <@includeModel object=mappingMethod input="${sourceBeanName}.${sourceAccessorName}()" targetType=targetType.name/>
+    </#compress></@assignResult>
 <#-- b) simple conversion -->
 <#elseif conversion??>
     <#if sourceType.primitive == false>
@@ -38,20 +42,50 @@ if ( ${sourceBeanName}.${sourceAccessorName}() != null ) {
     </#if>
 <#-- c) simply set -->
 <#else>
-    <#if targetType.collectionType || targetType.mapType>
+    <@assignResult
-        <#if targetAccessorSetter>
+        existingInstanceMapping=ext.existingInstanceMapping
+        targetAccessorSetter=targetAccessorSetter
+        targetType=targetType
+        targetBeanName=ext.targetBeanName
+        targetReadAccessorName=targetReadAccessorName
+        targetAccessorName=targetAccessorName
+        sourceBeanName=sourceBeanName
+        sourceAccessorName=sourceAccessorName
+        ; use_plain><#compress>
+        <#if use_plain>
+            ${sourceBeanName}.${sourceAccessorName}()
+        <#else>
+            new <#if targetType.implementationType??><@includeModel object=targetType.implementationType/><#else><@includeModel object=targetType/></#if>( ${sourceBeanName}.${sourceAccessorName}() )
+        </#if>
+    </#compress></@assignResult>
+</#if>
+<#macro assignResult existingInstanceMapping targetAccessorSetter targetType targetBeanName targetReadAccessorName targetAccessorName sourceBeanName sourceAccessorName>
+    <#if ( existingInstanceMapping || !targetAccessorSetter ) && ( targetType.collectionType || targetType.mapType ) >
+        if ( ${targetBeanName}.${targetReadAccessorName}() != null ) {
+            <#if existingInstanceMapping>
+                ${targetBeanName}.${targetReadAccessorName}().clear();
+            </#if><#t>
             if ( ${sourceBeanName}.${sourceAccessorName}() != null ) {
-    ${ext.targetBeanName}.${targetAccessorName}( new <#if targetType.implementationType??><@includeModel object=targetType.implementationType/><#else><@includeModel object=targetType/></#if>( ${sourceBeanName}.${sourceAccessorName}() ) );
+                <#if targetType.collectionType>
-}
+                    ${targetBeanName}.${targetReadAccessorName}().addAll( <#nested true> );
                 <#else>
-if ( ${sourceBeanName}.${sourceAccessorName}() != null && ${ext.targetBeanName}.${targetAccessorName}() != null ) {
+                    ${targetBeanName}.${targetReadAccessorName}().putAll( <#nested true> );
-    ${ext.targetBeanName}.${targetAccessorName}().addAll( ${sourceBeanName}.${sourceAccessorName}() );
+                </#if>
+            }
+        }<#if targetAccessorSetter> else if ( ${sourceBeanName}.${sourceAccessorName}() != null ) {
+            ${targetBeanName}.${targetAccessorName}( <#nested false> );
         }
         </#if>
+    <#elseif targetAccessorSetter>
+        <#if targetType.collectionType || targetType.mapType>
+        if ( ${sourceBeanName}.${sourceAccessorName}() != null ) {
+            ${targetBeanName}.${targetAccessorName}( <#nested false> );
+        }
         <#else>
-${ext.targetBeanName}.${targetAccessorName}( ${sourceBeanName}.${sourceAccessorName}() );
+            ${targetBeanName}.${targetAccessorName}( <#nested true> );
         </#if>
     </#if>
+</#macro>
 <#macro applyConversion targetBeanName targetAccessorName conversion>
     <#if (conversion.exceptionTypes?size == 0) >
         ${targetBeanName}.${targetAccessorName}( <@includeModel object=conversion/> );

processor/src/test/java/org/mapstruct/ap/test/collection/CollectionMappingTest.java

@@ -18,11 +18,14 @@
  */
 package org.mapstruct.ap.test.collection;
 
+import static org.fest.assertions.Assertions.assertThat;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -31,8 +34,6 @@ import org.mapstruct.ap.testutil.MapperTestBase;
 import org.mapstruct.ap.testutil.WithClasses;
 import org.testng.annotations.Test;
 
-import static org.fest.assertions.Assertions.assertThat;
-
 @WithClasses({ Source.class, Target.class, Colour.class, SourceTargetMapper.class })
 public class CollectionMappingTest extends MapperTestBase {
 
@@ -106,6 +107,26 @@ public class CollectionMappingTest extends MapperTestBase {
         assertThat( source.getStringList() ).containsExactly( "Bob", "Alice" );
     }
 
+    @Test
+    @IssueKey( "153" )
+    public void shouldMapListWithClearAndAddAll() {
+        Source source = new Source();
+        source.setOtherStringList( Arrays.asList( "Bob", "Alice" ) );
+
+        Target target = SourceTargetMapper.INSTANCE.sourceToTarget( source );
+        target.getOtherStringList().add( "Bill" );
+
+        assertThat( source.getOtherStringList() ).containsExactly( "Bob", "Alice" );
+
+        source.setOtherStringList( Arrays.asList( "Bob" ) );
+        List<String> originalInstance = target.getOtherStringList();
+
+        SourceTargetMapper.INSTANCE.sourceToTarget( source, target );
+
+        assertThat( target.getOtherStringList() ).isSameAs( originalInstance );
+        assertThat( target.getOtherStringList() ).containsExactly( "Bob" );
+    }
+
     @Test
     @IssueKey("6")
     public void shouldReverseMapListAsCopy() {
@@ -299,6 +320,32 @@ public class CollectionMappingTest extends MapperTestBase {
         target.getStringLongMap().put( "Bill", 789L );
 
         assertThat( source.getStringLongMap() ).hasSize( 2 );
+        assertThat( target.getStringLongMap() ).hasSize( 3 );
+    }
+
+    @Test
+    @IssueKey( "153" )
+    public void shouldMapMapWithClearAndPutAll() {
+        Source source = new Source();
+
+        Map<String, Long> map = new HashMap<String, Long>();
+        map.put( "Bob", 123L );
+        map.put( "Alice", 456L );
+        source.setOtherStringLongMap( map );
+
+        Target target = SourceTargetMapper.INSTANCE.sourceToTarget( source );
+        target.getOtherStringLongMap().put( "Bill", 789L );
+
+        assertThat( source.getOtherStringLongMap() ).hasSize( 2 );
+        assertThat( target.getOtherStringLongMap() ).hasSize( 3 );
+
+        source.getOtherStringLongMap().remove( "Alice" );
+        Map<String, Long> originalInstance = target.getOtherStringLongMap();
+
+        SourceTargetMapper.INSTANCE.sourceToTarget( source, target );
+
+        assertThat( target.getOtherStringLongMap() ).isSameAs( originalInstance );
+        assertThat( target.getOtherStringLongMap() ).hasSize( 1 );
     }
 
     @Test

processor/src/test/java/org/mapstruct/ap/test/collection/Source.java

@@ -28,6 +28,7 @@ import java.util.Set;
 public class Source {
 
     private List<String> stringList;
+    private List<String> otherStringList;
     private ArrayList<String> stringArrayList;
 
     private Set<String> stringSet;
@@ -45,6 +46,8 @@ public class Source {
 
     private Map<String, Long> stringLongMap;
 
+    private Map<String, Long> otherStringLongMap;
+
     private List<String> stringList2;
 
     public List<String> getStringList() {
@@ -135,4 +138,20 @@ public class Source {
         this.stringList2 = stringList2;
     }
 
+    public List<String> getOtherStringList() {
+        return otherStringList;
+    }
+
+    public void setOtherStringList(List<String> otherStringList) {
+        this.otherStringList = otherStringList;
+    }
+
+    public Map<String, Long> getOtherStringLongMap() {
+        return otherStringLongMap;
+    }
+
+    public void setOtherStringLongMap(Map<String, Long> otherStringLongMap) {
+        this.otherStringLongMap = otherStringLongMap;
+    }
+
 }

processor/src/test/java/org/mapstruct/ap/test/collection/SourceTargetMapper.java

@@ -22,6 +22,7 @@ import java.util.Set;
 
 import org.mapstruct.Mapper;
 import org.mapstruct.Mapping;
+import org.mapstruct.MappingTarget;
 import org.mapstruct.Mappings;
 import org.mapstruct.factory.Mappers;
 
@@ -40,6 +41,8 @@ public interface SourceTargetMapper {
 
     Source targetToSource(Target target);
 
+    Target sourceToTarget(Source source, @MappingTarget Target target);
+
     Set<String> integerSetToStringSet(Set<Integer> integers);
 
     Set<Integer> stringSetToIntegerSet(Set<String> strings);

processor/src/test/java/org/mapstruct/ap/test/collection/Target.java

@@ -25,9 +25,13 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+
 public class Target {
 
     private List<String> stringList;
+    private List<String> otherStringList;
     private ArrayList<String> stringArrayList;
 
     private Set<String> stringSet;
@@ -42,12 +46,20 @@ public class Target {
     private Set<String> colours;
 
     private Map<String, Long> stringLongMap;
+    private Map<String, Long> otherStringLongMap;
 
     private List<String> stringListNoSetter;
 
     @SuppressWarnings("rawtypes")
     private Set set;
 
+    public Target() {
+        otherStringLongMap = Maps.newHashMap();
+        otherStringLongMap.put( "not-present-after-mapping", 42L );
+
+        otherStringList = Lists.newArrayList( "not-present-after-mapping" );
+    }
+
     public List<String> getStringList() {
         return stringList;
     }
@@ -137,4 +149,20 @@ public class Target {
         return stringListNoSetter;
     }
 
+    public Map<String, Long> getOtherStringLongMap() {
+        return otherStringLongMap;
+    }
+
+    public void setOtherStringLongMap(Map<String, Long> otherStringLongMap) {
+        this.otherStringLongMap = otherStringLongMap;
+    }
+
+    public List<String> getOtherStringList() {
+        return otherStringList;
+    }
+
+    public void setOtherStringList(List<String> otherStringList) {
+        this.otherStringList = otherStringList;
+    }
+
 }