From 1ce282362cba827d985bcb18a720708b5336c2e3 Mon Sep 17 00:00:00 2001 From: Sjaak Derksen Date: Sat, 4 Jul 2020 21:50:20 +0200 Subject: [PATCH] #2101 inherited properties need to be analysed against redefined properties when inheriting mappings (#2103) --- .../model/source/MappingMethodOptions.java | 51 ++++++++++++-- .../processor/MapperCreationProcessor.java | 16 ++--- .../bugs/_2101/Issue2101AdditionalMapper.java | 45 +++++++++++++ .../ap/test/bugs/_2101/Issue2101Mapper.java | 49 ++++++++++++++ .../ap/test/bugs/_2101/Issue2101Test.java | 67 +++++++++++++++++++ 5 files changed, 212 insertions(+), 16 deletions(-) create mode 100644 processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101AdditionalMapper.java create mode 100644 processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Mapper.java create mode 100644 processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Test.java diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingMethodOptions.java b/processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingMethodOptions.java index 6c5cc1d04..4a06acdd4 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingMethodOptions.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingMethodOptions.java @@ -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; @@ -136,9 +137,8 @@ public class MappingMethodOptions { * * @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() ) { @@ -200,13 +200,56 @@ public class MappingMethodOptions { } // 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 inheritedMappings) { + Set redefinedSources = new HashSet<>(); + Set 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 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(); diff --git a/processor/src/main/java/org/mapstruct/ap/internal/processor/MapperCreationProcessor.java b/processor/src/main/java/org/mapstruct/ap/internal/processor/MapperCreationProcessor.java index c22377f98..e480eaf7a 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/processor/MapperCreationProcessor.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/processor/MapperCreationProcessor.java @@ -447,10 +447,10 @@ public class MapperCreationProcessor implements ModelElementProcessor 1 ) { messager.printMessage( @@ -477,11 +473,7 @@ public class MapperCreationProcessor implements ModelElementProcessor 1 ) { messager.printMessage( diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101AdditionalMapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101AdditionalMapper.java new file mode 100644 index 000000000..ea745d6bc --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101AdditionalMapper.java @@ -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 +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Mapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Mapper.java new file mode 100644 index 000000000..5a1f01fa7 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Mapper.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.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 +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Test.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Test.java new file mode 100644 index 000000000..36c2c3213 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_2101/Issue2101Test.java @@ -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" ); + + } +}