Repository navigation
Xpp3Dom.removeChild(int): fix childMap corruption with duplicate names - #395
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Xpp3Dom.removeChild(int) so the internal childMap stays consistent when multiple children share the same name, addressing the corruption described in issue #394.
Changes:
- Update
Xpp3Dom.removeChild(int)to only adjustchildMapwhen it currently points to the removed child, then re-point it to the last remaining child with that name (or remove the entry). - Add a regression test covering removal-by-index with duplicate child names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/apache/maven/shared/utils/xml/Xpp3Dom.java | Fixes removeChild(int) to correctly maintain childMap when duplicate-named children exist. |
| src/test/java/org/apache/maven/shared/utils/xml/pull/Xpp3DomTest.java | Adds a regression test ensuring removeChild(int) updates childMap correctly with duplicate child names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
LGTM, it just needs to remove the extra line. |
3b40b36 to
a0b3825
Compare
slachiewicz
left a comment
slachiewicz
left a comment
There was a problem hiding this comment.
The problem is real and the fix is right. After addChild(first); addChild(second) with the same name, childMap points at second. The old values().remove(child) dropped that entry when second was removed, so getChild("child") returned null although first was still in the list. The new code repoints the map at the last remaining child with that name, and leaves the map alone when it already points at a different child.
One non-blocking nit: Xpp3Dom(String) accepts a null name, and name.equals(c.getName()) would then throw where the old code did not; Objects.equals would cover it. Nothing in Xpp3DomBuilder creates a null-named element, so merging as is.
Verified: mvn -o test -Dtest=Xpp3DomTest on the rebased branch → 17 tests, 0 failures; CI green on all 12 jobs.
This comment was created with AI assistance.
Xpp3Dom.removeChild(int)(line 262) callschildMap.values().remove(child)to remove the child from the internal name-to-child map. This is incorrect when multiple children share the same name:childMapstores only the last child added with a given name (entries are overwritten byaddChild)values().remove(child)removes the first matching value — which is the only value for that name in the mapgetChild(name)correctly returns null but only by coincidence — the map entry was simply removedFix: After removing the child from
childList, check ifchildMappoints to that specific child (by reference). If so, scanchildListfor the last remaining child with the same name and update the map, or remove the entry if none remain.Fixes #394