From 042e2c0fc913b83fb64d5395d2066ab8d5bcbd2c Mon Sep 17 00:00:00 2001 From: Patrick Mooney Date: Thu, 31 Jul 2014 19:56:03 -0500 Subject: [PATCH] [issue #87] Kill vm.runInNewContext with fire Change default condition evaluation from vm.runInNewContext to eval-style functions. Use of "naked" variables (pid === 123, etc) no longer works, making this. variable qualification necessary. --- bin/bunyan | 111 ++++++++++++++++++++++++++++++++--------------- test/cli.test.js | 39 ++++++++++++----- 2 files changed, 105 insertions(+), 45 deletions(-) diff --git a/bin/bunyan b/bin/bunyan index 79dcb07..4942a10 100755 --- a/bin/bunyan +++ b/bin/bunyan @@ -276,9 +276,16 @@ function filterRecord(rec, opts) return false; } - if (opts.conditions) { - for (var i = 0; i < opts.conditions.length; i++) { - var pass = opts.conditions[i].runInNewContext(rec); + if (opts.condFuncs) { + var recCopy = objCopy(rec); + for (var i = 0; i < opts.condFuncs.length; i++) { + var pass = opts.condFuncs[i].call(recCopy); + if (!pass) + return false; + } + } else if (opts.condVm) { + for (var i = 0; i < opts.condVm.length; i++) { + var pass = opts.condVm[i].runInNewContext(rec); if (!pass) return false; } @@ -348,6 +355,33 @@ function emitNextRecord(opts, stylize) } } +/** + * Return a function for the given JS code that returns. + * + * If no 'return' in the given javascript snippet, then assume we are a single + * statement and wrap in 'return (...)'. This is for convenience for short + * '-c ...' snippets. + */ +function funcWithReturnFromSnippet(js) { + // auto-"return" + if (js.indexOf('return') === -1) { + if (js.substring(js.length - 1) === ';') { + js = js.substring(0, js.length - 1); + } + js = 'return (' + js + ')'; + } + + // Expose level definitions to condition func context + var varDefs = []; + Object.keys(upperNameFromLevel).forEach(function (lvl) { + varDefs.push(format('var %s = %d;', + upperNameFromLevel[lvl], lvl)); + }); + varDefs = varDefs.join('\n') + '\n'; + + return (new Function(varDefs + js)); +} + /** * Parse the command-line options and arguments into an object. * @@ -400,6 +434,7 @@ function parseArgv(argv) { } args = newArgs; + // Expose level definitions to condition vm context var condDefines = []; Object.keys(upperNameFromLevel).forEach(function (lvl) { condDefines.push( @@ -495,40 +530,46 @@ function parseArgv(argv) { case '-c': case '--condition': var condition = args.shift(); - parsed.conditions = parsed.conditions || []; - var scriptName = 'bunyan-condition-'+parsed.conditions.length; - var code = condDefines + condition; - try { - var script = vm.createScript(code, scriptName); - } catch (compileErr) { - throw new Error(format('illegal CONDITION code: %s\n' - + ' CONDITION script:\n' - + '%s\n' - + ' Error:\n' - + '%s', - compileErr, indent(code), indent(compileErr.stack))); - } + if (Boolean(process.env.JSON_EXEC && + process.env.JSON_EXEC === 'vm')) { + parsed.condVm = parsed.condVm || []; + var scriptName = 'bunyan-condition-'+parsed.condVm.length; + var code = condDefines + condition; + var script; + try { + script = vm.createScript(code, scriptName); + } catch (complErr) { + throw new Error(format('illegal CONDITION code: %s\n' + + ' CONDITION script:\n' + + '%s\n' + + ' Error:\n' + + '%s', + complErr, indent(code), indent(complErr.stack))); + } - // Ensure this is a reasonably safe CONDITION. - try { - script.runInNewContext(minValidRecord); - } catch (condErr) { - throw new Error(format( - /* JSSTYLED */ - 'CONDITION code cannot safely filter a minimal Bunyan log record\n' - + ' CONDITION script:\n' - + '%s\n' - + ' Minimal Bunyan log record:\n' - + '%s\n' - + ' Filter error:\n' - + '%s', - indent(code), - indent(JSON.stringify(minValidRecord, null, 2)), - indent(condErr.stack) - )); + // Ensure this is a reasonably safe CONDITION. + try { + script.runInNewContext(minValidRecord); + } catch (condErr) { + throw new Error(format( + /* JSSTYLED */ + 'CONDITION code cannot safely filter a minimal Bunyan log record\n' + + ' CONDITION script:\n' + + '%s\n' + + ' Minimal Bunyan log record:\n' + + '%s\n' + + ' Filter error:\n' + + '%s', + indent(code), + indent(JSON.stringify(minValidRecord, null, 2)), + indent(condErr.stack) + )); + } + parsed.condVm.push(script); + } else { + parsed.condFuncs = parsed.condFuncs || []; + parsed.condFuncs.push(funcWithReturnFromSnippet(condition)); } - - parsed.conditions.push(script); break; default: // arguments if (!endOfOptions && arg.length > 0 && arg[0] === '-') { diff --git a/test/cli.test.js b/test/cli.test.js index f71be5d..c9a210f 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -280,7 +280,7 @@ test('--level 40', function (t) { }); }); -test('--condition "level === 10 && pid === 123"', function (t) { +test('--condition "this.level === 10 && this.pid === 123"', function (t) { var expect = [ '# levels\n', /* JSSTYLED */ @@ -292,14 +292,14 @@ test('--condition "level === 10 && pid === 123"', function (t) { 'not a JSON line\n', '{"hi": "there"}\n' ].join(''); - var cmd = _('%s -c "level === 10 && pid === 123" %s/corpus/all.log', - BUNYAN, __dirname); + var cmd = _('%s -c "this.level === 10 && this.pid === 123"' + + ' %s/corpus/all.log', BUNYAN, __dirname); exec(cmd, function (err, stdout, stderr) { t.ifError(err); t.equal(stdout, expect); var cmd = _( - '%s --condition "level === 10 && pid === 123" %s/corpus/all.log', - BUNYAN, __dirname); + '%s --condition "this.level === 10 && this.pid === 123"' + + ' %s/corpus/all.log', BUNYAN, __dirname); exec(cmd, function (err, stdout, stderr) { t.ifError(err); t.equal(stdout, expect); @@ -308,13 +308,34 @@ test('--condition "level === 10 && pid === 123"', function (t) { }); }); +test('--condition "this.level === TRACE', function (t) { + var expect = [ + '# levels\n', + /* JSSTYLED */ + '[2012-02-08T22:56:50.856Z] TRACE: myservice/123 on example.com: My message\n', + '\n', + '# extra fields\n', + '\n', + '# bogus\n', + 'not a JSON line\n', + '{"hi": "there"}\n' + ].join(''); + var cmd = _('%s -c "this.level === TRACE" %s/corpus/all.log', + BUNYAN, __dirname); + exec(cmd, function (err, stdout, stderr) { + t.ifError(err); + t.equal(stdout, expect); + t.done(); + }); +}); + // multiple // not sure if this is a bug or a feature. let's call it a feature! test('multiple --conditions', function (t) { var expect = [ '# levels\n', /* JSSTYLED */ - '[2012-02-08T22:56:53.856Z] WARN: myservice/1 on example.com: My message\n', + '[2012-02-08T22:56:53.856Z] WARN: myservice/123 on example.com: My message\n', '\n', '# extra fields\n', '\n', @@ -322,10 +343,8 @@ test('multiple --conditions', function (t) { 'not a JSON line\n', '{"hi": "there"}\n' ].join(''); - exec(_('%s %s/corpus/all.log ' + - '-c "if (level === 40) pid = 1; true" ' + - '-c "pid === 1"', BUNYAN, __dirname), - function (err, stdout, stderr) { + exec(_('%s %s/corpus/all.log -c "this.level === 40" -c "this.pid === 123"', + BUNYAN, __dirname), function (err, stdout, stderr) { t.ifError(err); t.equal(stdout, expect); t.end();