Skip to content

#2101 inherited properties need to be analysed against redefined properties when inheriting mappings #2103

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

Merged
sjaakd merged 5 commits into from
Jul 4, 2020
Merged

#2101 inherited properties need to be analysed against redefined properties when inheriting mappings #2103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1866d90
#2101 reproducer
May 21, 2020
2a137be
#2101 removing nonused arg from method
Jul 4, 2020
c2f42a4
#2101 filtering solution
Jul 4, 2020
4470711
#2101 new insights and additional test
Jul 4, 2020
f2c1913
#2101 rework
Jul 4, 2020
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
51 changes: 47 additions & 4 deletions processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingMethodOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@
package org.mapstruct.ap.internal.model.source;

import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import org.mapstruct.ap.internal.gem.CollectionMappingStrategyGem;
import org.mapstruct.ap.internal.model.common.Type;
import org.mapstruct.ap.internal.model.common.TypeFactory;
import org.mapstruct.ap.internal.gem.CollectionMappingStrategyGem;
import org.mapstruct.ap.internal.util.accessor.Accessor;

import static org.mapstruct.ap.internal.model.source.MappingOptions.getMappingTargetNamesBy;
Expand Down Expand Up @@ -124,9 +125,8 @@ public void markAsFullyInitialized() {
*
* @param templateMethod the template method with the options to inherit, may be {@code null}
* @param isInverse if {@code true}, the specified options are from an inverse method
* @param method the source method
*/
public void applyInheritedOptions(SourceMethod templateMethod, boolean isInverse, SourceMethod method ) {
public void applyInheritedOptions(SourceMethod templateMethod, boolean isInverse) {
MappingMethodOptions templateOptions = templateMethod.getOptions();
if ( null != templateOptions ) {
if ( !getIterableMapping().hasAnnotation() && templateOptions.getIterableMapping().hasAnnotation() ) {
Expand Down Expand Up @@ -177,13 +177,56 @@ public void applyInheritedOptions(SourceMethod templateMethod, boolean isInverse
}

// now add all (does not override duplicates and leaves original mappings)
mappings.addAll( newMappings );
addAllNonRedefined( newMappings );

// filter new mappings
filterNestedTargetIgnores( mappings );
}
}

private void addAllNonRedefined(Set<MappingOptions> inheritedMappings) {
Set<String> redefinedSources = new HashSet<>();
Set<String> redefinedTargets = new HashSet<>();
for ( MappingOptions redefinedMappings : mappings ) {
if ( redefinedMappings.getSourceName() != null ) {
redefinedSources.add( redefinedMappings.getSourceName() );
}
if ( redefinedMappings.getTargetName() != null ) {
redefinedTargets.add( redefinedMappings.getTargetName() );
}
}
for ( MappingOptions inheritedMapping : inheritedMappings ) {
if ( inheritedMapping.isIgnored()
|| ( !isRedefined( redefinedSources, inheritedMapping.getSourceName() )
&& !isRedefined( redefinedTargets, inheritedMapping.getTargetName() ) )
) {
mappings.add( inheritedMapping );
}
}
}

private boolean isRedefined(Set<String> redefinedNames, String inheritedName ) {
for ( String redefinedName : redefinedNames ) {
if ( elementsAreContainedIn( redefinedName, inheritedName ) ) {
return true;
}
}
return false;
}

private boolean elementsAreContainedIn( String redefinedName, String inheritedName ) {
if ( redefinedName.startsWith( inheritedName ) ) {
// it is possible to redefine an exact matching source name, because the same source can be mapped to
// multiple targets. It is not possible for target, but caught by the Set and equals methoded in
// MappingOptions
if ( redefinedName.length() > inheritedName.length() ) {
// redefined.lenght() > inherited.length(), first following character should be separator
return '.' == redefinedName.charAt( inheritedName.length() );
}
}
return false;
}

public void applyIgnoreAll(SourceMethod method, TypeFactory typeFactory ) {
CollectionMappingStrategyGem cms = method.getOptions().getMapper().getCollectionMappingStrategy();
Type writeType = method.getResultType();
Expand Down
16 changes: 4 additions & 12 deletions processor/src/main/java/org/mapstruct/ap/internal/processor/MapperCreationProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,10 +441,10 @@ private void mergeInheritedOptions(SourceMethod method, MapperOptions mapperConf

// apply defined (@InheritConfiguration, @InheritInverseConfiguration) mappings
if ( forwardTemplateMethod != null ) {
mappingOptions.applyInheritedOptions( forwardTemplateMethod, false, method );
mappingOptions.applyInheritedOptions( forwardTemplateMethod, false );
}
if ( inverseTemplateMethod != null ) {
mappingOptions.applyInheritedOptions( inverseTemplateMethod, true, method );
mappingOptions.applyInheritedOptions( inverseTemplateMethod, true );
}

// apply auto inherited options
Expand All @@ -454,11 +454,7 @@ private void mergeInheritedOptions(SourceMethod method, MapperOptions mapperConf
// but.. there should not be an @InheritedConfiguration
if ( forwardTemplateMethod == null && inheritanceStrategy.isApplyForward() ) {
if ( applicablePrototypeMethods.size() == 1 ) {
mappingOptions.applyInheritedOptions(
first( applicablePrototypeMethods ),
false,
method
);
mappingOptions.applyInheritedOptions( first( applicablePrototypeMethods ), false );
}
else if ( applicablePrototypeMethods.size() > 1 ) {
messager.printMessage(
Expand All @@ -471,11 +467,7 @@ else if ( applicablePrototypeMethods.size() > 1 ) {
// or no @InheritInverseConfiguration
if ( inverseTemplateMethod == null && inheritanceStrategy.isApplyReverse() ) {
if ( applicableReversePrototypeMethods.size() == 1 ) {
mappingOptions.applyInheritedOptions(
first( applicableReversePrototypeMethods ),
true,
method
);
mappingOptions.applyInheritedOptions( first( applicableReversePrototypeMethods ), true );
}
else if ( applicableReversePrototypeMethods.size() > 1 ) {
messager.printMessage(
Expand Down
45 changes: 45 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101AdditionalMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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._2101;

import org.mapstruct.InheritConfiguration;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.factory.Mappers;

@Mapper
public interface Issue2101AdditionalMapper {

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

@Mapping(target = "value1", source = "value.nestedValue1")
@Mapping(target = "value2", source = "value.nestedValue2")
@Mapping(target = "value3", source = "valueThrowOffPath")
Target map1(Source source);

@InheritConfiguration
@Mapping(target = "value2", source = "value.nestedValue1")
@Mapping(target = "value3", constant = "test")
Target map2(Source source);

//CHECKSTYLE:OFF
class Source {
public String valueThrowOffPath;
public NestedSource value;
}

class Target {
public String value1;
public String value2;
public String value3;
}

class NestedSource {
public String nestedValue1;
public String nestedValue2;
}
//CHECKSTYLE:ON
}
49 changes: 49 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.bugs._2101;

import org.mapstruct.InheritInverseConfiguration;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.factory.Mappers;

@Mapper
public interface Issue2101Mapper {

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

@Mapping(target = "value1", source = "codeValue1")
@Mapping(target = "value2", source = "codeValue2")
Source map(Target target);

@InheritInverseConfiguration
@Mapping(target = "codeValue1.code", constant = "c1")
@Mapping(target = "codeValue1.value", source = "value1")
@Mapping(target = "codeValue2.code", constant = "c2")
@Mapping(target = "codeValue2.value", source = "value2")
Target map(Source source);

default String mapFrom( CodeValuePair cv ) {
return cv.code;
}

//CHECKSTYLE:OFF
class Source {
public String value1;
public String value2;
}

class Target {
public CodeValuePair codeValue1;
public CodeValuePair codeValue2;
}

class CodeValuePair {
public String code;
public String value;
}
//CHECKSTYLE:ON
}
67 changes: 67 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* 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._2101;

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.runner.AnnotationProcessorTestRunner;

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

@IssueKey("2101")
@RunWith(AnnotationProcessorTestRunner.class)
public class Issue2101Test {

@Test
@WithClasses(Issue2101Mapper.class)
public void shouldMap() {

Issue2101Mapper.Source source = new Issue2101Mapper.Source();
source.value1 = "v1";
source.value2 = "v2";

Issue2101Mapper.Target target = Issue2101Mapper.INSTANCE.map( source );

assertThat( target.codeValue1.code ).isEqualTo( "c1" );
assertThat( target.codeValue1.value ).isEqualTo( "v1" );
assertThat( target.codeValue2.code ).isEqualTo( "c2" );
assertThat( target.codeValue2.value ).isEqualTo( "v2" );

}

@Test
@WithClasses(Issue2101AdditionalMapper.class)
public void shouldMapSomeAdditionalTests1() {
Issue2101AdditionalMapper.Source source = new Issue2101AdditionalMapper.Source();
source.value = new Issue2101AdditionalMapper.NestedSource();
source.value.nestedValue1 = "value1";
source.value.nestedValue2 = "value2";
source.valueThrowOffPath = "value3";

Issue2101AdditionalMapper.Target target = Issue2101AdditionalMapper.INSTANCE.map1( source );
assertThat( target.value1 ).isEqualTo( "value1" );
assertThat( target.value2 ).isEqualTo( "value2" );
assertThat( target.value3 ).isEqualTo( "value3" );
}

@Test
@WithClasses(Issue2101AdditionalMapper.class)
public void shouldMapSomeAdditionalTests2() {
Issue2101AdditionalMapper.Source source = new Issue2101AdditionalMapper.Source();
source.value = new Issue2101AdditionalMapper.NestedSource();
source.value.nestedValue1 = "value1";
source.value.nestedValue2 = "value2";
source.valueThrowOffPath = "value3";

Issue2101AdditionalMapper.Target target = Issue2101AdditionalMapper.INSTANCE.map2( source );
assertThat( target.value1 ).isEqualTo( "value1" );
assertThat( target.value2 ).isEqualTo( "value1" );
assertThat( target.value3 ).isEqualTo( "test" );

}
}