#337 refining reverse mappings for ignore and nestedproperties, remove check on reverse

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

@@ -109,10 +109,6 @@ public class EnumMappingMethod extends MappingMethod {
 
             for ( List<Mapping> 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(

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<TypeMirror> 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<TypeMirror> 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

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<Parameter> parameters,
@@ -240,7 +239,6 @@ public class SourceMethod implements Method {
 
     public void setMappings(Map<String, List<Mapping>> 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) {

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;
+    }
+
 }

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;
+    }
+
 }

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

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() {
+    public void shouldNotPropagateIgnoredPropertyInReverseMappingWhenNameIsSame() {
-        AnimalDto animalDto = new AnimalDto( "Bruno", 100, 23 );
+        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();
+    }
 }

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),

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);
+
+}

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() {
+    }
 }

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() {
+    public String getSongTitle() {
-        return title;
+        return songTitle;
     }
 
-    public void setTitle(String title) {
+    public void setSongTitle(String songTitle) {
-        this.title = title;
+        this.songTitle = songTitle;
     }
 
     public String getArtistName() {

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."),
+                    + "@InheritInverseConfiguration annotation itself.")
-            @Diagnostic(type = SourceTargetMapperErroneouslyAnnotated.class,
-                kind = Kind.ERROR,
-                line = 55,
-                messageRegExp = "Unknown property \"propertyToIgnoreDownstream\" in return type.")
         }
     )
     public void shouldUseWronglyAnnotatedError() {