Add inspect for target property mapped more than once (#200)

hduelme · web-flow · commit 48f4d6af2baf · 2024-07-21T17:25:30.000+02:00
diff --git a/README.md b/README.md
@@ -37,6 +37,7 @@ To learn more about MapStruct have a look at the [mapstruct](https://github.com/
   * No `source` defined in `@Mapping` annotation
   * More than one `source` in `@Mapping` annotation defined with quick fixes: Remove `source`. Remove `constant`. Remove `expression`. Use `constant` as `defaultValue`. Use `expression` as `defaultExpression`. 
   * More than one default source in `@Mapping` annotation defined with quick fixes: Remove `defaultValue`. Remove `defaultExpression`.
+  * `target` mapped more than once by `@Mapping` annotations with quick fixes: Remove annotation and change target property.
  
 ## Requirements
 
diff --git a/description.html b/description.html
@@ -41,6 +41,7 @@
         <li>No <code>source</code> defined in <code>@Mapping</code> annotation</li>
         <li>More than one <code>source</code> in <code>@Mapping</code> annotation defined with quick fixes: Remove <code>source</code>. Remove <code>constant</code>. Remove <code>expression</code>. Use <code>constant</code> as <code>defaultValue</code>. Use <code>expression</code> as <code>defaultExpression</code>.</li>
         <li>More than one default source in <code>@Mapping</code> annotation defined with quick fixes: Remove <code>defaultValue</code>. Remove <code>defaultExpression</code>.</li>
+        <li><code>target</code> mapped more than once by <code>@Mapping</code> annotations with quick fixes: Remove annotation and change target property.</li>
     </ul>
     </li>
 </ul>
diff --git a/src/main/java/org/mapstruct/intellij/inspection/TargetPropertyMappedMoreThanOnceInspection.java b/src/main/java/org/mapstruct/intellij/inspection/TargetPropertyMappedMoreThanOnceInspection.java
@@ -0,0 +1,210 @@
+/*
+ * Copyright MapStruct Authors.
+ *
+ * Licensed under the Apache License version 2.0, available at https://www.apache.org/licenses/LICENSE-2.0
+ */
+package org.mapstruct.intellij.inspection;
+
+import com.intellij.codeInsight.intention.QuickFixFactory;
+import com.intellij.codeInspection.LocalQuickFix;
+import com.intellij.codeInspection.LocalQuickFixAndIntentionActionOnPsiElement;
+import com.intellij.codeInspection.LocalQuickFixOnPsiElement;
+import com.intellij.codeInspection.ProblemsHolder;
+import com.intellij.codeInspection.util.IntentionFamilyName;
+import com.intellij.codeInspection.util.IntentionName;
+import com.intellij.openapi.editor.CaretState;
+import com.intellij.openapi.editor.Editor;
+import com.intellij.openapi.editor.LogicalPosition;
+import com.intellij.openapi.editor.ScrollType;
+import com.intellij.openapi.fileEditor.FileEditor;
+import com.intellij.openapi.fileEditor.FileEditorManager;
+import com.intellij.openapi.fileEditor.TextEditor;
+import com.intellij.openapi.project.Project;
+import com.intellij.openapi.util.TextRange;
+import com.intellij.openapi.util.text.Strings;
+import com.intellij.psi.JavaElementVisitor;
+import com.intellij.psi.PsiAnnotation;
+import com.intellij.psi.PsiAnnotationMemberValue;
+import com.intellij.psi.PsiClass;
+import com.intellij.psi.PsiElement;
+import com.intellij.psi.PsiElementVisitor;
+import com.intellij.psi.PsiFile;
+import com.intellij.psi.PsiMethod;
+import com.intellij.psi.PsiType;
+import com.intellij.psi.impl.source.tree.java.PsiAnnotationImpl;
+import org.jetbrains.annotations.NotNull;
+import org.mapstruct.intellij.MapStructBundle;
+import org.mapstruct.intellij.util.MapStructVersion;
+import org.mapstruct.intellij.util.MapstructUtil;
+import org.mapstruct.intellij.util.TargetUtils;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import static com.intellij.codeInsight.AnnotationUtil.getStringAttributeValue;
+import static org.mapstruct.intellij.util.MapstructAnnotationUtils.extractMappingAnnotationsFromMappings;
+import static org.mapstruct.intellij.util.MapstructUtil.MAPPINGS_ANNOTATION_FQN;
+import static org.mapstruct.intellij.util.MapstructUtil.MAPPING_ANNOTATION_FQN;
+import static org.mapstruct.intellij.util.TargetUtils.getTargetType;
+
+/**
+ * @author hduelme
+ */
+public class TargetPropertyMappedMoreThanOnceInspection extends InspectionBase {
+    @NotNull
+    @Override
+    PsiElementVisitor buildVisitorInternal(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+        return new TargetPropertyMappedMoreThanOnceInspection.MyJavaElementVisitor( holder,
+                MapstructUtil.resolveMapStructProjectVersion( holder.getFile() ) );
+    }
+
+    private static class MyJavaElementVisitor extends JavaElementVisitor {
+        private final ProblemsHolder holder;
+        private final MapStructVersion mapStructVersion;
+
+        private MyJavaElementVisitor(ProblemsHolder holder, MapStructVersion mapStructVersion) {
+            this.holder = holder;
+            this.mapStructVersion = mapStructVersion;
+        }
+
+        @Override
+        public void visitMethod(PsiMethod method) {
+            if ( !MapstructUtil.isMapper( method.getContainingClass() ) ) {
+                return;
+            }
+            PsiType targetType = getTargetType( method );
+            if ( targetType == null ) {
+                return;
+            }
+            Map<String, List<PsiElement>> problemMap = new HashMap<>();
+            for (PsiAnnotation psiAnnotation : method.getAnnotations()) {
+                String qualifiedName = psiAnnotation.getQualifiedName();
+                if ( MAPPING_ANNOTATION_FQN.equals( qualifiedName ) ) {
+                    handleMappingAnnotation( psiAnnotation, problemMap );
+                }
+                else if (MAPPINGS_ANNOTATION_FQN.equals( qualifiedName )) {
+                    extractMappingAnnotationsFromMappings( psiAnnotation )
+                            .forEach( a -> handleMappingAnnotation( a, problemMap ) );
+                }
+                else {
+                    // Handle annotations containing at least one Mapping annotation
+                    handleAnnotationWithMappingAnnotation( psiAnnotation, problemMap );
+                }
+            }
+            QuickFixFactory quickFixFactory = QuickFixFactory.getInstance();
+            for (Map.Entry<String, List<PsiElement>> problem : problemMap.entrySet()) {
+                List<PsiElement> problemElements = problem.getValue();
+                if (problemElements.size() > 1) {
+                    for (PsiElement problemElement : problemElements) {
+                        LocalQuickFix[] quickFixes = getLocalQuickFixes( problemElement, quickFixFactory );
+                        holder.registerProblem( problemElement,
+                               MapStructBundle.message( "inspection.target.property.mapped.more.than.once",
+                                       problem.getKey() ), quickFixes );
+                    }
+                }
+            }
+        }
+
+        private static @NotNull  LocalQuickFix[] getLocalQuickFixes(PsiElement problemElement,
+                                                                       QuickFixFactory quickFixFactory) {
+            List<LocalQuickFix> quickFixes = new ArrayList<>(2);
+            if (problemElement instanceof PsiAnnotation) {
+                quickFixes.add( getDeleteFix( problemElement, quickFixFactory ) );
+            }
+            else if (problemElement instanceof PsiAnnotationMemberValue problemPsiAnnotationMemberValue) {
+                Optional.ofNullable( problemElement.getParent() ).map( PsiElement::getParent )
+                        .map( PsiElement::getParent ).filter( PsiAnnotation.class::isInstance )
+                        .ifPresent( annotation -> quickFixes.add(
+                                getDeleteFix( annotation, quickFixFactory ) ) );
+                quickFixes.add( new ChangeTargetQuickFix( problemPsiAnnotationMemberValue ) );
+            }
+            return quickFixes.toArray( new LocalQuickFix[]{} );
+        }
+
+        private static @NotNull LocalQuickFixAndIntentionActionOnPsiElement getDeleteFix(
+                @NotNull PsiElement problemElement, @NotNull QuickFixFactory quickFixFactory) {
+
+            String annotationName = PsiAnnotationImpl.getAnnotationShortName( problemElement.getText() );
+            return quickFixFactory.createDeleteFix( problemElement,
+                    MapStructBundle.message( "intention.remove.annotation", annotationName ) );
+        }
+
+        private void handleAnnotationWithMappingAnnotation(PsiAnnotation psiAnnotation,
+                                                           Map<String, List<PsiElement>> problemMap) {
+            PsiClass annotationClass = psiAnnotation.resolveAnnotationType();
+            if (annotationClass == null) {
+                return;
+            }
+            TargetUtils.findAllDefinedMappingTargets( annotationClass, mapStructVersion )
+                    .forEach( target ->
+                            problemMap.computeIfAbsent( target, k -> new ArrayList<>() ).add( psiAnnotation ) );
+        }
+
+        private static void handleMappingAnnotation(PsiAnnotation psiAnnotation,
+                                                    Map<String, List<PsiElement>> problemMap) {
+            PsiAnnotationMemberValue value = psiAnnotation.findDeclaredAttributeValue( "target" );
+            if (value != null) {
+                String target = getStringAttributeValue( value );
+                if (target != null) {
+                    problemMap.computeIfAbsent( target, k -> new ArrayList<>() ).add( value );
+                }
+            }
+        }
+
+        private static class ChangeTargetQuickFix extends LocalQuickFixOnPsiElement {
+
+            private final String myText;
+            private final String myFamilyName;
+
+            private ChangeTargetQuickFix(@NotNull PsiAnnotationMemberValue element) {
+                super( element );
+                myText = MapStructBundle.message( "intention.change.target.property" );
+                myFamilyName = MapStructBundle.message( "inspection.target.property.mapped.more.than.once",
+                        element.getText() );
+            }
+
+            @Override
+            public @IntentionName @NotNull String getText() {
+                return myText;
+            }
+
+            @Override
+            public void invoke(@NotNull Project project, @NotNull PsiFile psiFile, @NotNull PsiElement psiElement,
+                               @NotNull PsiElement psiElement1) {
+                FileEditor selectedEditor = FileEditorManager.getInstance( project ).getSelectedEditor();
+                if ( selectedEditor instanceof TextEditor textEditor) {
+                    Editor editor = textEditor.getEditor();
+
+                    TextRange textRange = psiElement.getTextRange();
+                    String textOfElement = String.valueOf( editor.getDocument()
+                            .getCharsSequence()
+                            .subSequence( textRange.getStartOffset(), textRange.getEndOffset() ) );
+                    int targetStart = Strings.indexOf( textOfElement, "\"" ) + 1;
+                    int targetEnd = textOfElement.lastIndexOf( "\"" );
+
+                    editor.getCaretModel().moveToOffset( textRange.getStartOffset() + targetStart );
+                    LogicalPosition startPosition = editor.getCaretModel().getLogicalPosition();
+                    editor.getCaretModel().moveToOffset( textRange.getStartOffset() + targetEnd );
+                    editor.getCaretModel().setCaretsAndSelections(
+                            Collections.singletonList( new CaretState(startPosition, startPosition,
+                                    editor.getCaretModel().getLogicalPosition() ) ) );
+                    editor.getScrollingModel().scrollToCaret( ScrollType.MAKE_VISIBLE );
+                }
+            }
+
+            @Override
+            public @IntentionFamilyName @NotNull String getFamilyName() {
+                return myFamilyName;
+            }
+
+            @Override
+            public boolean availableInBatchMode() {
+                return false;
+            }
+        }
+    }
+}
diff --git a/src/main/java/org/mapstruct/intellij/util/MapstructAnnotationUtils.java b/src/main/java/org/mapstruct/intellij/util/MapstructAnnotationUtils.java
@@ -296,26 +296,32 @@ public static Stream<PsiAnnotation> findAllDefinedMappingAnnotations(@NotNull Ps
     private static Stream<PsiAnnotation> findAllDefinedMappingAnnotations(@NotNull PsiModifierListOwner owner,
                                                                           boolean includeMetaAnnotations) {
         //TODO cache
-        Stream<PsiAnnotation> mappingsAnnotations = Stream.empty();
         PsiAnnotation mappings = findAnnotation( owner, true, MapstructUtil.MAPPINGS_ANNOTATION_FQN );
-        if ( mappings != null ) {
-            //TODO maybe there is a better way to do this, but currently I don't have that much knowledge
-            PsiAnnotationMemberValue mappingsValue = mappings.findDeclaredAttributeValue( null );
-            if ( mappingsValue instanceof PsiArrayInitializerMemberValue mappingsArrayInitializerMemberValue ) {
-                mappingsAnnotations = Stream.of( mappingsArrayInitializerMemberValue.getInitializers() )
-                    .filter( MapstructAnnotationUtils::isMappingPsiAnnotation )
-                    .map( PsiAnnotation.class::cast );
-            }
-            else if ( mappingsValue instanceof PsiAnnotation mappingsAnnotation ) {
-                mappingsAnnotations = Stream.of( mappingsAnnotation );
-            }
-        }
-
+        Stream<PsiAnnotation> mappingsAnnotations = extractMappingAnnotationsFromMappings( mappings );
         Stream<PsiAnnotation> mappingAnnotations = findMappingAnnotations( owner, includeMetaAnnotations );
 
         return Stream.concat( mappingAnnotations, mappingsAnnotations );
     }
 
+    @NotNull
+    public static Stream<PsiAnnotation> extractMappingAnnotationsFromMappings(@Nullable PsiAnnotation mappings) {
+        if (mappings == null) {
+            return Stream.empty();
+        }
+        //TODO maybe there is a better way to do this, but currently I don't have that much knowledge
+        PsiAnnotationMemberValue mappingsValue = mappings.findDeclaredAttributeValue( null );
+        if ( mappingsValue instanceof PsiArrayInitializerMemberValue mappingsArrayInitializerMemberValue) {
+            return Stream.of( mappingsArrayInitializerMemberValue
+                            .getInitializers() )
+                    .filter( MapstructAnnotationUtils::isMappingPsiAnnotation )
+                    .map( PsiAnnotation.class::cast );
+        }
+        else if ( mappingsValue instanceof PsiAnnotation mappingsAnnotation ) {
+            return Stream.of( mappingsAnnotation );
+        }
+        return Stream.empty();
+    }
+
     private static Stream<PsiAnnotation> findMappingAnnotations(@NotNull PsiModifierListOwner method,
                                                                 boolean includeMetaAnnotations) {
 
diff --git a/src/main/java/org/mapstruct/intellij/util/MapstructUtil.java b/src/main/java/org/mapstruct/intellij/util/MapstructUtil.java
@@ -93,7 +93,8 @@ public final class MapstructUtil {
     public static final String INHERIT_CONFIGURATION_FQN = InheritConfiguration.class.getName();
     public static final String INHERIT_INVERSE_CONFIGURATION_FQN = InheritInverseConfiguration.class.getName();
 
-    static final String MAPPINGS_ANNOTATION_FQN = Mappings.class.getName();
+    public static final String MAPPINGS_ANNOTATION_FQN = Mappings.class.getName();
+
     static final String VALUE_MAPPING_ANNOTATION_FQN = ValueMapping.class.getName();
     static final String VALUE_MAPPINGS_ANNOTATION_FQN = ValueMappings.class.getName();
     private static final String MAPPING_TARGET_ANNOTATION_FQN = MappingTarget.class.getName();
diff --git a/src/main/java/org/mapstruct/intellij/util/TargetUtils.java b/src/main/java/org/mapstruct/intellij/util/TargetUtils.java
@@ -387,14 +387,15 @@ private static boolean hasBuildMethod(@Nullable PsiType builderType, @NotNull Ps
     /**
      * Find all defined {@link org.mapstruct.Mapping#target()} for the given method
      *
-     * @param method that needs to be checked
+     * @param owner that needs to be checked
      * @param mapStructVersion the MapStruct project version
      *
      * @return see description
      */
-    public static Stream<String> findAllDefinedMappingTargets(@NotNull PsiMethod method,
+    @NotNull
+    public static Stream<String> findAllDefinedMappingTargets(@NotNull PsiModifierListOwner owner,
         MapStructVersion mapStructVersion) {
-        return findAllDefinedMappingAnnotations( method, mapStructVersion )
+        return findAllDefinedMappingAnnotations( owner, mapStructVersion )
             .map( psiAnnotation -> AnnotationUtil.getDeclaredStringAttributeValue( psiAnnotation, "target" ) )
             .filter( Objects::nonNull )
             .filter( s -> !s.isEmpty() );
diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml
@@ -128,6 +128,14 @@
             key="inspection.this.target.mapping.no.source.property"
             shortName="TargetThisMappingNoSourcePropertyInspection"
             implementationClass="org.mapstruct.intellij.inspection.TargetThisMappingNoSourcePropertyInspection"/>
+    <localInspection
+            language="JAVA"
+            enabledByDefault="true"
+            level="ERROR"
+            bundle="org.mapstruct.intellij.messages.MapStructBundle"
+            key="inspection.target.property.mapped.more.than.once.title"
+            shortName="TargetPropertyMappedMoreThanOnceInspection"
+            implementationClass="org.mapstruct.intellij.inspection.TargetPropertyMappedMoreThanOnceInspection"/>
   </extensions>
 
   <actions>
diff --git a/src/main/resources/inspectionDescriptions/TargetPropertyMappedMoreThanOnceInspection.html b/src/main/resources/inspectionDescriptions/TargetPropertyMappedMoreThanOnceInspection.html
@@ -0,0 +1,29 @@
+<html>
+<body>
+<p>
+    This inspection reports when a target property is explicit mapped more than once
+</p>
+<p>
+<pre><code>
+//wrong
+@Mapper
+public interface EmployeeMapper {
+    @Mapping(source = "employeeName", target = "name")
+    @Mapping(source = "employeeNameLast", target = "name")
+    Employee toEmployee(EmployeeDto employeeDto, @Context CycleAvoidingMappingContext context);
+}
+</code></pre>
+</p>
+<p>
+<pre><code>
+//correct
+@Mapper
+public interface EmployeeMapper {
+    @Mapping(source = "employeeName", target = "name")
+    Employee toEmployee(EmployeeDto employeeDto, @Context CycleAvoidingMappingContext context);
+}
+</code></pre>
+</p>
+<!-- tooltip end -->
+</body>
+</html>
diff --git a/src/main/resources/org/mapstruct/intellij/messages/MapStructBundle.properties b/src/main/resources/org/mapstruct/intellij/messages/MapStructBundle.properties
@@ -25,6 +25,8 @@ inspection.wrong.map.mapping.map.type.raw=Raw map used for mapping Map to Bean
 inspection.wrong.map.mapping.map.type.raw.set.default=Replace {0} with {0}<String, String>
 inspection.wrong.map.mapping.map.key=Key must be of type String for mapping Map to Bean
 inspection.wrong.map.mapping.map.key.change.to.string=Change key type to String
+inspection.target.property.mapped.more.than.once=Target property ''{0}'' must not be mapped more than once.
+inspection.target.property.mapped.more.than.once.title=Target properties must not be mapped more than once.
 intention.add.ignore.all.unmapped.target.properties=Add ignore all unmapped target properties
 intention.add.ignore.unmapped.target.property=Add ignore unmapped target property
 intention.add.unmapped.target.property=Add unmapped target property
@@ -34,6 +36,8 @@ intention.not.null.checkable.property.source.used.with.default.property=Remove d
 intention.java.expression.remove.unnecessary.whitespace=Remove unnecessary whitespaces
 intention.wrong.map.mapping.map.type.raw=Add type to Map for mapping Map to Bean
 intention.wrong.map.mapping.map.key=Use Map with key of type String for mapping Map to Bean
+intention.remove.annotation=Remove {0} annotation
+intention.change.target.property=Change target property
 plugin.settings.title=MapStruct
 plugin.settings.quickFix.title=Quick fix properties
 plugin.settings.quickFix.preferSourceBeforeTargetInMapping=Prefer source before target in @Mapping
diff --git a/src/test/java/org/mapstruct/intellij/inspection/TargetPropertyMappedMoreThanOnceInspectionTest.java b/src/test/java/org/mapstruct/intellij/inspection/TargetPropertyMappedMoreThanOnceInspectionTest.java
diff --git a/testData/inspection/TargetPropertyMappedMoreThanOnce.java b/testData/inspection/TargetPropertyMappedMoreThanOnce.java
diff --git a/testData/inspection/TargetPropertyMappedMoreThanOnce_after.java b/testData/inspection/TargetPropertyMappedMoreThanOnce_after.java
