From d801341a2d1106b087988b196eb960906bb388fe Mon Sep 17 00:00:00 2001 From: Nick McKinney Date: Wed, 1 Jun 2022 12:50:16 -0400 Subject: [PATCH] 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/yaml/ChangePropertyKey.java | 72 +------------------ .../org/openrewrite/yaml/DeleteProperty.java | 20 ++++-- .../org/openrewrite/yaml/YamlVisitor.java | 1 + .../openrewrite/yaml/ChangePropertyKeyTest.kt | 44 +++++++++++- .../openrewrite/yaml/DeletePropertyKeyTest.kt | 41 ++++++++++- 5 files changed, 99 insertions(+), 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 index 1926acfbc48..e6ba1f22fad 100755 --- 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

() { - @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

extends YamlIsoVisitor

{ @@ -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

extends YamlIsoVisitor

{ - 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 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 index 3309b5b5207..eaded2d6701 100755 --- 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

extends YamlVisitor

{ + public static class DeletePropertyVisitor

extends YamlVisitor

{ 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 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 index 3843e68d1b4..553b925fcb4 100755 --- 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 index d8b7a3457dd..85bde33770a 100755 --- 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 index d0e029380c8..ceda20ec458 100755 --- a/rewrite-yaml/src/test/kotlin/org/openrewrite/yaml/DeletePropertyKeyTest.kt +++ b/rewrite-yaml/src/test/kotlin/org/openrewrite/yaml/DeletePropertyKeyTest.kt @@ -25,7 +25,7 @@ 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( @@ -33,6 +33,42 @@ class DeletePropertyKeyTest : YamlRecipeTest { 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 """ )