Fix bug adding import to a list of imports containing duplicate impor…

…ts. (#4197) * Fix bug adding import to a list of imports containing duplicate imports. Problem: - The presence check does not take the member of static imports into consideration - The comparison by size cannot handle the case where the original imports contain duplicated imports making its the size be equal to that of the ideally ordered, distinct imports even after adding a new import Fix: check the presence using the fully qualified names of imports. * Use a for loop as is common elsewhere to limit object creations * Adopt TypeUtils instead of String comparison * Guard against a null added type --------- Co-authored-by: Hoan Nguyen <[email protected]> Co-authored-by: Tim te Beek <[email protected]>
openrewrite · May 18, 2024 · 46d21d6 · 46d21d6
1 parent a164e19
commit 46d21d6
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 17 deletions.
diff --git a/rewrite-java/src/main/java/org/openrewrite/java/style/ImportLayoutStyle.java b/rewrite-java/src/main/java/org/openrewrite/java/style/ImportLayoutStyle.java
@@ -120,30 +120,21 @@ public List<JRightPadded<J.Import>> addImport(List<JRightPadded<J.Import>> origi
             return singletonList(paddedToAdd);
         }
 
+        // Do not add the import if it is already present.
+        JavaType addedType = paddedToAdd.getElement().getQualid().getType();
+        for (JRightPadded<J.Import> originalImport : originalImports) {
+            if (addedType != null && TypeUtils.isOfType(addedType, originalImport.getElement().getQualid().getType())) {
+                return originalImports;
+            }
+        }
+
         // don't star fold just yet, because we are only going to star fold adjacent imports along with
         // the import to add at most. we don't even want to star fold other non-adjacent imports in the same
         // block that should be star folded according to the layout style (minimally invasive change).
         List<JRightPadded<J.Import>> ideallyOrdered =
                 new ImportLayoutStyle(Integer.MAX_VALUE, Integer.MAX_VALUE, layout, packagesToFold)
                         .orderImports(ListUtils.concat(originalImports, paddedToAdd), new HashSet<>());
 
-        if (ideallyOrdered.size() == originalImports.size()) {
-            Set<String> originalPaths = new HashSet<>();
-            for (JRightPadded<J.Import> originalImport : originalImports) {
-                originalPaths.add(originalImport.getElement().getTypeName());
-            }
-            int sharedImports = 0;
-            for (JRightPadded<J.Import> importJRightPadded : ideallyOrdered) {
-                if (originalPaths.contains(importJRightPadded.getElement().getTypeName())) {
-                    sharedImports++;
-                }
-            }
-            if (sharedImports == originalImports.size()) {
-                // must be a duplicate of an existing import
-                return originalImports;
-            }
-        }
-
         JRightPadded<J.Import> before = null;
         JRightPadded<J.Import> after = null;
 

diff --git a/rewrite-java/src/test/java/org/openrewrite/java/style/ImportLayoutStyleTest.java b/rewrite-java/src/test/java/org/openrewrite/java/style/ImportLayoutStyleTest.java
@@ -20,9 +20,17 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;
 import org.junit.jupiter.api.Test;
+import org.openrewrite.Issue;
 import org.openrewrite.config.DeclarativeNamedStyles;
+import org.openrewrite.java.tree.J;
+import org.openrewrite.java.tree.JLeftPadded;
+import org.openrewrite.java.tree.JRightPadded;
+import org.openrewrite.java.tree.Space;
+import org.openrewrite.java.tree.TypeTree;
+import org.openrewrite.marker.Markers;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 
@@ -80,4 +88,41 @@ void deserializeInDeclarativeNamedStyles() throws IOException {
 
         mapper.readValue(mapper.writeValueAsBytes(style), DeclarativeNamedStyles.class);
     }
+
+    @Test
+    @Issue("https://github.com/openrewrite/rewrite/issues/4196")
+    void addImportInPresenceOfDuplicateOtherImport() {
+        ImportLayoutStyle style = new ImportLayoutStyle(
+                Integer.MAX_VALUE, Integer.MAX_VALUE, Collections.emptyList(), Collections.emptyList());
+        JRightPadded<J.Import> import1 = new JRightPadded<>(
+                new J.Import(
+                        randomId(),
+                        Space.EMPTY,
+                        Markers.EMPTY,
+                        new JLeftPadded<>(Space.SINGLE_SPACE, true, Markers.EMPTY),
+                        TypeTree.build("pkg.Clazz.MEMBER_1").withPrefix(Space.SINGLE_SPACE),
+                        null),
+                Space.EMPTY,
+                Markers.EMPTY);
+        JRightPadded<J.Import> import2 = new JRightPadded<>(
+                new J.Import(
+                        randomId(),
+                        Space.EMPTY,
+                        Markers.EMPTY,
+                        new JLeftPadded<>(Space.SINGLE_SPACE, true, Markers.EMPTY),
+                        TypeTree.build("pkg.Clazz.MEMBER_1").withPrefix(Space.SINGLE_SPACE),
+                        null),
+                Space.EMPTY,
+                Markers.EMPTY);
+        J.Import importToAdd = new J.Import(
+                randomId(),
+                Space.EMPTY,
+                Markers.EMPTY,
+                new JLeftPadded<>(Space.SINGLE_SPACE, true, Markers.EMPTY),
+                TypeTree.build("pkg.Clazz.MEMBER_2").withPrefix(Space.SINGLE_SPACE),
+            null);
+        assertThat(style.addImport(List.of(import1, import2), importToAdd, null, Collections.emptyList()))
+                .containsExactlyInAnyOrder(
+                        import1, import1, new JRightPadded<>(importToAdd, Space.EMPTY, Markers.EMPTY));
+    }
 }