From 0237aba013e13745807aca7e7bca2df1f4e14cc6 Mon Sep 17 00:00:00 2001 From: sjaakd Date: Mon, 10 Nov 2014 17:40:16 +0100 Subject: [PATCH] #337 refining reverse mappings for ignore and nestedproperties, remove check on reverse --- .../mapstruct/ap/model/EnumMappingMethod.java | 4 -- .../mapstruct/ap/model/source/Mapping.java | 59 ++++++++++++++----- .../ap/model/source/SourceMethod.java | 4 -- .../org/mapstruct/ap/test/ignore/Animal.java | 13 +++- .../mapstruct/ap/test/ignore/AnimalDto.java | 13 +++- .../ap/test/ignore/AnimalMapper.java | 6 +- .../ap/test/ignore/IgnorePropertyTest.java | 24 +++++++- .../ArtistToChartEntry.java | 15 ++++- .../ArtistToChartEntryErroneous.java | 49 +++++++++++++++ .../NestedSourcePropertiesTest.java | 38 +++++++++++- .../target/ChartEntry.java | 10 ++-- .../InheritInverseConfigurationTest.java | 6 +- 12 files changed, 196 insertions(+), 45 deletions(-) create mode 100644 processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntryErroneous.java diff --git a/processor/src/main/java/org/mapstruct/ap/model/EnumMappingMethod.java b/processor/src/main/java/org/mapstruct/ap/model/EnumMappingMethod.java index e19654de1..84a5373bc 100644 --- a/processor/src/main/java/org/mapstruct/ap/model/EnumMappingMethod.java +++ b/processor/src/main/java/org/mapstruct/ap/model/EnumMappingMethod.java @@ -109,10 +109,6 @@ public class EnumMappingMethod extends MappingMethod { for ( List mappedConstants : method.getMappings().values() ) { for ( Mapping mappedConstant : mappedConstants ) { - // only report errors if this mapping is not inherited - if ( mappedConstant.isInheritedFromInverseMethod() ) { - continue; - } if ( mappedConstant.getSourceName() == null ) { ctx.getMessager().printMessage( diff --git a/processor/src/main/java/org/mapstruct/ap/model/source/Mapping.java b/processor/src/main/java/org/mapstruct/ap/model/source/Mapping.java index 66737975b..3730ac13f 100644 --- a/processor/src/main/java/org/mapstruct/ap/model/source/Mapping.java +++ b/processor/src/main/java/org/mapstruct/ap/model/source/Mapping.java @@ -38,6 +38,7 @@ import javax.tools.Diagnostic.Kind; import org.mapstruct.ap.model.common.TypeFactory; import org.mapstruct.ap.prism.MappingPrism; import org.mapstruct.ap.prism.MappingsPrism; +import org.mapstruct.ap.util.Executables; /** * Represents a property mapping as configured via {@code @Mapping}. @@ -55,7 +56,6 @@ public class Mapping { private final String dateFormat; private final List qualifiers; private final boolean isIgnored; - private final boolean isInheritedFromInverseMethod; private final AnnotationMirror mirror; private final AnnotationValue sourceAnnotationValue; @@ -148,7 +148,6 @@ public class Mapping { dateFormat, mappingPrism.qualifiedBy(), mappingPrism.ignore(), - false, mappingPrism.mirror, mappingPrism.values.source(), mappingPrism.values.target() @@ -176,10 +175,9 @@ public class Mapping { return javaExpressionMatcher.group( 1 ).trim(); } - //CHECKSTYLE:OFF private Mapping(String sourceName, String constant, String javaExpression, String targetName, String dateFormat, List qualifiers, - boolean isIgnored, boolean isInheritedFromInverseMethod, AnnotationMirror mirror, + boolean isIgnored, AnnotationMirror mirror, AnnotationValue sourceAnnotationValue, AnnotationValue targetAnnotationValue) { this.sourceName = sourceName; this.constant = constant; @@ -188,12 +186,10 @@ public class Mapping { this.dateFormat = dateFormat; this.qualifiers = qualifiers; this.isIgnored = isIgnored; - this.isInheritedFromInverseMethod = isInheritedFromInverseMethod; this.mirror = mirror; this.sourceAnnotationValue = sourceAnnotationValue; this.targetAnnotationValue = targetAnnotationValue; } - //CHECKSTYLE:ON public void init(SourceMethod method, Messager messager, TypeFactory typeFactory) { @@ -241,13 +237,6 @@ public class Mapping { return isIgnored; } - /** - * Whether this mapping originates from the inverse mapping method. - */ - public boolean isInheritedFromInverseMethod() { - return isInheritedFromInverseMethod; - } - public AnnotationMirror getMirror() { return mirror; } @@ -264,13 +253,54 @@ public class Mapping { return sourceReference; } + private boolean hasPropertyInReverseMethod( String name, SourceMethod method ) { + boolean match = false; + for ( ExecutableElement getter : method.getResultType().getGetters() ) { + if ( Executables.getPropertyName( getter ).equals( name ) ) { + match = true; + break; + } + } + for ( ExecutableElement getter : method.getResultType().getAlternativeTargetAccessors() ) { + if ( Executables.getPropertyName( getter ).equals( name ) ) { + match = true; + break; + } + } + return match; + } + public Mapping reverse(SourceMethod method, Messager messager, TypeFactory typeFactory) { - // mapping can only be reversed if the source was not a constant nor an expression + // mapping can only be reversed if the source was not a constant nor an expression nor a nested property if ( constant != null || javaExpression != null ) { return null; } + // should only ignore a property when 1) there is a sourceName defined or 2) there's a name match + if ( isIgnored ) { + if ( sourceName == null && !hasPropertyInReverseMethod( targetName, method ) ) { + return null; + } + } + + // should not reverse a nested property + if (sourceReference != null && sourceReference.getPropertyEntries().size() > 1 ) { + return null; + } + + // should generate error when parameter + if (sourceReference != null && sourceReference.getPropertyEntries().isEmpty() ) { + // parameter mapping only, apparantly the @InheritReverseConfiguration is intentional + // but erroneous. Lets raise an error to warn. + messager.printMessage( + Diagnostic.Kind.ERROR, + String.format( "Parameter %s cannot be reversed", sourceReference.getParameter() ), + method.getExecutable() + ); + return null; + } + Mapping reverse = new Mapping( sourceName != null ? targetName : null, null, // constant @@ -279,7 +309,6 @@ public class Mapping { dateFormat, qualifiers, isIgnored, - true, mirror, sourceAnnotationValue, targetAnnotationValue diff --git a/processor/src/main/java/org/mapstruct/ap/model/source/SourceMethod.java b/processor/src/main/java/org/mapstruct/ap/model/source/SourceMethod.java index a01a1fa2b..f97a46274 100644 --- a/processor/src/main/java/org/mapstruct/ap/model/source/SourceMethod.java +++ b/processor/src/main/java/org/mapstruct/ap/model/source/SourceMethod.java @@ -60,7 +60,6 @@ public class SourceMethod implements Method { private IterableMapping iterableMapping; private MapMapping mapMapping; - private boolean configuredByReverseMappingMethod = false; public static SourceMethod forMethodRequiringImplementation(ExecutableElement executable, List parameters, @@ -240,7 +239,6 @@ public class SourceMethod implements Method { public void setMappings(Map> mappings) { this.mappings = mappings; - this.configuredByReverseMappingMethod = true; } public IterableMapping getIterableMapping() { @@ -249,7 +247,6 @@ public class SourceMethod implements Method { public void setIterableMapping(IterableMapping iterableMapping) { this.iterableMapping = iterableMapping; - this.configuredByReverseMappingMethod = true; } public MapMapping getMapMapping() { @@ -258,7 +255,6 @@ public class SourceMethod implements Method { public void setMapMapping(MapMapping mapMapping) { this.mapMapping = mapMapping; - this.configuredByReverseMappingMethod = true; } public boolean reverses(SourceMethod method) { diff --git a/processor/src/test/java/org/mapstruct/ap/test/ignore/Animal.java b/processor/src/test/java/org/mapstruct/ap/test/ignore/Animal.java index d3fd00dc6..7ede3f184 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/ignore/Animal.java +++ b/processor/src/test/java/org/mapstruct/ap/test/ignore/Animal.java @@ -23,14 +23,16 @@ public class Animal { private String name; private int size; private Integer age; + private String colour; public Animal() { } - public Animal(String name, int size, Integer age) { + public Animal(String name, int size, Integer age, String colour) { this.name = name; this.size = size; this.age = age; + this.colour = colour; } public String getName() { @@ -56,4 +58,13 @@ public class Animal { public void setAge(Integer age) { this.age = age; } + + public String getColour() { + return colour; + } + + public void setColour( String colour ) { + this.colour = colour; + } + } diff --git a/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalDto.java b/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalDto.java index 3bafc18e0..ee7b22ac8 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalDto.java +++ b/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalDto.java @@ -23,15 +23,17 @@ public class AnimalDto { private String name; private Integer size; private Integer age; + private String color; public AnimalDto() { } - public AnimalDto(String name, Integer size, Integer age) { + public AnimalDto(String name, Integer size, Integer age, String color) { this.name = name; this.size = size; this.age = age; + this.color = color; } public String getName() { @@ -57,4 +59,13 @@ public class AnimalDto { public void setAge(Integer age) { this.age = age; } + + public String getColor() { + return color; + } + + public void setColor( String color ) { + this.color = color; + } + } diff --git a/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalMapper.java b/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalMapper.java index 2e071e93b..72d5d05d1 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalMapper.java +++ b/processor/src/test/java/org/mapstruct/ap/test/ignore/AnimalMapper.java @@ -21,6 +21,7 @@ package org.mapstruct.ap.test.ignore; import org.mapstruct.InheritInverseConfiguration; import org.mapstruct.Mapper; import org.mapstruct.Mapping; +import org.mapstruct.Mappings; import org.mapstruct.factory.Mappers; @Mapper @@ -28,7 +29,10 @@ public interface AnimalMapper { AnimalMapper INSTANCE = Mappers.getMapper( AnimalMapper.class ); - @Mapping(target = "age", ignore = true) + @Mappings({ + @Mapping(target = "age", ignore = true), + @Mapping(target = "color", source = "colour", ignore = true) + }) AnimalDto animalToDto(Animal animal); @InheritInverseConfiguration diff --git a/processor/src/test/java/org/mapstruct/ap/test/ignore/IgnorePropertyTest.java b/processor/src/test/java/org/mapstruct/ap/test/ignore/IgnorePropertyTest.java index 54e15190b..b36985c9c 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/ignore/IgnorePropertyTest.java +++ b/processor/src/test/java/org/mapstruct/ap/test/ignore/IgnorePropertyTest.java @@ -38,22 +38,40 @@ public class IgnorePropertyTest { @Test @IssueKey("72") public void shouldNotPropagateIgnoredPropertyGivenViaTargetAttribute() { - Animal animal = new Animal( "Bruno", 100, 23 ); + Animal animal = new Animal( "Bruno", 100, 23, "black" ); AnimalDto animalDto = AnimalMapper.INSTANCE.animalToDto( animal ); assertThat( animalDto ).isNotNull(); + assertThat( animalDto.getName() ).isEqualTo( "Bruno" ); + assertThat( animalDto.getSize() ).isEqualTo( 100 ); assertThat( animalDto.getAge() ).isNull(); + assertThat( animalDto.getColor() ).isNull(); } @Test @IssueKey("72") - public void shouldNotPropagateIgnoredPropertyInReverseMapping() { - AnimalDto animalDto = new AnimalDto( "Bruno", 100, 23 ); + public void shouldNotPropagateIgnoredPropertyInReverseMappingWhenNameIsSame() { + AnimalDto animalDto = new AnimalDto( "Bruno", 100, 23, "black" ); Animal animal = AnimalMapper.INSTANCE.animalDtoToAnimal( animalDto ); assertThat( animal ).isNotNull(); + assertThat( animalDto.getName() ).isEqualTo( "Bruno" ); + assertThat( animalDto.getSize() ).isEqualTo( 100 ); assertThat( animal.getAge() ).isNull(); } + + @Test + @IssueKey("337") + public void shouldNotPropagateIgnoredPropertyInReverseMappingWhenSourceAndTargetAreSpecified() { + AnimalDto animalDto = new AnimalDto( "Bruno", 100, 23, "black" ); + + Animal animal = AnimalMapper.INSTANCE.animalDtoToAnimal( animalDto ); + + assertThat( animal ).isNotNull(); + assertThat( animalDto.getName() ).isEqualTo( "Bruno" ); + assertThat( animalDto.getSize() ).isEqualTo( 100 ); + assertThat( animal.getColour() ).isNull(); + } } diff --git a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntry.java b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntry.java index dff1f649c..91967b309 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntry.java +++ b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntry.java @@ -18,6 +18,7 @@ */ package org.mapstruct.ap.test.nestedsourceproperties; +import org.mapstruct.InheritInverseConfiguration; import org.mapstruct.Mapper; import org.mapstruct.Mapping; import org.mapstruct.Mappings; @@ -36,7 +37,7 @@ public interface ArtistToChartEntry { @Mappings({ @Mapping(target = "chartName", source = "chart.name"), - @Mapping(target = "title", source = "song.title"), + @Mapping(target = "songTitle", source = "song.title"), @Mapping(target = "artistName", source = "song.artist.name"), @Mapping(target = "recordedAt", source = "song.artist.label.studio.name"), @Mapping(target = "city", source = "song.artist.label.studio.city"), @@ -46,7 +47,7 @@ public interface ArtistToChartEntry { @Mappings({ @Mapping(target = "chartName", ignore = true), - @Mapping(target = "title", source = "title"), + @Mapping(target = "songTitle", source = "title"), @Mapping(target = "artistName", source = "artist.name"), @Mapping(target = "recordedAt", source = "artist.label.studio.name"), @Mapping(target = "city", source = "artist.label.studio.city"), @@ -54,9 +55,17 @@ public interface ArtistToChartEntry { }) ChartEntry map(Song song); + @InheritInverseConfiguration + @Mappings({ + @Mapping(target = "artist", ignore = true), + @Mapping(target = "positions", ignore = true) + }) + Song map(ChartEntry song); + + @Mappings({ @Mapping(target = "chartName", source = "name"), - @Mapping(target = "title", ignore = true), + @Mapping(target = "songTitle", ignore = true), @Mapping(target = "artistName", ignore = true), @Mapping(target = "recordedAt", ignore = true), @Mapping(target = "city", ignore = true), diff --git a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntryErroneous.java b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntryErroneous.java new file mode 100644 index 000000000..acf8fdf53 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/ArtistToChartEntryErroneous.java @@ -0,0 +1,49 @@ +/** + * Copyright 2012-2014 Gunnar Morling (http://www.gunnarmorling.de/) + * and/or other contributors as indicated by the @authors tag. See the + * copyright.txt file in the distribution for a full listing of all + * contributors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.mapstruct.ap.test.nestedsourceproperties; + +import org.mapstruct.InheritInverseConfiguration; +import org.mapstruct.Mapper; +import org.mapstruct.Mapping; +import org.mapstruct.Mappings; +import org.mapstruct.ap.test.nestedsourceproperties.target.ChartEntry; +import org.mapstruct.factory.Mappers; + +/** + * @author Sjaak Derksen + */ +@Mapper +public interface ArtistToChartEntryErroneous { + + ArtistToChartEntryErroneous MAPPER = Mappers.getMapper( ArtistToChartEntryErroneous.class ); + + @Mappings({ + @Mapping(target = "chartName", ignore = true), + @Mapping(target = "songTitle", ignore = true), + @Mapping(target = "artistName", ignore = true), + @Mapping(target = "recordedAt", ignore = true), + @Mapping(target = "city", ignore = true), + @Mapping(target = "position", source = "position") + } ) + ChartEntry forward(Integer position); + + @InheritInverseConfiguration + Integer reverse(ChartEntry position); + +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/NestedSourcePropertiesTest.java b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/NestedSourcePropertiesTest.java index ff9be4f15..2520b58a5 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/NestedSourcePropertiesTest.java +++ b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/NestedSourcePropertiesTest.java @@ -37,6 +37,9 @@ import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner; import static org.fest.assertions.Assertions.assertThat; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import org.mapstruct.ap.testutil.compilation.annotation.CompilationResult; +import org.mapstruct.ap.testutil.compilation.annotation.Diagnostic; +import org.mapstruct.ap.testutil.compilation.annotation.ExpectedCompilationOutcome; /** * @author Sjaak Derksen @@ -74,7 +77,7 @@ public class NestedSourcePropertiesTest { assertThat( chartEntry.getCity() ).isEqualTo( "London" ); assertThat( chartEntry.getPosition() ).isEqualTo( 0 ); assertThat( chartEntry.getRecordedAt() ).isEqualTo( "Abbey Road" ); - assertThat( chartEntry.getTitle() ).isEqualTo( "A Hard Day's Night" ); + assertThat( chartEntry.getSongTitle() ).isEqualTo( "A Hard Day's Night" ); } @Test @@ -109,7 +112,7 @@ public class NestedSourcePropertiesTest { assertThat( chartEntry.getCity() ).isEqualTo( "London" ); assertThat( chartEntry.getPosition() ).isEqualTo( 1 ); assertThat( chartEntry.getRecordedAt() ).isEqualTo( "Abbey Road" ); - assertThat( chartEntry.getTitle() ).isEqualTo( "A Hard Day's Night" ); + assertThat( chartEntry.getSongTitle() ).isEqualTo( "A Hard Day's Night" ); } @Test @@ -128,7 +131,7 @@ public class NestedSourcePropertiesTest { assertThat( chartEntry.getCity() ).isNull(); assertThat( chartEntry.getPosition() ).isEqualTo( 0 ); assertThat( chartEntry.getRecordedAt() ).isNull(); - assertThat( chartEntry.getTitle() ).isNull(); + assertThat( chartEntry.getSongTitle() ).isNull(); } @Test @@ -166,4 +169,33 @@ public class NestedSourcePropertiesTest { assertFalse( AdderUsageObserver.isUsed() ); } + + + @Test + @IssueKey("337") + @WithClasses({ ArtistToChartEntry.class }) + public void reverseShouldIgnoreNestedProperties() { + + ChartEntry entry = new ChartEntry(); + entry.setSongTitle( "Another brick in the wall" ); + + Song song = ArtistToChartEntry.MAPPER.map( entry ); + assertThat( song ).isNotNull(); + assertThat( song.getTitle() ).isEqualTo( "Another brick in the wall" ); + } + + @Test + @IssueKey( "337" ) + @ExpectedCompilationOutcome( + value = CompilationResult.FAILED, + diagnostics = { + @Diagnostic( type = ArtistToChartEntryErroneous.class, + kind = javax.tools.Diagnostic.Kind.ERROR, + line = 47, + messageRegExp = "Parameter java.lang.Integer position cannot be reversed" ) + } + ) + @WithClasses({ ArtistToChartEntryErroneous.class }) + public void reverseShouldIgnoreParameter() { + } } diff --git a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/target/ChartEntry.java b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/target/ChartEntry.java index 5477742fa..0b64b926c 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/target/ChartEntry.java +++ b/processor/src/test/java/org/mapstruct/ap/test/nestedsourceproperties/target/ChartEntry.java @@ -24,7 +24,7 @@ package org.mapstruct.ap.test.nestedsourceproperties.target; public class ChartEntry { private String chartName; - private String title; + private String songTitle; private String artistName; private String recordedAt; private String city; @@ -38,12 +38,12 @@ public class ChartEntry { this.chartName = chartName; } - public String getTitle() { - return title; + public String getSongTitle() { + return songTitle; } - public void setTitle(String title) { - this.title = title; + public void setSongTitle(String songTitle) { + this.songTitle = songTitle; } public String getArtistName() { diff --git a/processor/src/test/java/org/mapstruct/ap/test/reverse/InheritInverseConfigurationTest.java b/processor/src/test/java/org/mapstruct/ap/test/reverse/InheritInverseConfigurationTest.java index ab2a27b7a..9b5b75b00 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/reverse/InheritInverseConfigurationTest.java +++ b/processor/src/test/java/org/mapstruct/ap/test/reverse/InheritInverseConfigurationTest.java @@ -128,11 +128,7 @@ public class InheritInverseConfigurationTest { kind = Kind.ERROR, line = 49, messageRegExp = "Resolved inverse mapping method reverse\\(\\) should not carry the " - + "@InheritInverseConfiguration annotation itself."), - @Diagnostic(type = SourceTargetMapperErroneouslyAnnotated.class, - kind = Kind.ERROR, - line = 55, - messageRegExp = "Unknown property \"propertyToIgnoreDownstream\" in return type.") + + "@InheritInverseConfiguration annotation itself.") } ) public void shouldUseWronglyAnnotatedError() {