Merge pull request #17 from donmendelson/issue16

Merge fails to remove attribute #16

Author: kleihan

.github/workflows/maven.yml

@@ -23,7 +23,7 @@ jobs:
     - name: Build with Maven
       run: mvn -B package --file pom.xml
     - name: Upload package
-      uses: actions/upload-artifact@v2
+      uses: actions/upload-artifact@v4
       with:
         name: Package
         path: target/*.jar

src/main/java/io/fixprotocol/xml/XmlMerge.java

@@ -35,16 +35,14 @@
 import javax.xml.xpath.XPathFactory;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.w3c.dom.Document;
-import org.w3c.dom.Element;
-import org.w3c.dom.Node;
-import org.w3c.dom.NodeList;
-import org.w3c.dom.Text;
+import org.w3c.dom.*;
 import org.xml.sax.SAXException;
 import io.fixprotocol.orchestra.event.EventListener;
 import io.fixprotocol.orchestra.event.EventListenerFactory;
 import io.fixprotocol.orchestra.event.TeeEventListener;
 
+import static org.w3c.dom.Node.ATTRIBUTE_NODE;
+
 /**
  * Merges a difference file created by {@link XmlDiff} into a baseline XML file to create a new XML
  * file
@@ -262,16 +260,25 @@ private void remove(final Document doc, XPath xpathEvaluator, Element patchOpEle
       final Node node =
           (Node) xpathEvaluator.compile(xpathExpression).evaluate(doc, XPathConstants.NODE);
       if (node != null) {
-        final Node parent = node.getParentNode();
-        if (parent != null) {
-          final NodeList children = parent.getChildNodes();
-          for (int i = 0; i < children.getLength(); i++) {
-            if (children.item(i) == node) {
-              parent.removeChild(node);
-              break;
+        if (ATTRIBUTE_NODE == node.getNodeType()) {
+          Attr attr = (Attr) node;
+          Element parent = attr.getOwnerElement();
+          parent.removeAttributeNode(attr);
+        } else {
+          Node parent = node.getParentNode();
+          if (parent != null) {
+            final NodeList children = parent.getChildNodes();
+            for (int i = 0; i < children.getLength(); i++) {
+              Node item = children.item(i);
+              if (item == node) {
+                parent.removeChild(node);
+                break;
+              }
             }
           }
         }
+      } else {
+        eventLogger.warn("XPath expression to remove not found; {0}", xpathExpression);
       }
     } catch (final XPathExpressionException e) {
       errors++;
@@ -304,7 +311,7 @@ private void replace(final Document doc, XPath xpathEvaluator, Element patchOpEl
             text.setNodeValue(value);
             siteNode.appendChild(text);
             break;
-          case Node.ATTRIBUTE_NODE:
+          case ATTRIBUTE_NODE:
           case Node.TEXT_NODE:
             siteNode.setNodeValue(value);
             break;

src/test/java/io/fixprotocol/xml/XmlDiffTest.java

@@ -34,6 +34,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.NodeList;
 
 class CustomLogFactory implements LoggerContextFactory {
   private final org.apache.logging.log4j.spi.LoggerContext ctx;
@@ -72,12 +74,15 @@ public void removeContext(org.apache.logging.log4j.spi.LoggerContext context) {
 
 public class XmlDiffTest {
 
+  private static DocumentBuilder docBuilder;
   private XmlMerge xmlMerge;
 
   @BeforeAll
   public static void setupOnce() throws Exception {
     new File("target/test").mkdirs();
     LogManager.setFactory(new CustomLogFactory());
+    DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
+    docBuilder = docFactory.newDocumentBuilder();
   }
 
   /**
@@ -95,9 +100,6 @@ public void unordered() throws Exception {
     final String diffFilename = "target/test/unordereddiff.xml";
     XmlDiff.main(new String[] {"src/test/resources/DiffTest1.xml", "src/test/resources/DiffTest2.xml", diffFilename, "-u"});
 
-
-    DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
-    DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
     Document doc = docBuilder.parse(diffFilename);
 
 //     Expectation:
@@ -127,9 +129,6 @@ public void ordered() throws Exception {
     final String diffFilename = "target/test/ordereddiff.xml";
     XmlDiff.main(new String[] {"src/test/resources/DiffTest1.xml", "src/test/resources/DiffTest2.xml", diffFilename});
 
-
-    DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
-    DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
     Document doc = docBuilder.parse(diffFilename);
 
     assertEquals(3, doc.getElementsByTagName("add").getLength());
@@ -160,11 +159,29 @@ public void epDiff() throws Exception {
     }
   }
 
-  
   @Test
   public void xsdDiff() throws Exception {
     final String diffFilename = "target/test/xsddiff.xml";    
     XmlDiff.main(new String[] {"src/test/resources/Enums-new.xsd", "src/test/resources/Enums-old.xsd", diffFilename, "-u"});
   }
 
+  @Test
+  public void removeAttribute() throws Exception {
+    final String mergedFilename = "target/test/Instrument-merged.xml";
+    final String diffFilename = "src/test/resources/Instrument-diff.xml";
+    final String baseFilename = "src/test/resources/Instrument-base.xml";
+
+    try (
+        final FileInputStream is1Baseline = new FileInputStream(baseFilename);
+        final FileInputStream isDiff = new FileInputStream(diffFilename);
+        final FileOutputStream osMerge = new FileOutputStream(mergedFilename)) {
+      xmlMerge.merge(is1Baseline, isDiff, osMerge);
+
+      Document doc = docBuilder.parse(mergedFilename);
+      NodeList elements = doc.getElementsByTagName("fixr:component");
+      Element element = (Element) elements.item(0);
+      assertEquals(0, element.getAttribute("added").length());
+    }
+  }
+
 }

src/test/resources/Instrument-base.xml

@@ -0,0 +1,7 @@
+<fixr:repository xmlns:dcterms="http://purl.org/dc/terms/" xmlns:fixml="http://fixprotocol.io/2024/orchestra/appinfo/fixml" xmlns:fixr="http://fixprotocol.io/2024/orchestra/repository" version="FIX.Latest_EP292" name="FIX.Latest">
+	<fixr:components>
+		<fixr:component id="1003" name="Instrument" scenario="base" added="FIX.Latest">
+			<fixr:fieldRef id="55" name="Symbol"/>
+		</fixr:component>
+	</fixr:components>
+</fixr:repository>

src/test/resources/Instrument-diff.xml

@@ -0,0 +1,3 @@
+<diff>
+    <remove sel="/fixr:repository/fixr:components[1]/fixr:component[@id='1003' and @scenario='base']/@added"/>
+</diff>