#2150 Change the rules for how a constructor for mapping is picked

New rules:

1. Constructor annotated with @Default (from any package) has highest precedence
2. If there is a single public constructor then it would be used to construct the object
3. If a parameterless constructor exists then it would be used to construct the object, and the other constructors will be ignored

documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc

@@ -537,8 +537,52 @@ When doing a mapping MapStruct checks if there is a builder for the type being m
 If there is no builder, then MapStruct looks for a single accessible constructor.
 When there are multiple constructors then the following is done to pick the one which should be used:
 
-* If a parameterless constructor exists then it would be used to construct the object, and the other constructors will be ignored
+* If a constructor is annotated with an annotation named `@Default` (from any package) it will be used.
-* If there are multiple constructors then the one annotated with annotation named `@Default` (from any package) will be used
+* If a single public constructor exists then it will be used to construct the object, and the other non public constructors will be ignored.
+* If a parameterless constructor exists then it will be used to construct the object, and the other constructors will be ignored.
+* If there are multiple eligible constructors then there will be a compilation error due to ambigious constructors. In order to break the ambiquity an annotation named `@Default` (from any package) can used.
+
+.Deciding which constructor to use
+====
+[source, java, linenums]
+[subs="verbatim,attributes"]
+----
+public class Vehicle {
+
+    protected Vehicle() { }
+
+    // MapStruct will use this constructor, because it is a single public constructor
+    public Vehicle(String color) { }
+}
+
+public class Car {
+
+    // MapStruct will use this constructor, because it is a parameterless empty constructor
+    public Car() { }
+
+    public Car(String make, String color) { }
+}
+
+public class Truck {
+
+    public Truck() { }
+
+    // MapStruct will use this constructor, because it is annotated with @Default
+    @Default
+    public Truck(String make, String color) { }
+}
+
+public class Van {
+
+    // There will be a compilation error when using this class because MapStruct cannot pick a constructor
+
+    public Van(String make) { }
+
+    public Van(String make, String color) { }
+
+}
+----
+====
 
 When using a constructor then the names of the parameters of the constructor will be used and matched to the target properties.
 When the constructor has an annotation named `@ConstructorProperties` (from any package) then this annotation will be used to get the names of the parameters.

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

@@ -603,23 +603,57 @@ public class BeanMappingMethod extends NormalTypeMappingMethod {
             List<ExecutableElement> constructors = ElementFilter.constructorsIn( type.getTypeElement()
                 .getEnclosedElements() );
 
-            ExecutableElement defaultConstructor = null;
+            // The rules for picking a constructor are the following:
+            // 1. Constructor annotated with @Default (from any package) has highest precedence
+            // 2. If there is a single public constructor then it would be used to construct the object
+            // 3. If a parameterless constructor exists then it would be used to construct the object, and the other
+            // constructors will be ignored
+            ExecutableElement defaultAnnotatedConstructor = null;
+            ExecutableElement parameterLessConstructor = null;
             List<ExecutableElement> accessibleConstructors = new ArrayList<>( constructors.size() );
+            List<ExecutableElement> publicConstructors = new ArrayList<>( );
 
             for ( ExecutableElement constructor : constructors ) {
                 if ( constructor.getModifiers().contains( Modifier.PRIVATE ) ) {
                     continue;
                 }
 
+                if ( hasDefaultAnnotationFromAnyPackage( constructor ) ) {
+                    // We found a constructor annotated with @Default everything else is irrelevant
+                    defaultAnnotatedConstructor = constructor;
+                    break;
+                }
+
                 if ( constructor.getParameters().isEmpty() ) {
-                    defaultConstructor = constructor;
+                    parameterLessConstructor = constructor;
                 }
                 else {
                     accessibleConstructors.add( constructor );
                 }
+
+                if ( constructor.getModifiers().contains( Modifier.PUBLIC ) ) {
+                    publicConstructors.add( constructor );
+                }
             }
 
-            if ( defaultConstructor != null ) {
+            if ( defaultAnnotatedConstructor != null ) {
+                // If a default annotated constructor exists it will be used, it has highest precedence
+                return getConstructorAccessor( type, defaultAnnotatedConstructor );
+            }
+
+            if ( publicConstructors.size() == 1 ) {
+                // If there is a single public constructor then use that one
+                ExecutableElement publicConstructor = publicConstructors.get( 0 );
+                if ( publicConstructor.getParameters().isEmpty() ) {
+                    // The public parameterless constructor
+                    return null;
+                }
+
+                return getConstructorAccessor( type, publicConstructor );
+            }
+
+            if ( parameterLessConstructor != null ) {
+                // If there is a constructor without parameters use it
                 return null;
             }
 
@@ -627,39 +661,26 @@ public class BeanMappingMethod extends NormalTypeMappingMethod {
                 return null;
             }
 
-            ExecutableElement constructor = null;
             if ( accessibleConstructors.size() > 1 ) {
 
-                for ( ExecutableElement accessibleConstructor : accessibleConstructors ) {
+                ctx.getMessager().printMessage(
-                    for ( AnnotationMirror annotationMirror : accessibleConstructor.getAnnotationMirrors() ) {
+                    method.getExecutable(),
-                        if ( annotationMirror.getAnnotationType()
+                    GENERAL_AMBIGIOUS_CONSTRUCTORS,
-                            .asElement()
+                    type,
-                            .getSimpleName()
+                    Strings.join( constructors, ", " )
-                            .contentEquals( "Default" ) ) {
+                );
-                            constructor = accessibleConstructor;
+                return null;
-                            break;
-                        }
-                    }
-                }
-
-                if ( constructor == null ) {
-                    ctx.getMessager().printMessage(
-                        method.getExecutable(),
-                        GENERAL_AMBIGIOUS_CONSTRUCTORS,
-                        type,
-                        Strings.join( constructors, ", " )
-                    );
-                    return null;
-                }
             }
             else {
-                constructor = accessibleConstructors.get( 0 );
+                return getConstructorAccessor( type, accessibleConstructors.get( 0 ) );
             }
 
+        }
+
+        private ConstructorAccessor getConstructorAccessor(Type type, ExecutableElement constructor) {
             List<Parameter> constructorParameters = ctx.getTypeFactory()
                 .getParameters( (DeclaredType) type.getTypeMirror(), constructor );
 
-
             List<String> constructorProperties = null;
             for ( AnnotationMirror annotationMirror : constructor.getAnnotationMirrors() ) {
                 if ( annotationMirror.getAnnotationType()
@@ -736,6 +757,19 @@ public class BeanMappingMethod extends NormalTypeMappingMethod {
             return new ParameterElementAccessor( element, safeParameterName );
         }
 
+        private boolean hasDefaultAnnotationFromAnyPackage(Element element) {
+            for ( AnnotationMirror annotationMirror : element.getAnnotationMirrors() ) {
+                if ( annotationMirror.getAnnotationType()
+                    .asElement()
+                    .getSimpleName()
+                    .contentEquals( "Default" ) ) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
         private List<String> getArrayValues(AnnotationValue av) {
 
             if ( av.getValue() instanceof List ) {

processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/ConstructorVisibilityTest.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright MapStruct Authors.
+ *
+ * Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
+ */
+package org.mapstruct.ap.test.constructor.visibility;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mapstruct.ap.test.constructor.Default;
+import org.mapstruct.ap.test.constructor.PersonDto;
+import org.mapstruct.ap.testutil.IssueKey;
+import org.mapstruct.ap.testutil.WithClasses;
+import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author Filip Hrisafov
+ */
+@IssueKey("2150")
+@RunWith(AnnotationProcessorTestRunner.class)
+@WithClasses({
+    PersonDto.class,
+    Default.class,
+})
+public class ConstructorVisibilityTest {
+
+    @Test
+    @WithClasses({
+        SimpleWithPublicConstructorMapper.class
+    })
+    public void shouldUseSinglePublicConstructorAlways() {
+        PersonDto source = new PersonDto();
+        source.setName( "Bob" );
+        source.setAge( 30 );
+
+        SimpleWithPublicConstructorMapper.Person target =
+            SimpleWithPublicConstructorMapper.INSTANCE.map( source );
+
+        assertThat( target ).isNotNull();
+        assertThat( target.getName() ).isEqualTo( "Bob" );
+        assertThat( target.getAge() ).isEqualTo( 30 );
+    }
+
+    @Test
+    @WithClasses({
+        SimpleWithPublicParameterlessConstructorMapper.class
+    })
+    public void shouldUsePublicParameterConstructorIfPresent() {
+        PersonDto source = new PersonDto();
+        source.setName( "Bob" );
+        source.setAge( 30 );
+
+        SimpleWithPublicParameterlessConstructorMapper.Person target =
+            SimpleWithPublicParameterlessConstructorMapper.INSTANCE.map( source );
+
+        assertThat( target ).isNotNull();
+        assertThat( target.getName() ).isEqualTo( "From Constructor" );
+        assertThat( target.getAge() ).isEqualTo( -1 );
+    }
+
+    @Test
+    @WithClasses({
+        SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.class
+    })
+    public void shouldUseDefaultAnnotatedConstructorAlways() {
+        PersonDto source = new PersonDto();
+        source.setName( "Bob" );
+        source.setAge( 30 );
+
+        SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.Person target =
+            SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.INSTANCE.map( source );
+
+        assertThat( target ).isNotNull();
+        assertThat( target.getName() ).isEqualTo( "Bob" );
+        assertThat( target.getAge() ).isEqualTo( 30 );
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicConstructorMapper.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright MapStruct Authors.
+ *
+ * Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
+ */
+package org.mapstruct.ap.test.constructor.visibility;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.ap.test.constructor.PersonDto;
+import org.mapstruct.factory.Mappers;
+
+/**
+ * @author Filip Hrisafov
+ */
+@Mapper
+public interface SimpleWithPublicConstructorMapper {
+
+    SimpleWithPublicConstructorMapper INSTANCE = Mappers.getMapper( SimpleWithPublicConstructorMapper.class );
+
+    Person map(PersonDto dto);
+
+    class Person {
+
+        private final String name;
+        private final int age;
+
+        protected Person() {
+            this( "From Constructor", -1 );
+        }
+
+        protected Person(String name) {
+            this( name, -1 );
+        }
+
+        public Person(String name, int age) {
+            this.name = name;
+            this.age = age;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public int getAge() {
+            return age;
+        }
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.java

@@ -0,0 +1,51 @@
+/*
+ * Copyright MapStruct Authors.
+ *
+ * Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
+ */
+package org.mapstruct.ap.test.constructor.visibility;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.ap.test.constructor.Default;
+import org.mapstruct.ap.test.constructor.PersonDto;
+import org.mapstruct.factory.Mappers;
+
+/**
+ * @author Filip Hrisafov
+ */
+@Mapper
+public interface SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper {
+
+    SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper INSTANCE = Mappers.getMapper(
+        SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.class );
+
+    Person map(PersonDto dto);
+
+    class Person {
+
+        private final String name;
+        private final int age;
+
+        protected Person() {
+            this( "From Constructor", -1 );
+        }
+
+        protected Person(String name) {
+            this( name, -1 );
+        }
+
+        @Default
+        public Person(String name, int age) {
+            this.name = name;
+            this.age = age;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public int getAge() {
+            return age;
+        }
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicParameterlessConstructorMapper.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright MapStruct Authors.
+ *
+ * Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
+ */
+package org.mapstruct.ap.test.constructor.visibility;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.ap.test.constructor.PersonDto;
+import org.mapstruct.factory.Mappers;
+
+/**
+ * @author Filip Hrisafov
+ */
+@Mapper
+public interface SimpleWithPublicParameterlessConstructorMapper {
+
+    SimpleWithPublicParameterlessConstructorMapper INSTANCE = Mappers.getMapper(
+        SimpleWithPublicParameterlessConstructorMapper.class );
+
+    Person map(PersonDto dto);
+
+    class Person {
+
+        private final String name;
+        private final int age;
+
+        public Person() {
+            this( "From Constructor", -1 );
+        }
+
+        protected Person(String name) {
+            this( name, -1 );
+        }
+
+        public Person(String name, int age) {
+            this.name = name;
+            this.age = age;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public int getAge() {
+            return age;
+        }
+    }
+}