From 3c797ce2158d2a185939dfcc7fb3da8c4d135191 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Fri, 1 Nov 2024 14:45:02 +0200 Subject: [PATCH] Remove optimization from $("node").item case --- .../__tests__/built-ins-parser.test.ts | 30 +++- .../built-ins-parser/acorn-helpers.ts | 28 ++++ .../built-ins-parser/built-ins-parser.ts | 141 +++++++++++++----- 3 files changed, 158 insertions(+), 41 deletions(-) create mode 100644 packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/acorn-helpers.ts diff --git a/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser.test.ts b/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser.test.ts index efeee2d12789b..a7168324d55e4 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser.test.ts @@ -56,9 +56,12 @@ describe('BuiltInsParser', () => { describe('$(...)', () => { const cases: Array<[string, BuiltInsParserState]> = [ - ['$("nodeName")', new BuiltInsParserState({ neededNodeNames: new Set(['nodeName']) })], [ - '$("nodeName"); $("secondNode")', + '$("nodeName").first()', + new BuiltInsParserState({ neededNodeNames: new Set(['nodeName']) }), + ], + [ + '$("nodeName").all(); $("secondNode").matchingItem()', new BuiltInsParserState({ neededNodeNames: new Set(['nodeName', 'secondNode']) }), ], ]; @@ -90,6 +93,29 @@ describe('BuiltInsParser', () => { const state = parseAndExpectOk(code); expect(state).toEqual(new BuiltInsParserState()); }); + + test.each([ + '$("node").item', + '$("node")["item"]', + '$("node")[variable]', + 'var a = $("node")', + 'let a = $("node")', + 'const a = $("node")', + 'a = $("node")', + ])('should require all nodes if %s is used', (code) => { + const state = parseAndExpectOk(code); + expect(state).toEqual(new BuiltInsParserState({ needsAllNodes: true })); + }); + + test.each(['$("node").first()', '$("node").last()', '$("node").all()', '$("node").params'])( + 'should require only accessed node if %s is used', + (code) => { + const state = parseAndExpectOk(code); + expect(state).toEqual( + new BuiltInsParserState({ needsAllNodes: false, neededNodeNames: new Set(['node']) }), + ); + }, + ); }); describe('ECMAScript syntax', () => { diff --git a/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/acorn-helpers.ts b/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/acorn-helpers.ts new file mode 100644 index 0000000000000..ccab4c1527b85 --- /dev/null +++ b/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/acorn-helpers.ts @@ -0,0 +1,28 @@ +import type { + AssignmentExpression, + Identifier, + Literal, + MemberExpression, + Node, + VariableDeclarator, +} from 'acorn'; + +export function isLiteral(node?: Node): node is Literal { + return node?.type === 'Literal'; +} + +export function isIdentifier(node?: Node): node is Identifier { + return node?.type === 'Identifier'; +} + +export function isMemberExpression(node?: Node): node is MemberExpression { + return node?.type === 'MemberExpression'; +} + +export function isVariableDeclarator(node?: Node): node is VariableDeclarator { + return node?.type === 'VariableDeclarator'; +} + +export function isAssignmentExpression(node?: Node): node is AssignmentExpression { + return node?.type === 'AssignmentExpression'; +} diff --git a/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts b/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts index 4fddc7b2bd78a..be368e71a7121 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts @@ -1,9 +1,16 @@ -import type { Program } from 'acorn'; +import type { CallExpression, Identifier, Node, Program } from 'acorn'; import { parse } from 'acorn'; -import { simple } from 'acorn-walk'; +import { ancestor } from 'acorn-walk'; import type { Result } from 'n8n-workflow'; import { toResult } from 'n8n-workflow'; +import { + isAssignmentExpression, + isIdentifier, + isLiteral, + isMemberExpression, + isVariableDeclarator, +} from './acorn-helpers'; import { BuiltInsParserState } from './built-ins-parser-state'; /** @@ -26,47 +33,103 @@ export class BuiltInsParser { private identifyBuiltInsByWalkingAst(ast: Program) { const accessedBuiltIns = new BuiltInsParserState(); - simple(ast, { - CallExpression(node) { - // $(...) - const isDollar = node.callee.type === 'Identifier' && node.callee.name === '$'; - if (!isDollar) return; + ancestor( + ast, + { + CallExpression: this.visitCallExpression, + Identifier: this.visitIdentifier, + }, + undefined, + accessedBuiltIns, + ); - // $(): This is not valid, ignore - if (node.arguments.length === 0) { - return; - } + return accessedBuiltIns; + } - const firstArg = node.arguments[0]; - if (firstArg.type === 'Literal') { - if (typeof firstArg.value === 'string') { - // $("nodeName"): Static value, mark 'nodeName' as needed - accessedBuiltIns.markNodeAsNeeded(firstArg.value); - } else { - // $(123): Static value, but not a string --> invalid code --> ignore - } - } else { - // $(variable): Can't determine statically, mark all nodes as needed - accessedBuiltIns.markNeedsAllNodes(); - } + private visitCallExpression = ( + node: CallExpression, + state: BuiltInsParserState, + ancestors: Node[], + ) => { + // $(...) + const isDollar = node.callee.type === 'Identifier' && node.callee.name === '$'; + if (!isDollar) return; - // TODO: We could determine if $('node') is followed by a function call (e.g. - // first()) or a property (e.g. isExecuted) and only get the one accessed - }, + // $(): This is not valid, ignore + if (node.arguments.length === 0) { + return; + } - Identifier(node) { - if (node.name === '$env') { - accessedBuiltIns.markEnvAsNeeded(); - } else if (node.name === '$input' || node.name === '$json') { - accessedBuiltIns.markInputAsNeeded(); - } else if (node.name === '$execution') { - accessedBuiltIns.markExecutionAsNeeded(); - } else if (node.name === '$prevNode') { - accessedBuiltIns.markPrevNodeAsNeeded(); - } - }, - }); + const firstArg = node.arguments[0]; + if (!isLiteral(firstArg)) { + // $(variable): Can't easily determine statically, mark all nodes as needed + state.markNeedsAllNodes(); + return; + } - return accessedBuiltIns; + if (typeof firstArg.value !== 'string') { + // $(123): Static value, but not a string --> invalid code --> ignore + return; + } + + // $("node"): Static value, mark 'nodeName' as needed + state.markNodeAsNeeded(firstArg.value); + + // Determine how $("node") is used + this.handlePrevNodeCall(node, state, ancestors); + }; + + private handlePrevNodeCall(_node: CallExpression, state: BuiltInsParserState, ancestors: Node[]) { + // $("node").item: In a case like this, the execution + // engine will traverse back from current node (i.e. the Code Node) to + // the "node" node and use `pairedItem`s to find which item is linked + // to the current item. So, we need to mark all nodes as needed. + // TODO: We could also mark all the nodes between the current node and + // the "node" node as needed, but that would require more complex logic. + const directParent = ancestors[ancestors.length - 2]; + if (isMemberExpression(directParent)) { + const accessedProperty = directParent.property; + + if (directParent.computed) { + // $("node")["item"] + if (isLiteral(accessedProperty)) { + if (accessedProperty.value === 'item') { + state.markNeedsAllNodes(); + } + // Else: $("node")[123]: Static value, but not 'item' --> ignore + } + // $("node")[variable] + else if (isIdentifier(accessedProperty)) { + state.markNeedsAllNodes(); + } + } + // $("node").item + else if (isIdentifier(accessedProperty) && accessedProperty.name === 'item') { + state.markNeedsAllNodes(); + } + } else if (isVariableDeclarator(directParent) || isAssignmentExpression(directParent)) { + // const variable = $("node") or variable = $("node"): + // In this case we would need to track down all the possible use sites + // of 'variable' and determine if `.item` is accessed on it. This is + // more complex and skipped for now. + // TODO: Optimize for this case + state.markNeedsAllNodes(); + } else { + // Something else than the cases above. Mark all nodes as needed as it + // could be a dynamic access. + state.markNeedsAllNodes(); + } } + + private visitIdentifier = (node: Identifier, state: BuiltInsParserState) => { + if (node.name === '$env') { + state.markEnvAsNeeded(); + } else if (node.name === '$input' || node.name === '$json') { + state.markInputAsNeeded(); + } else if (node.name === '$execution') { + state.markExecutionAsNeeded(); + } else if (node.name === '$prevNode') { + state.markPrevNodeAsNeeded(); + } + }; }