#330 Keying mappings by target property name; Avoiding nulls in mapping attributes

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

@@ -92,7 +92,7 @@ public class BeanMappingMethod extends MappingMethod {
 
                 String targetPropertyName = Executables.getPropertyName( targetAccessor );
 
-                Mapping mapping = method.getMappingByTargetPropertyName( targetPropertyName );
+                Mapping mapping = method.getSingleMappingByTargetPropertyName( targetPropertyName );
 
                 if ( mapping != null && mapping.isIgnored() ) {
                     ignoredTargetProperties.add( targetPropertyName );
@@ -146,7 +146,7 @@ public class BeanMappingMethod extends MappingMethod {
                     else if ( Executables.isSetterMethod( targetAccessor )
                         || Executables.isGetterMethod( targetAccessor ) ) {
 
-                        if ( !mapping.getConstant().isEmpty() ) {
+                        if ( mapping.getConstant() != null ) {
                             // its a constant
                             PropertyMapping.ConstantMappingBuilder builder =
                                 new PropertyMapping.ConstantMappingBuilder();
@@ -160,7 +160,7 @@ public class BeanMappingMethod extends MappingMethod {
                                 .build();
                         }
 
-                        else if ( !mapping.getJavaExpression().isEmpty() ) {
+                        else if ( mapping.getJavaExpression() != null ) {
                             // its an expression
                             PropertyMapping.JavaExpressionMappingBuilder builder =
                                 new PropertyMapping.JavaExpressionMappingBuilder();

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

@@ -20,8 +20,7 @@ package org.mapstruct.ap.model;
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
+
-import java.util.Set;
 import javax.tools.Diagnostic;
 
 import org.mapstruct.ap.model.common.Parameter;
@@ -67,12 +66,11 @@ public class EnumMappingMethod extends MappingMethod {
 
             List<String> sourceEnumConstants
                     = method.getSourceParameters().iterator().next().getType().getEnumConstants();
-            Map<String, List<Mapping>> mappings = method.getMappings();
 
             for ( String enumConstant : sourceEnumConstants ) {
-                List<Mapping> mappedConstants = mappings.get( enumConstant );
+                List<Mapping> mappedConstants = method.getMappingBySourcePropertyName( enumConstant );
 
-                if ( mappedConstants == null ) {
+                if ( mappedConstants.isEmpty() ) {
                     enumMappings.add( new EnumMapping( enumConstant, enumConstant ) );
                 }
                 else if ( mappedConstants.size() == 1 ) {
@@ -172,12 +170,11 @@ public class EnumMappingMethod extends MappingMethod {
             List<String> sourceEnumConstants =
                     method.getSourceParameters().iterator().next().getType().getEnumConstants();
             List<String> targetEnumConstants = method.getReturnType().getEnumConstants();
-            Set<String> mappedSourceEnumConstants = method.getMappings().keySet();
             List<String> unmappedSourceEnumConstants = new ArrayList<String>();
 
             for ( String sourceEnumConstant : sourceEnumConstants ) {
                 if ( !targetEnumConstants.contains( sourceEnumConstant )
-                        && !mappedSourceEnumConstants.contains( sourceEnumConstant ) ) {
+                        && method.getMappingBySourcePropertyName( sourceEnumConstant ).isEmpty() ) {
                     unmappedSourceEnumConstants.add( sourceEnumConstant );
                 }
             }

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

@@ -104,7 +104,7 @@ public class PropertyMapping extends ModelElement {
         public PropertyMapping build() {
 
             // check if there's a mapping defined
-            Mapping mapping = method.getMappingByTargetPropertyName( targetPropertyName );
+            Mapping mapping = method.getSingleMappingByTargetPropertyName( targetPropertyName );
             String dateFormat = null;
             List<TypeMirror> qualifiers = null;
             String sourcePropertyName;

processor/src/main/java/org/mapstruct/ap/model/source/Mapping.java

@@ -28,8 +28,13 @@ import javax.annotation.processing.Messager;
 import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.AnnotationValue;
 import javax.lang.model.element.Element;
+import javax.lang.model.element.ElementKind;
+import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.type.DeclaredType;
+import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
 import javax.tools.Diagnostic;
+import javax.tools.Diagnostic.Kind;
 
 import org.mapstruct.ap.prism.MappingPrism;
 import org.mapstruct.ap.prism.MappingsPrism;
@@ -59,24 +64,42 @@ public class Mapping {
     private final AnnotationValue targetAnnotationValue;
 
 
-    public static Map<String, List<Mapping>> fromMappingsPrism(MappingsPrism mappingsAnnotation, Element element,
+    public static Map<String, List<Mapping>> fromMappingsPrism(MappingsPrism mappingsAnnotation,
+                                                               ExecutableElement method,
                                                                Messager messager) {
         Map<String, List<Mapping>> mappings = new HashMap<String, List<Mapping>>();
 
         for ( MappingPrism mappingPrism : mappingsAnnotation.value() ) {
-            if ( !mappings.containsKey( mappingPrism.source() ) ) {
+            Mapping mapping = fromMappingPrism( mappingPrism, method, messager );
-                mappings.put( mappingPrism.source(), new ArrayList<Mapping>() );
-            }
-            Mapping mapping = fromMappingPrism( mappingPrism, element, messager );
             if ( mapping != null ) {
-                mappings.get( mappingPrism.source() ).add( mapping );
+                List<Mapping> mappingsOfProperty = mappings.get( mappingPrism.target() );
+                if ( mappingsOfProperty == null ) {
+                    mappingsOfProperty = new ArrayList<Mapping>();
+                    mappings.put( mappingPrism.target(), mappingsOfProperty );
+                }
+
+                mappingsOfProperty.add( mapping );
+
+                if ( mappingsOfProperty.size() > 1 && !isEnumType( method.getReturnType() ) ) {
+                    messager.printMessage(
+                        Kind.ERROR,
+                        "Target property \"" + mappingPrism.target() + "\" must not be mapped more than once.",
+                        method
+                    );
+                }
             }
         }
 
         return mappings;
     }
 
-    public static Mapping fromMappingPrism(MappingPrism mappingPrism, Element element, Messager messager) {
+    private static boolean isEnumType(TypeMirror mirror) {
+        return mirror.getKind() == TypeKind.DECLARED &&
+            ( (DeclaredType) mirror ).asElement().getKind() == ElementKind.ENUM;
+    }
+
+
+    public static Mapping fromMappingPrism(MappingPrism mappingPrism, ExecutableElement element, Messager messager) {
         String[] sourceNameParts = getSourceNameParts(
             mappingPrism.source(),
             element,
@@ -84,10 +107,21 @@ public class Mapping {
             mappingPrism.values.source()
         );
 
+        if ( mappingPrism.target().isEmpty() ) {
+            messager.printMessage(
+                Diagnostic.Kind.ERROR,
+                "Target must not be empty in @Mapping",
+                element,
+                mappingPrism.mirror,
+                mappingPrism.values.target()
+            );
+            return null;
+        }
+
         if ( !mappingPrism.source().isEmpty() && !mappingPrism.constant().isEmpty() ) {
             messager.printMessage(
                 Diagnostic.Kind.ERROR,
-                "Source and constant are both defined in Mapping, either define a source or a constant",
+                "Source and constant are both defined in @Mapping, either define a source or a constant",
                 element
             );
             return null;
@@ -95,7 +129,7 @@ public class Mapping {
         else if ( !mappingPrism.source().isEmpty() && !mappingPrism.expression().isEmpty() ) {
             messager.printMessage(
                 Diagnostic.Kind.ERROR,
-                "Source and expression are both defined in Mapping, either define a source or an expression",
+                "Source and expression are both defined in @Mapping, either define a source or an expression",
                 element
             );
             return null;
@@ -103,21 +137,22 @@ public class Mapping {
         else if ( !mappingPrism.expression().isEmpty() && !mappingPrism.constant().isEmpty() ) {
             messager.printMessage(
                 Diagnostic.Kind.ERROR,
-                "Expression and constant are both defined in Mapping, either define an expression or a constant",
+                "Expression and constant are both defined in @Mapping, either define an expression or a constant",
                 element
             );
             return null;
         }
 
         String source = mappingPrism.source().isEmpty() ? null : mappingPrism.source();
+
         return new Mapping(
             source,
             sourceNameParts != null ? sourceNameParts[0] : null,
             sourceNameParts != null ? sourceNameParts[1] : source,
-            mappingPrism.constant(),
+            mappingPrism.constant().isEmpty() ? null : mappingPrism.constant(),
-            mappingPrism.expression(),
+            mappingPrism.expression().isEmpty() ? null : mappingPrism.expression(),
             mappingPrism.target(),
-            mappingPrism.dateFormat(),
+            mappingPrism.dateFormat().isEmpty() ? null : mappingPrism.dateFormat(),
             mappingPrism.qualifiedBy(),
             mappingPrism.ignore(),
             mappingPrism.mirror,
@@ -155,8 +190,13 @@ public class Mapping {
         this.sourcePropertyName = sourcePropertyName;
         this.constant = constant;
         this.expression = expression;
+        if ( expression != null ) {
             Matcher javaExpressionMatcher = JAVA_EXPRESSION.matcher( expression );
-        this.javaExpression = javaExpressionMatcher.matches() ? javaExpressionMatcher.group( 1 ).trim() : "";
+            this.javaExpression = javaExpressionMatcher.matches() ? javaExpressionMatcher.group( 1 ).trim() : null;
+        }
+        else {
+            this.javaExpression = null;
+        }
         this.targetName = targetName;
         this.dateFormat = dateFormat;
         this.qualifiers = qualifiers;
@@ -234,7 +274,7 @@ public class Mapping {
     public Mapping reverse() {
         Mapping reverse = null;
         // mapping can only be reversed if the source was not a constant nor an expression
-        if ( constant.isEmpty() && expression.isEmpty() ) {
+        if ( constant == null && expression == null ) {
             reverse = new Mapping(
                 sourceName != null ? targetName : null,
                 null,

processor/src/main/java/org/mapstruct/ap/model/source/SourceMethod.java

@@ -146,6 +146,7 @@ public class SourceMethod implements Method {
         return declaringMapper;
     }
 
+    @Override
     public ExecutableElement getExecutable() {
         return executable;
     }
@@ -212,12 +213,18 @@ public class SourceMethod implements Method {
     }
 
     /**
-     * @return the {@link Mapping}s configured for this method, keyed by source property name.
+     * @return the {@link Mapping}s configured for this method, keyed by target property name. Only for enum mapping
+     *         methods a target will be mapped by several sources.
      */
     public Map<String, List<Mapping>> getMappings() {
         return mappings;
     }
 
+    public Mapping getSingleMappingByTargetPropertyName(String targetPropertyName) {
+        List<Mapping> all = mappings.get( targetPropertyName );
+        return all != null ? all.iterator().next() : null;
+    }
+
     public void setMappings(Map<String, List<Mapping>> mappings) {
         this.mappings = mappings;
         this.configuredByReverseMappingMethod = true;
@@ -298,17 +305,20 @@ public class SourceMethod implements Method {
     }
 
     /**
-     * Returns the {@link Mapping} for the given target property. May return {@code null}.
+     * Returns the {@link Mapping}s for the given source property.
      */
-    public Mapping getMappingByTargetPropertyName(String targetPropertyName) {
+    public List<Mapping> getMappingBySourcePropertyName(String sourcePropertyName) {
-        for ( Map.Entry<String, List<Mapping>> entry : mappings.entrySet() ) {
+        List<Mapping> mappingsOfSourceProperty = new ArrayList<Mapping>();
-            for ( Mapping mapping : entry.getValue() ) {
+
-                if ( mapping.getTargetName().equals( targetPropertyName ) ) {
+        for ( List<Mapping> mappingOfProperty : mappings.values() ) {
-                    return mapping;
+            for ( Mapping mapping : mappingOfProperty ) {
+                if ( mapping.getSourcePropertyName().equals( sourcePropertyName ) ) {
+                    mappingsOfSourceProperty.add( mapping );
                 }
             }
         }
-        return null;
+
+        return mappingsOfSourceProperty;
     }
 
     public Parameter getSourceParameter(String sourceParameterName) {

processor/src/main/java/org/mapstruct/ap/processor/MapperCreationProcessor.java

@@ -361,40 +361,23 @@ public class MapperCreationProcessor implements ModelElementProcessor<List<Sourc
         }
     }
 
-    private Map<String, List<Mapping>> reverse(Map<String, List<Mapping>> mappings) {
+    private void mergeWithReverseMappings(SourceMethod forwardMappingMethod, SourceMethod method) {
-        Map<String, List<Mapping>> reversed = new HashMap<String, List<Mapping>>();
-
-        for ( List<Mapping> mappingList : mappings.values() ) {
-            for ( Mapping mapping : mappingList ) {
-
-                Mapping reverseMapping = mapping.reverse();
-                if ( reverseMapping != null ) {
-                    if ( !reversed.containsKey( mapping.getTargetName() ) ) {
-                        reversed.put( mapping.getTargetName(), new ArrayList<Mapping>() );
-                    }
-                    reversed.get( mapping.getTargetName() ).add( reverseMapping );
-                }
-            }
-        }
-        return reversed;
-    }
-
-    private void mergeWithReverseMappings(SourceMethod reverseMappingMethod, SourceMethod method) {
         Map<String, List<Mapping>> newMappings = new HashMap<String, List<Mapping>>();
-        if ( reverseMappingMethod != null && !reverseMappingMethod.getMappings().isEmpty() ) {
-            // define all the base mappings based on its forward counterpart.
-            // however, remove the mappings that are designated as constant, expression or ignore.
-            // They are characterized by the key ""
 
-            Map<String, List<Mapping>> reverseMappings = new HashMap<String, List<Mapping>>();
+        if ( forwardMappingMethod != null && !forwardMappingMethod.getMappings().isEmpty() ) {
-            reverseMappings.putAll( reverseMappingMethod.getMappings() );
+            for ( List<Mapping> mappings : forwardMappingMethod.getMappings().values() ) {
-            List<Mapping> nonSourceMappings =  method.getMappings().get( "" );
+                for ( Mapping forwardMapping : mappings ) {
-            if (nonSourceMappings != null ) {
+                    Mapping reversed = forwardMapping.reverse();
-                for (Mapping nonSourceMapping : nonSourceMappings) {
+                    if ( reversed != null ) {
-                    reverseMappings.remove( nonSourceMapping.getTargetName() );
+                        List<Mapping> mappingsOfProperty = newMappings.get( reversed.getTargetName() );
+                        if ( mappingsOfProperty == null ) {
+                            mappingsOfProperty = new ArrayList<Mapping>();
+                            newMappings.put( reversed.getTargetName(), mappingsOfProperty );
+                        }
+                        mappingsOfProperty.add( reversed );
+                    }
                 }
             }
-            newMappings.putAll( reverse( reverseMappings ) );
         }
 
         if ( method.getMappings().isEmpty() ) {

processor/src/main/java/org/mapstruct/ap/processor/MethodRetrievalProcessor.java

@@ -22,7 +22,6 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-
 import javax.annotation.processing.Messager;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ExecutableElement;
@@ -121,6 +120,7 @@ public class MethodRetrievalProcessor implements ModelElementProcessor<Void, Lis
 
     /**
      * @param executable the executable to check
+     *
      * @return <code>true</code>, iff the executable does not represent {@link java.lang.Object#equals(Object)} or an
      * overridden version of it
      */
@@ -137,6 +137,7 @@ public class MethodRetrievalProcessor implements ModelElementProcessor<Void, Lis
      * @param methods the list of already collected methods of one type hierarchy (order is from sub-types to
      * super-types)
      * @param executable the method to check
+     *
      * @return <code>true</code>, iff the given executable was not yet overridden by a method in the given list.
      */
     private boolean wasNotYetOverridden(List<SourceMethod> methods, ExecutableElement executable) {
@@ -412,7 +413,7 @@ public class MethodRetrievalProcessor implements ModelElementProcessor<Void, Lis
      *
      * @param method The method of interest
      *
-     * @return The mappings for the given method, keyed by source property name
+     * @return The mappings for the given method, keyed by target property name
      */
     private Map<String, List<Mapping>> getMappings(ExecutableElement method) {
         Map<String, List<Mapping>> mappings = new HashMap<String, List<Mapping>>();
@@ -421,12 +422,12 @@ public class MethodRetrievalProcessor implements ModelElementProcessor<Void, Lis
         MappingsPrism mappingsAnnotation = MappingsPrism.getInstanceOn( method );
 
         if ( mappingAnnotation != null ) {
-            if ( !mappings.containsKey( mappingAnnotation.source() ) ) {
+            if ( !mappings.containsKey( mappingAnnotation.target() ) ) {
-                mappings.put( mappingAnnotation.source(), new ArrayList<Mapping>() );
+                mappings.put( mappingAnnotation.target(), new ArrayList<Mapping>() );
             }
             Mapping mapping = Mapping.fromMappingPrism( mappingAnnotation, method, messager );
             if ( mapping != null ) {
-                mappings.get( mappingAnnotation.source() ).add( mapping );
+                mappings.get( mappingAnnotation.target() ).add( mapping );
             }
         }
 

processor/src/test/java/org/mapstruct/ap/test/erroneous/attributereference/ErroneousMappingsTest.java

@@ -55,6 +55,10 @@ public class ErroneousMappingsTest {
                 kind = Kind.ERROR,
                 line = 31,
                 messageRegExp = "Unknown property \"bar\" in return type"),
+            @Diagnostic(type = ErroneousMapper.class,
+                kind = Kind.ERROR,
+                line = 34,
+                messageRegExp = "Target property \"foo\" must not be mapped more than once"),
             @Diagnostic(type = ErroneousMapper.class,
                 kind = Kind.ERROR,
                 line = 32,

processor/src/test/java/org/mapstruct/ap/test/source/constants/SourceConstantsTest.java

@@ -99,7 +99,7 @@ public class SourceConstantsTest {
             @Diagnostic(type = ErroneousMapper1.class,
                 kind = Kind.ERROR,
                 line = 41,
-                messageRegExp = "Source and constant are both defined in Mapping, either define a source or a "
+                messageRegExp = "Source and constant are both defined in @Mapping, either define a source or a "
                     + "constant"),
             @Diagnostic(type = ErroneousMapper1.class,
                 kind = Kind.WARNING,
@@ -124,7 +124,7 @@ public class SourceConstantsTest {
             @Diagnostic(type = ErroneousMapper3.class,
                 kind = Kind.ERROR,
                 line = 41,
-                messageRegExp = "Expression and constant are both defined in Mapping, either define an expression or a "
+                messageRegExp = "Expression and constant are both defined in @Mapping, either define an expression or a "
                     + "constant"),
             @Diagnostic(type = ErroneousMapper3.class,
                 kind = Kind.WARNING,
@@ -149,7 +149,7 @@ public class SourceConstantsTest {
             @Diagnostic(type = ErroneousMapper4.class,
                 kind = Kind.ERROR,
                 line = 41,
-                messageRegExp = "Source and expression are both defined in Mapping, either define a source or an "
+                messageRegExp = "Source and expression are both defined in @Mapping, either define a source or an "
                     + "expression"),
             @Diagnostic(type = ErroneousMapper4.class,
                 kind = Kind.WARNING,