Decreased yaml coalescing occurrences (#1841) (#1860)

* Removed coalescing from Yaml ChangePropertyKey, and deprecated coalescing in DeleteProperty and YamlVisitor (#1841) * Fixed edge case where DeleteProperty left an extraneous newline if deleting the first element in a document (#1841)
openrewrite · Jun 1, 2022 · d801341 · d801341
1 parent f24d4af
commit d801341
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 79 deletions.
diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/ChangePropertyKey.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/ChangePropertyKey.java
@@ -121,11 +121,7 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, P p) {
                                 propertyToTest,
                                 entry
                         ));
-                        doAfterVisit(new DeletePropertyVisitor<>(entry));
-                        //noinspection ConstantConditions
-                        if (!nonScalarMappingEntryExists(getCursor().firstEnclosing(Yaml.Document.class), propertyEntry, propertyToTest, p)) {
-                            maybeCoalesceProperties();
-                        }
+                        doAfterVisit(new DeleteProperty.DeletePropertyVisitor<>(entry));
                         break;
                     }
                     propertyToTest = propertyToTest.substring(value.length() + 1);
@@ -134,30 +130,6 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, P p) {
 
             return e;
         }
-
-        private boolean nonScalarMappingEntryExists(Yaml.Mapping.Document document, Yaml.Mapping.Entry entry, String property, P p) {
-            AtomicBoolean exists = new AtomicBoolean(false);
-            String propertyToCheck = Boolean.TRUE.equals(relaxedBinding) ? NameCaseConvention.format(NameCaseConvention.LOWER_CAMEL, property) : property;
-            new YamlIsoVisitor<P>() {
-                @Override
-                public Yaml.Mapping visitMapping(Yaml.Mapping mapping, P p) {
-                    Yaml.Mapping m = super.visitMapping(mapping, p);
-                    if (m.getEntries().contains(entry)) {
-                        for (Yaml.Mapping.Entry mEntry : m.getEntries()) {
-                            if (!(mEntry.getValue() instanceof Yaml.Scalar)) {
-                                String mKey = Boolean.TRUE.equals(relaxedBinding) ? NameCaseConvention.format(NameCaseConvention.LOWER_CAMEL, mEntry.getKey().getValue()) : mEntry.getKey().getValue();
-                                if (propertyToCheck.startsWith(mKey)) {
-                                    exists.set(true);
-                                    break;
-                                }
-                            }
-                        }
-                    }
-                    return m;
-                }
-            }.visitDocument(document, p);
-            return exists.get();
-        }
     }
 
     private static class InsertSubpropertyVisitor<P> extends YamlIsoVisitor<P> {
@@ -196,51 +168,11 @@ public Yaml.Mapping visitMapping(Yaml.Mapping mapping, P p) {
                     }));
                 } else {
                     m = m.withEntries(ListUtils.concat(m.getEntries(), newEntry));
-                    doAfterVisit(new DeletePropertyVisitor<>(entryToReplace));
-                }
-            }
-
-            return m;
-        }
-    }
-
-    private static class DeletePropertyVisitor<P> extends YamlIsoVisitor<P> {
-        private final Yaml.Mapping.Entry scope;
-
-        private DeletePropertyVisitor(Yaml.Mapping.Entry scope) {
-            this.scope = scope;
-        }
-
-        @Override
-        public Yaml.Mapping visitMapping(Yaml.Mapping mapping, P p) {
-            Yaml.Mapping m = super.visitMapping(mapping, p);
-
-            boolean changed = false;
-            List<Yaml.Mapping.Entry> entries = new ArrayList<>();
-            for (Yaml.Mapping.Entry entry : m.getEntries()) {
-                if (entry == scope || (entry.getValue() instanceof Yaml.Mapping && ((Yaml.Mapping) entry.getValue()).getEntries().isEmpty())) {
-                    changed = true;
-                } else {
-                    entries.add(entry);
+                    doAfterVisit(new DeleteProperty.DeletePropertyVisitor<>(entryToReplace));
                 }
             }
 
-            if (entries.size() == 1) {
-                entries = ListUtils.map(entries, e -> e.withPrefix(""));
-            }
-
-            if (changed) {
-                m = m.withEntries(entries);
-
-                if (getCursor().getParentOrThrow().getValue() instanceof Yaml.Document) {
-                    Yaml.Document document = getCursor().getParentOrThrow().getValue();
-                    if (!document.isExplicit()) {
-                        m = m.withEntries(m.getEntries());
-                    }
-                }
-            }
             return m;
         }
     }
-
 }
diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/DeleteProperty.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/DeleteProperty.java
@@ -39,9 +39,13 @@ public class DeleteProperty extends Recipe {
             example = "management.metrics.binders.files.enabled")
     String propertyKey;
 
+    @Deprecated
     @Option(displayName = "Coalesce",
-            description = "Simplify nested map hierarchies into their simplest dot separated property form.",
-            example = "true")
+            description = "(Deprecated: in a future version, this recipe will always use the `false` behavior)"
+                    + " Simplify nested map hierarchies into their simplest dot separated property form.",
+            example = "true",
+            required = false)
+    @Nullable
     Boolean coalesce;
 
     @Incubating(since = "7.17.0")
@@ -97,7 +101,7 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionC
 
                 if (!Boolean.FALSE.equals(relaxedBinding) ? NameCaseConvention.equalsRelaxedBinding(prop, propertyKey) : prop.equals(propertyKey)) {
                     doAfterVisit(new DeletePropertyVisitor<>(entry));
-                    if (coalesce) {
+                    if (Boolean.TRUE.equals(coalesce)) {
                         maybeCoalesceProperties();
                     }
                 }
@@ -107,10 +111,10 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionC
         };
     }
 
-    private static class DeletePropertyVisitor<P> extends YamlVisitor<P> {
+    public static class DeletePropertyVisitor<P> extends YamlVisitor<P> {
         private final Yaml.Mapping.Entry scope;
 
-        private DeletePropertyVisitor(Yaml.Mapping.Entry scope) {
+        public DeletePropertyVisitor(Yaml.Mapping.Entry scope) {
             this.scope = scope;
         }
 
@@ -120,10 +124,16 @@ public Yaml visitMapping(Yaml.Mapping mapping, P p) {
 
             boolean changed = false;
             List<Yaml.Mapping.Entry> entries = new ArrayList<>();
+            String deletedPrefix = null;
             for (Yaml.Mapping.Entry entry : m.getEntries()) {
                 if (entry == scope || (entry.getValue() instanceof Yaml.Mapping && ((Yaml.Mapping) entry.getValue()).getEntries().isEmpty())) {
+                    deletedPrefix = entry.getPrefix();
                     changed = true;
                 } else {
+                    if (deletedPrefix != null) {
+                        entry = entry.withPrefix(deletedPrefix);
+                        deletedPrefix = null;
+                    }
                     entries.add(entry);
                 }
             }

diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/YamlVisitor.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/YamlVisitor.java
@@ -111,6 +111,7 @@ public Yaml visitAlias(Yaml.Alias alias, P p) {
                 .withMarkers(visitMarkers(alias.getMarkers(), p));
     }
 
+    @Deprecated
     public void maybeCoalesceProperties() {
         if (getAfterVisit().stream().noneMatch(CoalescePropertiesVisitor.class::isInstance)) {
             doAfterVisit(new CoalescePropertiesVisitor<>());

diff --git a/rewrite-yaml/src/test/kotlin/org/openrewrite/yaml/ChangePropertyKeyTest.kt b/rewrite-yaml/src/test/kotlin/org/openrewrite/yaml/ChangePropertyKeyTest.kt
@@ -51,7 +51,8 @@ class ChangePropertyKeyTest : YamlRecipeTest {
         after = """
             unrelated.property: true
             management.metrics:
-                binders.jvm.enabled: true
+                binders:
+                    jvm.enabled: true
                 enable.process.files: true
         """
     )
@@ -66,7 +67,8 @@ class ChangePropertyKeyTest : YamlRecipeTest {
         """,
         after = """
             unrelated.property: true
-            management.metrics.enable.process.files: true
+            management.metrics:
+                enable.process.files: true
         """
     )
 
@@ -306,4 +308,42 @@ class ChangePropertyKeyTest : YamlRecipeTest {
         assertThat(valid.isValid).isTrue
     }
 
+    @Test
+    @Issue("https://github.com/openrewrite/rewrite/issues/1841")
+    fun doesNotReformatUnrelatedProperties() = assertChanged(
+        before = """
+            unrelated:
+              property: true
+            management.metrics:
+              binders.files.enabled: true
+            other:
+              property: true
+        """,
+        after = """
+            unrelated:
+              property: true
+            management.metrics:
+              enable.process.files: true
+            other:
+              property: true
+        """
+    )
+
+    @Test
+    @Issue("https://github.com/openrewrite/rewrite/issues/1841")
+    fun relocatesPropertyIfNothingElseInFamily() = assertChanged(
+        recipe = ChangePropertyKey("a.b.c", "x.y.z", true, null),
+        before = """
+            a:
+              b:
+                c: abc
+            something:
+              else: qwe
+        """,
+        after = """
+            something:
+              else: qwe
+            x.y.z: abc
+        """
+    )
 }
diff --git a/rewrite-yaml/src/test/kotlin/org/openrewrite/yaml/DeletePropertyKeyTest.kt b/rewrite-yaml/src/test/kotlin/org/openrewrite/yaml/DeletePropertyKeyTest.kt
@@ -25,14 +25,50 @@ import java.nio.file.Path
 
 class DeletePropertyKeyTest : YamlRecipeTest {
     override val recipe: Recipe
-        get() = DeleteProperty("management.metrics.binders.files.enabled", true, null, null)
+        get() = DeleteProperty("management.metrics.binders.files.enabled", null, null, null)
 
     @Test
     fun singleEntry() = assertChanged(
         before = "management.metrics.binders.files.enabled: true",
         after = ""
     )
 
+    @Test
+    @Issue("https://github.com/openrewrite/rewrite/issues/1841")
+    fun firstItem() = assertChanged(
+        before = """
+          management.metrics.binders.files.enabled: true
+          server.port: 8080
+        """,
+        after = """
+          server.port: 8080
+        """
+    )
+
+    @Test
+    fun lastItem() = assertChanged(
+        before = """
+          server.port: 8080
+          management.metrics.binders.files.enabled: true
+        """,
+        after = """
+          server.port: 8080
+        """
+    )
+
+    @Test
+    fun middleItem() = assertChanged(
+        before = """
+          app.name: foo
+          management.metrics.binders.files.enabled: true
+          server.port: 8080
+        """,
+        after = """
+          app.name: foo
+          server.port: 8080
+        """
+    )
+
     @Test
     fun downDeeper() = assertChanged(
         before = """
@@ -42,7 +78,8 @@ class DeletePropertyKeyTest : YamlRecipeTest {
           server.port: 8080
         """,
         after = """
-          management.metrics.enabled: true
+          management.metrics:
+            enabled: true
           server.port: 8080
         """
     )