Skip to content

Commit 7a9d305

Browse files
AddToTagVisitor: skip insert when a semantically-equal child already exists
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<Content>, 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.
1 parent ea81d80 commit 7a9d305

2 files changed

Lines changed: 115 additions & 1 deletion

File tree

rewrite-xml/src/main/java/org/openrewrite/xml/AddToTagVisitor.java

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,37 @@ public class AddToTagVisitor<P> extends XmlVisitor<P> {
3333
@Nullable
3434
private final Comparator<Content> tagComparator;
3535

36+
private final boolean allowDuplicates;
37+
3638
public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd) {
37-
this(scope, tagToAdd, null);
39+
this(scope, tagToAdd, null, false);
40+
}
41+
42+
public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd, boolean allowDuplicates) {
43+
this(scope, tagToAdd, null, allowDuplicates);
3844
}
3945

4046
public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd, @Nullable Comparator<Content> tagComparator) {
47+
this(scope, tagToAdd, tagComparator, false);
48+
}
49+
50+
public AddToTagVisitor(Xml.Tag scope, Xml.Tag tagToAdd, @Nullable Comparator<Content> tagComparator, boolean allowDuplicates) {
4151
this.scope = scope;
4252
this.tagToAdd = tagToAdd;
4353
this.tagComparator = tagComparator;
54+
this.allowDuplicates = allowDuplicates;
4455
}
4556

4657
@Override
4758
public Xml visitTag(Xml.Tag t, P p) {
4859
if (scope.isScope(t)) {
60+
if (!allowDuplicates && t.getContent() != null) {
61+
for (Content existing : t.getContent()) {
62+
if (existing instanceof Xml.Tag && SemanticallyEqual.areEqual(existing, tagToAdd)) {
63+
return super.visitTag(t, p);
64+
}
65+
}
66+
}
4967
assert getCursor().getParent() != null;
5068
if (t.getClosing() == null) {
5169
t = t.withClosing(autoFormat(new Xml.Tag.Closing(Tree.randomId(), "\n",

rewrite-xml/src/test/java/org/openrewrite/xml/AddToTagTest.java

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,102 @@ public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
143143
);
144144
}
145145

146+
@Test
147+
void doesNotAddSemanticallyEqualDuplicate() {
148+
rewriteRun(
149+
spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() {
150+
@Override
151+
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
152+
doAfterVisit(new AddToTagVisitor<>(x.getRoot(), Xml.Tag.build("<bean id=\"myBean\"/>")));
153+
return super.visitDocument(x, ctx);
154+
}
155+
})),
156+
xml(
157+
"""
158+
<beans>
159+
<bean id="myBean"/>
160+
</beans>
161+
"""
162+
)
163+
);
164+
}
165+
166+
@Test
167+
void doesNotAddDuplicateIgnoringAttributeOrder() {
168+
rewriteRun(
169+
spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() {
170+
@Override
171+
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
172+
doAfterVisit(new AddToTagVisitor<>(x.getRoot(),
173+
Xml.Tag.build("<bean class=\"C\" id=\"myBean\"/>")));
174+
return super.visitDocument(x, ctx);
175+
}
176+
})),
177+
xml(
178+
"""
179+
<beans>
180+
<bean id="myBean" class="C"/>
181+
</beans>
182+
"""
183+
)
184+
);
185+
}
186+
187+
@Test
188+
void addsSemanticallyEqualDuplicateWhenAllowDuplicatesTrue() {
189+
rewriteRun(
190+
spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() {
191+
@Override
192+
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
193+
if (x.getRoot().getChildren().size() == 1) {
194+
doAfterVisit(new AddToTagVisitor<>(x.getRoot(),
195+
Xml.Tag.build("<bean id=\"myBean\"/>"), true));
196+
}
197+
return super.visitDocument(x, ctx);
198+
}
199+
})),
200+
xml(
201+
"""
202+
<beans>
203+
<bean id="myBean"/>
204+
</beans>
205+
""",
206+
"""
207+
<beans>
208+
<bean id="myBean"/>
209+
<bean id="myBean"/>
210+
</beans>
211+
"""
212+
)
213+
);
214+
}
215+
216+
@Test
217+
void addsWhenChildrenShareNameButDifferentAttributes() {
218+
rewriteRun(
219+
spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() {
220+
@Override
221+
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
222+
doAfterVisit(new AddToTagVisitor<>(x.getRoot(), Xml.Tag.build("<bean id=\"myBean2\"/>")));
223+
return super.visitDocument(x, ctx);
224+
}
225+
})),
226+
xml(
227+
"""
228+
<beans>
229+
<bean id="myBean"/>
230+
</beans>
231+
""",
232+
"""
233+
<beans>
234+
<bean id="myBean"/>
235+
<bean id="myBean2"/>
236+
</beans>
237+
"""
238+
)
239+
);
240+
}
241+
146242
@Issue("https://github.com/openrewrite/rewrite/issues/1392")
147243
@Test
148244
void preserveNonTagContent() {

0 commit comments

Comments
 (0)