From 71ae6c66ef32ba86edf0bb9cdb9f24a6d40ee80c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 28 Apr 2023 15:53:59 +0200 Subject: [PATCH] fix(editor): Fix unique names for node duplication (#6134) * :bug: Fix unique names for node duplication * :bug: Fix i18n references --- .../editor-ui/src/components/VariablesRow.vue | 2 +- .../__tests__/useUniqueNodeName.test.ts | 79 ++++++++++++ packages/editor-ui/src/composables/useI18n.ts | 16 ++- .../editor-ui/src/composables/useToast.ts | 2 +- .../src/composables/useUniqueNodeName.ts | 108 +++++++++++++++++ .../src/composables/useUpgradeLink.ts | 2 +- packages/editor-ui/src/stores/workflows.ts | 6 + packages/editor-ui/src/views/NodeView.vue | 112 +++--------------- .../editor-ui/src/views/VariablesView.vue | 2 +- 9 files changed, 227 insertions(+), 102 deletions(-) create mode 100644 packages/editor-ui/src/composables/__tests__/useUniqueNodeName.test.ts create mode 100644 packages/editor-ui/src/composables/useUniqueNodeName.ts diff --git a/packages/editor-ui/src/components/VariablesRow.vue b/packages/editor-ui/src/components/VariablesRow.vue index 9112e1fcf80b3..19c59cd5d4682 100644 --- a/packages/editor-ui/src/components/VariablesRow.vue +++ b/packages/editor-ui/src/components/VariablesRow.vue @@ -7,7 +7,7 @@ import { EnterpriseEditionFeature } from '@/constants'; import { useSettingsStore, useUsersStore } from '@/stores'; import { getVariablesPermissions } from '@/permissions'; -const i18n = useI18n(); +const { i18n } = useI18n(); const copyToClipboard = useCopyToClipboard(); const { showMessage } = useToast(); const settingsStore = useSettingsStore(); diff --git a/packages/editor-ui/src/composables/__tests__/useUniqueNodeName.test.ts b/packages/editor-ui/src/composables/__tests__/useUniqueNodeName.test.ts new file mode 100644 index 0000000000000..3bdf653243d2c --- /dev/null +++ b/packages/editor-ui/src/composables/__tests__/useUniqueNodeName.test.ts @@ -0,0 +1,79 @@ +import { setActivePinia } from 'pinia'; +import { createTestingPinia } from '@pinia/testing'; +import { useUniqueNodeName } from '@/composables/useUniqueNodeName'; +import { useNodeTypesStore } from '@/stores/nodeTypes'; +import { useWorkflowsStore } from '@/stores/workflows'; + +describe('useUniqueNodeName', () => { + beforeAll(() => { + setActivePinia(createTestingPinia()); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + test('should return a unique node name for an alphabetic node name', () => { + const workflowsStore = useWorkflowsStore(); + + const mockCanvasNames = new Set(['Hello']); + + vi.spyOn(workflowsStore, 'canvasNames', 'get').mockReturnValue(mockCanvasNames); + + const { uniqueNodeName } = useUniqueNodeName(); + + expect(uniqueNodeName('Hello')).toBe('Hello1'); + + mockCanvasNames.add('Hello1'); + + expect(uniqueNodeName('Hello')).toBe('Hello2'); + }); + + test('should return a unique node name for a numeric node name', () => { + const workflowsStore = useWorkflowsStore(); + + const mockCanvasNames = new Set(['123']); + + vi.spyOn(workflowsStore, 'canvasNames', 'get').mockReturnValue(mockCanvasNames); + + const { uniqueNodeName } = useUniqueNodeName(); + + expect(uniqueNodeName('123')).toBe('123-1'); + + mockCanvasNames.add('123-1'); + + expect(uniqueNodeName('123')).toBe('123-2'); + }); + + test('should return a unique node name for a number-suffixed node name', () => { + const workflowsStore = useWorkflowsStore(); + const nodeTypesStore = useNodeTypesStore(); + + const mockCanvasNames = new Set(['S3']); + + vi.spyOn(workflowsStore, 'canvasNames', 'get').mockReturnValue(mockCanvasNames); + vi.spyOn(nodeTypesStore, 'allNodeTypes', 'get').mockReturnValue([ + { + displayName: 'S3', + name: 'S3', + description: '', + version: 1, + inputs: [''], + outputs: [''], + group: [''], + properties: [], + defaults: { + name: 'S3', + }, + }, + ]); + + const { uniqueNodeName } = useUniqueNodeName(); + + expect(uniqueNodeName('S3')).toBe('S31'); + + mockCanvasNames.add('S31'); + + expect(uniqueNodeName('S3')).toBe('S32'); + }); +}); diff --git a/packages/editor-ui/src/composables/useI18n.ts b/packages/editor-ui/src/composables/useI18n.ts index 61d7bf807a50d..f8b73cfd2a650 100644 --- a/packages/editor-ui/src/composables/useI18n.ts +++ b/packages/editor-ui/src/composables/useI18n.ts @@ -1,5 +1,19 @@ import { i18n } from '@/plugins/i18n'; +import { useRootStore } from '@/stores/n8nRootStore'; export function useI18n() { - return i18n; + const isEnglishLocale = useRootStore().defaultLocale === 'en'; + + function localizeNodeName(nodeName: string, type: string) { + if (isEnglishLocale) return nodeName; + + const nodeTypeName = i18n.shortNodeType(type); + + return i18n.headerText({ + key: `headers.${nodeTypeName}.displayName`, + fallback: nodeName, + }); + } + + return { i18n, localizeNodeName }; } diff --git a/packages/editor-ui/src/composables/useToast.ts b/packages/editor-ui/src/composables/useToast.ts index 97fff04d7030a..996de86e7c544 100644 --- a/packages/editor-ui/src/composables/useToast.ts +++ b/packages/editor-ui/src/composables/useToast.ts @@ -18,7 +18,7 @@ export function useToast() { const telemetry = useTelemetry(); const workflowsStore = useWorkflowsStore(); const externalHooks = useExternalHooks(); - const i18n = useI18n(); + const { i18n } = useI18n(); function showMessage( messageData: Omit & { message?: string }, diff --git a/packages/editor-ui/src/composables/useUniqueNodeName.ts b/packages/editor-ui/src/composables/useUniqueNodeName.ts new file mode 100644 index 0000000000000..23eba9a4f7dc2 --- /dev/null +++ b/packages/editor-ui/src/composables/useUniqueNodeName.ts @@ -0,0 +1,108 @@ +import { useWorkflowsStore } from '@/stores/workflows'; +import { useNodeTypesStore } from '@/stores/nodeTypes'; + +export function useUniqueNodeName() { + /** + * All in-store node name defaults ending with a number, e.g. + * `AWS S3`, `Magento 2`, `MSG91`, `S3`, `SIGNL4`, `sms77` + */ + function numberSuffixedNames() { + return useNodeTypesStore().allNodeTypes.reduce((acc, nodeType) => { + if (typeof nodeType.defaults.name !== 'string') { + throw new Error('Expected node name default to be a string'); + } + + if (/\d$/.test(nodeType.defaults.name)) acc.push(nodeType.defaults.name); + + return acc; + }, []); + } + + /** + * Create a unique node name from an original name, based on the names of + * all nodes on canvas and any extra names that cannot be used. + */ + function uniqueNodeName(originalName: string, extraNames: string[] = []) { + const { canvasNames } = useWorkflowsStore(); + + const isUnique = !canvasNames.has(originalName) && !extraNames.includes(originalName); + + if (isUnique) return originalName; + + const nsn = numberSuffixedNames().find((nsn) => originalName.startsWith(nsn)); + + // edge case, number suffix as part of name: S3 -> S31 -> S32 + + if (nsn) { + let unique = ''; + let index = 1; + + const remainder = originalName.split(nsn).pop(); + + const lastChar = remainder?.[remainder.length - 1]; + + if (lastChar && Number.isInteger(Number(lastChar))) { + index = parseInt(lastChar, 10); + originalName = originalName.slice(0, -1); + } + + unique = originalName; + + while (canvasNames.has(unique) || extraNames.includes(unique)) { + unique = originalName + index++; + } + + return unique; + } + + // edge case, all-number name: 123 -> 123-1 -> 123-2 + + if (/^\d+-?\d*$/.test(originalName)) { + let unique = ''; + let index = 1; + + const match = originalName.match(/(?\d+)-?(?\d*)/); + + if (!match?.groups) { + throw new Error('Failed to find match for unique name'); + } + + if (match?.groups?.suffix !== '') { + index = parseInt(match.groups.suffix, 10); + } + + unique = match.groups.base; + + while (canvasNames.has(unique) || extraNames.includes(unique)) { + unique = match.groups.base + '-' + index++; + } + + return unique; + } + + // normal case: A -> A1 -> A2 + + let unique = ''; + let index = 1; + + const match = originalName.match(/(?.*\D+)(?\d*)/); + + if (!match?.groups) { + throw new Error('Failed to find match for unique name'); + } + + if (match?.groups?.suffix !== '') { + index = parseInt(match.groups.suffix, 10); + } + + unique = match.groups.base; + + while (canvasNames.has(unique) || extraNames.includes(unique)) { + unique = match.groups.base + index++; + } + + return unique; + } + + return { uniqueNodeName }; +} diff --git a/packages/editor-ui/src/composables/useUpgradeLink.ts b/packages/editor-ui/src/composables/useUpgradeLink.ts index 0b9dd7889d0a3..cbe6fb6d83f39 100644 --- a/packages/editor-ui/src/composables/useUpgradeLink.ts +++ b/packages/editor-ui/src/composables/useUpgradeLink.ts @@ -6,7 +6,7 @@ import { computed } from 'vue'; export function useUpgradeLink(queryParams = { default: '', desktop: '' }) { const uiStore = useUIStore(); const usageStore = useUsageStore(); - const i18n = useI18n(); + const { i18n } = useI18n(); const upgradeLinkUrl = computed(() => { const linkUrlTranslationKey = uiStore.contextBasedTranslationKeys.upgradeLinkUrl as BaseTextKey; diff --git a/packages/editor-ui/src/stores/workflows.ts b/packages/editor-ui/src/stores/workflows.ts index b6aa9a1b0f6fc..de6fcef0cc425 100644 --- a/packages/editor-ui/src/stores/workflows.ts +++ b/packages/editor-ui/src/stores/workflows.ts @@ -204,6 +204,12 @@ export const useWorkflowsStore = defineStore(STORES.WORKFLOWS, { allNodes(): INodeUi[] { return this.workflow.nodes; }, + /** + * Names of all nodes currently on canvas. + */ + canvasNames(): Set { + return new Set(this.allNodes.map((n) => n.name)); + }, nodesByName(): { [name: string]: INodeUi } { return this.workflow.nodes.reduce((accu: { [name: string]: INodeUi }, node) => { accu[node.name] = node; diff --git a/packages/editor-ui/src/views/NodeView.vue b/packages/editor-ui/src/views/NodeView.vue index 9b1d082d04fb1..61b5850d4feb7 100644 --- a/packages/editor-ui/src/views/NodeView.vue +++ b/packages/editor-ui/src/views/NodeView.vue @@ -210,6 +210,8 @@ import useGlobalLinkActions from '@/composables/useGlobalLinkActions'; import useCanvasMouseSelect from '@/composables/useCanvasMouseSelect'; import { showMessage } from '@/mixins/showMessage'; import { useTitleChange } from '@/composables/useTitleChange'; +import { useUniqueNodeName } from '@/composables/useUniqueNodeName'; +import { useI18n } from '@/composables/useI18n'; import { workflowHelpers } from '@/mixins/workflowHelpers'; import { workflowRun } from '@/mixins/workflowRun'; @@ -341,6 +343,8 @@ export default mixins( ...useCanvasMouseSelect(), ...useGlobalLinkActions(), ...useTitleChange(), + ...useUniqueNodeName(), + ...useI18n(), }; }, errorCaptured: (err, vm, info) => { @@ -477,12 +481,6 @@ export default mixins( currentUser(): IUser | null { return this.usersStore.currentUser; }, - defaultLocale(): string { - return this.rootStore.defaultLocale; - }, - isEnglishLocale(): boolean { - return this.defaultLocale === 'en'; - }, activeNode(): INodeUi | null { return this.ndvStore.activeNode; }, @@ -678,78 +676,6 @@ export default mixins( this.workflowsStore.workflowExecutionData = null; this.updateNodesExecutionIssues(); }, - translateName(type: string, originalName: string) { - return this.$locale.headerText({ - key: `headers.${this.$locale.shortNodeType(type)}.displayName`, - fallback: originalName, - }); - }, - getUniqueNodeName({ - originalName, - additionalUsedNames = [], - type = '', - }: { - originalName: string; - additionalUsedNames?: string[]; - type?: string; - }) { - const allNodeNamesOnCanvas = this.workflowsStore.allNodes.map((n: INodeUi) => n.name); - originalName = this.isEnglishLocale ? originalName : this.translateName(type, originalName); - - if ( - !allNodeNamesOnCanvas.includes(originalName) && - !additionalUsedNames.includes(originalName) - ) { - return originalName; // already unique - } - - let natives: string[] = this.nativelyNumberSuffixedDefaults; - natives = this.isEnglishLocale - ? natives - : natives.map((name) => { - const type = name.toLowerCase().replace('_', ''); - return this.translateName(type, name); - }); - - const found = natives.find((n) => originalName.startsWith(n)); - - let ignore, baseName, nameIndex, uniqueName; - let index = 1; - - if (found) { - // name natively ends with number - nameIndex = originalName.split(found).pop(); - if (nameIndex) { - index = parseInt(nameIndex, 10); - } - baseName = uniqueName = found; - } else { - const nameMatch = originalName.match(/(.*\D+)(\d*)/); - - if (nameMatch === null) { - // name is only a number - index = parseInt(originalName, 10); - baseName = ''; - uniqueName = baseName + index; - } else { - // name is string or string/number combination - [ignore, baseName, nameIndex] = nameMatch; - if (nameIndex !== '') { - index = parseInt(nameIndex, 10); - } - uniqueName = baseName; - } - } - - while ( - allNodeNamesOnCanvas.includes(uniqueName) || - additionalUsedNames.includes(uniqueName) - ) { - uniqueName = baseName + index++; - } - - return uniqueName; - }, async onSaveKeyboardShortcut(e: KeyboardEvent) { let saved = await this.saveCurrentWorkflow(); if (saved) await this.settingsStore.fetchPromptsData(); @@ -1931,11 +1857,9 @@ export default mixins( newNodeData.position = NodeViewUtils.getNewNodePosition(this.nodes, position); } - // Check if node-name is unique else find one that is - newNodeData.name = this.getUniqueNodeName({ - originalName: newNodeData.name, - type: newNodeData.type, - }); + const localizedName = this.localizeNodeName(newNodeData.name, newNodeData.type); + + newNodeData.name = this.uniqueNodeName(localizedName); if (nodeTypeData.webhooks && nodeTypeData.webhooks.length) { newNodeData.webhookId = uuid(); @@ -2732,11 +2656,9 @@ export default mixins( const newNodeData = deepCopy(this.getNodeDataToSave(node)); newNodeData.id = uuid(); - // Check if node-name is unique else find one that is - newNodeData.name = this.getUniqueNodeName({ - originalName: newNodeData.name, - type: newNodeData.type, - }); + const localizedName = this.localizeNodeName(newNodeData.name, newNodeData.type); + + newNodeData.name = this.uniqueNodeName(localizedName); newNodeData.position = NodeViewUtils.getNewNodePosition( this.nodes, @@ -3116,10 +3038,7 @@ export default mixins( this.renamingActive = true; } - // Check if node-name is unique else find one that is - newName = this.getUniqueNodeName({ - originalName: newName, - }); + newName = this.uniqueNodeName(newName); // Rename the node and update the connections const workflow = this.getCurrentWorkflow(true); @@ -3386,11 +3305,10 @@ export default mixins( } oldName = node.name; - newName = this.getUniqueNodeName({ - originalName: node.name, - additionalUsedNames: newNodeNames, - type: node.type, - }); + + const localized = this.localizeNodeName(node.name, node.type); + + newName = this.uniqueNodeName(localized, newNodeNames); newNodeNames.push(newName); nodeNameTable[oldName] = newName; diff --git a/packages/editor-ui/src/views/VariablesView.vue b/packages/editor-ui/src/views/VariablesView.vue index d3dd983858c76..da02446222465 100644 --- a/packages/editor-ui/src/views/VariablesView.vue +++ b/packages/editor-ui/src/views/VariablesView.vue @@ -20,7 +20,7 @@ const environmentsStore = useEnvironmentsStore(); const usersStore = useUsersStore(); const uiStore = useUIStore(); const telemetry = useTelemetry(); -const i18n = useI18n(); +const { i18n } = useI18n(); const message = useMessage(); const layoutRef = ref | null>(null);