From c99f158120b3c1ffca1718be337afc73d6ec9e65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 2 May 2023 09:37:49 +0200 Subject: [PATCH] fix(core): Account for nodes with renamable content (#6109) :bug: Account for nodes with renamable content --- packages/workflow/src/Constants.ts | 10 +++++ packages/workflow/src/Workflow.ts | 32 ++++++++----- packages/workflow/test/Workflow.test.ts | 60 +++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 16 deletions(-) diff --git a/packages/workflow/src/Constants.ts b/packages/workflow/src/Constants.ts index e2a6fdf1e8..69c790beb4 100644 --- a/packages/workflow/src/Constants.ts +++ b/packages/workflow/src/Constants.ts @@ -1,3 +1,13 @@ /* eslint-disable @typescript-eslint/naming-convention */ export const BINARY_ENCODING = 'base64'; export const WAIT_TIME_UNLIMITED = '3000-01-01T00:00:00.000Z'; + +/** + * Nodes whose parameter values may refer to other nodes without expressions. + * Their content may need to be updated when the referenced node is renamed. + */ +export const NODES_WITH_RENAMABLE_CONTENT = new Set([ + 'n8n-nodes-base.code', + 'n8n-nodes-base.function', + 'n8n-nodes-base.functionItem', +]); diff --git a/packages/workflow/src/Workflow.ts b/packages/workflow/src/Workflow.ts index a3befc4580..ae5de6ac98 100644 --- a/packages/workflow/src/Workflow.ts +++ b/packages/workflow/src/Workflow.ts @@ -55,6 +55,7 @@ import * as NodeHelpers from './NodeHelpers'; import * as ObservableObject from './ObservableObject'; import { RoutingNode } from './RoutingNode'; import { Expression } from './Expression'; +import { NODES_WITH_RENAMABLE_CONTENT } from './Constants'; function dedupe(arr: T[]): T[] { return [...new Set(arr)]; @@ -405,21 +406,18 @@ export class Workflow { return this.pinData ? this.pinData[nodeName] : undefined; } - /** - * Renames nodes in expressions - * - * @param {(NodeParameterValue | INodeParameters | NodeParameterValue[] | INodeParameters[])} parameterValue The parameters to check for expressions - * @param {string} currentName The current name of the node - * @param {string} newName The new name - */ - renameNodeInExpressions( + renameNodeInParameterValue( parameterValue: NodeParameterValueType, currentName: string, newName: string, + { hasRenamableContent } = { hasRenamableContent: false }, ): NodeParameterValueType { if (typeof parameterValue !== 'object') { // Reached the actual value - if (typeof parameterValue === 'string' && parameterValue.charAt(0) === '=') { + if ( + typeof parameterValue === 'string' && + (parameterValue.charAt(0) === '=' || hasRenamableContent) + ) { // Is expression so has to be rewritten // To not run the "expensive" regex stuff when it is not needed // make a simple check first if it really contains the the node-name @@ -467,7 +465,7 @@ export class Workflow { const returnArray: any[] = []; for (const currentValue of parameterValue) { - returnArray.push(this.renameNodeInExpressions(currentValue, currentName, newName)); + returnArray.push(this.renameNodeInParameterValue(currentValue, currentName, newName)); } return returnArray; @@ -478,10 +476,11 @@ export class Workflow { for (const parameterName of Object.keys(parameterValue || {})) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - returnData[parameterName] = this.renameNodeInExpressions( + returnData[parameterName] = this.renameNodeInParameterValue( parameterValue![parameterName as keyof typeof parameterValue], currentName, newName, + { hasRenamableContent }, ); } @@ -505,11 +504,20 @@ export class Workflow { // Update the expressions which reference the node // with its old name for (const node of Object.values(this.nodes)) { - node.parameters = this.renameNodeInExpressions( + node.parameters = this.renameNodeInParameterValue( node.parameters, currentName, newName, ) as INodeParameters; + + if (NODES_WITH_RENAMABLE_CONTENT.has(node.type)) { + node.parameters.jsCode = this.renameNodeInParameterValue( + node.parameters.jsCode, + currentName, + newName, + { hasRenamableContent: true }, + ); + } } // Change all source connections diff --git a/packages/workflow/test/Workflow.test.ts b/packages/workflow/test/Workflow.test.ts index 98c151988b..95fdf03a0f 100644 --- a/packages/workflow/test/Workflow.test.ts +++ b/packages/workflow/test/Workflow.test.ts @@ -20,7 +20,7 @@ interface StubNode { } describe('Workflow', () => { - describe('renameNodeInExpressions', () => { + describe('renameNodeInParameterValue for expressions', () => { const tests = [ { description: 'do nothing if there is no expression', @@ -257,7 +257,7 @@ describe('Workflow', () => { for (const testData of tests) { test(testData.description, () => { - const result = workflow.renameNodeInExpressions( + const result = workflow.renameNodeInParameterValue( testData.input.parameters, testData.input.currentName, testData.input.newName, @@ -267,6 +267,58 @@ describe('Workflow', () => { } }); + describe('renameNodeInParameterValue for node with renamable content', () => { + const tests = [ + { + description: "should work with $('name')", + input: { + currentName: 'Old', + newName: 'New', + parameters: { jsCode: "$('Old').first();" }, + }, + output: { jsCode: "$('New').first();" }, + }, + { + description: "should work with $node['name'] and $node.name", + input: { + currentName: 'Old', + newName: 'New', + parameters: { jsCode: "$node['Old'].first(); $node.Old.first();" }, + }, + output: { jsCode: "$node['New'].first(); $node.New.first();" }, + }, + { + description: 'should work with $items()', + input: { + currentName: 'Old', + newName: 'New', + parameters: { jsCode: "$items('Old').first();" }, + }, + output: { jsCode: "$items('New').first();" }, + }, + ]; + + const workflow = new Workflow({ + nodes: [], + connections: {}, + active: false, + nodeTypes: Helpers.NodeTypes(), + }); + + for (const t of tests) { + test(t.description, () => { + expect( + workflow.renameNodeInParameterValue( + t.input.parameters, + t.input.currentName, + t.input.newName, + { hasRenamableContent: true }, + ), + ).toEqual(t.output); + }); + } + }); + describe('renameNode', () => { const tests = [ { @@ -605,9 +657,9 @@ describe('Workflow', () => { }, }, }, - // This does just a basic test if "renameNodeInExpressions" gets used. More complex + // This does just a basic test if "renameNodeInParameterValue" gets used. More complex // tests with different formats and levels are in the separate tests for the function - // "renameNodeInExpressions" + // "renameNodeInParameterValue" { description: 'change name also in expressions which use node-name (dot notation)', input: {