From 7a9d3055b167302cdc1a3b91db7356829dcdf7e7 Mon Sep 17 00:00:00 2001 From: Steve Elliott Date: Mon, 4 May 2026 12:22:38 -0400 Subject: [PATCH] AddToTagVisitor: skip insert when a semantically-equal child already exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make `AddToTagVisitor` idempotent by default: if `scope` already contains a child where `SemanticallyEqual.areEqual(existingChild, tagToAdd)` holds, return without mutating the tree. `SemanticallyEqual` ignores comments and attribute order, so this catches semantic duplicates that only differ in whitespace, attribute ordering, or interleaved comments. Preserves reference identity on no-op runs so that downstream "did this recipe change anything" checks (dirty-marker tracking, conditional `doAfterVisit` chains, recipe-cycle convergence) remain accurate. An `allowDuplicates` opt-out is provided for the rare caller that wants the prior unconditional-append behavior. Two new constructor overloads accept the flag without forcing callers to pass `null` for `tagComparator`: - `(Xml.Tag, Xml.Tag, boolean)` - `(Xml.Tag, Xml.Tag, @Nullable Comparator, boolean)` An audit of every in-tree caller (~22 sites in `rewrite-maven`, `rewrite-csharp`, and `rewrite-xml`) confirmed none want duplicate sibling tags — Maven POM, csproj, and `AddOrUpdateChild` (which already gates externally) all rely on the schemas forbidding duplicates or on caller-side checks that have been redundant until now. --- .../org/openrewrite/xml/AddToTagVisitor.java | 20 +++- .../org/openrewrite/xml/AddToTagTest.java | 96 +++++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/rewrite-xml/src/main/java/org/openrewrite/xml/AddToTagVisitor.java b/rewrite-xml/src/main/java/org/openrewrite/xml/AddToTagVisitor.java index fd3c7e333a8..8fc51b09b87 100755 --- a/rewrite-xml/src/main/java/org/openrewrite/xml/AddToTagVisitor.java +++ b/rewrite-xml/src/main/java/org/openrewrite/xml/AddToTagVisitor.java @@ -33,19 +33,37 @@ public class AddToTagVisitor

extends XmlVisitor

{ @Nullable private final Comparator tagComparator; + private final boolean allowDuplicates; + public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd) { - this(scope, tagToAdd, null); + this(scope, tagToAdd, null, false); + } + + public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd, boolean allowDuplicates) { + this(scope, tagToAdd, null, allowDuplicates); } public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd, @Nullable Comparator tagComparator) { + this(scope, tagToAdd, tagComparator, false); + } + + public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd, @Nullable Comparator tagComparator, boolean allowDuplicates) { this.scope = scope; this.tagToAdd = tagToAdd; this.tagComparator = tagComparator; + this.allowDuplicates = allowDuplicates; } @Override public Xml visitTag(Xml.Tag t, P p) { if (scope.isScope(t)) { + if (!allowDuplicates && t.getContent() != null) { + for (Content existing : t.getContent()) { + if (existing instanceof Xml.Tag && SemanticallyEqual.areEqual(existing, tagToAdd)) { + return super.visitTag(t, p); + } + } + } assert getCursor().getParent() != null; if (t.getClosing() == null) { t = t.withClosing(autoFormat(new Xml.Tag.Closing(Tree.randomId(), "\n", diff --git a/rewrite-xml/src/test/java/org/openrewrite/xml/AddToTagTest.java b/rewrite-xml/src/test/java/org/openrewrite/xml/AddToTagTest.java index bd0fba786e8..b3fa960b0ca 100755 --- a/rewrite-xml/src/test/java/org/openrewrite/xml/AddToTagTest.java +++ b/rewrite-xml/src/test/java/org/openrewrite/xml/AddToTagTest.java @@ -143,6 +143,102 @@ public Xml visitDocument(Xml.Document x, ExecutionContext ctx) { ); } + @Test + void doesNotAddSemanticallyEqualDuplicate() { + rewriteRun( + spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() { + @Override + public Xml visitDocument(Xml.Document x, ExecutionContext ctx) { + doAfterVisit(new AddToTagVisitor<>(x.getRoot(), Xml.Tag.build(""))); + return super.visitDocument(x, ctx); + } + })), + xml( + """ + + + + """ + ) + ); + } + + @Test + void doesNotAddDuplicateIgnoringAttributeOrder() { + rewriteRun( + spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() { + @Override + public Xml visitDocument(Xml.Document x, ExecutionContext ctx) { + doAfterVisit(new AddToTagVisitor<>(x.getRoot(), + Xml.Tag.build(""))); + return super.visitDocument(x, ctx); + } + })), + xml( + """ + + + + """ + ) + ); + } + + @Test + void addsSemanticallyEqualDuplicateWhenAllowDuplicatesTrue() { + rewriteRun( + spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() { + @Override + public Xml visitDocument(Xml.Document x, ExecutionContext ctx) { + if (x.getRoot().getChildren().size() == 1) { + doAfterVisit(new AddToTagVisitor<>(x.getRoot(), + Xml.Tag.build(""), true)); + } + return super.visitDocument(x, ctx); + } + })), + xml( + """ + + + + """, + """ + + + + + """ + ) + ); + } + + @Test + void addsWhenChildrenShareNameButDifferentAttributes() { + rewriteRun( + spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() { + @Override + public Xml visitDocument(Xml.Document x, ExecutionContext ctx) { + doAfterVisit(new AddToTagVisitor<>(x.getRoot(), Xml.Tag.build(""))); + return super.visitDocument(x, ctx); + } + })), + xml( + """ + + + + """, + """ + + + + + """ + ) + ); + } + @Issue("https://github.com/openrewrite/rewrite/issues/1392") @Test void preserveNonTagContent() {