diff --git a/documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc b/documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc index ad5d8111f..7d2dd052f 100644 --- a/documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc +++ b/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 there are multiple constructors then the one annotated with annotation named `@Default` (from any package) will be used +* If a constructor is annotated with an annotation named `@Default` (from any package) it 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. 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 1be610842..dc5d6ac41 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 @@ -603,23 +603,57 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { List 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 accessibleConstructors = new ArrayList<>( constructors.size() ); + List 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 ) { - for ( AnnotationMirror annotationMirror : accessibleConstructor.getAnnotationMirrors() ) { - if ( annotationMirror.getAnnotationType() - .asElement() - .getSimpleName() - .contentEquals( "Default" ) ) { - constructor = accessibleConstructor; - break; - } - } - } - - if ( constructor == null ) { - ctx.getMessager().printMessage( - method.getExecutable(), - GENERAL_AMBIGIOUS_CONSTRUCTORS, - type, - Strings.join( constructors, ", " ) - ); - return 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 constructorParameters = ctx.getTypeFactory() .getParameters( (DeclaredType) type.getTypeMirror(), constructor ); - List 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 getArrayValues(AnnotationValue av) { if ( av.getValue() instanceof List ) { diff --git a/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/ConstructorVisibilityTest.java b/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/ConstructorVisibilityTest.java new file mode 100644 index 000000000..b40dcbf17 --- /dev/null +++ b/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 ); + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicConstructorMapper.java b/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicConstructorMapper.java new file mode 100644 index 000000000..ce0a98aa3 --- /dev/null +++ b/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; + } + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.java b/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicParameterlessConstructorAndDefaultAnnotatedMapper.java new file mode 100644 index 000000000..923821681 --- /dev/null +++ b/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; + } + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicParameterlessConstructorMapper.java b/processor/src/test/java/org/mapstruct/ap/test/constructor/visibility/SimpleWithPublicParameterlessConstructorMapper.java new file mode 100644 index 000000000..c0ba14bc4 --- /dev/null +++ b/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; + } + } +}