Skip to content

#943: rework combine of text nodes in XML merger - #2324

Open
Paras14 wants to merge 3 commits into
devonfw:mainfrom
Paras14:feature/943-rework-combine-text-nodes
Open

#943: rework combine of text nodes in XML merger#2324
Paras14 wants to merge 3 commits into
devonfw:mainfrom
Paras14:feature/943-rework-combine-text-nodes

Conversation

@Paras14

@Paras14 Paras14 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

This PR fixes #943

Implemented changes:

  • Fixed XmlMergeStrategy.replaceTextNode so that it no longer restarts its search at the first child of the result element on every call. It now receives the index to start searching from and returns the index to continue from, so the n-th textual child of the template is merged into the n-th textual child of the result.
  • Fixed XmlMergeStrategy.combineChildNodes to carry that index across loop iterations instead of calling replaceTextNode with no position information.
  • Handled the case where the template has more textual children than the result. The surplus nodes are appended.
  • Handled the case where the result has more textual children than the template. The surplus nodes are left untouched.
  • The node type is now taken from the template, so merging a CDATA section over a plain text node replaces the node instead of writing CDATA content into a plain text node. Previously the append path preserved the node type but the overwrite path did not.
  • The append path now trims the template text, which the overwrite path already did. Please confirm this is wanted, as the previous inconsistency may have been intentional.
  • Added test cases combine-text-nodes and combine-text-nodes-mixed.

This only affects elements that have more than one textual child, which is why it went unnoticed. Almost all configuration files handled by the merger use a single text node per element, and that case was and still is merged correctly. Multiple textual children occur when a CDATA section sits next to plain text, for example in formatter or file header templates.

Note that the code referenced in the issue has been moved since the issue was written. It is now in cli/src/main/java/com/devonfw/tools/ide/merge/xml/XmlMergeStrategy.java and the reported behaviour was unchanged by that move.


Testing instructions

Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:

  1. Run the test class com.devonfw.tools.ide.merge.xml.XmlMergerTest from your IDE. The relevant test method is testMerger, which is a parameterized test with one execution per subfolder of cli/src/test/resources/xmlmerger. All 21 executions should pass, 19 of which existed before this PR.
  2. Check the two executions named combine-text-nodes and combine-text-nodes-mixed. Each test folder contains template.xml as the merge template, target.xml as the existing workspace file, and result.xml as the expected merge output.
  3. To see the previous behaviour, copy the folder cli/src/test/resources/xmlmerger/combine-text-nodes onto a checkout without the change in XmlMergeStrategy.java and run XmlMergerTest again. The combine-text-nodes execution then fails with expected:<...text1<![CDATA[text2]]>text3...> but was:<...text3<![CDATA[text5]]>text6...>.
    The test cases cover the following:
Test folder Element Template Workspace file Covers
combine-text-nodes elem1 text, CDATA, text text, CDATA, text Equal number of textual children, which is the case reported in the issue
combine-text-nodes elem2 text, CDATA, text text Template has more textual children, surplus is appended
combine-text-nodes elem3 text text, CDATA, text Workspace file has more textual children, surplus is kept
combine-text-nodes elem4 CDATA text Template determines whether the result node is text or CDATA
combine-text-nodes-mixed elem1 text, element, text text, element, text Textual children separated by a child element

Checklist for this PR

Make sure everything is checked before merging this PR. For further info please also see
our DoD.

  • When running mvn clean test locally all tests pass and build is successful
  • PR title is of the form #«issue-id»: «brief summary» (e.g. #921: fixed setup.bat and not feature/921 fixed setup.bat). If no issue ID exists, title only.
  • PR top-level comment summaries what has been done and contains link to addressed issue(s)
  • PR and issue(s) have suitable labels
  • Issue is set to In Progress and assigned to you or there is no issue (might happen for very small PRs)
  • You followed all coding conventions
  • You have added the issue implemented by your PR in CHANGELOG.adoc unless issue is labelled
    with internal
  • You have not changed any dependency in pom.xml files or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc
  • You have formulated clear instructions on how to test your contribution under "Testing instructions"

@github-project-automation github-project-automation Bot moved this to 🆕 New in IDEasy board Aug 13, 2026
@Paras14 Paras14 self-assigned this Aug 13, 2026
@Paras14 Paras14 moved this from 🆕 New to Team Review in IDEasy board Aug 13, 2026
@Paras14 Paras14 added the merger workspace template merger (XML, JSON, properties) label Aug 13, 2026
@coveralls

coveralls commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 31963392865

Warning

No base build found for commit 31dabbf on main.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 72.922%

Details

  • Patch coverage: No coverable lines changed in this PR.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 17555
Covered Lines: 13351
Line Coverage: 76.05%
Relevant Branches: 7761
Covered Branches: 5110
Branch Coverage: 65.84%
Branches in Coverage %: Yes
Coverage Strength: 3.23 hits per line

💛 - Coveralls

@krystynaShatkovska
krystynaShatkovska self-requested a review August 13, 2026 07:54
@krystynaShatkovska krystynaShatkovska self-assigned this Aug 13, 2026
@krystynaShatkovska

Copy link
Copy Markdown
Contributor

All tests cases pass. combine-text-nodes-mixed and combine-text-nodes cover what is needed. About trim behavior, it is now consistent. As it overwrite and append path trim. Because of getText() method trim, append path now matches overwrite path. Good work!

@krystynaShatkovska krystynaShatkovska moved this from Team Review to 👀 In review in IDEasy board Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merger workspace template merger (XML, JSON, properties)

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

XmlMerger: Rework Combine Text Nodes

4 participants