From 0773c1596295b4bfe2146427ace548c038e806d7 Mon Sep 17 00:00:00 2001
From: Louis-Dominique Dubeau <ldd@lddubeau.com>
Date: Wed, 4 Jul 2018 09:38:13 -0400
Subject: [PATCH] refactor: always defer attribute processing

sax would follow different processing paths depending on whether xmlns
was on or off. We've reduced it to a single path to simplify the
logic.
---
 lib/saxes.js                  | 31 +++++-----------
 test/attribute-no-space.js    |  2 +-
 test/duplicate-attribute.js   |  3 +-
 test/xml-internal-entities.js | 67 ++++++++++++++++-------------------
 4 files changed, 43 insertions(+), 60 deletions(-)

diff --git a/lib/saxes.js b/lib/saxes.js
index 495807d0..a00e0c53 100644
--- a/lib/saxes.js
+++ b/lib/saxes.js
@@ -858,12 +858,6 @@ class SAXParser {
   }
 
   attrib() {
-    if (this.attribList.indexOf(this.attribName) !== -1 ||
-        this.tag.attributes[this.attribName]) {
-      this.attribName = this.attribValue = "";
-      return;
-    }
-
     if (this.opt.xmlns) {
       const { prefix, local } = qname(this.attribName, true);
 
@@ -886,21 +880,9 @@ class SAXParser {
           tag.ns[local] = this.attribValue;
         }
       }
-
-      // defer onattribute events until all attributes have been seen
-      // so any new bindings can take effect. preserve attribute order
-      // so deferred events can be emitted in document order
-      this.attribList.push([this.attribName, this.attribValue]);
-    }
-    else {
-      // in non-xmlns mode, we can emit the event right away
-      this.tag.attributes[this.attribName] = this.attribValue;
-      this.emitNode("onattribute", {
-        name: this.attribName,
-        value: this.attribValue,
-      });
     }
 
+    this.attribList.push([this.attribName, this.attribValue]);
     this.attribName = this.attribValue = "";
   }
 
@@ -957,7 +939,6 @@ ${XML_NAMESPACE}.`);
         }
       }
 
-      // handle deferred onattribute events
       // Note: do not apply default ns to attributes:
       //   http://www.w3.org/TR/REC-xml-names/#defaulting
       for (const [name, value] of this.attribList) {
@@ -981,8 +962,15 @@ ${XML_NAMESPACE}.`);
         this.tag.attributes[name] = a;
         this.emitNode("onattribute", a);
       }
-      this.attribList.length = 0;
     }
+    else {
+      for (const [name, value] of this.attribList) {
+        const a = { name, value };
+        this.tag.attributes[name] = value;
+        this.emitNode("onattribute", a);
+      }
+    }
+    this.attribList.length = 0;
 
     this.tag.isSelfClosing = !!selfClosing;
 
@@ -996,7 +984,6 @@ ${XML_NAMESPACE}.`);
       this.tagName = "";
     }
     this.attribName = this.attribValue = "";
-    this.attribList.length = 0;
   }
 
   closeTag() {
diff --git a/test/attribute-no-space.js b/test/attribute-no-space.js
index f060b1b3..a7aeccc5 100644
--- a/test/attribute-no-space.js
+++ b/test/attribute-no-space.js
@@ -6,8 +6,8 @@ require(".").test({
   xml: "<root attr1=\"first\"attr2=\"second\"/>",
   expect: [
     ["opentagstart", { name: "root", attributes: {} }],
-    ["attribute", { name: "attr1", value: "first" }],
     ["error", "No whitespace between attributes\nLine: 0\nColumn: 20\nChar: a"],
+    ["attribute", { name: "attr1", value: "first" }],
     ["attribute", { name: "attr2", value: "second" }],
     ["opentag", { name: "root", attributes: { attr1: "first", attr2: "second" }, isSelfClosing: true }],
     ["closetag", "root"],
diff --git a/test/duplicate-attribute.js b/test/duplicate-attribute.js
index 93d59aee..4c806cc6 100644
--- a/test/duplicate-attribute.js
+++ b/test/duplicate-attribute.js
@@ -9,9 +9,10 @@ require(".").test({
       attributes: {},
     }],
     ["attribute", { name: "id", value: "hello" }],
+    ["attribute", { name: "id", value: "there" }],
     ["opentag", {
       name: "span",
-      attributes: { id: "hello" },
+      attributes: { id: "there" },
       isSelfClosing: false,
     }],
     ["closetag", "span"],
diff --git a/test/xml-internal-entities.js b/test/xml-internal-entities.js
index e0292784..d6087480 100644
--- a/test/xml-internal-entities.js
+++ b/test/xml-internal-entities.js
@@ -1,9 +1,5 @@
 "use strict";
 
-const iExpect = [];
-const myAttributes = {};
-const ENTITIES = {};
-
 // generates xml like test0="&control;"
 const entitiesToTest = {
   // 'ENTITY_NAME': IS_VALID || [invalidCharPos, invalidChar],
@@ -22,27 +18,20 @@ const entitiesToTest = {
 };
 
 let xmlStart = "<a test=\"&amp;\" ";
-const xmlEnd = "/>";
-
-iExpect.push([
-  "opentagstart",
-  {
-    name: "a",
-    attributes: {},
-  },
-]);
-
-iExpect.push([
+const myAttributes = {};
+const attributeEvents = [[
   "attribute",
   {
     name: "test",
     value: "&",
   },
-]);
+]];
 myAttributes.test = "&";
 
 let entI = 0;
 
+const attributeErrors = [];
+const ENTITIES = {};
 // eslint-disable-next-line guard-for-in
 for (const entity in entitiesToTest) {
   const attribName = `test${entI}`;
@@ -52,13 +41,13 @@ for (const entity in entitiesToTest) {
   xmlStart += `${attribName}="&`;
 
   if (typeof entitiesToTest[entity] === "object") {
-    iExpect.push([
+    attributeErrors.push([
       "error",
       `Invalid character in entity name\nLine: 0\nColumn: ${
       xmlStart.length + entitiesToTest[entity][0] + 1
       }\nChar: ${entitiesToTest[entity][1]}`,
     ]);
-    iExpect.push([
+    attributeEvents.push([
       "attribute",
       { name: attribName, value: `&${entity};` },
     ]);
@@ -66,7 +55,8 @@ for (const entity in entitiesToTest) {
   }
   else {
     ENTITIES[entity] = attribValue;
-    iExpect.push(["attribute", { name: attribName, value: attribValue }]);
+    attributeEvents.push(["attribute",
+                          { name: attribName, value: attribValue }]);
     myAttributes[attribName] = attribValue;
   }
 
@@ -74,25 +64,30 @@ for (const entity in entitiesToTest) {
   entI++;
 }
 
-iExpect.push([
-  "opentag",
-  {
-    name: "a",
-    attributes: myAttributes,
-    isSelfClosing: true,
-  },
-]);
-iExpect.push(["closetag", "a"]);
-
 require(".").test({
   name: "xml internal entities",
-  expect: iExpect,
+  expect: [
+    [
+      "opentagstart",
+      {
+        name: "a",
+        attributes: {},
+      },
+    ],
+    ...attributeErrors,
+    ...attributeEvents,
+    [
+      "opentag",
+      {
+        name: "a",
+        attributes: myAttributes,
+        isSelfClosing: true,
+      },
+    ],
+    ["closetag", "a"],
+  ],
   fn(parser) {
-    // eslint-disable-next-line guard-for-in
-    for (const entity in entitiesToTest) {
-      parser.ENTITIES[entity] = ENTITIES[entity];
-    }
-
-    parser.write(xmlStart + xmlEnd).close();
+    Object.assign(parser.ENTITIES, ENTITIES);
+    parser.write(`${xmlStart}/>`).close();
   },
 });