[#1457] Stricter matching for lifecycle methods / non-unique parameters (#1782)

In case a lifecycle method has multiple matching parameters (e. g. same type)
all parameter names must match exactly with the ones from the mapping method,
otherwise the lifecycle method will not be used and a warning will be shown.
This commit is contained in:
Christian Bandowski 2019-04-07 18:56:05 +02:00 committed by Filip Hrisafov
parent bd1f6e6f27
commit b2bb768f75
13 changed files with 499 additions and 34 deletions

View File

@ -91,7 +91,7 @@ public final class LifecycleMethodResolver {
MappingBuilderContext ctx, Set<String> existingVariableNames) {
MethodSelectors selectors =
new MethodSelectors( ctx.getTypeUtils(), ctx.getElementUtils(), ctx.getTypeFactory() );
new MethodSelectors( ctx.getTypeUtils(), ctx.getElementUtils(), ctx.getTypeFactory(), ctx.getMessager() );
Type targetType = method.getResultType();

View File

@ -5,11 +5,8 @@
*/
package org.mapstruct.ap.internal.model;
import static org.mapstruct.ap.internal.util.Collections.first;
import java.util.ArrayList;
import java.util.List;
import javax.lang.model.element.ElementKind;
import javax.lang.model.element.ExecutableElement;
@ -26,6 +23,8 @@ import org.mapstruct.ap.internal.model.source.selector.SelectionCriteria;
import org.mapstruct.ap.internal.util.Message;
import org.mapstruct.ap.internal.util.Strings;
import static org.mapstruct.ap.internal.util.Collections.first;
/**
*
* @author Sjaak Derksen
@ -52,7 +51,7 @@ public class ObjectFactoryMethodResolver {
MappingBuilderContext ctx) {
MethodSelectors selectors =
new MethodSelectors( ctx.getTypeUtils(), ctx.getElementUtils(), ctx.getTypeFactory() );
new MethodSelectors( ctx.getTypeUtils(), ctx.getElementUtils(), ctx.getTypeFactory(), ctx.getMessager() );
List<SelectedMethod<SourceMethod>> matchingFactoryMethods =
selectors.getMatchingMethods(

View File

@ -8,13 +8,13 @@ package org.mapstruct.ap.internal.model.source.selector;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import javax.lang.model.util.Elements;
import javax.lang.model.util.Types;
import org.mapstruct.ap.internal.model.common.Type;
import org.mapstruct.ap.internal.model.common.TypeFactory;
import org.mapstruct.ap.internal.model.source.Method;
import org.mapstruct.ap.internal.util.FormattingMessager;
/**
* Applies all known {@link MethodSelector}s in order.
@ -25,10 +25,11 @@ public class MethodSelectors {
private final List<MethodSelector> selectors;
public MethodSelectors(Types typeUtils, Elements elementUtils, TypeFactory typeFactory) {
public MethodSelectors(Types typeUtils, Elements elementUtils, TypeFactory typeFactory,
FormattingMessager messager) {
selectors = Arrays.asList(
new MethodFamilySelector(),
new TypeSelector( typeFactory ),
new TypeSelector( typeFactory, messager ),
new QualifierSelector( typeUtils, elementUtils ),
new TargetTypeSelector( typeUtils, elementUtils ),
new XmlElementDeclSelector( typeUtils ),

View File

@ -5,10 +5,9 @@
*/
package org.mapstruct.ap.internal.model.source.selector;
import static org.mapstruct.ap.internal.util.Collections.first;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import org.mapstruct.ap.internal.model.common.Parameter;
import org.mapstruct.ap.internal.model.common.ParameterBinding;
@ -17,6 +16,10 @@ import org.mapstruct.ap.internal.model.common.Type;
import org.mapstruct.ap.internal.model.common.TypeFactory;
import org.mapstruct.ap.internal.model.source.Method;
import org.mapstruct.ap.internal.model.source.MethodMatcher;
import org.mapstruct.ap.internal.util.FormattingMessager;
import org.mapstruct.ap.internal.util.Message;
import static org.mapstruct.ap.internal.util.Collections.first;
/**
* Selects those methods from the given input set which match the given source and target types (via
@ -27,9 +30,11 @@ import org.mapstruct.ap.internal.model.source.MethodMatcher;
public class TypeSelector implements MethodSelector {
private TypeFactory typeFactory;
private FormattingMessager messager;
public TypeSelector(TypeFactory typeFactory) {
public TypeSelector(TypeFactory typeFactory, FormattingMessager messager) {
this.typeFactory = typeFactory;
this.messager = messager;
}
@Override
@ -63,7 +68,7 @@ public class TypeSelector implements MethodSelector {
if ( parameterBindingPermutations != null ) {
SelectedMethod<T> matchingMethod =
getFirstMatchingParameterBinding( targetType, method, parameterBindingPermutations );
getMatchingParameterBinding( targetType, mappingMethod, method, parameterBindingPermutations );
if ( matchingMethod != null ) {
result.add( matchingMethod );
@ -77,7 +82,6 @@ public class TypeSelector implements MethodSelector {
SourceRHS sourceRHS) {
List<ParameterBinding> availableParams = new ArrayList<>( method.getParameters().size() + 3 );
addMappingTargetAndTargetTypeBindings( availableParams, targetType );
if ( sourceRHS != null ) {
availableParams.addAll( ParameterBinding.fromParameters( method.getContextParameters() ) );
availableParams.add( ParameterBinding.fromSourceRHS( sourceRHS ) );
@ -86,6 +90,8 @@ public class TypeSelector implements MethodSelector {
availableParams.addAll( ParameterBinding.fromParameters( method.getParameters() ) );
}
addMappingTargetAndTargetTypeBindings( availableParams, targetType );
return availableParams;
}
@ -94,8 +100,6 @@ public class TypeSelector implements MethodSelector {
List<ParameterBinding> availableParams = new ArrayList<>( sourceTypes.size() + 2 );
addMappingTargetAndTargetTypeBindings( availableParams, targetType );
for ( Type sourceType : sourceTypes ) {
availableParams.add( ParameterBinding.forSourceTypeBinding( sourceType ) );
}
@ -106,24 +110,124 @@ public class TypeSelector implements MethodSelector {
}
}
addMappingTargetAndTargetTypeBindings( availableParams, targetType );
return availableParams;
}
/**
* Adds default parameter bindings for the mapping-target and target-type if not already available.
*
* @param availableParams Already available params, new entries will be added to this list
* @param targetType Target type
*/
private void addMappingTargetAndTargetTypeBindings(List<ParameterBinding> availableParams, Type targetType) {
availableParams.add( ParameterBinding.forMappingTargetBinding( targetType ) );
availableParams.add( ParameterBinding.forTargetTypeBinding( typeFactory.classTypeOf( targetType ) ) );
}
boolean mappingTargetAvailable = false;
boolean targetTypeAvailable = false;
private <T extends Method> SelectedMethod<T> getFirstMatchingParameterBinding(Type targetType,
SelectedMethod<T> method, List<List<ParameterBinding>> parameterAssignmentVariants) {
for ( List<ParameterBinding> parameterAssignments : parameterAssignmentVariants ) {
if ( method.getMethod().matches( extractTypes( parameterAssignments ), targetType ) ) {
method.setParameterBindings( parameterAssignments );
return method;
// search available parameter bindings if mapping-target and/or target-type is available
for ( ParameterBinding pb : availableParams ) {
if ( pb.isMappingTarget() ) {
mappingTargetAvailable = true;
}
else if ( pb.isTargetType() ) {
targetTypeAvailable = true;
}
}
return null;
if ( !mappingTargetAvailable ) {
availableParams.add( ParameterBinding.forMappingTargetBinding( targetType ) );
}
if ( !targetTypeAvailable ) {
availableParams.add( ParameterBinding.forTargetTypeBinding( typeFactory.classTypeOf( targetType ) ) );
}
}
private <T extends Method> SelectedMethod<T> getMatchingParameterBinding(Type targetType,
Method mappingMethod, SelectedMethod<T> selectedMethodInfo,
List<List<ParameterBinding>> parameterAssignmentVariants) {
List<List<ParameterBinding>> matchingParameterAssignmentVariants = new ArrayList<>(
parameterAssignmentVariants
);
Method selectedMethod = selectedMethodInfo.getMethod();
// remove all assignment variants that doesn't match the types from the method
matchingParameterAssignmentVariants.removeIf( parameterAssignments ->
!selectedMethod.matches( extractTypes( parameterAssignments ), targetType )
);
if ( matchingParameterAssignmentVariants.isEmpty() ) {
// no matching variants found
return null;
}
else if ( matchingParameterAssignmentVariants.size() == 1 ) {
// we found exactly one set of variants, use this
selectedMethodInfo.setParameterBindings( first( matchingParameterAssignmentVariants ) );
return selectedMethodInfo;
}
// more than one variant matches, try to find one where also the parameter names are matching
// -> remove all variants where the binding var-name doesn't match the var-name of the parameter
List<Parameter> methodParameters = selectedMethod.getParameters();
matchingParameterAssignmentVariants.removeIf( parameterBindings ->
parameterBindingNotMatchesParameterVariableNames(
parameterBindings,
methodParameters
)
);
if ( matchingParameterAssignmentVariants.isEmpty() ) {
// we had some matching assignments before, but when checking the parameter names we can't find an
// appropriate one, in this case the user must chose identical parameter names for the mapping and lifecycle
// method
messager.printMessage(
selectedMethod.getExecutable(),
Message.LIFECYCLEMETHOD_AMBIGUOUS_PARAMETERS,
mappingMethod
);
return null;
}
// there should never be more then one assignment left after checking the parameter names as it is not possible
// to use the same parameter name more then once
// -> we can use the first variant that is left (that should also be the only one)
selectedMethodInfo.setParameterBindings( first( matchingParameterAssignmentVariants ) );
return selectedMethodInfo;
}
/**
* Checks if the given parameter-bindings have the same variable name than the given parameters.<br>
* The first entry in the parameter-bindings belongs to the first entry in the parameters and so on.<br>
*
* @param parameterBindings List of parameter bindings
* @param parameters List of parameters, must have the same size than the {@code parameterBindings} list
*
* @return {@code true} as soon as there is a parameter with a different variable name than the binding
*/
private boolean parameterBindingNotMatchesParameterVariableNames(List<ParameterBinding> parameterBindings,
List<Parameter> parameters) {
if ( parameterBindings.size() != parameters.size() ) {
return true;
}
int i = 0;
for ( ParameterBinding parameterBinding : parameterBindings ) {
Parameter parameter = parameters.get( i++ );
// if the parameterBinding contains a parameter name we must ensure that this matches the name from the
// method parameter -> remove all variants where this is not the case
if ( parameterBinding.getVariableName() != null &&
!parameter.getName().equals( parameterBinding.getVariableName() ) ) {
return true;
}
}
return false;
}
/**
@ -200,12 +304,8 @@ public class TypeSelector implements MethodSelector {
}
private static List<Type> extractTypes(List<ParameterBinding> parameters) {
List<Type> result = new ArrayList<>( parameters.size() );
for ( ParameterBinding param : parameters ) {
result.add( param.getType() );
}
return result;
return parameters.stream()
.map( ParameterBinding::getType )
.collect( Collectors.toList() );
}
}

View File

@ -91,7 +91,7 @@ public class MappingResolverImpl implements MappingResolver {
this.conversions = new Conversions( elementUtils, typeFactory );
this.builtInMethods = new BuiltInMappingMethods( typeFactory );
this.methodSelectors = new MethodSelectors( typeUtils, elementUtils, typeFactory );
this.methodSelectors = new MethodSelectors( typeUtils, elementUtils, typeFactory, messager );
}
@Override

View File

@ -81,6 +81,8 @@ public enum Message {
ENUMMAPPING_UNMAPPED_SOURCES( "The following constants from the source enum have no corresponding constant in the target enum and must be be mapped via adding additional mappings: %s." ),
ENUMMAPPING_DEPRECATED( "Mapping of Enums via @Mapping is going to be removed in future versions of MapStruct. Please use @ValueMapping instead!", Diagnostic.Kind.WARNING ),
LIFECYCLEMETHOD_AMBIGUOUS_PARAMETERS( "Lifecycle method has multiple matching parameters (e. g. same type), in this case please ensure to name the parameters in the lifecycle and mapping method identical. This lifecycle method will not be used for the mapping method '%s'.", Diagnostic.Kind.WARNING),
DECORATOR_NO_SUBTYPE( "Specified decorator type is no subtype of the annotated mapper type." ),
DECORATOR_CONSTRUCTOR( "Specified decorator type has no default constructor nor a constructor with a single parameter accepting the decorated mapper type." ),

View File

@ -0,0 +1,47 @@
/*
* 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.bugs._1457;
import org.mapstruct.AfterMapping;
import org.mapstruct.Mapper;
import org.mapstruct.MappingTarget;
import org.mapstruct.ReportingPolicy;
import org.mapstruct.factory.Mappers;
@Mapper(unmappedTargetPolicy = ReportingPolicy.IGNORE)
public abstract class BookMapper {
public static final BookMapper INSTANCE = Mappers.getMapper( BookMapper.class );
public abstract TargetBook mapBook(SourceBook sourceBook, String authorFirstName, String authorLastName);
@AfterMapping
protected void fillAuthor(@MappingTarget TargetBook targetBook, String authorFirstName, String authorLastName) {
targetBook.setAuthorFirstName( authorFirstName );
targetBook.setAuthorLastName( authorLastName );
}
@AfterMapping
protected void withoutAuthorNames(@MappingTarget TargetBook targetBook) {
targetBook.setAfterMappingWithoutAuthorName( true );
}
@AfterMapping
protected void withOnlyFirstName(@MappingTarget TargetBook targetBook, String authorFirstName) {
targetBook.setAfterMappingWithOnlyFirstName( authorFirstName );
}
@AfterMapping
protected void withOnlyLastName(@MappingTarget TargetBook targetBook, String authorLastName) {
targetBook.setAfterMappingWithOnlyLastName( authorLastName );
}
@AfterMapping
protected void withDifferentVariableName(@MappingTarget TargetBook targetBook, String author) {
targetBook.setAfterMappingWithDifferentVariableName( true );
}
}

View File

@ -0,0 +1,27 @@
/*
* 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.bugs._1457;
import org.mapstruct.AfterMapping;
import org.mapstruct.Mapper;
import org.mapstruct.MappingTarget;
import org.mapstruct.ReportingPolicy;
import org.mapstruct.factory.Mappers;
@Mapper(unmappedTargetPolicy = ReportingPolicy.IGNORE)
public abstract class DifferentOrderingBookMapper {
public static final DifferentOrderingBookMapper INSTANCE = Mappers.getMapper( DifferentOrderingBookMapper.class );
public abstract TargetBook mapBook(SourceBook sourceBook, String authorFirstName, String authorLastName);
@AfterMapping
protected void fillAuthor(String authorLastName, String authorFirstName, @MappingTarget TargetBook targetBook) {
targetBook.setAuthorLastName( authorLastName );
targetBook.setAuthorFirstName( authorFirstName );
}
}

View File

@ -0,0 +1,27 @@
/*
* 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.bugs._1457;
import org.mapstruct.AfterMapping;
import org.mapstruct.Mapper;
import org.mapstruct.MappingTarget;
import org.mapstruct.ReportingPolicy;
import org.mapstruct.factory.Mappers;
@Mapper(unmappedTargetPolicy = ReportingPolicy.IGNORE)
public abstract class ErroneousBookMapper {
public static final ErroneousBookMapper INSTANCE = Mappers.getMapper( ErroneousBookMapper.class );
public abstract TargetBook mapBook(SourceBook sourceBook, String authorFirstName, String authorLastName);
@AfterMapping
protected void fillAuthor(@MappingTarget TargetBook targetBook, String authorFirstN, String authorLastN) {
targetBook.setAuthorFirstName( authorFirstN );
targetBook.setAuthorLastName( authorLastN );
}
}

View File

@ -0,0 +1,123 @@
/*
* 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.bugs._1457;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mapstruct.ap.testutil.IssueKey;
import org.mapstruct.ap.testutil.WithClasses;
import org.mapstruct.ap.testutil.compilation.annotation.CompilationResult;
import org.mapstruct.ap.testutil.compilation.annotation.Diagnostic;
import org.mapstruct.ap.testutil.compilation.annotation.ExpectedCompilationOutcome;
import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
import static org.assertj.core.api.Assertions.assertThat;
@WithClasses({
SourceBook.class,
TargetBook.class
})
@RunWith(AnnotationProcessorTestRunner.class)
@IssueKey("1457")
public class Issue1457Test {
private SourceBook sourceBook;
private String authorFirstName;
private String authorLastName;
@Before
public void setup() {
sourceBook = new SourceBook();
sourceBook.setIsbn( "3453146972" );
sourceBook.setTitle( "Per Anhalter durch die Galaxis" );
authorFirstName = "Douglas";
authorLastName = "Adams";
}
@Test
@WithClasses({
BookMapper.class
})
@ExpectedCompilationOutcome(
value = CompilationResult.SUCCEEDED,
diagnostics = @Diagnostic(
messageRegExp =
"Lifecycle method has multiple matching parameters \\(e\\. g\\. same type\\), in this case " +
"please ensure to name the parameters in the lifecycle and mapping method identical\\. This " +
"lifecycle method will not be used for the mapping method '.*\\.TargetBook mapBook\\(" +
".*\\.SourceBook sourceBook, .*\\.String authorFirstName, .*\\.String authorLastName\\)'\\.",
kind = javax.tools.Diagnostic.Kind.WARNING,
line = 43
)
)
public void testMapperWithMatchingParameterNames() {
TargetBook targetBook = BookMapper.INSTANCE.mapBook( sourceBook, authorFirstName, authorLastName );
assertTargetBookMatchesSourceBook( targetBook );
assertThat( targetBook.isAfterMappingWithoutAuthorName() ).isTrue();
assertThat( targetBook.getAfterMappingWithOnlyFirstName() ).isEqualTo( authorFirstName );
assertThat( targetBook.getAfterMappingWithOnlyLastName() ).isEqualTo( authorLastName );
assertThat( targetBook.isAfterMappingWithDifferentVariableName() ).isFalse();
}
@Test
@WithClasses({
DifferentOrderingBookMapper.class
})
public void testMapperWithMatchingParameterNamesAndDifferentOrdering() {
TargetBook targetBook = DifferentOrderingBookMapper.INSTANCE.mapBook(
sourceBook,
authorFirstName,
authorLastName
);
assertTargetBookMatchesSourceBook( targetBook );
}
@Test
@WithClasses({
ObjectFactoryBookMapper.class
})
public void testMapperWithObjectFactory() {
TargetBook targetBook = ObjectFactoryBookMapper.INSTANCE.mapBook(
sourceBook,
authorFirstName,
authorLastName
);
assertTargetBookMatchesSourceBook( targetBook );
}
private void assertTargetBookMatchesSourceBook(TargetBook targetBook) {
assertThat( sourceBook.getIsbn() ).isEqualTo( targetBook.getIsbn() );
assertThat( sourceBook.getTitle() ).isEqualTo( targetBook.getTitle() );
assertThat( authorFirstName ).isEqualTo( targetBook.getAuthorFirstName() );
assertThat( authorLastName ).isEqualTo( targetBook.getAuthorLastName() );
}
@Test
@WithClasses({
ErroneousBookMapper.class
})
@ExpectedCompilationOutcome(
value = CompilationResult.SUCCEEDED,
diagnostics = @Diagnostic(
messageRegExp =
"Lifecycle method has multiple matching parameters \\(e\\. g\\. same type\\), in this case " +
"please ensure to name the parameters in the lifecycle and mapping method identical\\. This " +
"lifecycle method will not be used for the mapping method '.*\\.TargetBook mapBook\\(" +
".*\\.SourceBook sourceBook, .*\\.String authorFirstName, .*\\.String authorLastName\\)'\\.",
kind = javax.tools.Diagnostic.Kind.WARNING,
line = 22
)
)
public void testMapperWithoutMatchingParameterNames() {
}
}

View File

@ -0,0 +1,29 @@
/*
* 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.bugs._1457;
import org.mapstruct.Mapper;
import org.mapstruct.ObjectFactory;
import org.mapstruct.ReportingPolicy;
import org.mapstruct.factory.Mappers;
@Mapper(unmappedTargetPolicy = ReportingPolicy.IGNORE)
public abstract class ObjectFactoryBookMapper {
public static final ObjectFactoryBookMapper INSTANCE = Mappers.getMapper( ObjectFactoryBookMapper.class );
public abstract TargetBook mapBook(SourceBook sourceBook, String authorFirstName, String authorLastName);
@ObjectFactory
protected TargetBook createTargetBook(String authorFirstName, String authorLastName) {
TargetBook targetBook = new TargetBook();
targetBook.setAuthorFirstName( authorFirstName );
targetBook.setAuthorLastName( authorLastName );
return targetBook;
}
}

View File

@ -0,0 +1,27 @@
/*
* 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.bugs._1457;
public class SourceBook {
private String isbn;
private String title;
public String getIsbn() {
return isbn;
}
public void setIsbn(String isbn) {
this.isbn = isbn;
}
public String getTitle() {
return title;
}
public void setTitle(String title) {
this.title = title;
}
}

View File

@ -0,0 +1,83 @@
/*
* 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.bugs._1457;
public class TargetBook {
private String isbn;
private String title;
private String authorFirstName;
private String authorLastName;
private boolean afterMappingWithoutAuthorName;
private String afterMappingWithOnlyFirstName;
private String afterMappingWithOnlyLastName;
private boolean afterMappingWithDifferentVariableName;
public String getIsbn() {
return isbn;
}
public void setIsbn(String isbn) {
this.isbn = isbn;
}
public String getTitle() {
return title;
}
public void setTitle(String title) {
this.title = title;
}
public String getAuthorFirstName() {
return authorFirstName;
}
public void setAuthorFirstName(String authorFirstName) {
this.authorFirstName = authorFirstName;
}
public String getAuthorLastName() {
return authorLastName;
}
public void setAuthorLastName(String authorLastName) {
this.authorLastName = authorLastName;
}
public boolean isAfterMappingWithoutAuthorName() {
return afterMappingWithoutAuthorName;
}
public void setAfterMappingWithoutAuthorName(boolean afterMappingWithoutAuthorName) {
this.afterMappingWithoutAuthorName = afterMappingWithoutAuthorName;
}
public String getAfterMappingWithOnlyFirstName() {
return afterMappingWithOnlyFirstName;
}
public void setAfterMappingWithOnlyFirstName(String afterMappingWithOnlyFirstName) {
this.afterMappingWithOnlyFirstName = afterMappingWithOnlyFirstName;
}
public String getAfterMappingWithOnlyLastName() {
return afterMappingWithOnlyLastName;
}
public void setAfterMappingWithOnlyLastName(String afterMappingWithOnlyLastName) {
this.afterMappingWithOnlyLastName = afterMappingWithOnlyLastName;
}
public boolean isAfterMappingWithDifferentVariableName() {
return afterMappingWithDifferentVariableName;
}
public void setAfterMappingWithDifferentVariableName(boolean afterMappingWithDifferentVariableName) {
this.afterMappingWithDifferentVariableName = afterMappingWithDifferentVariableName;
}
}