fixes #321: Binding sometimes doesn't work - added tests

Author: JoernT

src/createNodes.js

@@ -184,6 +184,59 @@ export default function createNodes(xpath, baseElement, foreElement) {
 
   if (!steps.length) return null;
 
+  /**
+   * If the first step of this path already exists as a real child of baseElement, reuse it
+   * instead of creating a duplicate sibling. Without this, a multi-step ref whose last step is
+   * missing (e.g. `cac:InvoicePeriod/cbc:DescriptionCode` when `cac:InvoicePeriod` already holds
+   * real data) would create a brand new, empty `cac:InvoicePeriod` alongside the existing one,
+   * orphaning the real data behind a duplicate element with the same name.
+   */
+  const firstStep = steps[0];
+  const isSimpleElementStep =
+    firstStep.nameTest &&
+    firstStep.nameTest !== '.' &&
+    !firstStep.nameTest.trim().startsWith('@') &&
+    firstStep.predicates.length === 0;
+
+  if (isSimpleElementStep && baseElement.children) {
+    const parsedFirst = parseName(firstStep.nameTest);
+    if (!parsedFirst.isValue) {
+      const wantedNs = parsedFirst.namespaceURI || null;
+      const existingChild = Array.from(baseElement.children).find(
+        child =>
+          child.localName === parsedFirst.localName && (child.namespaceURI || null) === wantedNs,
+      );
+
+      if (existingChild) {
+        const remainingSteps = steps.slice(1);
+        if (!remainingSteps.length) {
+          // The requested step already exists in full - nothing to create.
+          return null;
+        }
+
+        const remainingXPath = remainingSteps
+          .map(step => `${step.nameTest}${step.predicates.map(p => `[${p}]`).join('')}`)
+          .join('/');
+
+        const tailResult = createNodes(remainingXPath, existingChild, foreElement);
+        if (tailResult) {
+          const alreadyAttached =
+            tailResult.nodeType === Node.ATTRIBUTE_NODE
+              ? !!tailResult.ownerElement
+              : !!tailResult.parentNode;
+          if (!alreadyAttached) {
+            if (tailResult.nodeType === Node.ATTRIBUTE_NODE) {
+              existingChild.setAttributeNode(tailResult);
+            } else {
+              existingChild.appendChild(tailResult);
+            }
+          }
+        }
+        return tailResult;
+      }
+    }
+  }
+
   /**
    * Process a single step
    *

src/fx-fore.js

@@ -2079,14 +2079,16 @@ export class FxFore extends HTMLElement {
         if (!newNode || !parentNodeset) {
           continue;
         }
-        if (newNode.nodeType === Node.ATTRIBUTE_NODE) {
-          parentNodeset.setAttributeNode(newNode);
-        } else {
-          const referenceNode = this._findReferenceNodeForNewElement(newNode, parentNodeset, null);
-          if (referenceNode) {
-            referenceNode.after(newNode);
+        if (!this._isNodeAlreadyAttached(newNode)) {
+          if (newNode.nodeType === Node.ATTRIBUTE_NODE) {
+            parentNodeset.setAttributeNode(newNode);
           } else {
-            parentNodeset.prepend(newNode);
+            const referenceNode = this._findReferenceNodeForNewElement(newNode, parentNodeset, null);
+            if (referenceNode) {
+              referenceNode.after(newNode);
+            } else {
+              parentNodeset.prepend(newNode);
+            }
           }
         }
         bound.evalInContext();
@@ -2130,23 +2132,25 @@ export class FxFore extends HTMLElement {
         continue;
       }
 
-      if (newNode.nodeType === Node.ATTRIBUTE_NODE) {
-        parentNodeset.setAttributeNode(newNode);
-      } else {
-        let referenceNode = this._findReferenceNodeForNewElement(
-          newNode,
-          parentNodeset,
-          siblingControl,
-        );
+      if (!this._isNodeAlreadyAttached(newNode)) {
+        if (newNode.nodeType === Node.ATTRIBUTE_NODE) {
+          parentNodeset.setAttributeNode(newNode);
+        } else {
+          let referenceNode = this._findReferenceNodeForNewElement(
+            newNode,
+            parentNodeset,
+            siblingControl,
+          );
 
-        if (referenceNode) {
-          if (referenceNode.nodeType === Node.DOCUMENT_NODE) {
-            referenceNode.firstElementChild.append(newNode);
+          if (referenceNode) {
+            if (referenceNode.nodeType === Node.DOCUMENT_NODE) {
+              referenceNode.firstElementChild.append(newNode);
+            } else {
+              referenceNode.after(newNode);
+            }
           } else {
-            referenceNode.after(newNode);
+            parentNodeset.prepend(newNode);
           }
-        } else {
-          parentNodeset.prepend(newNode);
         }
       }
 
@@ -2159,6 +2163,17 @@ export class FxFore extends HTMLElement {
       }
     }
   }
+  /**
+   * `createNodes()` sometimes splices a created node directly into an existing real parent
+   * (when it reused an already-existing intermediate step) instead of returning a detached
+   * node for the caller to position. Callers use this to skip their own insertion logic in
+   * that case.
+   * @param {Node} node
+   */
+  _isNodeAlreadyAttached(node) {
+    return node.nodeType === Node.ATTRIBUTE_NODE ? !!node.ownerElement : !!node.parentNode;
+  }
+
   /**
    * Create Nodes from an XPath
    * @param {string} ref

test/createNodes.test.js

@@ -103,6 +103,29 @@ describe('createNodes', () => {
     expect(result).to.equal(null, 'The result should be null');
   });
 
+  it('reuses an existing intermediate element instead of creating a duplicate sibling (#321)', () => {
+    baseElement = new window.DOMParser().parseFromString(
+      '<xml><cac:InvoicePeriod xmlns:cac="urn:cac"><cbc:StartDate xmlns:cbc="urn:cbc">2019-02-01</cbc:StartDate><cbc:EndDate xmlns:cbc="urn:cbc">2019-05-07</cbc:EndDate></cac:InvoicePeriod></xml>',
+      'application/xml',
+    ).documentElement;
+    foreElement.setAttribute('xmlns:cac', 'urn:cac');
+    foreElement.setAttribute('xmlns:cbc', 'urn:cbc');
+
+    const result = createNodes('cac:InvoicePeriod/cbc:DescriptionCode', baseElement, foreElement);
+
+    // The new DescriptionCode leaf is returned...
+    expect(result).to.not.equal(null, 'The result should not be null');
+    expect(result.localName).to.equal('DescriptionCode');
+    // ...already spliced into the *existing* InvoicePeriod, not a fresh duplicate.
+    expect(result.parentNode).to.equal(baseElement.querySelector('InvoicePeriod'));
+    expect(baseElement.querySelectorAll('InvoicePeriod')).to.have.lengthOf(
+      1,
+      'no duplicate InvoicePeriod should have been created',
+    );
+    expect(baseElement.querySelector('StartDate').textContent).to.equal('2019-02-01');
+    expect(baseElement.querySelector('EndDate').textContent).to.equal('2019-05-07');
+  });
+
   describe('recursive processing', () => {
     it('can make a path with new expressions in the predicate', () => {
       const xpath = 'a[b/c]';

test/multistep.test.js

@@ -64,7 +64,7 @@ describe('multi-step refs', () => {
 
     expect(el.getModel().getModelItem('$default/AllowanceCharge[1]')).to.exist;
     expect(el.getModel().getModelItem('$default/AllowanceCharge[1]/TaxCategory[1]/ID[1]')).to.exist;
-    expect(el.getModel().getModelItem('$default/AllowanceCharge[1]/TaxCategory[2]/Percent[1]')).to
+    expect(el.getModel().getModelItem('$default/AllowanceCharge[1]/TaxCategory[1]/Percent[1]')).to
       .exist;
   });
 
@@ -130,7 +130,7 @@ describe('multi-step refs', () => {
     expect(el.getModel().getModelItem('$default/AllowanceCharge[2]_1')).to.exist;
     expect(el.getModel().getModelItem('$default/AllowanceCharge[2]_1/TaxCategory[1]/ID[1]')).to
       .exist;
-    expect(el.getModel().getModelItem('$default/AllowanceCharge[2]_1/TaxCategory[2]/Percent[1]')).to
+    expect(el.getModel().getModelItem('$default/AllowanceCharge[2]_1/TaxCategory[1]/Percent[1]')).to
       .exist;
   });
 
@@ -279,6 +279,6 @@ describe('multi-step refs', () => {
     control = repeatitems[1].querySelector('#BT-96');
     expect(control).to.exist;
     expect(mi6.observers.has(control)).to.be.true;
-    expect(mi6.path).to.equal('$default/AllowanceCharge[2]_1/TaxCategory[2]/Percent[1]');
+    expect(mi6.path).to.equal('$default/AllowanceCharge[2]_1/TaxCategory[1]/Percent[1]');
   });
 });