fix(core): Expression extension failing with optional chaining (#5370)

* wip

* fix: working optional chaining polyfill

* fix: polyfill optional chaining on extended functions

* test: add optional chaining tests

packages/workflow/package.json

@@ -52,7 +52,9 @@
   },
   "dependencies": {
     "@n8n_io/riot-tmpl": "^2.0.0",
+    "ast-types": "0.15.2",
     "crypto-js": "^4.1.1",
+    "esprima-next": "5.8.4",
     "jmespath": "^0.16.0",
     "js-base64": "^3.7.2",
     "lodash.get": "^4.4.2",

packages/workflow/src/Expression.ts

@@ -19,7 +19,7 @@ import { WorkflowDataProxy } from './WorkflowDataProxy';
 import type { Workflow } from './Workflow';
 
 // eslint-disable-next-line import/no-cycle
-import { extend, hasExpressionExtension, hasNativeMethod } from './Extensions';
+import { extend, extendOptional, hasExpressionExtension, hasNativeMethod } from './Extensions';
 import type { ExpressionChunk, ExpressionCode } from './Extensions/ExpressionParser';
 import { joinExpression, splitExpression } from './Extensions/ExpressionParser';
 import { extendTransform } from './Extensions/ExpressionExtension';
@@ -268,6 +268,7 @@ export class Expression {
 
 		// expression extensions
 		data.extend = extend;
+		data.extendOptional = extendOptional;
 
 		Object.assign(data, extendedFunctions);
 
@@ -367,6 +368,7 @@ export class Expression {
 				}
 
 				let text = output.code;
+
 				// We need to cut off any trailing semicolons. These cause issues
 				// with certain types of expression and cause the whole expression
 				// to fail.

packages/workflow/src/Extensions/ExpressionExtension.ts

@@ -2,14 +2,18 @@
 import { DateTime } from 'luxon';
 import { ExpressionExtensionError } from '../ExpressionError';
 import { parse, visit, types, print } from 'recast';
+import { getOption } from 'recast/lib/util';
+import { parse as esprimaParse } from 'esprima-next';
 
 import { arrayExtensions } from './ArrayExtensions';
 import { dateExtensions } from './DateExtensions';
 import { numberExtensions } from './NumberExtensions';
 import { stringExtensions } from './StringExtensions';
 import { objectExtensions } from './ObjectExtensions';
+import type { ExpressionKind } from 'ast-types/gen/kinds';
 
 const EXPRESSION_EXTENDER = 'extend';
+const EXPRESSION_EXTENDER_OPTIONAL = 'extendOptional';
 
 function isEmpty(value: unknown) {
 	return value === null || value === undefined || !value;
@@ -45,7 +49,7 @@ const EXPRESSION_EXTENSION_METHODS = Array.from(
 );
 
 const EXPRESSION_EXTENSION_REGEX = new RegExp(
-	`(\\$if|\\.(${EXPRESSION_EXTENSION_METHODS.join('|')}))\\s*\\(`,
+	`(\\$if|\\.(${EXPRESSION_EXTENSION_METHODS.join('|')})\\s*(\\?\\.)?)\\s*\\(`,
 );
 
 const isExpressionExtension = (str: string) => EXPRESSION_EXTENSION_METHODS.some((m) => m === str);
@@ -74,10 +78,38 @@ export const hasNativeMethod = (method: string): boolean => {
 	});
 };
 
+// /**
+//  * recast's types aren't great and we need to use a lot of anys
+//  */
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function parseWithEsprimaNext(source: string, options?: any): any {
+	// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+	const ast = esprimaParse(source, {
+		loc: true,
+		locations: true,
+		comment: true,
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+		range: getOption(options, 'range', false),
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+		tolerant: getOption(options, 'tolerant', true),
+		tokens: true,
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+		jsx: getOption(options, 'jsx', false),
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+		sourceType: getOption(options, 'sourceType', 'module'),
+		// eslint-disable-next-line @typescript-eslint/no-explicit-any
+	} as any);
+
+	return ast;
+}
+
 /**
  * A function to inject an extender function call into the AST of an expression.
  * This uses recast to do the transform.
  *
+ * This function also polyfills optional chaining if using extended functions.
+ *
  * ```ts
  * 'a'.method('x') // becomes
  * extend('a', 'method', ['x']);
@@ -88,8 +120,213 @@ export const hasNativeMethod = (method: string): boolean => {
  */
 export const extendTransform = (expression: string): { code: string } | undefined => {
 	try {
-		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+		const ast = parse(expression, { parser: { parse: parseWithEsprimaNext } }) as types.ASTNode;
-		const ast = parse(expression);
+
+		let currentChain = 1;
+
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+		visit(ast, {
+			// Polyfill optional chaining
+			visitChainExpression(path) {
+				this.traverse(path);
+				const chainNumber = currentChain;
+				currentChain += 1;
+
+				// This is to match our fork of tmpl
+				const globalIdentifier = types.builders.identifier(
+					// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+					// @ts-ignore
+					typeof window !== 'object' ? 'global' : 'window',
+				);
+				const undefinedIdentifier = types.builders.identifier('undefined');
+				const cancelIdentifier = types.builders.identifier(`chainCancelToken${chainNumber}`);
+				const valueIdentifier = types.builders.identifier(`chainValue${chainNumber}`);
+				const cancelMemberExpression = types.builders.memberExpression(
+					globalIdentifier,
+					cancelIdentifier,
+				);
+				const valueMemberExpression = types.builders.memberExpression(
+					globalIdentifier,
+					valueIdentifier,
+				);
+
+				const patchedStack: ExpressionKind[] = [];
+
+				const buildCancelCheckWrapper = (node: ExpressionKind): ExpressionKind => {
+					return types.builders.conditionalExpression(
+						types.builders.binaryExpression(
+							'===',
+							cancelMemberExpression,
+							types.builders.booleanLiteral(true),
+						),
+						undefinedIdentifier,
+						node,
+					);
+				};
+
+				const buildValueAssignWrapper = (node: ExpressionKind): ExpressionKind => {
+					return types.builders.assignmentExpression('=', valueMemberExpression, node);
+				};
+
+				const buildOptionalWrapper = (node: ExpressionKind): ExpressionKind => {
+					return types.builders.binaryExpression(
+						'===',
+						types.builders.logicalExpression(
+							'??',
+							buildValueAssignWrapper(node),
+							undefinedIdentifier,
+						),
+						undefinedIdentifier,
+					);
+				};
+
+				const buildCancelAssignWrapper = (node: ExpressionKind): ExpressionKind => {
+					return types.builders.assignmentExpression('=', cancelMemberExpression, node);
+				};
+
+				let currentNode: ExpressionKind = path.node.expression;
+				let currentPatch: ExpressionKind | null = null;
+				let patchTop: ExpressionKind | null = null;
+				let wrapNextTopInOptionalExtend = false;
+
+				const updatePatch = (toPatch: ExpressionKind, node: ExpressionKind) => {
+					if (toPatch.type === 'MemberExpression' || toPatch.type === 'OptionalMemberExpression') {
+						toPatch.object = node;
+					} else if (
+						toPatch.type === 'CallExpression' ||
+						toPatch.type === 'OptionalCallExpression'
+					) {
+						toPatch.callee = node;
+					}
+				};
+
+				while (true) {
+					if (
+						currentNode.type === 'MemberExpression' ||
+						currentNode.type === 'OptionalMemberExpression' ||
+						currentNode.type === 'CallExpression' ||
+						currentNode.type === 'OptionalCallExpression'
+					) {
+						let patchNode: ExpressionKind;
+						if (
+							currentNode.type === 'MemberExpression' ||
+							currentNode.type === 'OptionalMemberExpression'
+						) {
+							patchNode = types.builders.memberExpression(
+								valueMemberExpression,
+								currentNode.property,
+							);
+						} else {
+							patchNode = types.builders.callExpression(
+								valueMemberExpression,
+								currentNode.arguments,
+							);
+						}
+
+						if (currentPatch) {
+							updatePatch(currentPatch, patchNode);
+						}
+
+						if (!patchTop) {
+							patchTop = patchNode;
+						}
+
+						currentPatch = patchNode;
+
+						if (currentNode.optional) {
+							// Implement polyfill described below
+							if (wrapNextTopInOptionalExtend) {
+								wrapNextTopInOptionalExtend = false;
+								// This shouldn't ever happen
+								if (
+									patchTop.type === 'MemberExpression' &&
+									patchTop.property.type === 'Identifier'
+								) {
+									patchTop = types.builders.callExpression(
+										types.builders.identifier(EXPRESSION_EXTENDER_OPTIONAL),
+										[patchTop.object, types.builders.stringLiteral(patchTop.property.name)],
+									);
+								}
+							}
+
+							patchedStack.push(patchTop);
+							patchTop = null;
+							currentPatch = null;
+
+							// Attempting to optional chain on an extended function. If we don't
+							// polyfill this most calls will always be undefined. Marking that the
+							// next part of the chain should be wrapped in our polyfill.
+							if (
+								(currentNode.type === 'CallExpression' ||
+									currentNode.type === 'OptionalCallExpression') &&
+								(currentNode.callee.type === 'MemberExpression' ||
+									currentNode.callee.type === 'OptionalMemberExpression') &&
+								currentNode.callee.property.type === 'Identifier' &&
+								isExpressionExtension(currentNode.callee.property.name)
+							) {
+								wrapNextTopInOptionalExtend = true;
+							}
+						}
+
+						if (
+							currentNode.type === 'MemberExpression' ||
+							currentNode.type === 'OptionalMemberExpression'
+						) {
+							currentNode = currentNode.object;
+						} else {
+							currentNode = currentNode.callee;
+						}
+					} else {
+						if (currentPatch) {
+							updatePatch(currentPatch, currentNode);
+							if (!patchTop) {
+								patchTop = currentPatch;
+							}
+						}
+
+						if (wrapNextTopInOptionalExtend) {
+							wrapNextTopInOptionalExtend = false;
+							// This shouldn't ever happen
+							if (
+								patchTop?.type === 'MemberExpression' &&
+								patchTop.property.type === 'Identifier'
+							) {
+								patchTop = types.builders.callExpression(
+									types.builders.identifier(EXPRESSION_EXTENDER_OPTIONAL),
+									[patchTop.object, types.builders.stringLiteral(patchTop.property.name)],
+								);
+							}
+						}
+
+						if (patchTop) {
+							patchedStack.push(patchTop);
+						} else {
+							patchedStack.push(currentNode);
+						}
+						break;
+					}
+				}
+
+				patchedStack.reverse();
+
+				for (let i = 0; i < patchedStack.length; i++) {
+					let node = patchedStack[i];
+
+					if (i !== patchedStack.length - 1) {
+						node = buildCancelAssignWrapper(buildOptionalWrapper(node));
+					}
+
+					if (i !== 0) {
+						node = buildCancelCheckWrapper(node);
+					}
+					patchedStack[i] = node;
+				}
+
+				const sequenceNode = types.builders.sequenceExpression(patchedStack);
+
+				path.replace(sequenceNode);
+			},
+		});
 
 		// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
 		visit(ast, {
@@ -158,14 +395,14 @@ function isDate(input: unknown): boolean {
 	return d instanceof Date && !isNaN(d.valueOf()) && d.toISOString() === input;
 }
 
-/**
+interface FoundFunction {
- * Extender function injected by expression extension plugin to allow calls to extensions.
+	type: 'native' | 'extended';
- *
+	// eslint-disable-next-line @typescript-eslint/ban-types
- * ```ts
+	function: Function;
- * extend(input, "functionName", [...args]);
+}
- * ```
+
- */
+// eslint-disable-next-line @typescript-eslint/ban-types
-export function extend(input: unknown, functionName: string, args: unknown[]) {
+function findExtendedFunction(input: unknown, functionName: string): FoundFunction | undefined {
 	// eslint-disable-next-line @typescript-eslint/ban-types
 	let foundFunction: Function | undefined;
 	if (Array.isArray(input)) {
@@ -191,23 +428,39 @@ export function extend(input: unknown, functionName: string, args: unknown[]) {
 		// eslint-disable-next-line @typescript-eslint/no-explicit-any
 		const inputAny: any = input;
 		// This is likely a builtin we're implementing for another type
-		// (e.g. toLocaleString). We'll just call it
+		// (e.g. toLocaleString). We'll return that instead
 		if (
 			inputAny &&
 			functionName &&
 			// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
 			typeof inputAny[functionName] === 'function'
 		) {
-			// I was having weird issues with eslint not finding rules on this line.
+			// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-			// Just disabling all eslint rules for now.
+			return { type: 'native', function: inputAny[functionName] };
-			// eslint-disable-next-line
-			return inputAny[functionName](...args);
 		}
 
 		// Use a generic version if available
 		foundFunction = genericExtensions[functionName];
 	}
 
+	if (!foundFunction) {
+		return undefined;
+	}
+
+	return { type: 'extended', function: foundFunction };
+}
+
+/**
+ * Extender function injected by expression extension plugin to allow calls to extensions.
+ *
+ * ```ts
+ * extend(input, "functionName", [...args]);
+ * ```
+ */
+export function extend(input: unknown, functionName: string, args: unknown[]) {
+	// eslint-disable-next-line @typescript-eslint/ban-types
+	const foundFunction = findExtendedFunction(input, functionName);
+
 	// No type specific or generic function found. Check to see if
 	// any types have a function with that name. Then throw an error
 	// letting the user know the available types.
@@ -232,6 +485,33 @@ export function extend(input: unknown, functionName: string, args: unknown[]) {
 		}
 	}
 
+	if (foundFunction.type === 'native') {
 		// eslint-disable-next-line @typescript-eslint/no-unsafe-return
-	return foundFunction(input, args);
+		return foundFunction.function.apply(input, args);
+	}
+
+	// eslint-disable-next-line @typescript-eslint/no-unsafe-return
+	return foundFunction.function(input, args);
+}
+
+export function extendOptional(
+	input: unknown,
+	functionName: string,
+	// eslint-disable-next-line @typescript-eslint/ban-types
+): Function | undefined {
+	const foundFunction = findExtendedFunction(input, functionName);
+
+	if (!foundFunction) {
+		return undefined;
+	}
+
+	if (foundFunction.type === 'native') {
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-return
+		return foundFunction.function.bind(input);
+	}
+
+	return (...args: unknown[]) => {
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-return
+		return foundFunction.function(input, args);
+	};
 }

packages/workflow/src/Extensions/index.ts

@@ -1,6 +1,7 @@
 // eslint-disable-next-line import/no-cycle
 export {
 	extend,
+	extendOptional,
 	hasExpressionExtension,
 	hasNativeMethod,
 	extendTransform,

packages/workflow/test/ExpressionExtensions/ExpressionExtension.test.ts

@@ -95,7 +95,7 @@ describe('tmpl Expression Parser', () => {
 			);
 		});
 
-		test('Escaped closinging bracket', () => {
+		test('Escaped closing bracket', () => {
 			expect(joinExpression(splitExpression('test {{ code.test("\\}}") }}'))).toEqual(
 				'test {{ code.test("\\}}") }}',
 			);
@@ -112,6 +112,62 @@ describe('tmpl Expression Parser', () => {
 		});
 	});
 
+	describe('Test newer ES syntax', () => {
+		test('Optional chaining transforms', () => {
+			expect(extendTransform('$json.something?.test.funcCall()')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = $json.something) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1.test.funcCall();',
+			);
+
+			expect(extendTransform('$json.something?.test.funcCall()?.somethingElse')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = $json.something) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainCancelToken1 = ((window.chainValue1 = window.chainValue1.test.funcCall()) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1.somethingElse;',
+			);
+
+			expect(extendTransform('$json.something?.test.funcCall().somethingElse')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = $json.something) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1.test.funcCall().somethingElse;',
+			);
+
+			expect(
+				extendTransform('$json.something?.test.funcCall()?.somethingElse.otherCall()')?.code,
+			).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = $json.something) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainCancelToken1 = ((window.chainValue1 = window.chainValue1.test.funcCall()) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1.somethingElse.otherCall();',
+			);
+
+			expect(evaluate('={{ [1, 2, 3, 4]?.sum() }}')).toBe(10);
+		});
+
+		test('Optional chaining transforms on calls', () => {
+			expect(extendTransform('Math.min?.(1)')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = extendOptional(Math, "min")) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1(1);',
+			);
+			expect(extendTransform('Math?.min?.(1)')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = Math) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainCancelToken1 = ((window.chainValue1 = extendOptional(window.chainValue1, "min")) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1(1);',
+			);
+
+			expect(extendTransform('$json.test.test2?.sum()')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = $json.test.test2) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : extend(window.chainValue1, "sum", []);',
+			);
+			expect(extendTransform('$json.test.test2?.sum?.()')?.code).toBe(
+				'window.chainCancelToken1 = ((window.chainValue1 = $json.test.test2) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainCancelToken1 = ((window.chainValue1 = extendOptional(window.chainValue1, "sum")) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1();',
+			);
+
+			expect(evaluate('={{ [1, 2, 3, 4].sum?.() }}')).toBe(10);
+		});
+
+		test('Multiple optional chains in an expression', () => {
+			expect(extendTransform('$json.test?.test2($json.test?.test2)')?.code)
+				.toBe(`window.chainCancelToken2 = ((window.chainValue2 = $json.test) ?? undefined) === undefined, window.chainCancelToken2 === true ? undefined : window.chainValue2.test2(
+  (window.chainCancelToken1 = ((window.chainValue1 = $json.test) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1.test2)
+);`);
+
+			expect(extendTransform('$json.test?.test2($json.test.sum?.())')?.code)
+				.toBe(`window.chainCancelToken2 = ((window.chainValue2 = $json.test) ?? undefined) === undefined, window.chainCancelToken2 === true ? undefined : window.chainValue2.test2(
+  (window.chainCancelToken1 = ((window.chainValue1 = extendOptional($json.test, "sum")) ?? undefined) === undefined, window.chainCancelToken1 === true ? undefined : window.chainValue1())
+);`);
+		});
+
+		expect(evaluate('={{ [1, 2, 3, 4]?.sum((undefined)?.test) }}')).toBe(10);
+	});
+
 	describe('Non dot extensions', () => {
 		test('min', () => {
 			expect(evaluate('={{ min(1, 2, 3, 4, 5, 6) }}')).toEqual(1);

pnpm-lock.yaml

@@ -930,7 +930,9 @@ importers:
       '@types/lodash.set': ^4.3.6
       '@types/luxon': ^2.0.9
       '@types/xml2js': ^0.4.3
+      ast-types: 0.15.2
       crypto-js: ^4.1.1
+      esprima-next: 5.8.4
       jmespath: ^0.16.0
       js-base64: ^3.7.2
       lodash.get: ^4.4.2
@@ -944,7 +946,9 @@ importers:
       xml2js: ^0.4.23
     dependencies:
       '@n8n_io/riot-tmpl': 2.0.0
+      ast-types: 0.15.2
       crypto-js: 4.1.1
+      esprima-next: 5.8.4
       jmespath: 0.16.0
       js-base64: 3.7.2
       lodash.get: 4.4.2