From e15ecc00ce2227d4af5a670ecfabeb84e1f5305d Mon Sep 17 00:00:00 2001 From: Steve-Mcl Date: Thu, 29 Sep 2022 13:11:25 +0100 Subject: [PATCH 1/4] remove old unused code (5y+ not used) --- .../@node-red/nodes/core/network/10-mqtt.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js b/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js index 532d54d8e..d57b23cba 100644 --- a/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js +++ b/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js @@ -459,7 +459,7 @@ module.exports = function(RED) { if(!opts || typeof opts !== "object") { return; //nothing to change, simply return } - const originalBrokerURL = node.brokerurl; + // const originalBrokerURL = node.brokerurl; //apply property changes (only if the property exists in the opts object) setIfHasProperty(opts, node, "url", init); @@ -468,7 +468,7 @@ module.exports = function(RED) { setIfHasProperty(opts, node, "clientid", init); setIfHasProperty(opts, node, "autoConnect", init); setIfHasProperty(opts, node, "usetls", init); - setIfHasProperty(opts, node, "usews", init); + // setIfHasProperty(opts, node, "usews", init); setIfHasProperty(opts, node, "verifyservercert", init); setIfHasProperty(opts, node, "compatmode", init); setIfHasProperty(opts, node, "protocolVersion", init); @@ -567,9 +567,9 @@ module.exports = function(RED) { if (typeof node.usetls === 'undefined') { node.usetls = false; } - if (typeof node.usews === 'undefined') { - node.usews = false; - } + // if (typeof node.usews === 'undefined') { + // node.usews = false; + // } if (typeof node.verifyservercert === 'undefined') { node.verifyservercert = false; } From f11b9c1e18956b8268933b01b8a42379c36c5f36 Mon Sep 17 00:00:00 2001 From: Steve-Mcl Date: Thu, 29 Sep 2022 13:12:15 +0100 Subject: [PATCH 2/4] add test bad birth topic part of #3865 --- test/nodes/core/network/21-mqtt_spec.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/nodes/core/network/21-mqtt_spec.js b/test/nodes/core/network/21-mqtt_spec.js index 655cc9c73..390e50b91 100644 --- a/test/nodes/core/network/21-mqtt_spec.js +++ b/test/nodes/core/network/21-mqtt_spec.js @@ -430,6 +430,25 @@ describe('MQTT Nodes', function () { }; testSendRecv(brokerOptions, { topic: brokerOptions.birthTopic }, {}, options, hooks); }); + itConditional('should fail with bad birth topic', function (done) { + if (skipTests) { return this.skip() } + this.timeout = 2000; + const baseTopic = nextTopic(); + const brokerOptions = { + protocolVersion: 4, + birthTopic: baseTopic + "#", + birthPayload: "broker connected", + birthQos: 2, + } + const options = {}; + const hooks = { done: done, beforeLoad: null, afterLoad: null, afterConnect: null }; + options.expectMsg = { + topic: brokerOptions.birthTopic, + payload: brokerOptions.birthPayload, + qos: brokerOptions.birthQos + }; + testSendRecv(brokerOptions, { topic: brokerOptions.birthTopic }, {}, options, hooks); + }); itConditional('should publish close message', function (done) { if (skipTests) { return this.skip() } this.timeout = 2000; From 81b4874a7cb3cf9a105c1210f771154fa483270a Mon Sep 17 00:00:00 2001 From: Steve-Mcl Date: Thu, 29 Sep 2022 19:05:53 +0100 Subject: [PATCH 3/4] fix new test and fix bug found in previous PR --- .../@node-red/nodes/core/network/10-mqtt.js | 2 +- test/nodes/core/network/21-mqtt_spec.js | 37 ++++++++++++++----- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js b/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js index d57b23cba..73af668ac 100644 --- a/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js +++ b/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js @@ -994,7 +994,7 @@ module.exports = function(RED) { node.client.publish(msg.topic, msg.payload, options, function (err) { if (done) { done(err) - } else { + } else if(err) { node.error(err, msg) } }) diff --git a/test/nodes/core/network/21-mqtt_spec.js b/test/nodes/core/network/21-mqtt_spec.js index 390e50b91..8c772ca60 100644 --- a/test/nodes/core/network/21-mqtt_spec.js +++ b/test/nodes/core/network/21-mqtt_spec.js @@ -430,24 +430,38 @@ describe('MQTT Nodes', function () { }; testSendRecv(brokerOptions, { topic: brokerOptions.birthTopic }, {}, options, hooks); }); - itConditional('should fail with bad birth topic', function (done) { + itConditional('should safely discard bad birth topic', function (done) { if (skipTests) { return this.skip() } this.timeout = 2000; const baseTopic = nextTopic(); const brokerOptions = { protocolVersion: 4, - birthTopic: baseTopic + "#", + birthTopic: baseTopic + "#", // a publish topic should never have a wildcard birthPayload: "broker connected", birthQos: 2, } const options = {}; - const hooks = { done: done, beforeLoad: null, afterLoad: null, afterConnect: null }; - options.expectMsg = { - topic: brokerOptions.birthTopic, - payload: brokerOptions.birthPayload, - qos: brokerOptions.birthQos - }; - testSendRecv(brokerOptions, { topic: brokerOptions.birthTopic }, {}, options, hooks); + const hooks = { done: null, beforeLoad: null, afterLoad: null, afterConnect: null }; + hooks.afterLoad = (helperNode, mqttBroker, mqttIn, mqttOut) => { + helperNode.on("input", function (msg) { + try { + msg.should.have.a.property("error").type("object"); + msg.error.should.have.a.property("source").type("object"); + msg.error.source.should.have.a.property("id", mqttIn.id); + done(); + } catch (err) { + done(err) + } + }); + return true; //handled + } + options.expectMsg = null; + try { + testSendRecv(brokerOptions, { topic: brokerOptions.birthTopic }, {}, options, hooks); + done() + } catch(err) { + done(e) + } }); itConditional('should publish close message', function (done) { if (skipTests) { return this.skip() } @@ -606,12 +620,13 @@ function testSendRecv(brokerOptions, inNodeOptions, outNodeOptions, options, hoo const mqttBroker = helper.getNode(brokerOptions.id); const mqttIn = helper.getNode(nodes.mqtt_in.id); const mqttOut = helper.getNode(nodes.mqtt_out.id); - let afterLoadHandled = false; + let afterLoadHandled = false, finished = false; if (hooks.afterLoad) { afterLoadHandled = hooks.afterLoad(helperNode, mqttBroker, mqttIn, mqttOut) } if (!afterLoadHandled) { helperNode.on("input", function (msg) { + finished = true try { compareMsgToExpected(msg, expectMsg); if (hooks.done) { hooks.done(); } @@ -636,10 +651,12 @@ function testSendRecv(brokerOptions, inNodeOptions, outNodeOptions, options, hoo } }) .catch((e) => { + if(finished) { return } if (hooks.done) { hooks.done(e); } else { throw e; } }); } catch (err) { + if(finished) { return } if (hooks.done) { hooks.done(err); } else { throw err; } } From b0abba15a603ca13a1af76c0c9a397522fbe1c0f Mon Sep 17 00:00:00 2001 From: Steve-Mcl Date: Thu, 29 Sep 2022 19:08:46 +0100 Subject: [PATCH 4/4] remove dud code instead of commenting --- .../node_modules/@node-red/nodes/core/network/10-mqtt.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js b/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js index 73af668ac..aa378c2fe 100644 --- a/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js +++ b/packages/node_modules/@node-red/nodes/core/network/10-mqtt.js @@ -459,7 +459,6 @@ module.exports = function(RED) { if(!opts || typeof opts !== "object") { return; //nothing to change, simply return } - // const originalBrokerURL = node.brokerurl; //apply property changes (only if the property exists in the opts object) setIfHasProperty(opts, node, "url", init); @@ -468,7 +467,6 @@ module.exports = function(RED) { setIfHasProperty(opts, node, "clientid", init); setIfHasProperty(opts, node, "autoConnect", init); setIfHasProperty(opts, node, "usetls", init); - // setIfHasProperty(opts, node, "usews", init); setIfHasProperty(opts, node, "verifyservercert", init); setIfHasProperty(opts, node, "compatmode", init); setIfHasProperty(opts, node, "protocolVersion", init); @@ -567,9 +565,6 @@ module.exports = function(RED) { if (typeof node.usetls === 'undefined') { node.usetls = false; } - // if (typeof node.usews === 'undefined') { - // node.usews = false; - // } if (typeof node.verifyservercert === 'undefined') { node.verifyservercert = false; }