From 31d822dca79c9162944c56abdc7a3ee09e859b1b Mon Sep 17 00:00:00 2001 From: lingenj Date: Wed, 26 Feb 2025 09:52:10 +0100 Subject: [PATCH 1/3] Parse the `incoming` yaml the first time we use this field --- .../java/org/openrewrite/yaml/MergeYaml.java | 50 +++++++++++-------- .../openrewrite/yaml/MergeYamlVisitor.java | 22 +------- 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java index 2a194544946..a8306e5ac1b 100644 --- a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java +++ b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java @@ -123,22 +123,13 @@ public TreeVisitor getVisitor() { return Preconditions.check(new FindSourceFiles(filePattern), new YamlIsoVisitor() { final JsonPathMatcher matcher = new JsonPathMatcher(key); - final Yaml incoming = new YamlParser().parse(yaml) - .findFirst() - .map(Yaml.Documents.class::cast) - .map(docs -> { - // Any comments will have been put on the parent Document node, preserve by copying to the mapping - Yaml.Document doc = docs.getDocuments().get(0); - if (doc.getBlock() instanceof Yaml.Mapping) { - Yaml.Mapping m = (Yaml.Mapping) doc.getBlock(); - return m.withEntries(ListUtils.mapFirst(m.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); - } else if (doc.getBlock() instanceof Yaml.Sequence) { - Yaml.Sequence s = (Yaml.Sequence) doc.getBlock(); - return s.withEntries(ListUtils.mapFirst(s.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); - } - return doc.getBlock().withPrefix(doc.getPrefix()); - }) - .orElseThrow(() -> new IllegalArgumentException("Could not parse as YAML")); + @Nullable Yaml incoming; + private Yaml incoming() { + if (incoming == null) { + incoming = parse(yaml); + } + return incoming; + } @Override public Yaml.Document visitDocument(Yaml.Document document, ExecutionContext ctx) { @@ -161,7 +152,7 @@ public Yaml.Document visitDocument(Yaml.Document document, ExecutionContext ctx) } // If there is no space between the colon and the value it will not be interpreted as a mapping String snippet; - if (incoming instanceof Yaml.Mapping) { + if (incoming() instanceof Yaml.Mapping) { snippet = valueKey + ":\n" + indent(yaml); } else { snippet = valueKey + ":" + (yaml.startsWith(" ") ? yaml : " " + yaml); @@ -213,7 +204,7 @@ public Yaml.Mapping visitMapping(Yaml.Mapping mapping, ExecutionContext ctx) { Yaml.Mapping m = super.visitMapping(mapping, ctx); if (matcher.matches(getCursor())) { getCursor().putMessageOnFirstEnclosing(Yaml.Document.class, FOUND_MATCHING_ELEMENT, true); - m = (Yaml.Mapping) new MergeYamlVisitor<>(mapping, incoming, Boolean.TRUE.equals(acceptTheirs), + m = (Yaml.Mapping) new MergeYamlVisitor<>(mapping, incoming(), Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty).visitNonNull(mapping, ctx, getCursor().getParentOrThrow()); } return m; @@ -223,7 +214,7 @@ public Yaml.Mapping visitMapping(Yaml.Mapping mapping, ExecutionContext ctx) { public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionContext ctx) { if (matcher.matches(getCursor())) { getCursor().putMessageOnFirstEnclosing(Yaml.Document.class, FOUND_MATCHING_ELEMENT, true); - Yaml.Block value = (Yaml.Block) new MergeYamlVisitor<>(entry.getValue(), incoming, + Yaml.Block value = (Yaml.Block) new MergeYamlVisitor<>(entry.getValue(), incoming(), Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty).visitNonNull(entry.getValue(), ctx, getCursor()); if (value instanceof Yaml.Scalar && value.getPrefix().isEmpty()) { @@ -239,7 +230,7 @@ public Yaml.Sequence visitSequence(Yaml.Sequence sequence, ExecutionContext ctx) if (matcher.matches(getCursor().getParentOrThrow())) { getCursor().putMessageOnFirstEnclosing(Yaml.Document.class, FOUND_MATCHING_ELEMENT, true); return sequence.withEntries(ListUtils.map(sequence.getEntries(), - entry -> entry.withBlock((Yaml.Block) new MergeYamlVisitor<>(entry.getBlock(), incoming, + entry -> entry.withBlock((Yaml.Block) new MergeYamlVisitor<>(entry.getBlock(), incoming(), Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty) .visitNonNull(entry.getBlock(), ctx, new Cursor(getCursor(), entry))))); } @@ -247,4 +238,23 @@ public Yaml.Sequence visitSequence(Yaml.Sequence sequence, ExecutionContext ctx) } }); } + + static Yaml parse(@Language("yml") String yaml) { + return new YamlParser().parse(yaml) + .findFirst() + .map(Yaml.Documents.class::cast) + .map(docs -> { + // Any comments will have been put on the parent Document node, preserve by copying to the mapping + Yaml.Document doc = docs.getDocuments().get(0); + if (doc.getBlock() instanceof Yaml.Mapping) { + Yaml.Mapping m = (Yaml.Mapping) doc.getBlock(); + return m.withEntries(ListUtils.mapFirst(m.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); + } else if (doc.getBlock() instanceof Yaml.Sequence) { + Yaml.Sequence s = (Yaml.Sequence) doc.getBlock(); + return s.withEntries(ListUtils.mapFirst(s.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); + } + return doc.getBlock().withPrefix(doc.getPrefix()); + }) + .orElseThrow(() -> new IllegalArgumentException("Could not parse as YAML")); + } } diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java index 86767d2e352..18b66a9764b 100644 --- a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java +++ b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java @@ -90,27 +90,7 @@ public MergeYamlVisitor(Yaml.Block block, Yaml incoming, boolean acceptTheirs, @ } public MergeYamlVisitor(Yaml scope, @Language("yml") String yamlString, boolean acceptTheirs, @Nullable String objectIdentifyingProperty, @Nullable InsertMode insertMode, @Nullable String insertProperty) { - this(scope, - new YamlParser().parse(yamlString) - .findFirst() - .map(Yaml.Documents.class::cast) - .map(docs -> { - // Any comments will have been put on the parent Yaml.Document node, preserve by copying to the mapping - Yaml.Document doc = docs.getDocuments().get(0); - if (doc.getBlock() instanceof Yaml.Mapping) { - Yaml.Mapping m = (Yaml.Mapping) doc.getBlock(); - return m.withEntries(mapFirst(m.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); - } else if (doc.getBlock() instanceof Yaml.Sequence) { - Yaml.Sequence s = (Yaml.Sequence) doc.getBlock(); - return s.withEntries(mapFirst(s.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); - } - return doc.getBlock().withPrefix(doc.getPrefix()); - }) - .orElseThrow(() -> new IllegalArgumentException("Could not parse as YAML")), - acceptTheirs, - objectIdentifyingProperty, - insertMode, - insertProperty); + this(scope, MergeYaml.parse(yamlString), acceptTheirs, objectIdentifyingProperty, insertMode, insertProperty); } @Override From a84db56332afcb9edb7076a2dd1ed49f594cc292 Mon Sep 17 00:00:00 2001 From: lingenj Date: Wed, 26 Feb 2025 13:33:08 +0100 Subject: [PATCH 2/3] Make `validate` able to handle other source files --- .../java/org/openrewrite/yaml/MergeYaml.java | 1 + .../org/openrewrite/yaml/MergeYamlTest.java | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java index a8306e5ac1b..104e2d5ec91 100644 --- a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java +++ b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java @@ -94,6 +94,7 @@ public Validated validate() { return super.validate() .and(Validated.test("yaml", "Must be valid YAML", yaml, y -> new YamlParser().parse(yaml) + .filter(it -> it instanceof Yaml.Documents) // Could also be a ParserError .findFirst() .map(doc -> !((Yaml.Documents) doc).getDocuments().isEmpty()) .orElse(false))) diff --git a/rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java b/rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java index 2c0c455bf5a..a3ea08ccdc4 100644 --- a/rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java +++ b/rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java @@ -20,6 +20,7 @@ import org.openrewrite.Issue; import org.openrewrite.test.RewriteTest; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.openrewrite.yaml.Assertions.yaml; import static org.openrewrite.yaml.MergeYaml.InsertMode.After; import static org.openrewrite.yaml.MergeYaml.InsertMode.Before; @@ -2820,4 +2821,23 @@ void preventKeysToBeAppendedToPreviousCommentIfManyLineBreaks() { ) ); } + + @Test + void invalidYaml() { + assertThrows(AssertionError.class, () -> rewriteRun( + spec -> spec + .recipe(new MergeYaml( + "$.some.object", + // language=yaml + """ + script: |-ParseError + """, + false, + "name", + null, + null, + null + )) + )); + } } From bbc10df4bbee1a2850b14f5f76b8478c13e863b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Thu, 27 Feb 2025 13:11:57 +0100 Subject: [PATCH 3/3] WIP to reduce yaml parsing --- .../java/org/openrewrite/yaml/MergeYaml.java | 76 +++++++++++++------ .../openrewrite/yaml/MergeYamlVisitor.java | 4 + 2 files changed, 57 insertions(+), 23 deletions(-) diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java index 104e2d5ec91..bd5e90a7408 100644 --- a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java +++ b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java @@ -15,23 +15,24 @@ */ package org.openrewrite.yaml; -import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; import lombok.Value; +import lombok.experimental.NonFinal; import org.intellij.lang.annotations.Language; import org.jspecify.annotations.Nullable; import org.openrewrite.*; import org.openrewrite.internal.ListUtils; import org.openrewrite.yaml.tree.Yaml; +import java.util.Optional; + import static org.openrewrite.internal.StringUtils.isBlank; import static org.openrewrite.yaml.MergeYaml.InsertMode.*; @Value @EqualsAndHashCode(callSuper = false) @NoArgsConstructor(force = true) // TODO: remove with @deprecated constructor -@AllArgsConstructor // TODO: remove with @deprecated constructor public class MergeYaml extends Recipe { @Option(displayName = "Key path", description = "A [JsonPath](https://docs.openrewrite.org/reference/jsonpath-and-jsonpathmatcher-reference) expression used to find matching keys.", @@ -87,17 +88,36 @@ public MergeYaml(String key, @Language("yml") String yaml, @Nullable Boolean acc this(key, yaml, acceptTheirs, objectIdentifyingProperty, filePattern, null, insertProperty); } + public MergeYaml(String key, @Language("yml") String yaml, @Nullable Boolean acceptTheirs, @Nullable String objectIdentifyingProperty, @Nullable String filePattern, @Nullable InsertMode insertMode, @Nullable String insertProperty) { + this.key = key; + this.yaml = yaml; + this.acceptTheirs = acceptTheirs; + this.objectIdentifyingProperty = objectIdentifyingProperty; + this.filePattern = filePattern; + this.insertMode = insertMode; + this.insertProperty = insertProperty; + } + public enum InsertMode { Before, After, Last } + @Nullable + @NonFinal + transient Yaml incoming = null; + @Override public Validated validate() { + // parse here return super.validate() .and(Validated.test("yaml", "Must be valid YAML", - yaml, y -> new YamlParser().parse(yaml) - .filter(it -> it instanceof Yaml.Documents) // Could also be a ParserError - .findFirst() - .map(doc -> !((Yaml.Documents) doc).getDocuments().isEmpty()) - .orElse(false))) + yaml, y -> { + Optional firstDocument = MergeYaml.maybeParse(yaml); + if (firstDocument.isPresent()) { + incoming = MergeYaml.parse(yaml); + return true; + } else { + return false; + } + })) .and(Validated.test("insertProperty", "Insert property must be filed when `insert mode` is either `BeforeProperty` or `AfterProperty`.", insertProperty, s -> insertMode == null || insertMode == Last || !isBlank(s))); } @@ -121,22 +141,18 @@ public String getDescription() { @Override public TreeVisitor getVisitor() { + //assert incoming != null; // silence compiler warning, incoming cannot be null here. Otherwise, validate() would have failed. return Preconditions.check(new FindSourceFiles(filePattern), new YamlIsoVisitor() { final JsonPathMatcher matcher = new JsonPathMatcher(key); - @Nullable Yaml incoming; - private Yaml incoming() { - if (incoming == null) { - incoming = parse(yaml); - } - return incoming; - } - @Override public Yaml.Document visitDocument(Yaml.Document document, ExecutionContext ctx) { + if (incoming == null) { + return super.visitDocument(document, ctx); + } if ("$".equals(key)) { Yaml.Document d = document.withBlock((Yaml.Block) - new MergeYamlVisitor<>(document.getBlock(), yaml, Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty) + new MergeYamlVisitor<>(document.getBlock(), incoming, Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty) .visitNonNull(document.getBlock(), ctx, getCursor()) ); if (getCursor().getMessage(REMOVE_PREFIX, false)) { @@ -153,14 +169,14 @@ public Yaml.Document visitDocument(Yaml.Document document, ExecutionContext ctx) } // If there is no space between the colon and the value it will not be interpreted as a mapping String snippet; - if (incoming() instanceof Yaml.Mapping) { + if (incoming instanceof Yaml.Mapping) { snippet = valueKey + ":\n" + indent(yaml); } else { snippet = valueKey + ":" + (yaml.startsWith(" ") ? yaml : " " + yaml); } // No matching element already exists, so it must be constructed //noinspection LanguageMismatch - return d.withBlock((Yaml.Block) new MergeYamlVisitor<>(d.getBlock(), snippet, + return d.withBlock((Yaml.Block) new MergeYamlVisitor<>(d.getBlock(), MergeYaml.parse(snippet), Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty).visitNonNull(d.getBlock(), ctx, getCursor())); } @@ -202,10 +218,13 @@ private String indent(String text) { @Override public Yaml.Mapping visitMapping(Yaml.Mapping mapping, ExecutionContext ctx) { + if (incoming == null) { + return super.visitMapping(mapping, ctx); + } Yaml.Mapping m = super.visitMapping(mapping, ctx); if (matcher.matches(getCursor())) { getCursor().putMessageOnFirstEnclosing(Yaml.Document.class, FOUND_MATCHING_ELEMENT, true); - m = (Yaml.Mapping) new MergeYamlVisitor<>(mapping, incoming(), Boolean.TRUE.equals(acceptTheirs), + m = (Yaml.Mapping) new MergeYamlVisitor<>(mapping, incoming, Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty).visitNonNull(mapping, ctx, getCursor().getParentOrThrow()); } return m; @@ -213,9 +232,12 @@ public Yaml.Mapping visitMapping(Yaml.Mapping mapping, ExecutionContext ctx) { @Override public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionContext ctx) { + if (incoming == null) { + return super.visitMappingEntry(entry, ctx); + } if (matcher.matches(getCursor())) { getCursor().putMessageOnFirstEnclosing(Yaml.Document.class, FOUND_MATCHING_ELEMENT, true); - Yaml.Block value = (Yaml.Block) new MergeYamlVisitor<>(entry.getValue(), incoming(), + Yaml.Block value = (Yaml.Block) new MergeYamlVisitor<>(entry.getValue(), incoming, Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty).visitNonNull(entry.getValue(), ctx, getCursor()); if (value instanceof Yaml.Scalar && value.getPrefix().isEmpty()) { @@ -228,10 +250,13 @@ public Yaml.Mapping.Entry visitMappingEntry(Yaml.Mapping.Entry entry, ExecutionC @Override public Yaml.Sequence visitSequence(Yaml.Sequence sequence, ExecutionContext ctx) { + if (incoming == null) { + return super.visitSequence(sequence, ctx); + } if (matcher.matches(getCursor().getParentOrThrow())) { getCursor().putMessageOnFirstEnclosing(Yaml.Document.class, FOUND_MATCHING_ELEMENT, true); return sequence.withEntries(ListUtils.map(sequence.getEntries(), - entry -> entry.withBlock((Yaml.Block) new MergeYamlVisitor<>(entry.getBlock(), incoming(), + entry -> entry.withBlock((Yaml.Block) new MergeYamlVisitor<>(entry.getBlock(), incoming, Boolean.TRUE.equals(acceptTheirs), objectIdentifyingProperty, insertMode, insertProperty) .visitNonNull(entry.getBlock(), ctx, new Cursor(getCursor(), entry))))); } @@ -240,9 +265,10 @@ public Yaml.Sequence visitSequence(Yaml.Sequence sequence, ExecutionContext ctx) }); } - static Yaml parse(@Language("yml") String yaml) { + static Optional maybeParse(@Language("yml") String yaml) { return new YamlParser().parse(yaml) .findFirst() + .filter(Yaml.Documents.class::isInstance) .map(Yaml.Documents.class::cast) .map(docs -> { // Any comments will have been put on the parent Document node, preserve by copying to the mapping @@ -255,7 +281,11 @@ static Yaml parse(@Language("yml") String yaml) { return s.withEntries(ListUtils.mapFirst(s.getEntries(), entry -> entry.withPrefix(doc.getPrefix()))); } return doc.getBlock().withPrefix(doc.getPrefix()); - }) + }); + } + + static Yaml parse(@Language("yml") String yaml) { + return maybeParse(yaml) .orElseThrow(() -> new IllegalArgumentException("Could not parse as YAML")); } } diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java index 18b66a9764b..68a6c2a6752 100644 --- a/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java +++ b/rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java @@ -89,6 +89,10 @@ public MergeYamlVisitor(Yaml.Block block, Yaml incoming, boolean acceptTheirs, @ this.shouldAutoFormat = shouldAutoFormat; } + /** + * @deprecated Use {@link #MergeYamlVisitor(Yaml.Block, Yaml, boolean, String, boolean, InsertMode, String)} instead. + */ + @Deprecated public MergeYamlVisitor(Yaml scope, @Language("yml") String yamlString, boolean acceptTheirs, @Nullable String objectIdentifyingProperty, @Nullable InsertMode insertMode, @Nullable String insertProperty) { this(scope, MergeYaml.parse(yamlString), acceptTheirs, objectIdentifyingProperty, insertMode, insertProperty); }