From fd4fdb31b5154b7e96e42d9af0c9d4748f42b71b Mon Sep 17 00:00:00 2001
From: Nick O'Leary <nick.oleary@gmail.com>
Date: Mon, 15 Jan 2018 23:20:20 +0000
Subject: [PATCH] Better error reporting when module provides duplicate type

---
 red/runtime/index.js                          |  6 ++-
 red/runtime/locales/en-US/runtime.json        |  1 +
 red/runtime/nodes/registry/loader.js          |  6 +--
 red/runtime/nodes/registry/registry.js        | 18 +++++++-
 .../runtime/nodes/registry/registry_spec.js   | 46 +++++++++++++++++--
 5 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/red/runtime/index.js b/red/runtime/index.js
index 47ff205f7..9cf113207 100644
--- a/red/runtime/index.js
+++ b/red/runtime/index.js
@@ -117,7 +117,11 @@ function start() {
                 if (nodeErrors.length > 0) {
                     log.warn("------------------------------------------------------");
                     for (i=0;i<nodeErrors.length;i+=1) {
-                        log.warn("["+nodeErrors[i].name+"] "+nodeErrors[i].err);
+                        if (nodeErrors[i].err.code === "type_already_registered") {
+                            log.warn("["+nodeErrors[i].id+"] "+log._("server.type-already-registered",{type:nodeErrors[i].err.details.type,module: nodeErrors[i].err.details.moduleA}));
+                        } else {
+                            log.warn("["+nodeErrors[i].id+"] "+nodeErrors[i].err);
+                        }
                     }
                     log.warn("------------------------------------------------------");
                 }
diff --git a/red/runtime/locales/en-US/runtime.json b/red/runtime/locales/en-US/runtime.json
index 168e461ba..7cd1be2ff 100644
--- a/red/runtime/locales/en-US/runtime.json
+++ b/red/runtime/locales/en-US/runtime.json
@@ -20,6 +20,7 @@
         "errors-help": "Run with -v for details",
         "missing-modules": "Missing node modules:",
         "node-version-mismatch": "Node module cannot be loaded on this version. Requires: __version__ ",
+        "type-already-registered": "'__type__' already registered by module __module__",
         "removing-modules": "Removing modules from config",
         "added-types": "Added node types:",
         "removed-types": "Removed node types:",
diff --git a/red/runtime/nodes/registry/loader.js b/red/runtime/nodes/registry/loader.js
index 817b18668..4dbd62736 100644
--- a/red/runtime/nodes/registry/loader.js
+++ b/red/runtime/nodes/registry/loader.js
@@ -214,15 +214,15 @@ function loadNodeConfig(fileInfo) {
             } else {
                 var types = [];
 
-                var regExp = /<script ([^>]*)data-template-name=['"]([^'"]*)['"]/gi;
+                var regExp = /<script (?:[^>]*)data-template-name\s*=\s*['"]([^'"]*)['"]/gi;
                 var match = null;
 
                 while ((match = regExp.exec(content)) !== null) {
-                    types.push(match[2]);
+                    types.push(match[1]);
                 }
                 node.types = types;
 
-                var langRegExp = /^<script[^>]* data-lang=['"](.+?)['"]/i;
+                var langRegExp = /^<script[^>]* data-lang\s*=\s*['"](.+?)['"]/i;
                 regExp = /(<script[^>]* data-help-name=[\s\S]*?<\/script>)/gi;
                 match = null;
                 var mainContent = "";
diff --git a/red/runtime/nodes/registry/registry.js b/red/runtime/nodes/registry/registry.js
index 469c9830c..84f3f8504 100644
--- a/red/runtime/nodes/registry/registry.js
+++ b/red/runtime/nodes/registry/registry.js
@@ -69,7 +69,7 @@ function filterNodeInfo(n) {
         r.module = n.module;
     }
     if (n.hasOwnProperty("err")) {
-        r.err = n.err.toString();
+        r.err = n.err;
     }
     return r;
 }
@@ -184,8 +184,22 @@ function loadNodeConfigs() {
 function addNodeSet(id,set,version) {
     if (!set.err) {
         set.types.forEach(function(t) {
-            nodeTypeToId[t] = id;
+            if (nodeTypeToId.hasOwnProperty(t)) {
+                set.err = new Error("Type already registered");
+                set.err.code = "type_already_registered";
+                set.err.details = {
+                    type: t,
+                    moduleA: getNodeInfo(t).module,
+                    moduleB: set.module
+                }
+
+            }
         });
+        if (!set.err) {
+            set.types.forEach(function(t) {
+                nodeTypeToId[t] = id;
+            });
+        }
     }
     moduleNodes[set.module] = moduleNodes[set.module]||[];
     moduleNodes[set.module].push(set.name);
diff --git a/test/red/runtime/nodes/registry/registry_spec.js b/test/red/runtime/nodes/registry/registry_spec.js
index 2fffc9fb3..ce761c012 100644
--- a/test/red/runtime/nodes/registry/registry_spec.js
+++ b/test/red/runtime/nodes/registry/registry_spec.js
@@ -72,7 +72,15 @@ describe("red/nodes/registry/registry",function() {
         config: "configC",
         types: [ "test-c","test-d"]
     };
-
+    var testNodeSet3 = {
+        id: "test-module-2/test-name-3",
+        module: "test-module-2",
+        name: "test-name-3",
+        enabled: true,
+        loaded: false,
+        config: "configB",
+        types: [ "test-a","test-e"]
+    };
 
 
 
@@ -198,7 +206,39 @@ describe("red/nodes/registry/registry",function() {
            typeRegistry.addNodeSet("test-module/test-name-2",testNodeSet2WithError, "0.0.1");
 
            should.not.exist(typeRegistry.getTypeId("test-c"));
-         });
+       });
+
+       it('doesnt add node set if type already exists', function() {
+           typeRegistry.init(settings);
+           typeRegistry.getNodeList().should.have.lengthOf(0);
+           typeRegistry.getModuleList().should.eql({});
+
+           should.not.exist(typeRegistry.getTypeId("test-e"));
+
+           typeRegistry.addNodeSet("test-module/test-name",testNodeSet1, "0.0.1");
+           typeRegistry.getNodeList().should.have.lengthOf(1);
+           should.exist(typeRegistry.getTypeId("test-a"));
+           typeRegistry.addNodeSet(testNodeSet3.id,testNodeSet3, "0.0.1");
+           typeRegistry.getNodeList().should.have.lengthOf(2);
+
+           // testNodeSet3 registers a duplicate test-a and unique test-e
+           // as test-a is a duplicate, test-e should not get registered
+           should.not.exist(typeRegistry.getTypeId("test-e"));
+
+           var testNodeSet3Result = typeRegistry.getNodeList()[1];
+           should.exist(testNodeSet3Result.err);
+           testNodeSet3Result.err.code.should.equal("type_already_registered");
+           testNodeSet3Result.err.details.type.should.equal("test-a");
+           testNodeSet3Result.err.details.moduleA.should.equal("test-module");
+           testNodeSet3Result.err.details.moduleB.should.equal("test-module-2");
+
+           //
+           // typeRegistry.addNodeSet("test-module/test-name-2",testNodeSet2WithError, "0.0.1");
+           //
+           // should.not.exist(typeRegistry.getTypeId("test-c"));
+       });
+
+
     });
 
     describe("#enableNodeSet", function() {
@@ -334,7 +374,7 @@ describe("red/nodes/registry/registry",function() {
                 enabled: true,
                 loaded: false,
                 config: "configB",
-                types: [ "test-a","test-b"]
+                types: [ "test-c","test-d"]
             }, "0.0.1");
             typeRegistry.getNodeConfig("test-module/test-name-2").should.eql('configBHEtest-name-2LP');
             typeRegistry.getAllNodeConfigs().should.eql('configAHEtest-nameLPconfigBHEtest-name-2LP');