diff --git a/packages/@n8n/eslint-plugin-community-nodes/docs/rules/prefer-declarative-style.md b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/prefer-declarative-style.md index cb6111e6bd2..51bda608c75 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/docs/rules/prefer-declarative-style.md +++ b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/prefer-declarative-style.md @@ -6,6 +6,6 @@ ## Rule Details -This rule reports a programmatic node when its `execute()` method only sends one HTTP request for each input item. Use `routing`, `requestDefaults`, and credential `authenticate` settings for this node instead. +This rule reports a programmatic node when its `execute()` method sends one HTTP request for each input item. It recognizes loops over `getInputData()` and indexed loops that use the input item index. Use `routing`, `requestDefaults`, and credential `authenticate` settings for this node instead. -The rule does not report trigger nodes, pagination logic, binary data handling, non-HTTP transports, multiple requests, multiple outputs, or workflow static data access. +The rule does not report trigger nodes, pagination logic, binary data handling, non-item or nested request loops, non-HTTP transports, multiple requests, multiple outputs, or workflow static data access. diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.test.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.test.ts index 753c3ee490f..044d0003635 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.test.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.test.ts @@ -5,12 +5,39 @@ import communityNodesPlugin, { configs } from '../plugin.js'; const ruleName = '@n8n/community-nodes/prefer-declarative-style'; +function lint(code: string) { + return new Linter().verify( + code, + [ + { + files: ['**/*.ts'], + plugins: { '@n8n/community-nodes': communityNodesPlugin }, + rules: { [ruleName]: 'warn' }, + }, + ], + { filename: 'Test.node.ts' }, + ); +} + +function nodeWithExecute(body: string, group = 'transform', outputs = "['main']") { + return ` +class TestNode extends Node { + description = { group: ['${group}'], outputs: ${outputs} }; + async execute() { + const items = this.getInputData(); + ${body} + } +}`; +} + +const request = "await this.helpers.httpRequest({ url: 'https://api.example.com/items' });"; + describe('prefer-declarative-style', () => { it('warns for a non-trigger node that sends one HTTP request per item', () => { const severity = Reflect.get(configs.recommended.rules ?? {}, ruleName); expect(severity).toBe('warn'); - const messages = new Linter().verify( + const messages = lint( ` class TestNode extends Node { description = { @@ -41,14 +68,6 @@ class TestNode extends Node { } } `, - [ - { - files: ['**/*.ts'], - plugins: { '@n8n/community-nodes': communityNodesPlugin }, - rules: { [ruleName]: 'warn' }, - }, - ], - { filename: 'Test.node.ts' }, ); expect(messages).toEqual([ @@ -58,4 +77,77 @@ class TestNode extends Node { }), ]); }); + + it.each([ + ['for-of', `for (const item of items) { ${request} }`], + [ + 'while', + `let itemIndex = 0; while (itemIndex < items.length) { this.getNodeParameter('id', itemIndex); ${request} itemIndex++; }`, + ], + [ + 'do-while', + `let itemIndex = 0; do { this.getNodeParameter('id', itemIndex); ${request} itemIndex++; } while (itemIndex < items.length);`, + ], + ])('warns for a per-item %s loop', (_name, body) => { + expect(lint(nodeWithExecute(body))).toEqual([ + expect.objectContaining({ ruleId: ruleName, severity: 1 }), + ]); + }); + + it.each([ + ['retry loop', `for (let retry = 0; retry < 3; retry++) { ${request} }`], + [ + 'retry bounded by item count', + `for (let retry = 0; retry < items.length; retry++) { ${request} }`, + ], + [ + 'nested retry loop', + `for (let i = 0; i < items.length; i++) { for (let retry = 0; retry < 3; retry++) { ${request} } }`, + ], + [ + 'retry before item loop', + `for (let retry = 0; retry < 3; retry++) { ${request} } for (let i = 0; i < items.length; i++) {}`, + ], + [ + 'pagination parameter', + `for (const item of items) { this.getNodeParameter('returnAll', 0); ${request} }`, + ], + [ + 'binary parameter', + `for (const item of items) { this.getNodeParameter('binaryPropertyName', 0); ${request} }`, + ], + [ + 'template parameter', + `for (const item of items) { this.getNodeParameter(\`page\`, 0); ${request} }`, + ], + ['multiple requests', `for (const item of items) { ${request} ${request} }`], + ['non-HTTP transport', `for (const item of items) { this.sendMessage(item); ${request} }`], + ['static data', `for (const item of items) { this.getWorkflowStaticData('node'); ${request} }`], + ])('does not warn for %s', (_name, body) => { + expect(lint(nodeWithExecute(body))).toEqual([]); + }); + + it('does not warn for a trigger node', () => { + expect(lint(nodeWithExecute(`for (const item of items) { ${request} }`, 'trigger'))).toEqual( + [], + ); + }); + + it('does not warn for a node with multiple outputs', () => { + expect( + lint( + nodeWithExecute( + `for (const item of items) { ${request} }`, + 'transform', + "['main', 'main']", + ), + ), + ).toEqual([]); + }); + + it('does not warn for a class that is not a node', () => { + expect( + lint('class Test { async execute() { for (const item of items) { ' + request + ' } } }'), + ).toEqual([]); + }); }); diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.ts index 4cae5568288..e7b9f4fe301 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/prefer-declarative-style.ts @@ -9,6 +9,7 @@ import { isFileType, isNodeTypeClass, isThisHelpersAccess, + isThisMethodCall, isTriggerNode, } from '../utils/index.js'; @@ -21,25 +22,98 @@ type ExecuteState = { httpRequestCount: number; hasEscapeHatch: boolean; requestInLoop: boolean; - loopDepth: number; + inputItems: Set; + zeroIndexes: Set; + loopStack: Array<{ index: string | null; referencesItem: boolean; hasRequest: boolean }>; }; +function isInputItems(node: TSESTree.Expression, inputItems: Set): boolean { + return ( + (node.type === AST_NODE_TYPES.Identifier && inputItems.has(node.name)) || + (node.type === AST_NODE_TYPES.CallExpression && isThisMethodCall(node, 'getInputData')) + ); +} + +function isInputItemsLength(node: TSESTree.Expression, inputItems: Set): boolean { + return ( + node.type === AST_NODE_TYPES.MemberExpression && + !node.computed && + node.property.type === AST_NODE_TYPES.Identifier && + node.property.name === 'length' && + isInputItems(node.object, inputItems) + ); +} + +function getBoundedIndex(test: TSESTree.Expression | null, inputItems: Set): string | null { + return test?.type === AST_NODE_TYPES.BinaryExpression && + test.operator === '<' && + test.left.type === AST_NODE_TYPES.Identifier && + isInputItemsLength(test.right, inputItems) + ? test.left.name + : null; +} + +function incrementsIndex(node: TSESTree.Expression, index: string): boolean { + return ( + node.type === AST_NODE_TYPES.UpdateExpression && + node.operator === '++' && + node.argument.type === AST_NODE_TYPES.Identifier && + node.argument.name === index + ); +} + +function getItemLoopIndex( + node: + | TSESTree.ForStatement + | TSESTree.ForInStatement + | TSESTree.ForOfStatement + | TSESTree.WhileStatement + | TSESTree.DoWhileStatement, + state: ExecuteState, +): string | null { + if (node.type === AST_NODE_TYPES.ForOfStatement || node.type === AST_NODE_TYPES.ForInStatement) { + return isInputItems(node.right, state.inputItems) ? '' : null; + } + + const index = getBoundedIndex(node.test, state.inputItems); + if (!index) return null; + + if (node.type === AST_NODE_TYPES.ForStatement) { + const isItemLoop = + node.init?.type === AST_NODE_TYPES.VariableDeclaration && + node.init.declarations.some( + (declaration) => + declaration.id.type === AST_NODE_TYPES.Identifier && + declaration.id.name === index && + declaration.init?.type === AST_NODE_TYPES.Literal && + declaration.init.value === 0, + ) && + node.update !== null && + incrementsIndex(node.update, index); + return isItemLoop ? index : null; + } + + const isItemLoop = + state.zeroIndexes.has(index) && + node.body.type === AST_NODE_TYPES.BlockStatement && + node.body.body.some( + (statement) => + statement.type === AST_NODE_TYPES.ExpressionStatement && + incrementsIndex(statement.expression, index), + ); + return isItemLoop ? index : null; +} + function getMemberName(node: TSESTree.CallExpression): string | null { if (node.callee.type !== AST_NODE_TYPES.MemberExpression) return null; - return node.callee.property.type === AST_NODE_TYPES.Identifier - ? node.callee.property.name - : null; + return node.callee.property.type === AST_NODE_TYPES.Identifier ? node.callee.property.name : null; } function getHttpRequestMethod(node: TSESTree.CallExpression): string | null { if (node.callee.type !== AST_NODE_TYPES.MemberExpression) return null; const directMethod = getMemberName(node); - if ( - directMethod && - HTTP_REQUEST_METHODS.has(directMethod) && - isThisHelpersAccess(node.callee) - ) { + if (directMethod && HTTP_REQUEST_METHODS.has(directMethod) && isThisHelpersAccess(node.callee)) { return directMethod; } @@ -76,11 +150,28 @@ export const PreferDeclarativeStyleRule = createRule({ const eligibleClassStack: boolean[] = []; let executeState: ExecuteState | null = null; - const enterLoop = () => { - if (executeState) executeState.loopDepth++; + const enterLoop = ( + node: + | TSESTree.ForStatement + | TSESTree.ForInStatement + | TSESTree.ForOfStatement + | TSESTree.WhileStatement + | TSESTree.DoWhileStatement, + ) => { + if (executeState) { + const index = getItemLoopIndex(node, executeState); + executeState.loopStack.push({ index, referencesItem: index === '', hasRequest: false }); + } }; const exitLoop = () => { - if (executeState) executeState.loopDepth--; + if (!executeState) return; + const loop = executeState.loopStack.pop(); + executeState.requestInLoop ||= + executeState.loopStack.length === 0 && + loop !== undefined && + loop.index !== null && + loop.referencesItem && + loop.hasRequest; }; return { @@ -88,7 +179,8 @@ export const PreferDeclarativeStyleRule = createRule({ const description = findNodeDescriptionObject(node); const outputs = description ? findObjectProperty(description, 'outputs') : null; const hasOneOutput = - outputs?.value.type === AST_NODE_TYPES.ArrayExpression && outputs.value.elements.length === 1; + outputs?.value.type === AST_NODE_TYPES.ArrayExpression && + outputs.value.elements.length === 1; eligibleClassStack.push( isNodeTypeClass(node) && @@ -108,7 +200,9 @@ export const PreferDeclarativeStyleRule = createRule({ httpRequestCount: 0, hasEscapeHatch: false, requestInLoop: false, - loopDepth: 0, + inputItems: new Set(), + zeroIndexes: new Set(), + loopStack: [], }; } }, @@ -125,15 +219,71 @@ export const PreferDeclarativeStyleRule = createRule({ executeState = null; }, + VariableDeclarator(node) { + if (!executeState || node.id.type !== AST_NODE_TYPES.Identifier) return; + if ( + node.init?.type === AST_NODE_TYPES.CallExpression && + isThisMethodCall(node.init, 'getInputData') + ) { + executeState.inputItems.add(node.id.name); + } + if (node.init?.type === AST_NODE_TYPES.Literal && node.init.value === 0) { + executeState.zeroIndexes.add(node.id.name); + } + }, ForStatement: enterLoop, ForInStatement: enterLoop, ForOfStatement: enterLoop, + WhileStatement: enterLoop, + DoWhileStatement: enterLoop, 'ForStatement:exit': exitLoop, 'ForInStatement:exit': exitLoop, 'ForOfStatement:exit': exitLoop, + 'WhileStatement:exit': exitLoop, + 'DoWhileStatement:exit': exitLoop, Identifier(node) { - if (executeState && ESCAPE_HATCH_NAME.test(node.name)) { + if (!executeState) return; + if (ESCAPE_HATCH_NAME.test(node.name)) { + executeState.hasEscapeHatch = true; + } + const loop = executeState.loopStack.at(-1); + if (loop?.index !== node.name) return; + if ( + (node.parent.type === AST_NODE_TYPES.MemberExpression && + node.parent.computed && + node.parent.property === node && + isInputItems(node.parent.object, executeState.inputItems)) || + (node.parent.type === AST_NODE_TYPES.CallExpression && + isThisMethodCall(node.parent, 'getNodeParameter') && + node.parent.arguments[1] === node) || + (node.parent.type === AST_NODE_TYPES.Property && + node.parent.value === node && + getPropertyKeyName(node.parent) === 'item') + ) { + loop.referencesItem = true; + } + }, + Literal(node) { + if (!executeState || typeof node.value !== 'string' || !ESCAPE_HATCH_NAME.test(node.value)) + return; + if ( + (node.parent.type === AST_NODE_TYPES.CallExpression && + getMemberName(node.parent) === 'getNodeParameter' && + node.parent.arguments[0] === node) || + (node.parent.type === AST_NODE_TYPES.Property && node.parent.key === node) + ) { + executeState.hasEscapeHatch = true; + } + }, + TemplateLiteral(node) { + if ( + executeState && + node.parent.type === AST_NODE_TYPES.CallExpression && + getMemberName(node.parent) === 'getNodeParameter' && + node.parent.arguments[0] === node && + node.quasis.some((part) => ESCAPE_HATCH_NAME.test(part.value.cooked ?? '')) + ) { executeState.hasEscapeHatch = true; } }, @@ -143,12 +293,19 @@ export const PreferDeclarativeStyleRule = createRule({ const httpRequestMethod = getHttpRequestMethod(node); if (httpRequestMethod) { executeState.httpRequestCount++; - executeState.requestInLoop ||= executeState.loopDepth > 0; + const loop = executeState.loopStack.length === 1 ? executeState.loopStack[0] : null; + if (loop) { + loop.hasRequest = true; + } return; } const methodName = getMemberName(node); - if (methodName === 'getWorkflowStaticData' || !methodName || !ALLOWED_CALLS.has(methodName)) { + if ( + methodName === 'getWorkflowStaticData' || + !methodName || + !ALLOWED_CALLS.has(methodName) + ) { executeState.hasEscapeHatch = true; } },