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

Author: hohwille

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>