From dcb57362c4116075c87350c78513796aede91efa Mon Sep 17 00:00:00 2001 From: mfsiega <93014743+mfsiega@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:51:48 +0000 Subject: [PATCH] fix(core): Restore mutating array methods on $json data in expressions (#37172) Co-authored-by: Claude Fable 5 --- .../src/extensions/array-extensions.ts | 37 ++++++++++++ .../src/extensions/array-extensions.ts | 37 ++++++++++++ .../expression-array-proxy-semantics.test.ts | 59 +++++++++++++++++++ 3 files changed, 133 insertions(+) diff --git a/packages/@n8n/expression-runtime/src/extensions/array-extensions.ts b/packages/@n8n/expression-runtime/src/extensions/array-extensions.ts index d7fffc66882..1ac88bdd8ef 100644 --- a/packages/@n8n/expression-runtime/src/extensions/array-extensions.ts +++ b/packages/@n8n/expression-runtime/src/extensions/array-extensions.ts @@ -34,6 +34,37 @@ function reverse(value: unknown[]): unknown[] { return [...value].reverse(); } +// TODO(CAT-4266): reconsider this approach — copy-on-write proxy traps would +// make these shims unnecessary. +// Copy-first shims for native in-place mutators, like reverse(): the VM engine's +// data proxies are read-only (the natives would throw), and mutations from +// expressions must not leak into workflow data. +function sort(value: unknown[], extraArgs: unknown[]): unknown[] { + const [comparator] = extraArgs as [((a: unknown, b: unknown) => number)?]; + return value.slice().sort(comparator); +} + +function splice(value: unknown[], extraArgs: unknown[]): unknown[] { + const copy = value.slice(); + return copy.splice(...(extraArgs as [number, number, ...unknown[]])); +} + +function fill(value: unknown[], extraArgs: unknown[]): unknown[] { + return value.slice().fill(...(extraArgs as [unknown, number?, number?])); +} + +function copyWithin(value: unknown[], extraArgs: unknown[]): unknown[] { + return value.slice().copyWithin(...(extraArgs as [number, number, number?])); +} + +function shift(value: unknown[]): unknown { + return value.slice().shift(); +} + +function unshift(value: unknown[], extraArgs: unknown[]): number { + return value.slice().unshift(...extraArgs); +} + function pluck(value: unknown[], extraArgs: unknown[]): unknown[] { if (!Array.isArray(extraArgs)) { throw new ExpressionExtensionError('arguments must be passed to pluck'); @@ -738,6 +769,12 @@ export const arrayExtensions: ExtensionMap = { first, last, reverse, + sort, + splice, + fill, + copyWithin, + shift, + unshift, pluck, randomItem, sum, diff --git a/packages/workflow/src/extensions/array-extensions.ts b/packages/workflow/src/extensions/array-extensions.ts index 7e51cd6f1ef..94936ba8479 100644 --- a/packages/workflow/src/extensions/array-extensions.ts +++ b/packages/workflow/src/extensions/array-extensions.ts @@ -34,6 +34,37 @@ function reverse(value: unknown[]): unknown[] { return [...value].reverse(); } +// TODO(CAT-4266): reconsider this approach — copy-on-write proxy traps would +// make these shims unnecessary. +// Copy-first shims for native in-place mutators, like reverse(): the VM engine's +// data proxies are read-only (the natives would throw), and mutations from +// expressions must not leak into workflow data. +function sort(value: unknown[], extraArgs: unknown[]): unknown[] { + const [comparator] = extraArgs as [((a: unknown, b: unknown) => number)?]; + return value.slice().sort(comparator); +} + +function splice(value: unknown[], extraArgs: unknown[]): unknown[] { + const copy = value.slice(); + return copy.splice(...(extraArgs as [number, number, ...unknown[]])); +} + +function fill(value: unknown[], extraArgs: unknown[]): unknown[] { + return value.slice().fill(...(extraArgs as [unknown, number?, number?])); +} + +function copyWithin(value: unknown[], extraArgs: unknown[]): unknown[] { + return value.slice().copyWithin(...(extraArgs as [number, number, number?])); +} + +function shift(value: unknown[]): unknown { + return value.slice().shift(); +} + +function unshift(value: unknown[], extraArgs: unknown[]): number { + return value.slice().unshift(...extraArgs); +} + function pluck(value: unknown[], extraArgs: unknown[]): unknown[] { if (!Array.isArray(extraArgs)) { throw new ExpressionError('arguments must be passed to pluck'); @@ -740,6 +771,12 @@ export const arrayExtensions: ExtensionMap = { first, last, reverse, + sort, + splice, + fill, + copyWithin, + shift, + unshift, pluck, randomItem, sum, diff --git a/packages/workflow/test/expression-array-proxy-semantics.test.ts b/packages/workflow/test/expression-array-proxy-semantics.test.ts index b46fd88bdba..1e32ceae0a7 100644 --- a/packages/workflow/test/expression-array-proxy-semantics.test.ts +++ b/packages/workflow/test/expression-array-proxy-semantics.test.ts @@ -78,4 +78,63 @@ describe('Expression — array proxy semantics (engine parity)', () => { [3, 2, 1], ]); }); + + describe('mutating array methods return a mutated copy', () => { + it('sort() returns the sorted array', () => { + const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; + expect(evaluate('={{ $json.arr.sort() }}', json)).toEqual([ + 'Apple', + 'Kiwi', + 'Mango', + 'Orange', + ]); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); + }); + + it('sort() forwards the comparator', () => { + const json = { arr: [3, 1, 10, 2] }; + expect(evaluate('={{ $json.arr.sort((a, b) => b - a) }}', json)).toEqual([10, 3, 2, 1]); + expect(json.arr).toEqual([3, 1, 10, 2]); + }); + + it('splice() returns the removed elements', () => { + const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; + expect(evaluate('={{ $json.arr.splice(0, 2) }}', json)).toEqual(['Mango', 'Apple']); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); + }); + + it('fill() returns the filled array', () => { + const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; + expect(evaluate('={{ $json.arr.fill("X", 0, 2) }}', json)).toEqual([ + 'X', + 'X', + 'Kiwi', + 'Orange', + ]); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); + }); + + it('shift() returns the removed first element', () => { + const json = { arr: ['Mango', 'Apple', 'Kiwi'] }; + expect(evaluate('={{ $json.arr.shift() }}', json)).toBe('Mango'); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi']); + }); + + it('unshift() returns the new length', () => { + const json = { arr: ['Mango', 'Apple', 'Kiwi'] }; + expect(evaluate('={{ $json.arr.unshift("Peach", "Grape") }}', json)).toBe(5); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi']); + }); + + it('copyWithin() returns the copied-within array', () => { + const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; + expect(evaluate('={{ $json.arr.copyWithin(0, 2, 4) }}', json)).toEqual([ + 'Kiwi', + 'Orange', + 'Kiwi', + 'Orange', + ]); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); + }); + }); });