From cf19a6b637e2570580b32a94b357e0f31559c1de Mon Sep 17 00:00:00 2001
From: Filip Hrisafov <filip.hrisafov@gmail.com>
Date: Sat, 28 Apr 2018 09:09:44 +0200
Subject: [PATCH] #1423 Updating types that have a builder should be allowed

It is possible that a type has both a builder and accessors.
In such case doing an update to this type should be allowed
---
 .../ap/internal/model/BeanMappingMethod.java  | 13 +++-
 .../ap/internal/model/PropertyMapping.java    |  6 +-
 .../model/source/TargetReference.java         | 22 +++++-
 .../processor/MethodRetrievalProcessor.java   |  5 --
 .../mapstruct/ap/internal/util/Message.java   |  1 -
 .../simple/BuilderInfoTargetTest.java         | 55 +++++++++----
 .../simple/ErroneousSimpleBuilderMapper.java  | 27 -------
 .../mappingTarget/simple/MutableTarget.java   | 78 +++++++++++++++++++
 .../simple/SimpleBuilderMapper.java           |  9 +++
 9 files changed, 162 insertions(+), 54 deletions(-)
 delete mode 100644 processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/ErroneousSimpleBuilderMapper.java
 create mode 100644 processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/MutableTarget.java

diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java b/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
index 4886e475e..059a1c7ba 100644
--- a/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
+++ b/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
@@ -115,8 +115,12 @@ public class BeanMappingMethod extends NormalTypeMappingMethod {
             this.method = sourceMethod;
             this.methodMappings = sourceMethod.getMappingOptions().getMappings();
             CollectionMappingStrategyPrism cms = sourceMethod.getMapperConfiguration().getCollectionMappingStrategy();
-            Map<String, Accessor> accessors = method.getResultType()
-                .getEffectiveType()
+            Type mappingType = method.getResultType();
+            if ( !method.isUpdateMethod() ) {
+                mappingType = mappingType.getEffectiveType();
+            }
+
+            Map<String, Accessor> accessors = mappingType
                 .getPropertyWriteAccessors( cms );
             this.targetProperties = accessors.keySet();
 
@@ -885,7 +889,10 @@ public class BeanMappingMethod extends NormalTypeMappingMethod {
             types.addAll( propertyMapping.getImportTypes() );
         }
 
-        types.add( getResultType().getEffectiveType() );
+        if ( !isExistingInstanceMapping() ) {
+            types.addAll( getResultType().getEffectiveType().getImportTypes() );
+        }
+
 
         return types;
     }
diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java b/processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java
index 3ed0abaf6..cb48c23d5 100644
--- a/processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java
+++ b/processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java
@@ -155,7 +155,11 @@ public class PropertyMapping extends ModelElement {
 
         private Type determineTargetType() {
             // This is a bean mapping method, so we know the result is a declared type
-            DeclaredType resultType = (DeclaredType) method.getResultType().getEffectiveType().getTypeMirror();
+            Type mappingType = method.getResultType();
+            if ( !method.isUpdateMethod() ) {
+                mappingType = mappingType.getEffectiveType();
+            }
+            DeclaredType resultType = (DeclaredType) mappingType.getTypeMirror();
 
             switch ( targetWriteAccessorType ) {
                 case ADDER:
diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java b/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java
index fc9da4869..5b48d54cc 100644
--- a/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java
+++ b/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java
@@ -153,7 +153,7 @@ public class TargetReference {
 
             boolean foundEntryMatch;
             Type resultType = method.getResultType();
-            resultType = resultType.getEffectiveType();
+            resultType = typeBasedOnMethod( resultType );
 
             // there can be 4 situations
             // 1. Return type
@@ -191,7 +191,7 @@ public class TargetReference {
             // last entry
             for ( int i = 0; i < entryNames.length; i++ ) {
 
-                Type mappingType = nextType.getEffectiveType();
+                Type mappingType = typeBasedOnMethod( nextType );
                 Accessor targetReadAccessor = mappingType.getPropertyReadAccessors().get( entryNames[i] );
                 Accessor targetWriteAccessor = mappingType.getPropertyWriteAccessors( cms ).get( entryNames[i] );
                 boolean isLast = i == entryNames.length - 1;
@@ -237,13 +237,13 @@ public class TargetReference {
             if ( Executables.isGetterMethod( toUse ) ||
                 Executables.isFieldAccessor( toUse ) ) {
                 nextType = typeFactory.getReturnType(
-                    (DeclaredType) initial.getEffectiveType().getTypeMirror(),
+                    (DeclaredType) typeBasedOnMethod( initial ).getTypeMirror(),
                     toUse
                 );
             }
             else {
                 nextType = typeFactory.getSingleParameter(
-                    (DeclaredType) initial.getEffectiveType().getTypeMirror(),
+                    (DeclaredType) typeBasedOnMethod( initial ).getTypeMirror(),
                     toUse
                 ).getType();
             }
@@ -264,6 +264,20 @@ public class TargetReference {
             }
         }
 
+        /**
+         * When we are in an update method, i.e. source parameter with {@code @MappingTarget} then the type should
+         * be itself, otherwise, we always get the effective type. The reason is that when doing updates we always
+         * search for setters and getters within the updating type.
+         */
+        private Type typeBasedOnMethod(Type type) {
+            if ( method.isUpdateMethod() ) {
+                return type;
+            }
+            else {
+                return type.getEffectiveType();
+            }
+        }
+
         /**
          * A write accessor is not valid if it is {@code null} and it is not last. i.e. for nested target mappings
          * there must be a write accessor for all entries except the last one.
diff --git a/processor/src/main/java/org/mapstruct/ap/internal/processor/MethodRetrievalProcessor.java b/processor/src/main/java/org/mapstruct/ap/internal/processor/MethodRetrievalProcessor.java
index 6dfed43c5..19e3d538f 100644
--- a/processor/src/main/java/org/mapstruct/ap/internal/processor/MethodRetrievalProcessor.java
+++ b/processor/src/main/java/org/mapstruct/ap/internal/processor/MethodRetrievalProcessor.java
@@ -403,11 +403,6 @@ public class MethodRetrievalProcessor implements ModelElementProcessor<Void, Lis
             return false;
         }
 
-        if ( targetParameter != null && targetParameter.getType().getBuilderType() != null  ) {
-            messager.printMessage( method, Message.RETRIEVAL_IMMUTABLE_TARGET );
-            return false;
-        }
-
         if ( isVoid( resultType ) ) {
             messager.printMessage( method, Message.RETRIEVAL_VOID_MAPPING_METHOD );
             return false;
diff --git a/processor/src/main/java/org/mapstruct/ap/internal/util/Message.java b/processor/src/main/java/org/mapstruct/ap/internal/util/Message.java
index fb1bdfcb9..bc5f8dfc0 100644
--- a/processor/src/main/java/org/mapstruct/ap/internal/util/Message.java
+++ b/processor/src/main/java/org/mapstruct/ap/internal/util/Message.java
@@ -101,7 +101,6 @@ public enum Message {
 
     RETRIEVAL_NO_INPUT_ARGS( "Can't generate mapping method with no input arguments." ),
     RETRIEVAL_DUPLICATE_MAPPING_TARGETS( "Can't generate mapping method with more than one @MappingTarget parameter." ),
-    RETRIEVAL_IMMUTABLE_TARGET( "Can't generate mapping method when @MappingTarget is supposed to be immutable (has a builder)." ),
     RETRIEVAL_VOID_MAPPING_METHOD( "Can't generate mapping method with return type void." ),
     RETRIEVAL_NON_ASSIGNABLE_RESULTTYPE( "The result type is not assignable to the the return type." ),
     RETRIEVAL_ITERABLE_TO_NON_ITERABLE( "Can't generate mapping method from iterable type to non-iterable type." ),
diff --git a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/BuilderInfoTargetTest.java b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/BuilderInfoTargetTest.java
index fa8c7d9c8..3a0d6fa5d 100644
--- a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/BuilderInfoTargetTest.java
+++ b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/BuilderInfoTargetTest.java
@@ -26,11 +26,9 @@ import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
 import org.mapstruct.ap.testutil.runner.GeneratedSource;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import org.mapstruct.ap.testutil.compilation.annotation.CompilationResult;
-import org.mapstruct.ap.testutil.compilation.annotation.Diagnostic;
-import org.mapstruct.ap.testutil.compilation.annotation.ExpectedCompilationOutcome;
 
 @WithClasses({
+    MutableTarget.class,
     SimpleMutableSource.class,
     SimpleImmutableTarget.class,
     SimpleBuilderMapper.class
@@ -54,17 +52,48 @@ public class BuilderInfoTargetTest {
         assertThat( targetObject.getName() ).isEqualTo( "Bob" );
     }
 
-    @WithClasses(ErroneousSimpleBuilderMapper.class)
     @Test
-    @ExpectedCompilationOutcome(value = CompilationResult.FAILED,
-        diagnostics = {
-            @Diagnostic(type = ErroneousSimpleBuilderMapper.class,
-                kind = javax.tools.Diagnostic.Kind.ERROR,
-                line = 26,
-                messageRegExp = "^Can't generate mapping method when @MappingTarget is supposed to be immutable "
-                + "\\(has a builder\\)\\.$")
-        })
-    public void shouldFailCannotModifyImmutable() {
+    public void testMutableTargetWithBuilder() {
+        SimpleMutableSource source = new SimpleMutableSource();
+        source.setAge( 20 );
+        source.setFullName( "Filip" );
+        MutableTarget target = SimpleBuilderMapper.INSTANCE.toMutableTarget( source );
+        assertThat( target.getAge() ).isEqualTo( 20 );
+        assertThat( target.getName() ).isEqualTo( "Filip" );
+        assertThat( target.getSource() ).isEqualTo( "Builder" );
+    }
 
+    @Test
+    public void testUpdateMutableWithBuilder() {
+        SimpleMutableSource source = new SimpleMutableSource();
+        source.setAge( 20 );
+        source.setFullName( "Filip" );
+        MutableTarget target = new MutableTarget();
+        target.setAge( 10 );
+        target.setName( "Fil" );
+
+        assertThat( target.getAge() ).isEqualTo( 10 );
+        assertThat( target.getName() ).isEqualTo( "Fil" );
+        assertThat( target.getSource() ).isEqualTo( "Empty constructor" );
+
+        SimpleBuilderMapper.INSTANCE.updateMutableTarget( source, target );
+        assertThat( target.getAge() ).isEqualTo( 20 );
+        assertThat( target.getName() ).isEqualTo( "Filip" );
+        assertThat( target.getSource() ).isEqualTo( "Empty constructor" );
+    }
+
+    @Test
+    public void updatingTargetWithNoSettersShouldNotFail() {
+
+        SimpleMutableSource source = new SimpleMutableSource();
+        source.setAge( 10 );
+
+        SimpleImmutableTarget target = SimpleImmutableTarget.builder()
+            .age( 20 )
+            .build();
+
+        assertThat( target.getAge() ).isEqualTo( 20 );
+        SimpleBuilderMapper.INSTANCE.toImmutable( source, target );
+        assertThat( target.getAge() ).isEqualTo( 20 );
     }
 }
diff --git a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/ErroneousSimpleBuilderMapper.java b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/ErroneousSimpleBuilderMapper.java
deleted file mode 100644
index 9776c0ecf..000000000
--- a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/ErroneousSimpleBuilderMapper.java
+++ /dev/null
@@ -1,27 +0,0 @@
-/**
- *  Copyright 2012-2017 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.builder.mappingTarget.simple;
-
-import org.mapstruct.Mapper;
-import org.mapstruct.MappingTarget;
-
-@Mapper
-public interface ErroneousSimpleBuilderMapper {
-    void toImmutable(SimpleMutableSource source, @MappingTarget SimpleImmutableTarget target);
-}
diff --git a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/MutableTarget.java b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/MutableTarget.java
new file mode 100644
index 000000000..374d12f42
--- /dev/null
+++ b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/MutableTarget.java
@@ -0,0 +1,78 @@
+/**
+ *  Copyright 2012-2017 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.builder.mappingTarget.simple;
+
+public class MutableTarget {
+    private String name;
+    private int age;
+    private String source;
+
+    public MutableTarget() {
+        this.source = "Empty constructor";
+    }
+
+    MutableTarget(Builder builder) {
+        this.name = builder.name;
+        this.age = builder.age;
+        this.source = "Builder";
+    }
+
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    public String getName() {
+        return name;
+    }
+
+    public void setName(String name) {
+        this.name = name;
+    }
+
+    public int getAge() {
+        return age;
+    }
+
+    public void setAge(int age) {
+        this.age = age;
+    }
+
+    public String getSource() {
+        return source;
+    }
+
+    public static class Builder {
+        private String name;
+        private int age;
+
+        public Builder age(int age) {
+            this.age = age;
+            return this;
+        }
+
+        public MutableTarget build() {
+            return new MutableTarget( this );
+        }
+
+        public Builder name(String name) {
+            this.name = name;
+            return this;
+        }
+    }
+}
diff --git a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/SimpleBuilderMapper.java b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/SimpleBuilderMapper.java
index 0c98c2930..b79d626ff 100644
--- a/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/SimpleBuilderMapper.java
+++ b/processor/src/test/java/org/mapstruct/ap/test/builder/mappingTarget/simple/SimpleBuilderMapper.java
@@ -34,6 +34,15 @@ public interface SimpleBuilderMapper {
     @Mapping(target = "builder.name", source = "source.fullName")
     void updateImmutable(SimpleMutableSource source, @MappingTarget SimpleImmutableTarget.Builder builder);
 
+    // This method is fine as if the mapping target has setters it would use them, otherwise it won't
+    void toImmutable(SimpleMutableSource source, @MappingTarget SimpleImmutableTarget target);
+
     @Mapping(target = "name", source = "fullName")
     SimpleImmutableTarget toImmutable(SimpleMutableSource source);
+
+    @Mapping(target = "name", source = "fullName")
+    MutableTarget toMutableTarget(SimpleMutableSource simpleMutableSource);
+
+    @Mapping(target = "name", source = "fullName")
+    void updateMutableTarget(SimpleMutableSource simpleMutableSource, @MappingTarget MutableTarget target);
 }