Skip to content

#4029 Fall back from ADDER_PREFERRED to setter when adder path not applicable #4108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arimu1 wants to merge 1 commit into
base: main
Choose a base branch
from
+291 −35
Open

#4029 Fall back from ADDER_PREFERRED to setter when adder path not applicable #4108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 133 additions & 35 deletions processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Supplier;
Expand All @@ -18,6 +19,7 @@
import javax.lang.model.element.TypeElement;

import org.mapstruct.ap.internal.gem.BuilderGem;
import org.mapstruct.ap.internal.gem.CollectionMappingStrategyGem;
import org.mapstruct.ap.internal.gem.NullValueCheckStrategyGem;
import org.mapstruct.ap.internal.gem.NullValuePropertyMappingStrategyGem;
import org.mapstruct.ap.internal.model.assignment.AdderWrapper;
Expand Down Expand Up @@ -236,43 +238,35 @@ public PropertyMapping build() {
// handle source
this.rightHandSide = getSourceRHS( sourceReference );

ctx.getMessager().note( 2, Message.PROPERTYMAPPING_MAPPING_NOTE, rightHandSide, targetWriteAccessor );

rightHandSide.setUseElementAsSourceTypeForMatching(
targetWriteAccessorType == AccessorType.ADDER );

// all the tricky cases will be excluded for the time being.
boolean preferUpdateMethods;
if ( targetWriteAccessorType == AccessorType.ADDER ) {
preferUpdateMethods = false;
}
else {
preferUpdateMethods = method.getMappingTargetParameter() != null;
// #4029 ADDER_PREFERRED is a preference, not a hard requirement. When the adder path is
// not applicable for this source (e.g. Map — not element-iterable for matching) and a
// setter exists, fall back to SETTER_PREFERRED so a direct collection mapping method
// can be selected instead of failing while trying to map into the adder parameter type.
if ( targetWriteAccessorType == AccessorType.ADDER
&& !isSourceSuitableForAdderElementMatching( rightHandSide.getSourceType() ) ) {
Accessor setterFallback = findSetterFallbackWhenAdderNotApplicable();
if ( setterFallback != null ) {
reconfigureTargetWriteAccessor( setterFallback );
}
}

SelectionCriteria criteria = SelectionCriteria.forMappingMethods(
selectionParameters,
mappingControl,
targetPropertyName,
preferUpdateMethods
);
ctx.getMessager().note( 2, Message.PROPERTYMAPPING_MAPPING_NOTE, rightHandSide, targetWriteAccessor );

// forge a method instead of resolving one when there are mapping options.
Assignment assignment = null;
if ( forgeMethodWithMappingReferences == null ) {
assignment = ctx.getMappingResolver().getTargetAssignment(
method,
getForgedMethodHistory( rightHandSide ),
targetType,
formattingParameters,
criteria,
rightHandSide,
positionHint,
this::forge
);
}
else {
assignment = forge();
Assignment assignment = resolveAssignment();

// #4029 If the adder path still could not produce an assignment, try the setter.
if ( assignment == null && targetWriteAccessorType == AccessorType.ADDER ) {
Accessor setterFallback = findSetterFallbackWhenAdderNotApplicable();
if ( setterFallback != null ) {
reconfigureTargetWriteAccessor( setterFallback );
ctx.getMessager().note(
2,
Message.PROPERTYMAPPING_MAPPING_NOTE,
rightHandSide,
targetWriteAccessor
);
assignment = resolveAssignment();
}
}

// JSpecify: raise a hard compile error when a source that is not guaranteed @NonNull
Expand Down Expand Up @@ -333,6 +327,103 @@ else if ( targetType.isArrayType() && sourceType.isArrayType() && assignment.get
);
}

/**
* Resolves a mapping assignment for the current target write accessor.
* When the accessor is an adder, element type matching is used.
*/
private Assignment resolveAssignment() {
rightHandSide.setUseElementAsSourceTypeForMatching(
targetWriteAccessorType == AccessorType.ADDER );

// all the tricky cases will be excluded for the time being.
boolean preferUpdateMethods;
if ( targetWriteAccessorType == AccessorType.ADDER ) {
preferUpdateMethods = false;
}
else {
preferUpdateMethods = method.getMappingTargetParameter() != null;
}

SelectionCriteria criteria = SelectionCriteria.forMappingMethods(
selectionParameters,
mappingControl,
targetPropertyName,
preferUpdateMethods
);

// forge a method instead of resolving one when there are mapping options.
if ( forgeMethodWithMappingReferences == null ) {
return ctx.getMappingResolver().getTargetAssignment(
method,
getForgedMethodHistory( rightHandSide ),
targetType,
formattingParameters,
criteria,
rightHandSide,
positionHint,
this::forge
);
}
return forge();
}

/**
* When {@link CollectionMappingStrategyGem#ADDER_PREFERRED} selected an adder but no
* assignment could be created for the adder path, look up the setter for the same property
* so the mapping can fall back to setter-preferred behavior.
*
* @return the setter (or field) write accessor for the property, or {@code null} if none
*/
private Accessor findSetterFallbackWhenAdderNotApplicable() {
Element adderElement = targetWriteAccessor.getElement();
if ( adderElement == null ) {
return null;
}
Element enclosing = adderElement.getEnclosingElement();
while ( enclosing != null && !( enclosing instanceof TypeElement ) ) {
enclosing = enclosing.getEnclosingElement();
}
if ( enclosing == null ) {
return null;
}

Type beanType = ctx.getTypeFactory().getType( (TypeElement) enclosing );
Map<String, Accessor> writeAccessors =
beanType.getPropertyWriteAccessors( CollectionMappingStrategyGem.SETTER_PREFERRED );
Accessor candidate = writeAccessors.get( targetPropertyName );
if ( candidate != null
&& ( candidate.getAccessorType() == AccessorType.SETTER
|| candidate.getAccessorType().isFieldAssignment() ) ) {
return candidate;
}
return null;
}

private void reconfigureTargetWriteAccessor(Accessor newAccessor) {
this.targetWriteAccessor = newAccessor;
this.targetWriteAccessorType = newAccessor.getAccessorType();
this.targetType = ctx.getTypeFactory().getType( newAccessor.getAccessedType() );
BuilderGem builder = method.getOptions().getBeanMapping().getBuilder();
this.targetBuilderType = ctx.getTypeFactory().builderTypeFor( this.targetType, builder );
}

/**
* Whether the source type can supply elements for an adder (collection / iterable / stream /
* array). Maps are not treated as element-iterable for matching (see
* {@link SourceRHS#getSourceTypeForMatching()}), so adder is not applicable for them.
* Non-collection sources (single element → adder) remain applicable.
*/
private static boolean isSourceSuitableForAdderElementMatching(Type sourceType) {
if ( sourceType.isMapType() ) {
return false;
}
return sourceType.isCollectionType()
|| sourceType.isIterableType()
|| sourceType.isStreamType()
|| sourceType.isArrayType()
|| !sourceType.isCollectionOrMapType();
}

private Assignment forge( ) {
Assignment assignment;
Type sourceType = rightHandSide.getSourceType();
Expand All @@ -344,7 +435,14 @@ else if ( sourceType.isMapType() && targetType.isMapType() ) {
assignment = forgeMapMapping( sourceType, targetType, rightHandSide );
}
else if ( sourceType.isMapType() && !targetType.isMapType() ) {
assignment = forgeMapping( sourceType, targetType.withoutBounds(), rightHandSide );
// #4029 Do not forge Map → adder-element (e.g. Map → String). That path is not
// meaningful for adders; return null so the caller can fall back to the setter.
if ( targetWriteAccessorType == AccessorType.ADDER ) {
assignment = null;
}
else {
assignment = forgeMapping( sourceType, targetType.withoutBounds(), rightHandSide );
}
}
else if ( ( sourceType.isIterableType() && targetType.isStreamType() )
|| ( sourceType.isStreamType() && targetType.isStreamType() )
Expand Down
101 changes: 101 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_4029/Issue4029Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
* 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._4029;

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import org.mapstruct.CollectionMappingStrategy;
import org.mapstruct.Mapper;
import org.mapstruct.factory.Mappers;

/**
* Reproducer for #4029: with {@link CollectionMappingStrategy#ADDER_PREFERRED}, mapping a
* {@link Map} source property onto a collection target that has both an adder and a setter
* should fall back to the setter when a direct collection mapping method is available (adder
* path not applicable for the whole-map mapping).
*/
@Mapper(collectionMappingStrategy = CollectionMappingStrategy.ADDER_PREFERRED)
public interface Issue4029Mapper {

Issue4029Mapper INSTANCE = Mappers.getMapper( Issue4029Mapper.class );

Target fromMapSource(MapSource source);

Target fromListSource(ListSource source);

/**
* Direct whole-collection mapping from Map to List. With ADDER_PREFERRED this must still
* be usable via the setter fallback when the adder path cannot apply.
*/
default List<String> map(Map<String, String> map) {
if ( map == null ) {
return null;
}
List<String> result = new ArrayList<>( map.size() );
for ( Map.Entry<String, String> entry : map.entrySet() ) {
result.add( entry.getKey() + "=" + entry.getValue() );
}
return result;
}

class MapSource {
private Map<String, String> values = new LinkedHashMap<>();

public Map<String, String> getValues() {
return values;
}

public void setValues(Map<String, String> values) {
this.values = values;
}
}

class ListSource {
private List<String> values = new ArrayList<>();

public List<String> getValues() {
return values;
}

public void setValues(List<String> values) {
this.values = values;
}
}

class Target {
private List<String> values;
private boolean adderUsed;
private boolean setterUsed;

public List<String> getValues() {
return values;
}

public void setValues(List<String> values) {
this.setterUsed = true;
this.values = values;
}

public void addValue(String value) {
this.adderUsed = true;
if ( this.values == null ) {
this.values = new ArrayList<>();
}
this.values.add( value );
}

public boolean isAdderUsed() {
return adderUsed;
}

public boolean isSetterUsed() {
return setterUsed;
}
}
}
57 changes: 57 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_4029/Issue4029MapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* 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._4029;

import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.Map;

import org.mapstruct.ap.testutil.IssueKey;
import org.mapstruct.ap.testutil.ProcessorTest;
import org.mapstruct.ap.testutil.WithClasses;

import static org.assertj.core.api.Assertions.assertThat;

/**
* @author arimu1
*/
@WithClasses(Issue4029Mapper.class)
@IssueKey("4029")
class Issue4029MapperTest {

@ProcessorTest
void adderPreferredFallsBackToSetterWhenDirectMapToListMappingIsGiven() {
Issue4029Mapper.MapSource source = new Issue4029Mapper.MapSource();
Map<String, String> values = new LinkedHashMap<>();
values.put( "a", "1" );
values.put( "b", "2" );
source.setValues( values );

Issue4029Mapper.Target target = Issue4029Mapper.INSTANCE.fromMapSource( source );

assertThat( target ).isNotNull();
assertThat( target.getValues() ).containsExactly( "a=1", "b=2" );
assertThat( target.isSetterUsed() )
.as( "Map→List with custom method should fall back to setter under ADDER_PREFERRED" )
.isTrue();
assertThat( target.isAdderUsed() ).isFalse();
}

@ProcessorTest
void adderPreferredStillUsesAdderWhenElementMappingIsApplicable() {
Issue4029Mapper.ListSource source = new Issue4029Mapper.ListSource();
source.setValues( Arrays.asList( "x", "y" ) );

Issue4029Mapper.Target target = Issue4029Mapper.INSTANCE.fromListSource( source );

assertThat( target ).isNotNull();
assertThat( target.getValues() ).containsExactly( "x", "y" );
assertThat( target.isAdderUsed() )
.as( "List→List with matching element type should still prefer adder" )
.isTrue();
assertThat( target.isSetterUsed() ).isFalse();
}
}