From 84902318f92f8b69a51b3bf0663fa58ae22c1bff Mon Sep 17 00:00:00 2001 From: Andreas Gudian Date: Thu, 27 Feb 2014 21:10:17 +0100 Subject: [PATCH] #153 Use clear() / addAll(..) for collection properties in @MappingTarget mappings --- .../mapstruct/ap/model/PropertyMapping.java | 10 +++ ...g.mapstruct.ap.model.BeanMappingMethod.ftl | 4 +- ...org.mapstruct.ap.model.PropertyMapping.ftl | 78 +++++++++++++------ .../collection/CollectionMappingTest.java | 51 +++++++++++- .../mapstruct/ap/test/collection/Source.java | 19 +++++ .../test/collection/SourceTargetMapper.java | 3 + .../mapstruct/ap/test/collection/Target.java | 28 +++++++ 7 files changed, 167 insertions(+), 26 deletions(-) diff --git a/processor/src/main/java/org/mapstruct/ap/model/PropertyMapping.java b/processor/src/main/java/org/mapstruct/ap/model/PropertyMapping.java index b19dd69f9..f59dd203e 100644 --- a/processor/src/main/java/org/mapstruct/ap/model/PropertyMapping.java +++ b/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 getImportTypes() { Set importTypes = new HashSet(); diff --git a/processor/src/main/resources/org.mapstruct.ap.model.BeanMappingMethod.ftl b/processor/src/main/resources/org.mapstruct.ap.model.BeanMappingMethod.ftl index 478941c05..70af263fc 100644 --- a/processor/src/main/resources/org.mapstruct.ap.model.BeanMappingMethod.ftl +++ b/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/> } <#else> <#list propertyMappingsByParameter[sourceParameters[0].name] as propertyMapping> - <@includeModel object=propertyMapping targetBeanName=resultName/> + <@includeModel object=propertyMapping targetBeanName=resultName existingInstanceMapping=existingInstanceMapping/> <#if returnType.name != "void"> diff --git a/processor/src/main/resources/org.mapstruct.ap.model.PropertyMapping.ftl b/processor/src/main/resources/org.mapstruct.ap.model.PropertyMapping.ftl index 759ad5a56..c0ad6846b 100644 --- a/processor/src/main/resources/org.mapstruct.ap.model.PropertyMapping.ftl +++ b/processor/src/main/resources/org.mapstruct.ap.model.PropertyMapping.ftl @@ -20,38 +20,72 @@ --> <#-- a) invoke mapping method --> <#if mappingMethod??> -<#if targetAccessorSetter> -${ext.targetBeanName}.${targetAccessorName}( <@includeModel object=mappingMethod input="${sourceBeanName}.${sourceAccessorName}()" targetType="${targetType.name}"/> ); -<#elseif targetType.collectionType> -if ( ${sourceBeanName}.${sourceAccessorName}() != null && ${ext.targetBeanName}.${targetAccessorName}() != null ) { - ${ext.targetBeanName}.${targetAccessorName}().addAll( <@includeModel object=mappingMethod input="${sourceBeanName}.${sourceAccessorName}()" targetType="${targetType.name}"/> ); -} - + <@assignResult + existingInstanceMapping=ext.existingInstanceMapping + targetAccessorSetter=targetAccessorSetter + targetType=targetType + targetBeanName=ext.targetBeanName + targetReadAccessorName=targetReadAccessorName + targetAccessorName=targetAccessorName + sourceBeanName=sourceBeanName + sourceAccessorName=sourceAccessorName><#compress> + <@includeModel object=mappingMethod input="${sourceBeanName}.${sourceAccessorName}()" targetType=targetType.name/> + <#-- b) simple conversion --> <#elseif conversion??> <#if sourceType.primitive == false> -if ( ${sourceBeanName}.${sourceAccessorName}() != null ) { - <@applyConversion targetBeanName=ext.targetBeanName targetAccessorName=targetAccessorName conversion=conversion/> -} + if ( ${sourceBeanName}.${sourceAccessorName}() != null ) { + <@applyConversion targetBeanName=ext.targetBeanName targetAccessorName=targetAccessorName conversion=conversion/> + } <#else> -<@applyConversion targetBeanName=ext.targetBeanName targetAccessorName=targetAccessorName conversion=conversion/> + <@applyConversion targetBeanName=ext.targetBeanName targetAccessorName=targetAccessorName conversion=conversion/> <#-- c) simply set --> <#else> - <#if targetType.collectionType || targetType.mapType> - <#if targetAccessorSetter> -if ( ${sourceBeanName}.${sourceAccessorName}() != null ) { - ${ext.targetBeanName}.${targetAccessorName}( new <#if targetType.implementationType??><@includeModel object=targetType.implementationType/><#else><@includeModel object=targetType/>( ${sourceBeanName}.${sourceAccessorName}() ) ); -} + <@assignResult + 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> -if ( ${sourceBeanName}.${sourceAccessorName}() != null && ${ext.targetBeanName}.${targetAccessorName}() != null ) { - ${ext.targetBeanName}.${targetAccessorName}().addAll( ${sourceBeanName}.${sourceAccessorName}() ); -} + new <#if targetType.implementationType??><@includeModel object=targetType.implementationType/><#else><@includeModel object=targetType/>( ${sourceBeanName}.${sourceAccessorName}() ) - <#else> -${ext.targetBeanName}.${targetAccessorName}( ${sourceBeanName}.${sourceAccessorName}() ); - + +<#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(); + <#t> + if ( ${sourceBeanName}.${sourceAccessorName}() != null ) { + <#if targetType.collectionType> + ${targetBeanName}.${targetReadAccessorName}().addAll( <#nested true> ); + <#else> + ${targetBeanName}.${targetReadAccessorName}().putAll( <#nested true> ); + + } + }<#if targetAccessorSetter> else if ( ${sourceBeanName}.${sourceAccessorName}() != null ) { + ${targetBeanName}.${targetAccessorName}( <#nested false> ); + } + + <#elseif targetAccessorSetter> + <#if targetType.collectionType || targetType.mapType> + if ( ${sourceBeanName}.${sourceAccessorName}() != null ) { + ${targetBeanName}.${targetAccessorName}( <#nested false> ); + } + <#else> + ${targetBeanName}.${targetAccessorName}( <#nested true> ); + + + <#macro applyConversion targetBeanName targetAccessorName conversion> <#if (conversion.exceptionTypes?size == 0) > ${targetBeanName}.${targetAccessorName}( <@includeModel object=conversion/> ); diff --git a/processor/src/test/java/org/mapstruct/ap/test/collection/CollectionMappingTest.java b/processor/src/test/java/org/mapstruct/ap/test/collection/CollectionMappingTest.java index b634f245a..beb88024f 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/collection/CollectionMappingTest.java +++ b/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 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 map = new HashMap(); + 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 originalInstance = target.getOtherStringLongMap(); + + SourceTargetMapper.INSTANCE.sourceToTarget( source, target ); + + assertThat( target.getOtherStringLongMap() ).isSameAs( originalInstance ); + assertThat( target.getOtherStringLongMap() ).hasSize( 1 ); } @Test diff --git a/processor/src/test/java/org/mapstruct/ap/test/collection/Source.java b/processor/src/test/java/org/mapstruct/ap/test/collection/Source.java index 78676394c..f3ddf76b7 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/collection/Source.java +++ b/processor/src/test/java/org/mapstruct/ap/test/collection/Source.java @@ -28,6 +28,7 @@ import java.util.Set; public class Source { private List stringList; + private List otherStringList; private ArrayList stringArrayList; private Set stringSet; @@ -45,6 +46,8 @@ public class Source { private Map stringLongMap; + private Map otherStringLongMap; + private List stringList2; public List getStringList() { @@ -135,4 +138,20 @@ public class Source { this.stringList2 = stringList2; } + public List getOtherStringList() { + return otherStringList; + } + + public void setOtherStringList(List otherStringList) { + this.otherStringList = otherStringList; + } + + public Map getOtherStringLongMap() { + return otherStringLongMap; + } + + public void setOtherStringLongMap(Map otherStringLongMap) { + this.otherStringLongMap = otherStringLongMap; + } + } diff --git a/processor/src/test/java/org/mapstruct/ap/test/collection/SourceTargetMapper.java b/processor/src/test/java/org/mapstruct/ap/test/collection/SourceTargetMapper.java index 2ff7368cc..96f150a0c 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/collection/SourceTargetMapper.java +++ b/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 integerSetToStringSet(Set integers); Set stringSetToIntegerSet(Set strings); diff --git a/processor/src/test/java/org/mapstruct/ap/test/collection/Target.java b/processor/src/test/java/org/mapstruct/ap/test/collection/Target.java index 0efcf66f4..fb6546b61 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/collection/Target.java +++ b/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 stringList; + private List otherStringList; private ArrayList stringArrayList; private Set stringSet; @@ -42,12 +46,20 @@ public class Target { private Set colours; private Map stringLongMap; + private Map otherStringLongMap; private List 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 getStringList() { return stringList; } @@ -137,4 +149,20 @@ public class Target { return stringListNoSetter; } + public Map getOtherStringLongMap() { + return otherStringLongMap; + } + + public void setOtherStringLongMap(Map otherStringLongMap) { + this.otherStringLongMap = otherStringLongMap; + } + + public List getOtherStringList() { + return otherStringList; + } + + public void setOtherStringList(List otherStringList) { + this.otherStringList = otherStringList; + } + }