From fbf1ed1a126f51376be2d7fb99ef42ac3b76cc62 Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Tue, 2 Jun 2026 21:45:02 +0200 Subject: [PATCH] refactor(core): Route $('Foo').pairedItem / .itemMatching / .item through typed-RPC dispatcher (backport to 1.x) (#31593) Co-authored-by: Danny Martini --- .../src/__tests__/typed-rpc.test.ts | 119 +++++++++++++++ .../bridge/__tests__/bridge-messages.test.ts | 63 ++++++++ .../src/bridge/bridge-messages.ts | 52 +++++++ .../src/bridge/isolated-vm-bridge.ts | 45 ++++++ .../expression-runtime/src/runtime/context.ts | 43 +++++- .../expression-runtime/src/types/evaluator.ts | 11 ++ packages/workflow/test/expression.test.ts | 141 ++++++++++++++++++ 7 files changed, 473 insertions(+), 1 deletion(-) diff --git a/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts b/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts index 9c5138d6d1d..f414c0f21ca 100644 --- a/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts +++ b/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts @@ -599,3 +599,122 @@ describe('Typed RPC: $fromAI() routes via fromAi', () => { expect(result).toBeUndefined(); }); }); + +describe("Typed RPC: $('Foo').pairedItem / .itemMatching / .item route via getNodePairedItem", () => { + let evaluator: ExpressionEvaluator; + const caller = {}; + + beforeAll(async () => { + evaluator = new ExpressionEvaluator({ + createBridge: () => new IsolatedVmBridge({ timeout: 5000 }), + maxCodeCacheSize: 64, + }); + await evaluator.initialize(); + await evaluator.acquire(caller); + }); + + afterAll(async () => { + await evaluator.release(caller); + await evaluator.dispose(); + }); + + it('$("Foo").pairedItem(idx) calls data.$(name).pairedItem with the idx', () => { + const calls: Array = []; + const data: Record = { + $: (_nodeName: string) => ({ + pairedItem: (...args: unknown[]) => { + calls.push(args); + return { json: { resolved: true } }; + }, + }), + }; + + const result = evaluator.evaluate("{{ $('Foo').pairedItem(2) }}", data, caller); + expect(result).toEqual({ json: { resolved: true } }); + expect(calls).toEqual([[2]]); + }); + + it('$("Foo").pairedItem() forwards undefined itemIndex (host applies its default)', () => { + const calls: Array = []; + const data: Record = { + $: (_nodeName: string) => ({ + pairedItem: (...args: unknown[]) => { + calls.push(args); + return { json: {} }; + }, + }), + }; + + evaluator.evaluate("{{ $('Foo').pairedItem() }}", data, caller); + expect(calls).toEqual([[undefined]]); + }); + + it('$("Foo").itemMatching(idx) reads the literal `itemMatching` property', () => { + // Distinct discriminator from `.pairedItem` so the host's + // `property === 'itemMatching'` branch fires (e.g. for the + // "Missing item index" error path). + const pairedCalls: Array = []; + const matchingCalls: Array = []; + const data: Record = { + $: (_nodeName: string) => ({ + pairedItem: (...args: unknown[]) => { + pairedCalls.push(args); + return { json: { src: 'pairedItem' } }; + }, + itemMatching: (...args: unknown[]) => { + matchingCalls.push(args); + return { json: { src: 'itemMatching' } }; + }, + }), + }; + + const result = evaluator.evaluate("{{ $('Foo').itemMatching(3) }}", data, caller); + expect(result).toEqual({ json: { src: 'itemMatching' } }); + expect(matchingCalls).toEqual([[3]]); + expect(pairedCalls).toEqual([]); + }); + + it('$("Foo").item reads the literal `item` getter (no args)', () => { + // `.item` is a host getter — accessing it invokes the resolver + // immediately. Distinct discriminator so the host's getter path + // fires (not the `.pairedItem` method path). + let pairedCalls = 0; + let itemAccessed = 0; + const data: Record = { + $: (_nodeName: string) => + Object.defineProperty( + { + pairedItem: () => { + pairedCalls += 1; + return undefined; + }, + } as Record, + 'item', + { + get() { + itemAccessed += 1; + return { json: { fetched: true } }; + }, + enumerable: true, + }, + ), + }; + + const result = evaluator.evaluate("{{ $('Foo').item }}", data, caller); + expect(result).toEqual({ json: { fetched: true } }); + expect(itemAccessed).toBe(1); + expect(pairedCalls).toBe(0); + }); + + it("'pairedItem', 'itemMatching', 'item' are reported by the synthetic proxy `has` trap", () => { + const data: Record = { + $: (_nodeName: string) => ({ + pairedItem: () => undefined, + }), + }; + + expect(evaluator.evaluate("{{ 'pairedItem' in $('Foo') }}", data, caller)).toBe(true); + expect(evaluator.evaluate("{{ 'itemMatching' in $('Foo') }}", data, caller)).toBe(true); + expect(evaluator.evaluate("{{ 'item' in $('Foo') }}", data, caller)).toBe(true); + }); +}); diff --git a/packages/@n8n/expression-runtime/src/bridge/__tests__/bridge-messages.test.ts b/packages/@n8n/expression-runtime/src/bridge/__tests__/bridge-messages.test.ts index 89d66639ff6..2a045b4be73 100644 --- a/packages/@n8n/expression-runtime/src/bridge/__tests__/bridge-messages.test.ts +++ b/packages/@n8n/expression-runtime/src/bridge/__tests__/bridge-messages.test.ts @@ -70,6 +70,27 @@ describe('bridgeMessageSchema', () => { expect(parsed.type).toBe('fromAi'); }); + it.each([['getNodePairedItem'], ['getNodeItemMatching']] as const)( + 'parses a valid %s envelope with itemIndex', + (type) => { + const parsed = bridgeMessageSchema.parse({ type, nodeName: 'Foo', itemIndex: 2 }); + expect(parsed.type).toBe(type); + }, + ); + + it.each([['getNodePairedItem'], ['getNodeItemMatching']] as const)( + 'parses a valid %s envelope without itemIndex', + (type) => { + const parsed = bridgeMessageSchema.parse({ type, nodeName: 'Foo' }); + expect(parsed.type).toBe(type); + }, + ); + + it('parses a valid getNodeItem envelope', () => { + const parsed = bridgeMessageSchema.parse({ type: 'getNodeItem', nodeName: 'Foo' }); + expect(parsed.type).toBe('getNodeItem'); + }); + it('rejects an unknown discriminator value', () => { expect(() => bridgeMessageSchema.parse({ type: 'evalArbitrary', nodeName: 'Foo' })).toThrow(); }); @@ -89,6 +110,48 @@ describe('bridgeMessageSchema', () => { ); }); + describe('paired-item cluster', () => { + it.each([['getNodePairedItem'], ['getNodeItemMatching']] as const)( + '%s rejects negative itemIndex', + (type) => { + expect(() => bridgeMessageSchema.parse({ type, nodeName: 'Foo', itemIndex: -1 })).toThrow(); + }, + ); + + it.each([['getNodePairedItem'], ['getNodeItemMatching']] as const)( + '%s rejects non-integer itemIndex', + (type) => { + expect(() => + bridgeMessageSchema.parse({ type, nodeName: 'Foo', itemIndex: 1.5 }), + ).toThrow(); + }, + ); + + it.each([['getNodePairedItem'], ['getNodeItemMatching'], ['getNodeItem']] as const)( + '%s rejects missing nodeName', + (type) => { + expect(() => bridgeMessageSchema.parse({ type })).toThrow(); + }, + ); + + it.each([['getNodePairedItem'], ['getNodeItemMatching'], ['getNodeItem']] as const)( + '%s rejects extra fields (.strict)', + (type) => { + expect(() => + bridgeMessageSchema.parse({ type, nodeName: 'Foo', branchIndex: 0 }), + ).toThrow(); + }, + ); + + it('getNodeItem rejects itemIndex field', () => { + // getNodeItem covers the getter form (no args) — schema doesn't + // permit itemIndex since the host's getter takes none. + expect(() => + bridgeMessageSchema.parse({ type: 'getNodeItem', nodeName: 'Foo', itemIndex: 0 }), + ).toThrow(); + }); + }); + describe('fromAi', () => { it('accepts a minimal envelope (type only)', () => { // `name` is optional in the schema so empty calls reach the host's diff --git a/packages/@n8n/expression-runtime/src/bridge/bridge-messages.ts b/packages/@n8n/expression-runtime/src/bridge/bridge-messages.ts index f52c8e0e5a2..fe35b83b35a 100644 --- a/packages/@n8n/expression-runtime/src/bridge/bridge-messages.ts +++ b/packages/@n8n/expression-runtime/src/bridge/bridge-messages.ts @@ -126,6 +126,55 @@ export const fromAiMessage = z }) .strict(); +/** + * `$('NodeName').pairedItem(itemIndex?)` / `.itemMatching(itemIndex)` / + * `.item` — resolve the paired item for a referenced node. + * + * All three host-side surface forms share one internal resolver + * (`pairedItemMethod` in `WorkflowDataProxy`), but the resolver closes + * over the literal property name the bridge accessed on the host proxy + * — so the error message and getter-vs-method form depend on *which* + * property the bridge reads. Three separate discriminators, each + * mapping a handler to a fixed literal property name, are the only way + * to preserve the host's friendly errors (e.g. "Missing item index for + * .itemMatching()") and the `.item` getter semantics without + * duplicating logic in-isolate. + * + * `itemIndex` is optional on all three at the schema level; the host + * throws the appropriate `ExpressionError` when it's missing for + * `.itemMatching()`, and applies the current-itemIndex default for + * `.pairedItem` and `.item`. + */ +export const getNodePairedItemMessage = z + .object({ + type: z.literal('getNodePairedItem'), + nodeName: z.string(), + itemIndex: z.number().int().nonnegative().optional(), + }) + .strict(); + +/** + * `itemIndex` is `.optional()` even though `.itemMatching()` requires it + * at the host. The host's `pairedItemMethod` closure throws the friendly + * `"Missing item index for .itemMatching()"` error when the field is + * absent — keeping the schema permissive lets that host error surface + * verbatim instead of being replaced by a generic zod parse failure. + */ +export const getNodeItemMatchingMessage = z + .object({ + type: z.literal('getNodeItemMatching'), + nodeName: z.string(), + itemIndex: z.number().int().nonnegative().optional(), + }) + .strict(); + +export const getNodeItemMessage = z + .object({ + type: z.literal('getNodeItem'), + nodeName: z.string(), + }) + .strict(); + /** * The full set of messages the bridge will accept. Discriminator is `type`. * @@ -142,6 +191,9 @@ export const bridgeMessageSchema = z.discriminatedUnion('type', [ getInputAllMessage, getItemsMessage, fromAiMessage, + getNodePairedItemMessage, + getNodeItemMatchingMessage, + getNodeItemMessage, ]); export type BridgeMessage = z.infer; diff --git a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts index 09dd9141548..f56d6c2cfa1 100644 --- a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts +++ b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts @@ -498,6 +498,12 @@ export class IsolatedVmBridge implements RuntimeBridge { return this.handleGetItems(msg, data); case 'fromAi': return this.handleFromAi(msg, data); + case 'getNodePairedItem': + return this.handleGetNodePairedItem(msg, data); + case 'getNodeItemMatching': + return this.handleGetNodeItemMatching(msg, data); + case 'getNodeItem': + return this.handleGetNodeItem(msg, data); default: { // Unreachable at runtime — zod rejects unknown `type` values // before the switch. The `never` assignment is the compile-time @@ -613,6 +619,45 @@ export class IsolatedVmBridge implements RuntimeBridge { return data.$fromAI?.(msg.name, msg.description, msg.valueType, msg.defaultValue); } + /** + * Handlers for the `$('Foo').pairedItem(itemIndex?)` / `.itemMatching(...)` / + * `.item` cluster. Three separate typed RPCs, each reading exactly one + * literal property off the host node proxy. + * + * The split is load-bearing: the host's `pairedItemMethod` closure + * captures which property name the proxy `get` trap saw, and uses + * that to pick the right error message (e.g. "Missing item index for + * .itemMatching()") and to decide between method-call vs getter + * semantics for `.item`. Reading the matching property here lets + * those host-side branches fire exactly as they do in the legacy + * engine; no in-isolate validation needed. + * + * @private + */ + private handleGetNodePairedItem( + msg: Extract, + data: WorkflowData, + ): unknown { + return data.$?.(msg.nodeName)?.pairedItem?.(msg.itemIndex); + } + + private handleGetNodeItemMatching( + msg: Extract, + data: WorkflowData, + ): unknown { + return data.$?.(msg.nodeName)?.itemMatching?.(msg.itemIndex); + } + + private handleGetNodeItem( + msg: Extract, + data: WorkflowData, + ): unknown { + // `.item` is a host getter — accessing it invokes the resolver and + // returns the value immediately. Optional chaining only short- + // circuits on null/undefined; the getter still fires on access. + return data.$?.(msg.nodeName)?.item; + } + /** * Execute JavaScript code in the isolated context. * diff --git a/packages/@n8n/expression-runtime/src/runtime/context.ts b/packages/@n8n/expression-runtime/src/runtime/context.ts index b847a714475..b393ecab7a1 100644 --- a/packages/@n8n/expression-runtime/src/runtime/context.ts +++ b/packages/@n8n/expression-runtime/src/runtime/context.ts @@ -212,17 +212,58 @@ export function buildContext( return result; }; }; + // Paired-item cluster: `.pairedItem(idx?)`, `.itemMatching(idx)`, + // `.item`. Each surface form has its own typed-RPC discriminator + // (`getNodePairedItem` / `getNodeItemMatching` / `getNodeItem`) + // because the host's resolver closes over the literal property + // name to pick error messages and getter-vs-method semantics. + // The bridge handler for each reads the matching property name. + const sendPairedRpc = ( + type: 'getNodePairedItem' | 'getNodeItemMatching', + itemIndex?: number, + ) => { + const result = callbacks.callHost.applySync(null, [{ type, nodeName, itemIndex }], { + arguments: { copy: true }, + result: { copy: true }, + }); + throwIfErrorSentinel(result); + return result; + }; + const sendGetNodeItem = () => { + const result = callbacks.callHost.applySync(null, [{ type: 'getNodeItem', nodeName }], { + arguments: { copy: true }, + result: { copy: true }, + }); + throwIfErrorSentinel(result); + return result; + }; return new Proxy({} as Record, { get(_emptyTarget, prop) { if (isKeyOf(NODE_RPC_TYPES, prop)) { return sendNodeMethod(NODE_RPC_TYPES[prop]); } + if (prop === 'pairedItem') { + return (itemIndex?: number) => sendPairedRpc('getNodePairedItem', itemIndex); + } + if (prop === 'itemMatching') { + return (itemIndex?: number) => sendPairedRpc('getNodeItemMatching', itemIndex); + } + if (prop === 'item') { + // Getter form: invoke immediately, return the value. + return sendGetNodeItem(); + } // Everything else: delegate to the lazy proxy. The lazy proxy's // own `get` trap handles caching, host fetching, and metadata. return lazyProxy[prop]; }, has(_emptyTarget, prop) { - return isKeyOf(NODE_RPC_TYPES, prop) || prop in lazyProxy; + return ( + isKeyOf(NODE_RPC_TYPES, prop) || + prop === 'pairedItem' || + prop === 'itemMatching' || + prop === 'item' || + prop in lazyProxy + ); }, }); }; diff --git a/packages/@n8n/expression-runtime/src/types/evaluator.ts b/packages/@n8n/expression-runtime/src/types/evaluator.ts index 451ce569752..ab018d00b3c 100644 --- a/packages/@n8n/expression-runtime/src/types/evaluator.ts +++ b/packages/@n8n/expression-runtime/src/types/evaluator.ts @@ -111,6 +111,17 @@ export interface NodeProxy { first?: (branchIndex?: number, runIndex?: number) => unknown; last?: (branchIndex?: number, runIndex?: number) => unknown; all?: (branchIndex?: number, runIndex?: number) => unknown; + /** + * Paired-item resolvers. All three host-side surface forms exist as + * separate properties on the proxy because the host's closure + * captures which property name was accessed (to choose error + * messages and getter-vs-method semantics). The bridge reads the + * matching property per discriminator. + */ + pairedItem?: (itemIndex?: number) => unknown; + itemMatching?: (itemIndex?: number) => unknown; + /** Host getter — accessing it invokes the resolver immediately. */ + item?: unknown; } /** diff --git a/packages/workflow/test/expression.test.ts b/packages/workflow/test/expression.test.ts index bd83afe5cde..f722c8222a7 100644 --- a/packages/workflow/test/expression.test.ts +++ b/packages/workflow/test/expression.test.ts @@ -6,6 +6,7 @@ import { workflow, asDuration, asInterval } from './ExpressionExtensions/helpers import { baseFixtures } from './ExpressionFixtures/base'; import type { ExpressionTestEvaluation, ExpressionTestTransform } from './ExpressionFixtures/base'; import * as Helpers from './helpers'; +import { createRunExecutionData } from '../src'; import { ExpressionReservedVariableError } from '../src/errors/expression-reserved-variable.error'; import { ExpressionError } from '../src/errors/expression.error'; import { extendSyntax } from '../src/extensions/expression-extension'; @@ -851,4 +852,144 @@ describe('Expression', () => { }); } }); + + describe('$() node reference through expression engine', () => { + const nodeTypes = Helpers.NodeTypes(); + + function createTestWorkflow(connected: boolean) { + return new Workflow({ + id: 'test-dollar-ref', + name: 'Test', + nodes: [ + { + id: 'source-id', + name: 'source', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + { + id: 'consumer-id', + name: 'consumer', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [200, 0], + parameters: {}, + }, + ], + connections: connected + ? { source: { main: [[{ node: 'consumer', type: 'main', index: 0 }]] } } + : {}, + active: false, + nodeTypes, + }); + } + + const runExecutionData = createRunExecutionData({ + resultData: { + runData: { + source: [ + { + startTime: 1, + executionTime: 1, + executionIndex: 0, + source: [], + data: { + main: [[{ json: { city: 'Prague' }, pairedItem: { item: 0 } }]], + }, + }, + ], + }, + }, + }); + + it("should resolve $('source').item.json.city", async () => { + const testWorkflow = createTestWorkflow(true); + + await testWorkflow.expression.acquireIsolate(); + try { + const result = testWorkflow.expression.getParameterValue( + "={{ $('source').item.json.city }}", + runExecutionData, + 0, + 0, + 'consumer', + [{ json: { city: 'Prague' }, pairedItem: { item: 0 } }], + 'manual', + {}, + { + node: testWorkflow.getNode('consumer')!, + data: {}, + source: { + main: [{ previousNode: 'source', previousNodeOutput: 0, previousNodeRun: 0 }], + }, + }, + ); + + expect(result).toBe('Prague'); + } finally { + await testWorkflow.expression.releaseIsolate(); + } + }); + + it('should throw ExpressionError when nodes are not connected', async () => { + const testWorkflow = createTestWorkflow(false); + + await testWorkflow.expression.acquireIsolate(); + try { + expect(() => + testWorkflow.expression.getParameterValue( + "={{ $('source').item.json.city }}", + runExecutionData, + 0, + 0, + 'consumer', + [{ json: {} }], + 'manual', + {}, + ), + ).toThrow(ExpressionError); + } finally { + await testWorkflow.expression.releaseIsolate(); + } + }); + + it("should throw 'Missing item index' for $('source').itemMatching() (engine parity)", async () => { + // Both engines surface the host's `ExpressionError("Missing item + // index for .itemMatching()")`. The legacy engine throws directly + // from `pairedItemMethod` in `WorkflowDataProxy`. The VM engine + // sends the `getNodeItemMatching` typed-RPC with `itemIndex: + // undefined`; the host's `pairedItemMethod` closure throws because + // `property === PAIRED_ITEM_METHOD.ITEM_MATCHING`, and the bridge + // round-trips that error through the sentinel back into the + // isolate, where tournament's `E()` re-throws it. + const testWorkflow = createTestWorkflow(true); + + await testWorkflow.expression.acquireIsolate(); + try { + expect(() => + testWorkflow.expression.getParameterValue( + "={{ $('source').itemMatching() }}", + runExecutionData, + 0, + 0, + 'consumer', + [{ json: { city: 'Prague' }, pairedItem: { item: 0 } }], + 'manual', + {}, + { + node: testWorkflow.getNode('consumer')!, + data: {}, + source: { + main: [{ previousNode: 'source', previousNodeOutput: 0, previousNodeRun: 0 }], + }, + }, + ), + ).toThrowError(/Missing item index for \.itemMatching\(\)/); + } finally { + await testWorkflow.expression.releaseIsolate(); + } + }); + }); });