From ed351eee549e99171a3e63d72c51a6c99cf51303 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Mon, 12 Apr 2021 20:30:31 +0100 Subject: [PATCH 1/8] Move hooks to util package --- .../node_modules/@node-red/runtime/lib/flows/Flow.js | 2 +- packages/node_modules/@node-red/runtime/lib/index.js | 3 +-- .../node_modules/@node-red/runtime/lib/nodes/Node.js | 2 +- packages/node_modules/@node-red/util/index.js | 10 +++++++++- .../@node-red/{runtime => util}/lib/hooks.js | 4 ++-- .../unit/@node-red/{runtime => util}/lib/hooks_spec.js | 6 +++--- 6 files changed, 17 insertions(+), 10 deletions(-) rename packages/node_modules/@node-red/{runtime => util}/lib/hooks.js (99%) rename test/unit/@node-red/{runtime => util}/lib/hooks_spec.js (98%) diff --git a/packages/node_modules/@node-red/runtime/lib/flows/Flow.js b/packages/node_modules/@node-red/runtime/lib/flows/Flow.js index c0eb122c9..35fdc67fc 100644 --- a/packages/node_modules/@node-red/runtime/lib/flows/Flow.js +++ b/packages/node_modules/@node-red/runtime/lib/flows/Flow.js @@ -19,7 +19,7 @@ var redUtil = require("@node-red/util").util; const events = require("@node-red/util").events; var flowUtil = require("./util"); const context = require('../nodes/context'); -const hooks = require("../hooks"); +const hooks = require("@node-red/util").hooks; var Subflow; var Log; diff --git a/packages/node_modules/@node-red/runtime/lib/index.js b/packages/node_modules/@node-red/runtime/lib/index.js index 30481740f..79c69e77c 100644 --- a/packages/node_modules/@node-red/runtime/lib/index.js +++ b/packages/node_modules/@node-red/runtime/lib/index.js @@ -20,7 +20,6 @@ var redNodes = require("./nodes"); var flows = require("./flows"); var storage = require("./storage"); var library = require("./library"); -var hooks = require("./hooks"); var plugins = require("./plugins"); var settings = require("./settings"); @@ -29,7 +28,7 @@ var path = require('path'); var fs = require("fs"); var os = require("os"); -const {log,i18n,events,exec,util} = require("@node-red/util"); +const {log,i18n,events,exec,util,hooks} = require("@node-red/util"); var runtimeMetricInterval = null; diff --git a/packages/node_modules/@node-red/runtime/lib/nodes/Node.js b/packages/node_modules/@node-red/runtime/lib/nodes/Node.js index faf07bda5..c13c812a9 100644 --- a/packages/node_modules/@node-red/runtime/lib/nodes/Node.js +++ b/packages/node_modules/@node-red/runtime/lib/nodes/Node.js @@ -21,7 +21,7 @@ var redUtil = require("@node-red/util").util; var Log = require("@node-red/util").log; var context = require("./context"); var flows = require("../flows"); -const hooks = require("../hooks"); +const hooks = require("@node-red/util").hooks; const NOOP_SEND = function() {} diff --git a/packages/node_modules/@node-red/util/index.js b/packages/node_modules/@node-red/util/index.js index aeb566540..f5dfd3449 100644 --- a/packages/node_modules/@node-red/util/index.js +++ b/packages/node_modules/@node-red/util/index.js @@ -19,6 +19,7 @@ const i18n = require("./lib/i18n"); const util = require("./lib/util"); const events = require("./lib/events"); const exec = require("./lib/exec"); +const hooks = require("./lib/hooks"); /** * This module provides common utilities for the Node-RED runtime and editor @@ -69,5 +70,12 @@ module.exports = { * @mixes @node-red/util_exec * @memberof @node-red/util */ - exec: exec + exec: exec, + + /** + * Runtime hooks + * @mixes @node-red/util_hooks + * @memberof @node-red/util + */ + hooks: hooks } diff --git a/packages/node_modules/@node-red/runtime/lib/hooks.js b/packages/node_modules/@node-red/util/lib/hooks.js similarity index 99% rename from packages/node_modules/@node-red/runtime/lib/hooks.js rename to packages/node_modules/@node-red/util/lib/hooks.js index 4ae537cc4..19912c7f2 100644 --- a/packages/node_modules/@node-red/runtime/lib/hooks.js +++ b/packages/node_modules/@node-red/util/lib/hooks.js @@ -1,4 +1,4 @@ -const Log = require("@node-red/util").log; +const Log = require("./log.js"); const VALID_HOOKS = [ // Message Routing Path @@ -179,4 +179,4 @@ module.exports = { add, remove, trigger -} \ No newline at end of file +} diff --git a/test/unit/@node-red/runtime/lib/hooks_spec.js b/test/unit/@node-red/util/lib/hooks_spec.js similarity index 98% rename from test/unit/@node-red/runtime/lib/hooks_spec.js rename to test/unit/@node-red/util/lib/hooks_spec.js index 4fb95b68a..d24729070 100644 --- a/test/unit/@node-red/runtime/lib/hooks_spec.js +++ b/test/unit/@node-red/util/lib/hooks_spec.js @@ -1,9 +1,9 @@ const should = require("should"); const NR_TEST_UTILS = require("nr-test-utils"); -const hooks = NR_TEST_UTILS.require("@node-red/runtime/lib/hooks"); +const hooks = NR_TEST_UTILS.require("@node-red/util/lib/hooks"); -describe("runtime/hooks", function() { +describe("util/hooks", function() { afterEach(function() { hooks.clear(); }) @@ -81,7 +81,7 @@ describe("runtime/hooks", function() { hooks.has("onSend.A").should.be.false(); hooks.has("onSend.B").should.be.false(); hooks.has("onSend").should.be.false(); - + done(err); } catch(err2) { done(err2); From 22df59e2295bb4902079609c7a226d54d54e827d Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Thu, 15 Apr 2021 15:11:27 +0100 Subject: [PATCH 2/8] Update hooks api to support promise api --- .../node_modules/@node-red/util/lib/hooks.js | 44 +++++++++++++---- test/unit/@node-red/util/lib/hooks_spec.js | 47 +++++++++++++++++++ 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/packages/node_modules/@node-red/util/lib/hooks.js b/packages/node_modules/@node-red/util/lib/hooks.js index 19912c7f2..a49104813 100644 --- a/packages/node_modules/@node-red/util/lib/hooks.js +++ b/packages/node_modules/@node-red/util/lib/hooks.js @@ -8,7 +8,12 @@ const VALID_HOOKS = [ "postDeliver", "onReceive", "postReceive", - "onComplete" + "onComplete", + // Module install hooks + "preInstall", + "postInstall", + "preUninstall", + "postUninstall" ] @@ -35,12 +40,12 @@ let labelledHooks = { } * - `postReceive` - passed a `ReceiveEvent` when the message has been given to the node's `input` handler(s) * - `onComplete` - passed a `CompleteEvent` when the node has completed with a message or logged an error * - * @mixin @node-red/runtime_hooks + * @mixin @node-red/util_hooks */ /** * Register a handler to a named hook - * @memberof @node-red/runtime_hooks + * @memberof @node-red/util_hooks * @param {String} hookId - the name of the hook to attach to * @param {Function} callback - the callback function for the hook */ @@ -68,7 +73,7 @@ function add(hookId, callback) { /** * Remove a handled from a named hook - * @memberof @node-red/runtime_hooks + * @memberof @node-red/util_hooks * @param {String} hookId - the name of the hook event to remove - must be `name.label` */ function remove(hookId) { @@ -110,11 +115,33 @@ function removeHook(id,callback) { function trigger(hookId, payload, done) { const hookStack = hooks[hookId]; if (!hookStack || hookStack.length === 0) { - done(); - return; + if (done) { + done(); + return; + } else { + return Promise.resolve(); + } } + if (!done) { + return new Promise((resolve,reject) => { + invokeStack(hookStack,payload,function(err) { + if (err !== undefined && err !== false) { + if (!(err instanceof Error)) { + err = new Error(err); + } + err.hook = hookId + reject(err); + } else { + resolve(err); + } + }) + }); + } else { + invokeStack(hookStack,payload,done) + } +} +function invokeStack(hookStack,payload,done) { let i = 0; - function callNextHook(err) { if (i === hookStack.length || err) { done(err); @@ -148,8 +175,6 @@ function trigger(hookId, payload, done) { } } } - callNextHook(); - function handleResolve(result) { if (result === undefined) { callNextHook(); @@ -157,6 +182,7 @@ function trigger(hookId, payload, done) { done(result); } } + callNextHook(); } function clear() { diff --git a/test/unit/@node-red/util/lib/hooks_spec.js b/test/unit/@node-red/util/lib/hooks_spec.js index d24729070..121486ade 100644 --- a/test/unit/@node-red/util/lib/hooks_spec.js +++ b/test/unit/@node-red/util/lib/hooks_spec.js @@ -249,4 +249,51 @@ describe("util/hooks", function() { done(); }) }) + + + it("handler can use callback function - promise API", function(done) { + hooks.add("onSend.A", function(payload, done) { + setTimeout(function() { + payload.order.push("A") + done() + },30) + }) + hooks.add("onSend.B", function(payload) { payload.order.push("B") } ) + + let data = { order:[] }; + hooks.trigger("onSend",data).then(() => { + data.order.should.eql(["A","B"]) + done() + }).catch(done) + }) + + it("handler can halt execution - promise API", function(done) { + hooks.add("onSend.A", function(payload, done) { + setTimeout(function() { + payload.order.push("A") + done(false) + },30) + }) + hooks.add("onSend.B", function(payload) { payload.order.push("B") } ) + + let data = { order:[] }; + hooks.trigger("onSend",data).then(() => { + data.order.should.eql(["A"]) + done() + }).catch(done) + }) + + it("handler can halt execution on error - promise API", function(done) { + hooks.add("onSend.A", function(payload, done) { + throw new Error("error"); + }) + hooks.add("onSend.B", function(payload) { payload.order.push("B") } ) + + let data = { order:[] }; + hooks.trigger("onSend",data).then(() => { + done("hooks.trigger resolved unexpectedly") + }).catch(err => { + done(); + }) + }) }); From 8140057beaa51f4ee2ee8b12a97de501bfe1f5a5 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Thu, 15 Apr 2021 15:11:45 +0100 Subject: [PATCH 3/8] Add pre/postInstall hooks to module install path --- .../@node-red/registry/lib/externalModules.js | 19 +++- .../@node-red/registry/lib/installer.js | 101 ++++++++++++------ .../runtime/locales/en-US/runtime.json | 1 + .../registry/lib/externalModules_spec.js | 23 +++- .../@node-red/registry/lib/installer_spec.js | 62 ++++++++++- 5 files changed, 170 insertions(+), 36 deletions(-) diff --git a/packages/node_modules/@node-red/registry/lib/externalModules.js b/packages/node_modules/@node-red/registry/lib/externalModules.js index 9587adf60..2bedcce1d 100644 --- a/packages/node_modules/@node-red/registry/lib/externalModules.js +++ b/packages/node_modules/@node-red/registry/lib/externalModules.js @@ -9,6 +9,7 @@ const path = require("path"); const clone = require("clone"); const exec = require("@node-red/util").exec; const log = require("@node-red/util").log; +const hooks = require("@node-red/util").hooks; const BUILTIN_MODULES = require('module').builtinModules; const EXTERNAL_MODULES_DIR = "externalModules"; @@ -190,12 +191,22 @@ async function installModule(moduleDetails) { await ensureModuleDir(); var args = ["install", installSpec, "--production"]; - return exec.run(NPM_COMMAND, args, { - cwd: installDir - },true).then(result => { + let triggerPayload = { + "module": moduleDetails.module, + "version": moduleDetails.version, + "dir": installDir, + } + return hooks.trigger("preInstall", triggerPayload).then(() => { + // preInstall passed + // - run install + log.trace(NPM_COMMAND + JSON.stringify(args)); + return exec.run(NPM_COMMAND, args, { cwd: installDir },true) + }).then(() => { + return hooks.trigger("postInstall", triggerPayload) + }).then(() => { log.info(log._("server.install.installed", { name: installSpec })); }).catch(result => { - var output = result.stderr; + var output = result.stderr || result.toString(); var e; if (/E404/.test(output) || /ETARGET/.test(output)) { log.error(log._("server.install.install-failed-not-found",{name:installSpec})); diff --git a/packages/node_modules/@node-red/registry/lib/installer.js b/packages/node_modules/@node-red/registry/lib/installer.js index 459220774..ef38b89a2 100644 --- a/packages/node_modules/@node-red/registry/lib/installer.js +++ b/packages/node_modules/@node-red/registry/lib/installer.js @@ -23,7 +23,7 @@ const tar = require("tar"); const registry = require("./registry"); const registryUtil = require("./util"); const library = require("./library"); -const {exec,log,events} = require("@node-red/util"); +const {exec,log,events,hooks} = require("@node-red/util"); const child_process = require('child_process'); const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm'; let installerEnabled = false; @@ -169,10 +169,23 @@ async function installModule(module,version,url) { var installDir = settings.userDir || process.env.NODE_RED_HOME || "."; var args = ['install','--no-audit','--no-update-notifier','--no-fund','--save','--save-prefix=~','--production',installName]; - log.trace(npmCommand + JSON.stringify(args)); - return exec.run(npmCommand,args,{ - cwd: installDir - }, true).then(result => { + let triggerPayload = { + "module": module, + "version": version, + "url": url, + "dir": installDir, + "isExisting": isExisting, + "isUpgrade": isUpgrade + } + + return hooks.trigger("preInstall", triggerPayload).then(() => { + // preInstall passed + // - run install + log.trace(npmCommand + JSON.stringify(args)); + return exec.run(npmCommand,args,{ cwd: installDir}, true) + }).then(() => { + return hooks.trigger("postInstall", triggerPayload) + }).then(() => { if (isExisting) { // This is a module we already have installed as a non-user module. // That means it was discovered when loading, but was not listed @@ -191,29 +204,45 @@ async function installModule(module,version,url) { events.emit("runtime-event",{id:"restart-required",payload:{type:"warning",text:"notification.warnings.restartRequired"},retain:true}); return require("./registry").setModulePendingUpdated(module,version); } - }).catch(result => { - var output = result.stderr; - var e; - var lookFor404 = new RegExp(" 404 .*"+module,"m"); - var lookForVersionNotFound = new RegExp("version not found: "+module+"@"+version,"m"); - if (lookFor404.test(output)) { - log.warn(log._("server.install.install-failed-not-found",{name:module})); - e = new Error("Module not found"); - e.code = 404; - throw e; - } else if (isUpgrade && lookForVersionNotFound.test(output)) { - log.warn(log._("server.install.upgrade-failed-not-found",{name:module})); - e = new Error("Module not found"); - e.code = 404; - throw e; - } else { + }).catch(err => { + let e; + if (err.hook) { + // preInstall failed log.warn(log._("server.install.install-failed-long",{name:module})); log.warn("------------------------------------------"); - log.warn(output); + log.warn(err.toString()); log.warn("------------------------------------------"); - throw new Error(log._("server.install.install-failed")); + e = new Error(log._("server.install.install-failed")+": "+err.toString()); + if (err.hook === "postInstall") { + return exec.run(npmCommand,["remove",module],{ cwd: installDir}, false).finally(() => { + throw e; + }) + } + } else { + // npm install failed + let output = err.stderr; + let lookFor404 = new RegExp(" 404 .*"+module,"m"); + let lookForVersionNotFound = new RegExp("version not found: "+module+"@"+version,"m"); + if (lookFor404.test(output)) { + log.warn(log._("server.install.install-failed-not-found",{name:module})); + e = new Error("Module not found"); + e.code = 404; + } else if (isUpgrade && lookForVersionNotFound.test(output)) { + log.warn(log._("server.install.upgrade-failed-not-found",{name:module})); + e = new Error("Module not found"); + e.code = 404; + } else { + log.warn(log._("server.install.install-failed-long",{name:module})); + log.warn("------------------------------------------"); + log.warn(output); + log.warn("------------------------------------------"); + e = new Error(log._("server.install.install-failed")); + } } - }) + if (e) { + throw e; + } + }); }).catch(err => { // In case of error, reset activePromise to be resolvable activePromise = Promise.resolve(); @@ -412,17 +441,29 @@ function uninstallModule(module) { log.info(log._("server.install.uninstalling",{name:module})); var args = ['remove','--no-audit','--no-update-notifier','--no-fund','--save',module]; - log.trace(npmCommand + JSON.stringify(args)); - exec.run(npmCommand,args,{ - cwd: installDir, - },true).then(result => { + let triggerPayload = { + "module": module, + "dir": installDir, + } + return hooks.trigger("preUninstall", triggerPayload).then(() => { + // preUninstall passed + // - run uninstall + log.trace(npmCommand + JSON.stringify(args)); + return exec.run(npmCommand,args,{ cwd: installDir}, true) + }).then(() => { log.info(log._("server.install.uninstalled",{name:module})); reportRemovedModules(list); library.removeExamplesDir(module); - resolve(list); + return hooks.trigger("postUninstall", triggerPayload).catch((err)=>{ + log.warn("------------------------------------------"); + log.warn(err.toString()); + log.warn("------------------------------------------"); + }).finally(() => { + resolve(list); + }) }).catch(result => { - var output = result.stderr; + let output = result.stderr || result; log.warn(log._("server.install.uninstall-failed-long",{name:module})); log.warn("------------------------------------------"); log.warn(output.toString()); diff --git a/packages/node_modules/@node-red/runtime/locales/en-US/runtime.json b/packages/node_modules/@node-red/runtime/locales/en-US/runtime.json index 1a29f32b4..029d36b16 100644 --- a/packages/node_modules/@node-red/runtime/locales/en-US/runtime.json +++ b/packages/node_modules/@node-red/runtime/locales/en-US/runtime.json @@ -34,6 +34,7 @@ "install-failed-not-found": "$t(server.install.install-failed-long) module not found", "install-failed-name": "$t(server.install.install-failed-long) invalid module name: __name__", "install-failed-url": "$t(server.install.install-failed-long) invalid url: __url__", + "post-install-error": "Error running 'postInstall' hook:", "upgrading": "Upgrading module: __name__ to version: __version__", "upgraded": "Upgraded module: __name__. Restart Node-RED to use the new version", "upgrade-failed-not-found": "$t(server.install.install-failed-long) version not found", diff --git a/test/unit/@node-red/registry/lib/externalModules_spec.js b/test/unit/@node-red/registry/lib/externalModules_spec.js index d49129bc4..99c74786f 100644 --- a/test/unit/@node-red/registry/lib/externalModules_spec.js +++ b/test/unit/@node-red/registry/lib/externalModules_spec.js @@ -14,6 +14,7 @@ const os = require("os"); const NR_TEST_UTILS = require("nr-test-utils"); const externalModules = NR_TEST_UTILS.require("@node-red/registry/lib/externalModules"); const exec = NR_TEST_UTILS.require("@node-red/util/lib/exec"); +const hooks = NR_TEST_UTILS.require("@node-red/util/lib/hooks"); let homeDir; @@ -40,6 +41,7 @@ describe("externalModules api", function() { await createUserDir() }) afterEach(async function() { + hooks.clear(); await fs.remove(homeDir); }) describe("checkFlowDependencies", function() { @@ -102,6 +104,25 @@ describe("externalModules api", function() { fs.existsSync(path.join(homeDir,"externalModules")).should.be.true(); }) + + it("calls pre/postInstall hooks", async function() { + externalModules.init({userDir: homeDir}); + externalModules.register("function", "libs"); + let receivedPreEvent,receivedPostEvent; + hooks.add("preInstall", function(event) { receivedPreEvent = event; }) + hooks.add("postInstall", function(event) { receivedPostEvent = event; }) + + await externalModules.checkFlowDependencies([ + {type: "function", libs:[{module: "foo"}]} + ]) + exec.run.called.should.be.true(); + receivedPreEvent.should.have.property("module","foo") + receivedPreEvent.should.have.property("version") + receivedPreEvent.should.have.property("dir") + receivedPreEvent.should.eql(receivedPostEvent) + fs.existsSync(path.join(homeDir,"externalModules")).should.be.true(); + }) + it("installs missing modules from inside subflow module", async function() { externalModules.init({userDir: homeDir}); externalModules.register("function", "libs"); @@ -299,4 +320,4 @@ describe("externalModules api", function() { } }) }) -}); \ No newline at end of file +}); diff --git a/test/unit/@node-red/registry/lib/installer_spec.js b/test/unit/@node-red/registry/lib/installer_spec.js index 4cba4b812..a9b6b71b8 100644 --- a/test/unit/@node-red/registry/lib/installer_spec.js +++ b/test/unit/@node-red/registry/lib/installer_spec.js @@ -25,7 +25,7 @@ var NR_TEST_UTILS = require("nr-test-utils"); var installer = NR_TEST_UTILS.require("@node-red/registry/lib/installer"); var registry = NR_TEST_UTILS.require("@node-red/registry/lib/index"); var typeRegistry = NR_TEST_UTILS.require("@node-red/registry/lib/registry"); -const { events, exec, log } = NR_TEST_UTILS.require("@node-red/util"); +const { events, exec, log, hooks } = NR_TEST_UTILS.require("@node-red/util"); describe('nodes/registry/installer', function() { @@ -68,6 +68,7 @@ describe('nodes/registry/installer', function() { fs.statSync.restore(); } exec.run.restore(); + hooks.clear(); }); describe("installs module", function() { @@ -251,6 +252,65 @@ describe('nodes/registry/installer', function() { }).catch(done); }); + it("triggers preInstall and postInstall hooks", function(done) { + let receivedPreEvent,receivedPostEvent; + hooks.add("preInstall", function(event) { receivedPreEvent = event; }) + hooks.add("postInstall", function(event) { receivedPostEvent = event; }) + var nodeInfo = {nodes:{module:"foo",types:["a"]}}; + var res = {code: 0,stdout:"",stderr:""} + var p = Promise.resolve(res); + p.catch((err)=>{}); + execResponse = p; + + var addModule = sinon.stub(registry,"addModule",function(md) { + return Promise.resolve(nodeInfo); + }); + + installer.installModule("this_wont_exist","1.2.3").then(function(info) { + info.should.eql(nodeInfo); + should.exist(receivedPreEvent) + receivedPreEvent.should.have.property("module","this_wont_exist") + receivedPreEvent.should.have.property("version","1.2.3") + receivedPreEvent.should.have.property("dir") + receivedPreEvent.should.have.property("url") + receivedPreEvent.should.have.property("isExisting") + receivedPreEvent.should.have.property("isUpgrade") + receivedPreEvent.should.eql(receivedPostEvent) + done(); + }).catch(done); + }); + + it("fails install if preInstall hook fails", function(done) { + let receivedEvent; + hooks.add("preInstall", function(event) { throw new Error("preInstall-error"); }) + var nodeInfo = {nodes:{module:"foo",types:["a"]}}; + + installer.installModule("this_wont_exist","1.2.3").catch(function(err) { + exec.run.called.should.be.false(); + done(); + }).catch(done); + }); + + it("fails install if preInstall hook fails", function(done) { + let receivedEvent; + hooks.add("preInstall", function(event) { throw new Error("preInstall-error"); }) + var nodeInfo = {nodes:{module:"foo",types:["a"]}}; + + installer.installModule("this_wont_exist","1.2.3").catch(function(err) { + exec.run.called.should.be.false(); + done(); + }).catch(done); + }); + it("rollsback install if postInstall hook fails", function(done) { + hooks.add("postInstall", function(event) { throw new Error("fail"); }) + installer.installModule("this_wont_exist","1.2.3").catch(function(err) { + exec.run.calledTwice.should.be.true(); + exec.run.firstCall.args[1].includes("install").should.be.true(); + exec.run.secondCall.args[1].includes("remove").should.be.true(); + done(); + }).catch(done); + }); + }); describe("uninstalls module", function() { it("rejects invalid module names", function(done) { From 52ef85cba3ad55d1fd38eb05a234f74e49013252 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Thu, 15 Apr 2021 15:15:52 +0100 Subject: [PATCH 4/8] Update test for latest sinon --- test/unit/@node-red/registry/lib/installer_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/@node-red/registry/lib/installer_spec.js b/test/unit/@node-red/registry/lib/installer_spec.js index a9b6b71b8..5b03ed309 100644 --- a/test/unit/@node-red/registry/lib/installer_spec.js +++ b/test/unit/@node-red/registry/lib/installer_spec.js @@ -262,7 +262,7 @@ describe('nodes/registry/installer', function() { p.catch((err)=>{}); execResponse = p; - var addModule = sinon.stub(registry,"addModule",function(md) { + var addModule = sinon.stub(registry,"addModule").callsFake(function(md) { return Promise.resolve(nodeInfo); }); From d2432716ea5054c83188342f5c336a50f5f8e9b7 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Thu, 15 Apr 2021 15:30:02 +0100 Subject: [PATCH 5/8] Fix hook requires in unit tests --- test/unit/@node-red/runtime/lib/flows/Flow_spec.js | 2 +- test/unit/@node-red/runtime/lib/nodes/Node_spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/@node-red/runtime/lib/flows/Flow_spec.js b/test/unit/@node-red/runtime/lib/flows/Flow_spec.js index 1495660db..8caa31349 100644 --- a/test/unit/@node-red/runtime/lib/flows/Flow_spec.js +++ b/test/unit/@node-red/runtime/lib/flows/Flow_spec.js @@ -26,7 +26,7 @@ var flowUtils = NR_TEST_UTILS.require("@node-red/runtime/lib/flows/util"); var Flow = NR_TEST_UTILS.require("@node-red/runtime/lib/flows/Flow"); var flows = NR_TEST_UTILS.require("@node-red/runtime/lib/flows"); var Node = NR_TEST_UTILS.require("@node-red/runtime/lib/nodes/Node"); -var hooks = NR_TEST_UTILS.require("@node-red/runtime/lib/hooks"); +var hooks = NR_TEST_UTILS.require("@node-red/util/lib/hooks"); var typeRegistry = NR_TEST_UTILS.require("@node-red/registry"); diff --git a/test/unit/@node-red/runtime/lib/nodes/Node_spec.js b/test/unit/@node-red/runtime/lib/nodes/Node_spec.js index 10935f97d..3fe676f1e 100644 --- a/test/unit/@node-red/runtime/lib/nodes/Node_spec.js +++ b/test/unit/@node-red/runtime/lib/nodes/Node_spec.js @@ -19,7 +19,7 @@ var sinon = require('sinon'); var NR_TEST_UTILS = require("nr-test-utils"); var RedNode = NR_TEST_UTILS.require("@node-red/runtime/lib/nodes/Node"); var Log = NR_TEST_UTILS.require("@node-red/util").log; -var hooks = NR_TEST_UTILS.require("@node-red/runtime/lib/hooks"); +var hooks = NR_TEST_UTILS.require("@node-red/util/lib/hooks"); var flows = NR_TEST_UTILS.require("@node-red/runtime/lib/flows"); describe('Node', function() { From b4a03a56b443b2358938efbea0b47d4518c9e491 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Mon, 19 Apr 2021 20:29:30 +0100 Subject: [PATCH 6/8] Allow preInstall hook to return false to skip npm install --- .../@node-red/registry/lib/externalModules.js | 10 +++++++--- .../@node-red/registry/lib/installer.js | 10 +++++++--- .../registry/lib/externalModules_spec.js | 19 +++++++++++++++++++ .../@node-red/registry/lib/installer_spec.js | 7 +++++-- 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/packages/node_modules/@node-red/registry/lib/externalModules.js b/packages/node_modules/@node-red/registry/lib/externalModules.js index 2bedcce1d..839f63b23 100644 --- a/packages/node_modules/@node-red/registry/lib/externalModules.js +++ b/packages/node_modules/@node-red/registry/lib/externalModules.js @@ -196,11 +196,15 @@ async function installModule(moduleDetails) { "version": moduleDetails.version, "dir": installDir, } - return hooks.trigger("preInstall", triggerPayload).then(() => { + return hooks.trigger("preInstall", triggerPayload).then((result) => { // preInstall passed // - run install - log.trace(NPM_COMMAND + JSON.stringify(args)); - return exec.run(NPM_COMMAND, args, { cwd: installDir },true) + if (result !== false) { + log.trace(NPM_COMMAND + JSON.stringify(args)); + return exec.run(NPM_COMMAND, args, { cwd: installDir },true) + } else { + log.trace("skipping npm install"); + } }).then(() => { return hooks.trigger("postInstall", triggerPayload) }).then(() => { diff --git a/packages/node_modules/@node-red/registry/lib/installer.js b/packages/node_modules/@node-red/registry/lib/installer.js index ef38b89a2..f501f0d4c 100644 --- a/packages/node_modules/@node-red/registry/lib/installer.js +++ b/packages/node_modules/@node-red/registry/lib/installer.js @@ -178,11 +178,15 @@ async function installModule(module,version,url) { "isUpgrade": isUpgrade } - return hooks.trigger("preInstall", triggerPayload).then(() => { + return hooks.trigger("preInstall", triggerPayload).then((result) => { // preInstall passed // - run install - log.trace(npmCommand + JSON.stringify(args)); - return exec.run(npmCommand,args,{ cwd: installDir}, true) + if (result !== false) { + log.trace(npmCommand + JSON.stringify(args)); + return exec.run(npmCommand,args,{ cwd: installDir}, true) + } else { + log.trace("skipping npm install"); + } }).then(() => { return hooks.trigger("postInstall", triggerPayload) }).then(() => { diff --git a/test/unit/@node-red/registry/lib/externalModules_spec.js b/test/unit/@node-red/registry/lib/externalModules_spec.js index 99c74786f..f226f45e5 100644 --- a/test/unit/@node-red/registry/lib/externalModules_spec.js +++ b/test/unit/@node-red/registry/lib/externalModules_spec.js @@ -123,6 +123,25 @@ describe("externalModules api", function() { fs.existsSync(path.join(homeDir,"externalModules")).should.be.true(); }) + it("skips npm install if preInstall returns false", async function() { + externalModules.init({userDir: homeDir}); + externalModules.register("function", "libs"); + let receivedPreEvent,receivedPostEvent; + hooks.add("preInstall", function(event) { receivedPreEvent = event; return false }) + hooks.add("postInstall", function(event) { receivedPostEvent = event; }) + + await externalModules.checkFlowDependencies([ + {type: "function", libs:[{module: "foo"}]} + ]) + exec.run.called.should.be.false(); + receivedPreEvent.should.have.property("module","foo") + receivedPreEvent.should.have.property("version") + receivedPreEvent.should.have.property("dir") + receivedPreEvent.should.eql(receivedPostEvent) + fs.existsSync(path.join(homeDir,"externalModules")).should.be.true(); + }) + + it("installs missing modules from inside subflow module", async function() { externalModules.init({userDir: homeDir}); externalModules.register("function", "libs"); diff --git a/test/unit/@node-red/registry/lib/installer_spec.js b/test/unit/@node-red/registry/lib/installer_spec.js index 5b03ed309..5311e55d0 100644 --- a/test/unit/@node-red/registry/lib/installer_spec.js +++ b/test/unit/@node-red/registry/lib/installer_spec.js @@ -291,16 +291,19 @@ describe('nodes/registry/installer', function() { }).catch(done); }); - it("fails install if preInstall hook fails", function(done) { + it("skips invoking npm if preInstall returns false", function(done) { let receivedEvent; - hooks.add("preInstall", function(event) { throw new Error("preInstall-error"); }) + hooks.add("preInstall", function(event) { return false }) + hooks.add("postInstall", function(event) { receivedEvent = event; }) var nodeInfo = {nodes:{module:"foo",types:["a"]}}; installer.installModule("this_wont_exist","1.2.3").catch(function(err) { exec.run.called.should.be.false(); + should.exist(receivedEvent); done(); }).catch(done); }); + it("rollsback install if postInstall hook fails", function(done) { hooks.add("postInstall", function(event) { throw new Error("fail"); }) installer.installModule("this_wont_exist","1.2.3").catch(function(err) { From 250005ad16b6d7a1367afa4fd73bee15a6ce8df8 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Tue, 20 Apr 2021 22:55:06 +0100 Subject: [PATCH 7/8] Allow npm install args to be customised by preInstall trigger --- .../@node-red/registry/lib/externalModules.js | 4 +++- .../node_modules/@node-red/registry/lib/installer.js | 6 ++++-- test/unit/@node-red/registry/lib/externalModules_spec.js | 9 +++++---- test/unit/@node-red/registry/lib/installer_spec.js | 4 +++- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/packages/node_modules/@node-red/registry/lib/externalModules.js b/packages/node_modules/@node-red/registry/lib/externalModules.js index 839f63b23..726c5384e 100644 --- a/packages/node_modules/@node-red/registry/lib/externalModules.js +++ b/packages/node_modules/@node-red/registry/lib/externalModules.js @@ -190,16 +190,18 @@ async function installModule(moduleDetails) { await ensureModuleDir(); - var args = ["install", installSpec, "--production"]; let triggerPayload = { "module": moduleDetails.module, "version": moduleDetails.version, "dir": installDir, + "args": ["--production"] } return hooks.trigger("preInstall", triggerPayload).then((result) => { // preInstall passed // - run install if (result !== false) { + let extraArgs = triggerPayload.args || []; + let args = ['install', ...extraArgs, installSpec] log.trace(NPM_COMMAND + JSON.stringify(args)); return exec.run(NPM_COMMAND, args, { cwd: installDir },true) } else { diff --git a/packages/node_modules/@node-red/registry/lib/installer.js b/packages/node_modules/@node-red/registry/lib/installer.js index f501f0d4c..9efea696f 100644 --- a/packages/node_modules/@node-red/registry/lib/installer.js +++ b/packages/node_modules/@node-red/registry/lib/installer.js @@ -168,20 +168,22 @@ async function installModule(module,version,url) { } var installDir = settings.userDir || process.env.NODE_RED_HOME || "."; - var args = ['install','--no-audit','--no-update-notifier','--no-fund','--save','--save-prefix=~','--production',installName]; let triggerPayload = { "module": module, "version": version, "url": url, "dir": installDir, "isExisting": isExisting, - "isUpgrade": isUpgrade + "isUpgrade": isUpgrade, + "args": ['--no-audit','--no-update-notifier','--no-fund','--save','--save-prefix=~','--production'] } return hooks.trigger("preInstall", triggerPayload).then((result) => { // preInstall passed // - run install if (result !== false) { + let extraArgs = triggerPayload.args || []; + let args = ['install', ...extraArgs, installName] log.trace(npmCommand + JSON.stringify(args)); return exec.run(npmCommand,args,{ cwd: installDir}, true) } else { diff --git a/test/unit/@node-red/registry/lib/externalModules_spec.js b/test/unit/@node-red/registry/lib/externalModules_spec.js index f226f45e5..5a2e8cb36 100644 --- a/test/unit/@node-red/registry/lib/externalModules_spec.js +++ b/test/unit/@node-red/registry/lib/externalModules_spec.js @@ -48,13 +48,13 @@ describe("externalModules api", function() { beforeEach(function() { sinon.stub(exec,"run").callsFake(async function(cmd, args, options) { let error; - if (args[1] === "moduleNotFound") { + if (args[2] === "moduleNotFound") { error = new Error(); error.stderr = "E404"; - } else if (args[1] === "moduleVersionNotFound") { + } else if (args[2] === "moduleVersionNotFound") { error = new Error(); error.stderr = "ETARGET"; - } else if (args[1] === "moduleFail") { + } else if (args[2] === "moduleFail") { error = new Error(); error.stderr = "Some unexpected install error"; } @@ -109,13 +109,14 @@ describe("externalModules api", function() { externalModules.init({userDir: homeDir}); externalModules.register("function", "libs"); let receivedPreEvent,receivedPostEvent; - hooks.add("preInstall", function(event) { receivedPreEvent = event; }) + hooks.add("preInstall", function(event) { event.args = ["a"]; receivedPreEvent = event; }) hooks.add("postInstall", function(event) { receivedPostEvent = event; }) await externalModules.checkFlowDependencies([ {type: "function", libs:[{module: "foo"}]} ]) exec.run.called.should.be.true(); + // exec.run.lastCall.args[1].should.eql([ 'install', 'a', 'foo' ]); receivedPreEvent.should.have.property("module","foo") receivedPreEvent.should.have.property("version") receivedPreEvent.should.have.property("dir") diff --git a/test/unit/@node-red/registry/lib/installer_spec.js b/test/unit/@node-red/registry/lib/installer_spec.js index 5311e55d0..63075ff24 100644 --- a/test/unit/@node-red/registry/lib/installer_spec.js +++ b/test/unit/@node-red/registry/lib/installer_spec.js @@ -254,7 +254,7 @@ describe('nodes/registry/installer', function() { it("triggers preInstall and postInstall hooks", function(done) { let receivedPreEvent,receivedPostEvent; - hooks.add("preInstall", function(event) { receivedPreEvent = event; }) + hooks.add("preInstall", function(event) { event.args = ["a"]; receivedPreEvent = event; }) hooks.add("postInstall", function(event) { receivedPostEvent = event; }) var nodeInfo = {nodes:{module:"foo",types:["a"]}}; var res = {code: 0,stdout:"",stderr:""} @@ -267,6 +267,8 @@ describe('nodes/registry/installer', function() { }); installer.installModule("this_wont_exist","1.2.3").then(function(info) { + exec.run.called.should.be.true(); + exec.run.lastCall.args[1].should.eql([ 'install', 'a', 'this_wont_exist@1.2.3' ]); info.should.eql(nodeInfo); should.exist(receivedPreEvent) receivedPreEvent.should.have.property("module","this_wont_exist") From f7210effec875c1db426003019f546999b151716 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Mon, 26 Apr 2021 21:14:42 +0100 Subject: [PATCH 8/8] Rework hooks structure to be a linkedlist Allows for safe removal of hooks whilst they are being invoked --- .../node_modules/@node-red/util/lib/hooks.js | 87 +++++++++++++------ test/unit/@node-red/util/lib/hooks_spec.js | 39 +++++++++ 2 files changed, 100 insertions(+), 26 deletions(-) diff --git a/packages/node_modules/@node-red/util/lib/hooks.js b/packages/node_modules/@node-red/util/lib/hooks.js index a49104813..59a8749c4 100644 --- a/packages/node_modules/@node-red/util/lib/hooks.js +++ b/packages/node_modules/@node-red/util/lib/hooks.js @@ -20,7 +20,16 @@ const VALID_HOOKS = [ // Flags for what hooks have handlers registered let states = { } -// Hooks by id +// Doubly-LinkedList of hooks by id. +// - hooks[id] points to head of list +// - each list item looks like: +// { +// cb: the callback function +// location: filename/line of code that added the hook +// previousHook: reference to previous hook in list +// nextHook: reference to next hook in list +// removed: a flag that is set if the item was removed +// } let hooks = { } // Hooks by label @@ -54,20 +63,33 @@ function add(hookId, callback) { if (VALID_HOOKS.indexOf(id) === -1) { throw new Error(`Invalid hook '${id}'`); } - if (label) { - if (labelledHooks[label] && labelledHooks[label][id]) { - throw new Error("Hook "+hookId+" already registered") - } - labelledHooks[label] = labelledHooks[label]||{}; - labelledHooks[label][id] = callback; + if (label && labelledHooks[label] && labelledHooks[label][id]) { + throw new Error("Hook "+hookId+" already registered") } // Get location of calling code const stack = new Error().stack; const callModule = stack.split("\n")[2].split("(")[1].slice(0,-1); Log.debug(`Adding hook '${hookId}' from ${callModule}`); - hooks[id] = hooks[id] || []; - hooks[id].push({cb:callback,location:callModule}); + const hookItem = {cb:callback, location: callModule, previousHook: null, nextHook: null } + + let tailItem = hooks[id]; + if (tailItem === undefined) { + hooks[id] = hookItem; + } else { + while(tailItem.nextHook !== null) { + tailItem = tailItem.nextHook + } + tailItem.nextHook = hookItem; + hookItem.previousHook = tailItem; + } + + if (label) { + labelledHooks[label] = labelledHooks[label]||{}; + labelledHooks[label][id] = hookItem; + } + + // TODO: get rid of this; states[id] = true; } @@ -100,21 +122,29 @@ function remove(hookId) { } } -function removeHook(id,callback) { - let i = hooks[id].findIndex(hook => hook.cb === callback); - if (i !== -1) { - hooks[id].splice(i,1); - if (hooks[id].length === 0) { - delete hooks[id]; - delete states[id]; - } +function removeHook(id,hookItem) { + let previousHook = hookItem.previousHook; + let nextHook = hookItem.nextHook; + + if (previousHook) { + previousHook.nextHook = nextHook; + } else { + hooks[id] = nextHook; + } + if (nextHook) { + nextHook.previousHook = previousHook; + } + hookItem.removed = true; + if (!previousHook && !nextHook) { + delete hooks[id]; + delete states[id]; } } function trigger(hookId, payload, done) { - const hookStack = hooks[hookId]; - if (!hookStack || hookStack.length === 0) { + let hookItem = hooks[hookId]; + if (!hookItem) { if (done) { done(); return; @@ -124,7 +154,7 @@ function trigger(hookId, payload, done) { } if (!done) { return new Promise((resolve,reject) => { - invokeStack(hookStack,payload,function(err) { + invokeStack(hookItem,payload,function(err) { if (err !== undefined && err !== false) { if (!(err instanceof Error)) { err = new Error(err); @@ -137,18 +167,21 @@ function trigger(hookId, payload, done) { }) }); } else { - invokeStack(hookStack,payload,done) + invokeStack(hookItem,payload,done) } } -function invokeStack(hookStack,payload,done) { - let i = 0; +function invokeStack(hookItem,payload,done) { function callNextHook(err) { - if (i === hookStack.length || err) { + if (!hookItem || err) { done(err); return; } - const hook = hookStack[i++]; - const callback = hook.cb; + if (hookItem.removed) { + hookItem = hookItem.nextHook; + callNextHook(); + return; + } + const callback = hookItem.cb; if (callback.length === 1) { try { let result = callback(payload); @@ -161,6 +194,7 @@ function invokeStack(hookStack,payload,done) { result.then(handleResolve, callNextHook) return; } + hookItem = hookItem.nextHook; callNextHook(); } catch(err) { done(err); @@ -177,6 +211,7 @@ function invokeStack(hookStack,payload,done) { } function handleResolve(result) { if (result === undefined) { + hookItem = hookItem.nextHook; callNextHook(); } else { done(result); diff --git a/test/unit/@node-red/util/lib/hooks_spec.js b/test/unit/@node-red/util/lib/hooks_spec.js index 121486ade..4b25f33d2 100644 --- a/test/unit/@node-red/util/lib/hooks_spec.js +++ b/test/unit/@node-red/util/lib/hooks_spec.js @@ -121,7 +121,46 @@ describe("util/hooks", function() { }) }) }) + it("allows a hook to remove itself whilst being called", function(done) { + let data = { order: [] } + hooks.add("onSend.A", function(payload) { payload.order.push("A") } ) + hooks.add("onSend.B", function(payload) { + hooks.remove("*.B"); + }) + hooks.add("onSend.C", function(payload) { payload.order.push("C") } ) + hooks.add("onSend.D", function(payload) { payload.order.push("D") } ) + hooks.trigger("onSend", data, err => { + try { + should.not.exist(err); + data.order.should.eql(["A","C","D"]) + done(); + } catch(e) { + done(e); + } + }) + }); + + it("allows a hook to remove itself and others whilst being called", function(done) { + let data = { order: [] } + hooks.add("onSend.A", function(payload) { payload.order.push("A") } ) + hooks.add("onSend.B", function(payload) { + hooks.remove("*.B"); + hooks.remove("*.C"); + }) + hooks.add("onSend.C", function(payload) { payload.order.push("C") } ) + hooks.add("onSend.D", function(payload) { payload.order.push("D") } ) + + hooks.trigger("onSend", data, err => { + try { + should.not.exist(err); + data.order.should.eql(["A","D"]) + done(); + } catch(e) { + done(e); + } + }) + }); it("halts execution on return false", function(done) { hooks.add("onSend.A", function(payload) { payload.order.push("A"); return false } )