devonfw#640: fix xml merger if merge:id is omitted for repated element (

devonfw#643)

cli/pom.xml

@@ -96,6 +96,17 @@
  <version>1.14.15</version>
  <scope>test</scope>
  </dependency>
+ <!-- required for XML assertion (XmlMergerTest) -->
+ <dependency>
+ <groupId>org.xmlunit</groupId>
+ <artifactId>xmlunit-core</artifactId>
+ <version>2.9.0</version>
+ </dependency>
+ <dependency>
+ <groupId>org.xmlunit</groupId>
+ <artifactId>xmlunit-assertj3</artifactId>
+ <version>2.9.1</version>
+ </dependency>
  </dependencies>
 
  <build>

cli/src/main/java/com/devonfw/tools/ide/merge/xmlmerger/MergeStrategy.java

@@ -90,9 +90,6 @@ protected void updateAndRemoveNsAttributes(MergeElement mergeElement, ElementMat
 
  for (MergeAttribute attribute : mergeElement.getElementAttributes()) {
  if (attribute.isMergeNsAttr()) {
- if (attribute.isMergeNsIdAttr()) {
- elementMatcher.updateId(mergeElement.getQName(), attribute.getValue());
- }
  mergeElement.getElement().removeAttributeNode(attribute.getAttr());
  }
  }
@@ -154,7 +151,10 @@ protected void combineChildNodes(MergeElement updateElement, MergeElement target
  MergeElement sourceChildElement = new MergeElement((Element) updateChild, updateElement.getDocumentPath());
  MergeElement matchedTargetChild = elementMatcher.matchElement(sourceChildElement, targetElement);
  if (matchedTargetChild != null) {
- MergeStrategy childStrategy = MergeStrategy.of(sourceChildElement.getMergingStrategy());
+ MergeStrategy childStrategy = sourceChildElement.getMergingStrategy();
+ if (childStrategy == null) {
+ childStrategy = this;
+ }
  childStrategy.merge(sourceChildElement, matchedTargetChild, elementMatcher);
  } else {
  appendElement(sourceChildElement, targetElement, elementMatcher);

cli/src/main/java/com/devonfw/tools/ide/merge/xmlmerger/XmlMerger.java

@@ -99,7 +99,10 @@ public void merge(Document sourceDocument, Document targetDocument, Path source,
  MergeElement targetRoot = new MergeElement(targetDocument.getDocumentElement(), target);
 
  if (areRootsCompatible(sourceRoot, targetRoot)) {
- MergeStrategy strategy = MergeStrategy.of(sourceRoot.getMergingStrategy());
+ MergeStrategy strategy = sourceRoot.getMergingStrategy();
+ if (strategy == null) {
+ strategy = MergeStrategy.KEEP; // default strategy used as fallback
+ }
  strategy.merge(sourceRoot, targetRoot, new ElementMatcher(this.context));
  } else {
  this.context.warning("Root elements do not match. Skipping merge operation.");

cli/src/main/java/com/devonfw/tools/ide/merge/xmlmerger/matcher/ElementMatcher.java

@@ -24,19 +24,13 @@ public ElementMatcher(IdeContext context) {
  this.qNameIdMap = new HashMap<>();
  }
 
- /**
- * Updates the ID strategy for a given QName (qualified name) of an XML element.
- *
- * @param qname the qualified name (ns + local name) of the XML element
- * @param id the ID value (value of merge:id) to be used for matching the element
- */
- public void updateId(QName qname, String id) {
+ private IdComputer createIdComputer(String id, QName qname, MergeElement sourceElement) {
 
- IdComputer idComputer = new IdComputer(id);
- IdComputer duplicate = this.qNameIdMap.put(qname, idComputer);
- if (duplicate != null) {
- this.context.debug("ID replaced for element '{}': old ID '{}' -> new ID '{}'", qname, duplicate.getId(), idComputer.getId());
+ if ((id == null) || id.isEmpty()) {
+ throw new IllegalStateException(
+ "No merge:id value defined for element " + sourceElement.getXPath() + " in document " + sourceElement.getDocumentPath());
  }
+ return new IdComputer(id);
  }
 
  /**
@@ -51,11 +45,7 @@ public MergeElement matchElement(MergeElement sourceElement, MergeElement target
  String id = sourceElement.getId();
  QName qName = sourceElement.getQName();
 
- IdComputer idComputer = this.qNameIdMap.get(qName);
- if (idComputer == null) {
- updateId(qName, id);
- idComputer = this.qNameIdMap.get(qName);
- }
+ IdComputer idComputer = this.qNameIdMap.computeIfAbsent(qName, k -> createIdComputer(id, qName, sourceElement));
  Element matchedNode = idComputer.evaluateExpression(sourceElement, targetElement);
  if (matchedNode != null) {
  return new MergeElement(matchedNode, targetElement.getDocumentPath());

cli/src/main/java/com/devonfw/tools/ide/merge/xmlmerger/matcher/IdComputer.java

@@ -29,6 +29,7 @@ public class IdComputer {
 
  public IdComputer(String id) {
 
+ super();
  this.id = id;
  }
 

cli/src/main/java/com/devonfw/tools/ide/merge/xmlmerger/model/MergeElement.java

@@ -53,23 +53,15 @@ public Path getDocumentPath() {
  }
 
  /**
- * Retrieves the merge strategy associated with this MergeElement.
- *
- * @return the merge strategy
+ * @return the {@link MergeStrategy} of this element or {@code null} if undefined.
  */
- public String getMergingStrategy() {
+ public MergeStrategy getMergingStrategy() {
 
  String strategy = this.element.getAttributeNS(XmlMerger.MERGE_NS_URI, "strategy").toLowerCase();
  if (!strategy.isEmpty()) {
- return strategy;
- }
-
- // Inherit merging strategy from parent
- Element parent = getParentElement();
- if (parent != null) {
- return new MergeElement(parent, this.documentPath).getMergingStrategy();
+ return MergeStrategy.of(strategy);
  }
- return MergeStrategy.KEEP.name(); // should the default be keep?
+ return null;
  }
 
  /**
@@ -90,10 +82,7 @@ public String getId() {
  String idAttr = this.element.getAttribute("id");
  if (idAttr.isEmpty()) {
  idAttr = this.element.getAttribute("name");
- if (idAttr.isEmpty()) {
- throw new IllegalStateException(
- "No merge:id value defined for element " + getXPath() + " in document " + getDocumentPath());
- } else {
+ if (!idAttr.isEmpty()) {
  id = "@name";
  }
  } else {

cli/src/test/java/com/devonfw/tools/ide/merge/XmlMergerTest.java

@@ -7,11 +7,12 @@
 import java.nio.file.Path;
 import java.util.stream.Stream;
 
-import org.assertj.core.api.SoftAssertions;
-import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.xmlunit.assertj3.XmlAssert;
 
 import com.devonfw.tools.ide.context.AbstractIdeContextTest;
 import com.devonfw.tools.ide.context.IdeContext;
@@ -21,7 +22,7 @@ class XmlMergerTest extends AbstractIdeContextTest {
 
  private static Logger LOG = LoggerFactory.getLogger(XmlMergerTest.class);
 
- private static final Path TEST_RESOURCES = Path.of("src", "test", "resources", "xmlmerger");
+ private static final Path XML_TEST_RESOURCES = Path.of("src", "test", "resources", "xmlmerger");
 
  private static final String SOURCE_XML = "source.xml";
 
@@ -37,28 +38,24 @@ class XmlMergerTest extends AbstractIdeContextTest {
  * Tests the XML merger functionality across multiple test cases. This test method iterates through all subdirectories in the test resources folder, each
  * representing a different test case.
  */
- @Test
- void testMerger(@TempDir Path tempDir) throws Exception {
-
- try (Stream<Path> folders = Files.list(TEST_RESOURCES)) {
- // arrange
- SoftAssertions softly = new SoftAssertions();
- folders.forEach(folder -> {
- LOG.info("Testing XML merger for test-case {}", folder.getFileName());
- Path sourcePath = folder.resolve(SOURCE_XML);
- Path targetPath = tempDir.resolve(TARGET_XML);
- Path resultPath = folder.resolve(RESULT_XML);
- try {
- Files.copy(folder.resolve(TARGET_XML), targetPath, REPLACE_EXISTING);
- // act
- this.merger.merge(null, sourcePath, this.context.getVariables(), targetPath);
- // assert
- softly.assertThat(targetPath).hasContent(Files.readString(resultPath));
- } catch (IOException e) {
- throw new IllegalStateException(e);
- }
- });
- softly.assertAll();
- }
+ @ParameterizedTest
+ @MethodSource("xmlMergerTestCases")
+ void testMerger(Path folder, @TempDir Path tempDir) throws Exception {
+
+ // arrange
+ LOG.info("Testing XML merger for test-case {}", folder.getFileName());
+ Path sourcePath = folder.resolve(SOURCE_XML);
+ Path targetPath = tempDir.resolve(TARGET_XML);
+ Path resultPath = folder.resolve(RESULT_XML);
+ Files.copy(folder.resolve(TARGET_XML), targetPath, REPLACE_EXISTING);
+ // act
+ this.merger.merge(null, sourcePath, this.context.getVariables(), targetPath);
+ // assert
+ XmlAssert.assertThat(targetPath).and(resultPath.toFile()).areIdentical();
+ }
+
+ private static Stream<Path> xmlMergerTestCases() throws IOException {
+
+ return Files.list(XML_TEST_RESOURCES).filter(Files::isDirectory);
  }
 }

cli/src/test/resources/xmlmerger/id-fallback/result.xml

@@ -8,4 +8,7 @@
  <parent attr1="oldSomething2" id="parent2">
  <child>Old Child Text2</child>
  </parent>
+ <special class="id2">new special2</special>
+ <special class="id3">old special3</special>
+ <special class="id1">new special1</special>
 </root>

cli/src/test/resources/xmlmerger/id-fallback/source.xml

@@ -4,4 +4,6 @@
  <parent id="parent" attr1="newSomething">
  <child>New Child Text</child>
  </parent>
+ <special class="id1" merge:id="@class">new special1</special>
+ <special class="id2">new special2</special>
 </root>

cli/src/test/resources/xmlmerger/id-fallback/target.xml

@@ -8,4 +8,6 @@
  <parent id="parent2" attr1="oldSomething2">
  <child>Old Child Text2</child>
  </parent>
+ <special class="id2">old special2</special>
+ <special class="id3">old special3</special>
 </root>