This is an automated email from the ASF dual-hosted git repository. jamesthomas pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-openwhisk-runtime-nodejs.git
The following commit(s) were added to refs/heads/master by this push: new 7c8461c Reverts runner refactoring changes. (#141) 7c8461c is described below commit 7c8461c99390aff12e7bad33a2d79f65150b9d03 Author: rodric rabbah <rod...@gmail.com> AuthorDate: Fri Jul 5 11:53:15 2019 -0400 Reverts runner refactoring changes. (#141) * Partially revert runner changes related to code eval. * Add webpacked test case. * Simplify test case. --- core/nodejsActionBase/runner.js | 109 +++++++++------------ core/nodejsActionBase/src/service.js | 13 +-- .../NodeJsActionContainerTests.scala | 24 +++++ 3 files changed, 77 insertions(+), 69 deletions(-) diff --git a/core/nodejsActionBase/runner.js b/core/nodejsActionBase/runner.js index 08bb07e..25cdc6c 100644 --- a/core/nodejsActionBase/runner.js +++ b/core/nodejsActionBase/runner.js @@ -23,56 +23,50 @@ const fs = require('fs'); const path = require('path'); +/** Initializes the handler for the user function. */ +function initializeActionHandler(message) { + if (message.binary) { + // The code is a base64-encoded zip file. + return unzipInTmpDir(message.code) + .then(moduleDir => { + let parts = splitMainHandler(message.main); + if (parts === undefined) { + // message.main is guaranteed to not be empty but be defensive anyway + return Promise.reject('Name of main function is not valid.'); + } + + // If there is only one property in the "main" handler, it is the function name + // and the module name is specified either from package.json or assumed to be index.js. + let [index, main] = parts; + + // Set the executable directory to the project dir. + process.chdir(moduleDir); + + if (index === undefined && !fs.existsSync('package.json') && !fs.existsSync('index.js')) { + return Promise.reject('Zipped actions must contain either package.json or index.js at the root.'); + } + + // The module to require. + let whatToRequire = index !== undefined ? path.join(moduleDir, index) : moduleDir; + let handler = eval('require("' + whatToRequire + '").' + main); + return assertMainIsFunction(handler, message.main); + }) + .catch(error => Promise.reject(error)); + } else try { + // The code is a plain old JS file. + let handler = eval('(function(){' + message.code + '\nreturn ' + message.main + '})()'); + return assertMainIsFunction(handler, message.main); + } catch (e) { + return Promise.reject(e); + } +} + class NodeActionRunner { - constructor() { - this.userScriptMain = undefined; + constructor(handler) { + this.userScriptMain = handler; } - /** Initializes the runner with the user function. */ - init(message) { - if (message.binary) { - // The code is a base64-encoded zip file. - return unzipInTmpDir(message.code) - .then(moduleDir => { - let parts = splitMainHandler(message.main); - if (parts === undefined) { - // message.main is guaranteed to not be empty but be defensive anyway - return Promise.reject('Name of main function is not valid.'); - } - - // If there is only one property in the "main" handler, it is the function name - // and the module name is specified either from package.json or assumed to be index.js. - let [index, main] = parts; - - // Set the executable directory to the project dir. - process.chdir(moduleDir); - - if (index === undefined && !fs.existsSync('package.json') && !fs.existsSync('index.js')) { - return Promise.reject('Zipped actions must contain either package.json or index.js at the root.'); - } - - // The module to require. - let whatToRequire = index !== undefined ? path.join(moduleDir, index) : moduleDir; - this.userScriptMain = evalScript(main, whatToRequire) - assertMainIsFunction(this.userScriptMain, message.main); - - // The value 'true' has no special meaning here; the successful state is - // fully reflected in the successful resolution of the promise. - return true; - }) - .catch(error => Promise.reject(error)); - } else try { - // The code is a plain old JS file. - this.userScriptMain = evalScript(message.main, false, message.code) - assertMainIsFunction(this.userScriptMain, message.main); - - return Promise.resolve(true); // See comment above about 'true'; it has no specific meaning. - } catch (e) { - return Promise.reject(e); - } - }; - run(args) { return new Promise((resolve, reject) => { try { @@ -163,22 +157,15 @@ function splitMainHandler(handler) { } else return undefined } -function assertMainIsFunction(userScriptMain, main) { - if (typeof userScriptMain !== 'function') { - throw "Action entrypoint '" + main + "' is not a function."; - } -} - -/** - * Evals the code to execute. This is a global function so that the eval is in the global context - * and hence functions which use variables without 'var' are permitted. - */ -function evalScript(main, whatToRequire, code) { - if (whatToRequire) { - return eval('require("' + whatToRequire + '").' + main); +function assertMainIsFunction(handler, name) { + if (typeof handler === 'function') { + return Promise.resolve(handler); } else { - return eval('(function(){' + code + '\nreturn ' + main + '})()'); + return Promise.reject("Action entrypoint '" + name + "' is not a function."); } } -module.exports = NodeActionRunner; +module.exports = { + NodeActionRunner, + initializeActionHandler +}; diff --git a/core/nodejsActionBase/src/service.js b/core/nodejsActionBase/src/service.js index 11ffeb7..4d994af 100644 --- a/core/nodejsActionBase/src/service.js +++ b/core/nodejsActionBase/src/service.js @@ -15,7 +15,7 @@ * limitations under the License. */ -const NodeActionRunner = require('../runner'); +const { initializeActionHandler, NodeActionRunner } = require('../runner'); function NodeActionService(config) { @@ -160,13 +160,10 @@ function NodeActionService(config) { }; function doInit(message) { - userCodeRunner = new NodeActionRunner(); - - return userCodeRunner - .init(message) - // returning 'true' has no particular meaning here. The fact that the promise resolved - // successfully in itself carries the intended message that initialization succeeded. - .then(_ => true) + return initializeActionHandler(message) + .then(handler => { + userCodeRunner = new NodeActionRunner(handler); + }) // emit error to activation log then flush the logs as this is the end of the activation .catch(error => { console.error('Error during initialization:', error); diff --git a/tests/src/test/scala/runtime/actionContainers/NodeJsActionContainerTests.scala b/tests/src/test/scala/runtime/actionContainers/NodeJsActionContainerTests.scala index f677882..63f30fd 100644 --- a/tests/src/test/scala/runtime/actionContainers/NodeJsActionContainerTests.scala +++ b/tests/src/test/scala/runtime/actionContainers/NodeJsActionContainerTests.scala @@ -291,6 +291,30 @@ abstract class NodeJsActionContainerTests extends BasicActionRunnerTests with Ws }) } + it should "support webpacked function" in { + val (out, err) = withNodeJsContainer { c => + val code = + """ + |function foo() { + | return { bar: true } + |} + |global.main = foo + """.stripMargin + + c.init(initPayload(code))._1 should be(200) + + val (runCode, result) = c.run(JsObject.empty) + runCode should be(200) + result should be(Some(JsObject("bar" -> JsTrue))) + } + + checkStreams(out, err, { + case (o, e) => + o shouldBe empty + e shouldBe empty + }) + } + it should "error when requiring a non-existent package" in { // NPM package names cannot start with a dot, and so there is no danger // of the package below ever being valid.