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 39e942965..4e9acc991 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 @@ -322,7 +322,7 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { // first we have to handle nested target mappings if ( method.getMappingOptions().hasNestedTargetReferences() ) { - handleDefinedNestedTargetMapping( handledTargets ); + errorOccurred = handleDefinedNestedTargetMapping( handledTargets ); } for ( Map.Entry> entry : methodMappings.entrySet() ) { @@ -335,7 +335,7 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { } } } - else if ( reportErrorOnTargetObject( mapping ) ) { + else { errorOccurred = true; } } @@ -350,7 +350,7 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { return errorOccurred; } - private void handleDefinedNestedTargetMapping(Set handledTargets) { + private boolean handleDefinedNestedTargetMapping(Set handledTargets) { NestedTargetPropertyMappingHolder holder = new NestedTargetPropertyMappingHolder.Builder() .mappingContext( ctx ) @@ -368,6 +368,7 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { } unprocessedDefinedTargets.put( entry.getKey().getName(), entry.getValue() ); } + return holder.hasErrorOccurred(); } private boolean handleDefinedMapping(Mapping mapping, Set handledTargets) { @@ -474,36 +475,6 @@ public class BeanMappingMethod extends NormalTypeMappingMethod { return errorOccured; } - private boolean reportErrorOnTargetObject(Mapping mapping) { - - boolean errorOccurred = false; - - boolean hasReadAccessor - = method.getResultType().getPropertyReadAccessors().containsKey( mapping.getTargetName() ); - - if ( hasReadAccessor ) { - if ( !mapping.isIgnored() ) { - ctx.getMessager().printMessage( - method.getExecutable(), - mapping.getMirror(), - mapping.getSourceAnnotationValue(), - Message.BEANMAPPING_PROPERTY_HAS_NO_WRITE_ACCESSOR_IN_RESULTTYPE, - mapping.getTargetName() ); - errorOccurred = true; - } - } - else { - ctx.getMessager().printMessage( - method.getExecutable(), - mapping.getMirror(), - mapping.getSourceAnnotationValue(), - Message.BEANMAPPING_UNKNOWN_PROPERTY_IN_RESULTTYPE, - mapping.getTargetName() ); - errorOccurred = true; - } - return errorOccurred; - } - /** * Iterates over all target properties and all source parameters. *

diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/NestedTargetPropertyMappingHolder.java b/processor/src/main/java/org/mapstruct/ap/internal/model/NestedTargetPropertyMappingHolder.java index 744112338..e6bb66274 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/NestedTargetPropertyMappingHolder.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/NestedTargetPropertyMappingHolder.java @@ -31,6 +31,7 @@ import org.mapstruct.ap.internal.model.source.MappingOptions; import org.mapstruct.ap.internal.model.source.Method; import org.mapstruct.ap.internal.model.source.PropertyEntry; import org.mapstruct.ap.internal.model.source.SourceReference; +import org.mapstruct.ap.internal.model.source.TargetReference; import org.mapstruct.ap.internal.util.Extractor; import static org.mapstruct.ap.internal.util.Collections.first; @@ -63,15 +64,17 @@ public class NestedTargetPropertyMappingHolder { private final Set handledTargets; private final List propertyMappings; private final Map> unprocessedDefinedTarget; + private final boolean errorOccurred; public NestedTargetPropertyMappingHolder( List processedSourceParameters, Set handledTargets, List propertyMappings, - Map> unprocessedDefinedTarget) { + Map> unprocessedDefinedTarget, boolean errorOccurred) { this.processedSourceParameters = processedSourceParameters; this.handledTargets = handledTargets; this.propertyMappings = propertyMappings; this.unprocessedDefinedTarget = unprocessedDefinedTarget; + this.errorOccurred = errorOccurred; } /** @@ -102,6 +105,13 @@ public class NestedTargetPropertyMappingHolder { return unprocessedDefinedTarget; } + /** + * @return {@code true} if an error occurred during the creation of the nested mappings + */ + public boolean hasErrorOccurred() { + return errorOccurred; + } + public static class Builder { private Method method; @@ -227,7 +237,8 @@ public class NestedTargetPropertyMappingHolder { processedSourceParameters, handledTargets, propertyMappings, - unprocessedDefinedTarget + unprocessedDefinedTarget, + groupedByTP.errorOccurred ); } @@ -284,9 +295,16 @@ public class NestedTargetPropertyMappingHolder { = new LinkedHashMap>(); Map> singleTargetReferences = new LinkedHashMap>(); + boolean errorOccurred = false; for ( List mapping : mappings.values() ) { - PropertyEntry property = first( first( mapping ).getTargetReference().getPropertyEntries() ); - Mapping newMapping = first( mapping ).popTargetReference(); + Mapping firstMapping = first( mapping ); + TargetReference targetReference = firstMapping.getTargetReference(); + if ( !targetReference.isValid() ) { + errorOccurred = true; + continue; + } + PropertyEntry property = first( targetReference.getPropertyEntries() ); + Mapping newMapping = firstMapping.popTargetReference(); if ( newMapping != null ) { // group properties on current name. if ( !mappingsKeyedByProperty.containsKey( property ) ) { @@ -298,11 +316,11 @@ public class NestedTargetPropertyMappingHolder { if ( !singleTargetReferences.containsKey( property ) ) { singleTargetReferences.put( property, new ArrayList() ); } - singleTargetReferences.get( property ).add( first( mapping ) ); + singleTargetReferences.get( property ).add( firstMapping ); } } - return new GroupedTargetReferences( mappingsKeyedByProperty, singleTargetReferences ); + return new GroupedTargetReferences( mappingsKeyedByProperty, singleTargetReferences, errorOccurred ); } /** @@ -576,11 +594,13 @@ public class NestedTargetPropertyMappingHolder { private static class GroupedTargetReferences { private final Map> poppedTargetReferences; private final Map> singleTargetReferences; + private final boolean errorOccurred; private GroupedTargetReferences(Map> poppedTargetReferences, - Map> singleTargetReferences) { + Map> singleTargetReferences, boolean errorOccurred) { this.poppedTargetReferences = poppedTargetReferences; this.singleTargetReferences = singleTargetReferences; + this.errorOccurred = errorOccurred; } } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/source/Mapping.java b/processor/src/main/java/org/mapstruct/ap/internal/model/source/Mapping.java index c225adc13..61d1358b5 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/source/Mapping.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/source/Mapping.java @@ -252,7 +252,21 @@ public class Mapping { ( (DeclaredType) mirror ).asElement().getKind() == ElementKind.ENUM; } - public void init(SourceMethod method, FormattingMessager messager, TypeFactory typeFactory, boolean isReverse) { + public void init(SourceMethod method, FormattingMessager messager, TypeFactory typeFactory) { + init( method, messager, typeFactory, false, null ); + } + + /** + * Initialize the Mapping. + * + * @param method the source method that the mapping belongs to + * @param messager the messager that can be used for outputting messages + * @param typeFactory the type factory + * @param isReverse whether the init is for a reverse mapping + * @param reverseSourceParameter the source parameter from the revers mapping + */ + private void init(SourceMethod method, FormattingMessager messager, TypeFactory typeFactory, boolean isReverse, + Parameter reverseSourceParameter) { if ( !method.isEnumMapping() ) { sourceReference = new SourceReference.BuilderFromMapping() @@ -268,6 +282,7 @@ public class Mapping { .method( method ) .messager( messager ) .typeFactory( typeFactory ) + .reverseSourceParameter( reverseSourceParameter ) .build(); } } @@ -408,7 +423,13 @@ public class Mapping { Collections.emptyList() ); - reverse.init( method, messager, typeFactory, true ); + reverse.init( + method, + messager, + typeFactory, + true, + sourceReference != null ? sourceReference.getParameter() : null + ); return reverse; } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/source/SourceMethod.java b/processor/src/main/java/org/mapstruct/ap/internal/model/source/SourceMethod.java index 3c7142395..73ebc0825 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/source/SourceMethod.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/source/SourceMethod.java @@ -200,7 +200,7 @@ public class SourceMethod implements Method { if ( mappings != null ) { for ( Map.Entry> entry : mappings.entrySet() ) { for ( Mapping mapping : entry.getValue() ) { - mapping.init( sourceMethod, messager, typeFactory, false ); + mapping.init( sourceMethod, messager, typeFactory ); } } } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java b/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java index 00487d23e..fc2aaab9d 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/source/TargetReference.java @@ -72,6 +72,30 @@ public class TargetReference { private SourceMethod method; private FormattingMessager messager; private TypeFactory typeFactory; + private boolean isReverse; + /** + * Needed when we are building from reverse mapping. It is needed, so we can remove the first level if it is + * needed. + * E.g. If we have a mapping like: + * + * {@literal @}Mapping( target = "letterSignature", source = "dto.signature" ) + * + * + * When it is reversed it will look like: + * + * {@literal @}Mapping( target = "dto.signature", source = "letterSignature" ) + * + * + * The {@code dto} needs to be considered as a possibility for a target name only if a Target Reference for + * a reverse is created. + */ + private Parameter reverseSourceParameter; + /** + * During {@link #getTargetEntries(Type, String[])} an error can occur. However, we are invoking + * that multiple times because the first entry can also be the name of the parameter. Therefore we keep + * the error message until the end when we can report it. + */ + private Message errorMessage; public BuilderFromTargetMapping messager(FormattingMessager messager) { this.messager = messager; @@ -94,6 +118,12 @@ public class TargetReference { } public BuilderFromTargetMapping isReverse(boolean isReverse) { + this.isReverse = isReverse; + return this; + } + + public BuilderFromTargetMapping reverseSourceParameter(Parameter reverseSourceParameter) { + this.reverseSourceParameter = reverseSourceParameter; return this; } @@ -112,20 +142,28 @@ public class TargetReference { boolean foundEntryMatch; Type resultType = method.getResultType(); - // there can be 3 situations + // there can be 4 situations // 1. Return type - // 2. @MappingTarget, with - // 3. or without parameter name. + // 2. A reverse target reference where the source parameter name is used in the original mapping + // 3. @MappingTarget, with + // 4. or without parameter name. String[] targetPropertyNames = segments; List entries = getTargetEntries( resultType, targetPropertyNames ); foundEntryMatch = (entries.size() == targetPropertyNames.length); - if ( !foundEntryMatch && segments.length > 1 ) { + if ( !foundEntryMatch && segments.length > 1 + && matchesSourceOrTargetParameter( segments[0], parameter, reverseSourceParameter, isReverse ) ) { targetPropertyNames = Arrays.copyOfRange( segments, 1, segments.length ); entries = getTargetEntries( resultType, targetPropertyNames ); foundEntryMatch = (entries.size() == targetPropertyNames.length); } - // foundEntryMatch = isValid, errors are handled in the BeanMapping, where the error context is known + if ( !foundEntryMatch && errorMessage != null) { + // This is called only for reporting errors + reportMappingError( errorMessage, mapping.getTargetName() ); + } + + // foundEntryMatch = isValid, errors are handled here, and the BeanMapping uses that to ignore + // the creation of mapping for invalid TargetReferences return new TargetReference( parameter, entries, foundEntryMatch ); } @@ -142,46 +180,133 @@ public class TargetReference { Accessor targetReadAccessor = nextType.getPropertyReadAccessors().get( entryNames[i] ); Accessor targetWriteAccessor = nextType.getPropertyWriteAccessors( cms ).get( entryNames[i] ); - if ( targetWriteAccessor == null || ( i < entryNames.length - 1 && targetReadAccessor == null) ) { - // there should always be a write accessor and there should be read accessor mandatory for all - // but the last - // TODO error handling when full fledged target mapping is in place. + boolean isLast = i == entryNames.length - 1; + boolean isNotLast = i < entryNames.length - 1; + if ( isWriteAccessorNotValidWhenNotLast( targetWriteAccessor, isNotLast ) + || isWriteAccessorNotValidWhenLast( targetWriteAccessor, targetReadAccessor, mapping, isLast ) + || ( isNotLast && targetReadAccessor == null ) ) { + // there should always be a write accessor (except for the last when the mapping is ignored and + // there is a read accessor) and there should be read accessor mandatory for all but the last + setErrorMessage( targetWriteAccessor, targetReadAccessor ); break; } - if ( ( i == entryNames.length - 1 ) || ( Executables.isSetterMethod( targetWriteAccessor ) + if ( isLast || ( Executables.isSetterMethod( targetWriteAccessor ) || Executables.isFieldAccessor( targetWriteAccessor ) ) ) { // only intermediate nested properties when they are a true setter or field accessor // the last may be other readAccessor (setter / getter / adder). - if ( Executables.isGetterMethod( targetWriteAccessor ) || - Executables.isFieldAccessor( targetWriteAccessor ) ) { - nextType = typeFactory.getReturnType( - (DeclaredType) nextType.getTypeMirror(), - targetWriteAccessor ); - } - else { - nextType = typeFactory.getSingleParameter( - (DeclaredType) nextType.getTypeMirror(), - targetWriteAccessor ).getType(); - } + nextType = findNextType( nextType, targetWriteAccessor, targetReadAccessor ); // check if an entry alread exists, otherwise create String[] fullName = Arrays.copyOfRange( entryNames, 0, i + 1 ); PropertyEntry propertyEntry = PropertyEntry.forTargetReference( fullName, targetReadAccessor, targetWriteAccessor, nextType ); targetEntries.add( propertyEntry ); - } - } + } + return targetEntries; } + /** + * Finds the next type based on the initial type. + * + * @param initial for which a next type should be found + * @param targetWriteAccessor the write accessor + * @param targetReadAccessor the read accessor + * @return the next type that should be used for finding a property entry + */ + private Type findNextType(Type initial, Accessor targetWriteAccessor, Accessor targetReadAccessor) { + Type nextType; + Accessor toUse = targetWriteAccessor != null ? targetWriteAccessor : targetReadAccessor; + if ( Executables.isGetterMethod( toUse ) || + Executables.isFieldAccessor( toUse ) ) { + nextType = typeFactory.getReturnType( + (DeclaredType) initial.getTypeMirror(), + toUse + ); + } + else { + nextType = typeFactory.getSingleParameter( + (DeclaredType) initial.getTypeMirror(), + toUse + ).getType(); + } + return nextType; + } + + private void setErrorMessage(Accessor targetWriteAccessor, Accessor targetReadAccessor) { + if ( targetWriteAccessor == null && targetReadAccessor == null ) { + errorMessage = Message.BEANMAPPING_UNKNOWN_PROPERTY_IN_RESULTTYPE; + } + else if ( targetWriteAccessor == null ) { + errorMessage = Message.BEANMAPPING_PROPERTY_HAS_NO_WRITE_ACCESSOR_IN_RESULTTYPE; + } + } + private void reportMappingError(Message msg, Object... objects) { messager.printMessage( method.getExecutable(), mapping.getMirror(), mapping.getSourceAnnotationValue(), msg, objects ); } + + /** + * A write accessor is not valid if it is {@code null} and it is not last. i.e. for nested target mappings + * there must be a write accessor for all entries except the last one. + * + * @param accessor that needs to be checked + * @param isNotLast whether or not this is the last write accessor in the entry chain + * + * @return {@code true} if the accessor is not valid, {@code false} otherwise + */ + private static boolean isWriteAccessorNotValidWhenNotLast(Accessor accessor, boolean isNotLast) { + return accessor == null && isNotLast; + } + + /** + * For a last accessor to be valid, a read accessor should exist and the mapping should be ignored. All other + * cases represent an invalid write accessor. This method will evaluate to {@code true} if the following is + * {@code true}: + *

    + *
  • {@code writeAccessor} is {@code null}
    • + *
  • It is for the last entry
    • + *
  • A read accessor does not exist, or the mapping is not ignored
    • + *
+ * + * @param writeAccessor that needs to be checked + * @param readAccessor that is used + * @param mapping that is used + * @param isLast whether or not this is the last write accessor in the entry chain + * + * @return {@code true} if the write accessor is not valid, {@code false} otherwise. See description for more + * information + */ + private static boolean isWriteAccessorNotValidWhenLast(Accessor writeAccessor, Accessor readAccessor, + Mapping mapping, boolean isLast) { + return writeAccessor == null && isLast && ( readAccessor == null || !mapping.isIgnored() ); + } + + /** + * Validates that the {@code segment} is the same as the {@code targetParameter} or the {@code + * reverseSourceParameter} names + * + * @param segment that needs to be checked + * @param targetParameter the target parameter if it exists + * @param reverseSourceParameter the reverse source parameter if it exists + * @param isReverse whether a reverse {@link TargetReference} is being built + * + * @return {@code true} if the segment matches the name of the {@code targetParameter} or the name of the + * {@code reverseSourceParameter} when this is a reverse {@link TargetReference} is being built, {@code + * false} otherwise + */ + private static boolean matchesSourceOrTargetParameter(String segment, Parameter targetParameter, + Parameter reverseSourceParameter, boolean isReverse) { + boolean matchesTargetParameter = + targetParameter != null && targetParameter.getName().equals( segment ); + return matchesTargetParameter + || isReverse && reverseSourceParameter != null && reverseSourceParameter.getName().equals( segment ); + } } private TargetReference(Parameter sourceParameter, List sourcePropertyEntries, boolean isValid) { diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_1153/ErroneousIssue1153Mapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_1153/ErroneousIssue1153Mapper.java new file mode 100644 index 000000000..67f87bcf9 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_1153/ErroneousIssue1153Mapper.java @@ -0,0 +1,99 @@ +/** + * Copyright 2012-2017 Gunnar Morling (http://www.gunnarmorling.de/) + * and/or other contributors as indicated by the @authors tag. See the + * copyright.txt file in the distribution for a full listing of all + * contributors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.mapstruct.ap.test.bugs._1153; + +import org.mapstruct.Mapper; +import org.mapstruct.Mapping; +import org.mapstruct.Mappings; + +/** + * @author Filip Hrisafov + */ +@Mapper +public interface ErroneousIssue1153Mapper { + + @Mappings( { + @Mapping( target = "readOnly", source = "nonNested"), + @Mapping( target = "nestedTarget.readOnly", source = "nestedSource.nested"), + @Mapping( target = "nestedTarget.writable", source = "nestedSource.writable"), + @Mapping( target = "nestedTarget2.readOnly", ignore = true), + @Mapping( target = "nestedTarget2.writable2", source = "nestedSource.writable"), + } ) + Target map(Source source); + + class Source { + + public static class NestedSource { + //CHECKSTYLE:OFF + public String nested; + public String writable; + //CHECKSTYLE:ON + } + + //CHECKSTYLE:OFF + public String nonNested; + public NestedSource nestedSource; + public NestedSource nestedSource2; + //CHECKSTYLE:ON + } + + class Target { + + public static class NestedTarget { + private String readOnly; + private String writable; + + public String getReadOnly() { + return readOnly; + } + + public String getWritable() { + return writable; + } + + public void setWritable(String writable) { + this.writable = writable; + } + } + + private String readOnly; + private NestedTarget nestedTarget; + private NestedTarget nestedTarget2; + + public String getReadOnly() { + return readOnly; + } + + public NestedTarget getNestedTarget() { + return nestedTarget; + } + + public void setNestedTarget(NestedTarget nestedTarget) { + this.nestedTarget = nestedTarget; + } + + public NestedTarget getNestedTarget2() { + return nestedTarget2; + } + + public void setNestedTarget2(NestedTarget nestedTarget2) { + this.nestedTarget2 = nestedTarget2; + } + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_1153/Issue1153Test.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_1153/Issue1153Test.java new file mode 100644 index 000000000..7ddbcd427 --- /dev/null +++ b/processor/src/test/java/org/mapstruct/ap/test/bugs/_1153/Issue1153Test.java @@ -0,0 +1,56 @@ +/** + * Copyright 2012-2017 Gunnar Morling (http://www.gunnarmorling.de/) + * and/or other contributors as indicated by the @authors tag. See the + * copyright.txt file in the distribution for a full listing of all + * contributors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.mapstruct.ap.test.bugs._1153; + +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; + +/** + * @author Filip Hrisafov + */ +@RunWith(AnnotationProcessorTestRunner.class) +@WithClasses(ErroneousIssue1153Mapper.class) +@IssueKey("1153") +public class Issue1153Test { + + @ExpectedCompilationOutcome(value = CompilationResult.FAILED, + diagnostics = { + @Diagnostic(type = ErroneousIssue1153Mapper.class, + kind = javax.tools.Diagnostic.Kind.ERROR, + line = 32, + messageRegExp = "Property \"readOnly\" has no write accessor\\."), + @Diagnostic(type = ErroneousIssue1153Mapper.class, + kind = javax.tools.Diagnostic.Kind.ERROR, + line = 33, + messageRegExp = "Property \"nestedTarget.readOnly\" has no write accessor\\."), + @Diagnostic(type = ErroneousIssue1153Mapper.class, + kind = javax.tools.Diagnostic.Kind.ERROR, + line = 36, + messageRegExp = "Unknown property \"nestedTarget2.writable2\" in return type\\.") + }) + @Test + public void shouldReportErrorsCorrectly() { + } +}