mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 13:39:44 +02:00
fix(core): Address review feedback on Python Code node lint
- Thread the node execution mode into `lintPythonCode` and flag reading the accessor the runner only defines in the other mode, mirroring the JS rule's `CODE_MODE_API_MISUSE`. - Detect relative imports; the runner rejects them with "Relative imports are disallowed" and the previous root extraction produced no module for them. - Make the import message module-neutral. It now names both allowlists rather than reporting the standard-library error for an external package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
29d29ac753
commit
ba0f19abb3
|
|
@ -136,11 +136,50 @@ describe('lintPythonCode — native runner constraints', () => {
|
|||
]);
|
||||
});
|
||||
|
||||
it('does not flag _items or _item', () => {
|
||||
it('does not flag _items or _item when the mode is unknown', () => {
|
||||
expect(lintPythonCode('return _items').map((i) => i.code)).toEqual([]);
|
||||
expect(lintPythonCode('return _item').map((i) => i.code)).toEqual([]);
|
||||
});
|
||||
|
||||
it('flags _item in runOnceForAllItems mode', () => {
|
||||
const issues = lintPythonCode('return [_item]', { mode: 'runOnceForAllItems' });
|
||||
expect(issues.map((i) => i.code)).toEqual(['CODE_MODE_API_MISUSE']);
|
||||
expect(issues[0].message).toContain('_items');
|
||||
});
|
||||
|
||||
it('flags _items in runOnceForEachItem mode', () => {
|
||||
const issues = lintPythonCode('return _items[0]', { mode: 'runOnceForEachItem' });
|
||||
expect(issues.map((i) => i.code)).toEqual(['CODE_MODE_API_MISUSE']);
|
||||
expect(issues[0].message).toContain('_item');
|
||||
});
|
||||
|
||||
it('does not flag the accessor that matches the mode', () => {
|
||||
expect(
|
||||
lintPythonCode('return _items', { mode: 'runOnceForAllItems' }).map((i) => i.code),
|
||||
).toEqual([]);
|
||||
expect(
|
||||
lintPythonCode('return _item', { mode: 'runOnceForEachItem' }).map((i) => i.code),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('flags a relative import, which the runner rejects outright', () => {
|
||||
const issues = lintPythonCode('from .helpers import parse\nreturn []');
|
||||
expect(issues.map((i) => i.code)).toEqual(['CODE_NODE_PYTHON_IMPORT']);
|
||||
expect(issues[0].message).toContain('.helpers');
|
||||
});
|
||||
|
||||
it('flags a bare relative import', () => {
|
||||
expect(lintPythonCode('from . import helpers\nreturn []').map((i) => i.code)).toEqual([
|
||||
'CODE_NODE_PYTHON_IMPORT',
|
||||
]);
|
||||
});
|
||||
|
||||
it('names both allowlists rather than assuming the module is stdlib', () => {
|
||||
const issues = lintPythonCode('import pandas\nreturn []');
|
||||
expect(issues[0].message).toContain('N8N_RUNNERS_STDLIB_ALLOW');
|
||||
expect(issues[0].message).toContain('N8N_RUNNERS_EXTERNAL_ALLOW');
|
||||
});
|
||||
|
||||
it('does not flag an unsupported global named only in a comment', () => {
|
||||
expect(
|
||||
lintPythonCode('# _json and $input are not available\nreturn _items').map((i) => i.code),
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import type { CodeExecutionMode } from './extract-snippets';
|
||||
import { lintIssue, type SourceLintIssue } from '../types';
|
||||
|
||||
/** Modules that reach the network — reported as CODE_NODE_NETWORK_CALL instead. */
|
||||
|
|
@ -29,6 +30,12 @@ const UNSUPPORTED_GLOBAL = new RegExp(
|
|||
String.raw`(?<![\w.$])(?:${UNSUPPORTED_GLOBAL_NAMES.join('|')})\b|(?<![\w.$])_\s*\(|(?<![\w$])\$[A-Za-z_]`,
|
||||
);
|
||||
|
||||
/** The item accessor the runner defines, per node mode. The other one is undefined. */
|
||||
const MODE_ACCESSOR: Record<CodeExecutionMode, { defined: string; undefined: string }> = {
|
||||
runOnceForAllItems: { defined: '_items', undefined: '_item' },
|
||||
runOnceForEachItem: { defined: '_item', undefined: '_items' },
|
||||
};
|
||||
|
||||
/** `import a.b, c as d` and `from a.b import c`, anchored so prose never matches. */
|
||||
const IMPORT_LINE = /^[ \t]*(?:import[ \t]+([\w.,\t ]+)|from[ \t]+([\w.]+)[ \t]+import\b)/gm;
|
||||
|
||||
|
|
@ -37,19 +44,37 @@ function stripComments(pythonCode: string): string {
|
|||
return pythonCode.replace(/#.*$/gm, '');
|
||||
}
|
||||
|
||||
/** Top-level module of every import in the snippet — `http.client` yields `http`. */
|
||||
/**
|
||||
* Top-level module of an import specifier — `http.client` yields `http`. Relative
|
||||
* specifiers keep their leading dots: they resolve to no top-level module, and the
|
||||
* runner rejects them outright with "Relative imports are disallowed".
|
||||
*/
|
||||
function importRoot(specifier: string): string {
|
||||
const trimmed = specifier.trim();
|
||||
return trimmed.startsWith('.') ? trimmed : trimmed.split(/[\s.]/)[0];
|
||||
}
|
||||
|
||||
/** Every module the snippet imports, keyed by top-level name. */
|
||||
function importedRootModules(pythonCode: string): Set<string> {
|
||||
const roots = new Set<string>();
|
||||
for (const [, names, fromModule] of pythonCode.matchAll(IMPORT_LINE)) {
|
||||
for (const specifier of names ? names.split(',') : [fromModule]) {
|
||||
const root = specifier.trim().split(/[\s.]/)[0];
|
||||
const root = importRoot(specifier);
|
||||
if (root) roots.add(root);
|
||||
}
|
||||
}
|
||||
return roots;
|
||||
}
|
||||
|
||||
/** The accessor belonging to the other mode, when the snippet reads it. */
|
||||
function wrongModeAccessor(pythonCode: string, mode?: CodeExecutionMode): string | undefined {
|
||||
if (!mode) return undefined;
|
||||
const { undefined: wrong } = MODE_ACCESSOR[mode];
|
||||
return new RegExp(String.raw`(?<![\w.$])${wrong}\b`).test(pythonCode) ? wrong : undefined;
|
||||
}
|
||||
|
||||
export interface LintPythonCodeOptions {
|
||||
mode?: CodeExecutionMode;
|
||||
nodeName?: string;
|
||||
}
|
||||
|
||||
|
|
@ -91,9 +116,26 @@ export function lintPythonCode(
|
|||
code: 'CODE_NODE_PYTHON_IMPORT',
|
||||
message:
|
||||
`${namePrefix}Code node imports ${otherModules.join(', ')}, but the native Python runner ` +
|
||||
'allows no imports unless the deployment sets N8N_RUNNERS_STDLIB_ALLOW, which is empty by default. ' +
|
||||
'Each import fails at runtime with "Import of standard library module \'…\' is disallowed". ' +
|
||||
'Rewrite using builtins and str/list/dict methods, or switch the node to JavaScript.',
|
||||
'blocks every import that the deployment has not allowlisted — N8N_RUNNERS_STDLIB_ALLOW for ' +
|
||||
'standard-library modules, N8N_RUNNERS_EXTERNAL_ALLOW for packages — and both are empty by ' +
|
||||
'default. Relative imports are rejected outright. Rewrite using builtins and str/list/dict ' +
|
||||
'methods, or switch the node to JavaScript.',
|
||||
lintTarget: 'pythonCode',
|
||||
nodeName: options.nodeName,
|
||||
parameterPath: 'pythonCode',
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
const wrongAccessor = wrongModeAccessor(code, options.mode);
|
||||
if (wrongAccessor !== undefined && options.mode) {
|
||||
const { defined } = MODE_ACCESSOR[options.mode];
|
||||
issues.push(
|
||||
lintIssue({
|
||||
code: 'CODE_MODE_API_MISUSE',
|
||||
message:
|
||||
`${namePrefix}uses mode: '${options.mode}' but reads ${wrongAccessor}, which the runner ` +
|
||||
`only defines in the other mode. Read ${defined} instead, or switch the node mode.`,
|
||||
lintTarget: 'pythonCode',
|
||||
nodeName: options.nodeName,
|
||||
parameterPath: 'pythonCode',
|
||||
|
|
|
|||
|
|
@ -58,4 +58,30 @@ export default workflow('id', 'name').add(transform);
|
|||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it('passes the execution mode to the Python linter', () => {
|
||||
const source = `
|
||||
const transform = node({
|
||||
type: 'n8n-nodes-base.code',
|
||||
version: 2,
|
||||
config: {
|
||||
name: 'Transform',
|
||||
parameters: {
|
||||
mode: 'runOnceForEachItem',
|
||||
language: 'pythonNative',
|
||||
pythonCode: 'return _items[0]',
|
||||
},
|
||||
},
|
||||
});
|
||||
export default workflow('id', 'name').add(transform);
|
||||
`;
|
||||
expect(lintWorkflowSource(source)).toEqual(
|
||||
expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
lintTarget: 'pythonCode',
|
||||
code: 'CODE_MODE_API_MISUSE',
|
||||
}),
|
||||
]),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -48,7 +48,7 @@ export function lintWorkflowSource(source: string): SourceLintIssue[] {
|
|||
);
|
||||
} else {
|
||||
embeddedIssues.push(
|
||||
...lintPythonCode(snippet.code).map((issue) => ({
|
||||
...lintPythonCode(snippet.code, { mode: snippet.mode }).map((issue) => ({
|
||||
...issue,
|
||||
...base,
|
||||
})),
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ const commonDescription: INodeProperties = {
|
|||
noDataExpression: true,
|
||||
builderHint: {
|
||||
propertyHint:
|
||||
'Runs in a locked-down native Python sandbox. NO imports: the stdlib allowlist (N8N_RUNNERS_STDLIB_ALLOW) is empty by default, so `import re`, `import json`, `import math`, `import datetime` and every third-party package FAIL at runtime with "Import of standard library module \'…\' is disallowed". Write import-free Python using builtins and str/list/dict methods, or set language to javaScript when the task genuinely needs a library. NO network access either: requests, urllib, httpx and other HTTP libraries are unavailable — use the HTTP Request node and process its output in this node instead. The ONLY globals are _items (runOnceForAllItems mode), _item (runOnceForEachItem mode) and print(); there are no cross-node helpers — _("Node Name"), _input, _json, _today and _jmespath are all undefined and raise NameError, so read data from the connected upstream node only.',
|
||||
'Runs in a locked-down native Python sandbox. NO imports: both allowlists (N8N_RUNNERS_STDLIB_ALLOW for standard-library modules, N8N_RUNNERS_EXTERNAL_ALLOW for packages) are empty by default, so `import re`, `import json`, `import math`, `import datetime`, `import pandas` and relative imports all FAIL at runtime with "Import of ... is disallowed". Write import-free Python using builtins and str/list/dict methods, or set language to javaScript when the task genuinely needs a library. NO network access either: requests, urllib, httpx and other HTTP libraries are unavailable — use the HTTP Request node and process its output in this node instead. The ONLY globals are _items (runOnceForAllItems mode ONLY), _item (runOnceForEachItem mode ONLY) and print(); reading the accessor belonging to the other mode raises NameError. There are no cross-node helpers either — _("Node Name"), _input, _json, _today and _jmespath are all undefined, so read data from the connected upstream node only.',
|
||||
},
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user