From e785e4e7f3851d022314a3985151ff6783412466 Mon Sep 17 00:00:00 2001 From: Konstantin Tieber <46342664+konstantintieber@users.noreply.github.com> Date: Tue, 2 Jun 2026 14:48:50 +0200 Subject: [PATCH] fix(core): The n8n import:workflow --activeState=fromJson cli can fail for subworkflow dependencies (#31377) --- packages/cli/src/commands/import/workflow.ts | 8 +- .../services/__tests__/import.service.test.ts | 256 ++++++++++++++++-- packages/cli/src/services/import.service.ts | 184 ++++++++----- .../integration/commands/import.cmd.test.ts | 31 ++- .../test/integration/import.service.test.ts | 196 +++++++------- 5 files changed, 490 insertions(+), 185 deletions(-) diff --git a/packages/cli/src/commands/import/workflow.ts b/packages/cli/src/commands/import/workflow.ts index e238967f0a3..c89e615b1d2 100644 --- a/packages/cli/src/commands/import/workflow.ts +++ b/packages/cli/src/commands/import/workflow.ts @@ -131,6 +131,12 @@ export class ImportWorkflowsCommand extends BaseCommand ({ WithTimestampsAndStringId: class {}, })); -jest.mock('@/active-workflow-manager', () => ({ - ActiveWorkflowManager: mock(), -})); - describe('ImportService', () => { let importService: ImportService; let mockLogger: Logger; @@ -47,11 +38,10 @@ describe('ImportService', () => { let mockTagRepository: TagRepository; let mockEntityManager: EntityManager; let mockCipher: Cipher; - let mockActiveWorkflowManager: ActiveWorkflowManager; let mockWorkflowIndexService: WorkflowIndexService; let mockDataTableDDLService: DataTableDDLService; - let mockWorkflowRepository: WorkflowRepository; - let mockWorkflowPublishHistoryRepository: WorkflowPublishHistoryRepository; + let mockUserRepository: UserRepository; + let mockWorkflowService: WorkflowService; beforeEach(() => { jest.clearAllMocks(); @@ -62,11 +52,10 @@ describe('ImportService', () => { mockTagRepository = mock(); mockEntityManager = mock(); mockCipher = mock(); - mockActiveWorkflowManager = mock(); mockWorkflowIndexService = mock(); mockDataTableDDLService = mock(); - mockWorkflowRepository = mock(); - mockWorkflowPublishHistoryRepository = mock(); + mockUserRepository = mock(); + mockWorkflowService = mock(); // Set up cipher mock mockCipher.decryptV2 = jest.fn(async (data: string) => @@ -116,11 +105,10 @@ describe('ImportService', () => { mockTagRepository, mockDataSource, mockCipher, - mockActiveWorkflowManager, mockWorkflowIndexService, mockDataTableDDLService, - mockWorkflowRepository, - mockWorkflowPublishHistoryRepository, + mockUserRepository, + mockWorkflowService, ); }); @@ -1252,6 +1240,234 @@ describe('ImportService', () => { }); }); + describe('extractSubworkflowId', () => { + it('should extract workflow ID from legacy string format', () => { + const node = { + parameters: { workflowId: 'abc123' }, + }; + + // @ts-expect-error accessing private method for testing + expect(importService.extractSubworkflowId(node)).toBe('abc123'); + }); + + it('should extract workflow ID from resource-locator object format', () => { + const node = { + parameters: { + workflowId: { __rl: true, value: 'LCEM9GnTcIVSy1D8', mode: 'list' }, + }, + }; + + // @ts-expect-error accessing private method for testing + expect(importService.extractSubworkflowId(node)).toBe('LCEM9GnTcIVSy1D8'); + }); + + it('should return undefined when workflowId is missing', () => { + const node = { + parameters: {}, + }; + + // @ts-expect-error accessing private method for testing + expect(importService.extractSubworkflowId(node)).toBeUndefined(); + }); + }); + + describe('sortWorkflowsForActivation', () => { + function makeNode(id: string, type: string, parameters: Record = {}) { + return { id, type, parameters, disabled: false } as any; + } + + function makeExecuteWorkflowNode(id: string, calleeId: string) { + return makeNode(id, 'n8n-nodes-base.executeWorkflow', { + workflowId: calleeId, + }); + } + + function makeWorkflow(id: string, nodes: any[] = []) { + return { id, nodes } as any; + } + + function makeToActivate(ids: string[]) { + return ids.map((id) => ({ workflowId: id, versionId: `v-${id}` })); + } + + it('should return single workflow unchanged', () => { + const workflows = [makeWorkflow('A')]; + const toActivate = makeToActivate(['A']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result.map((w) => w.workflowId)).toEqual(['A']); + }); + + it('should activate callee (B) before caller (A) — simple A→B case', () => { + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'B')]), + makeWorkflow('B'), + ]; + const toActivate = makeToActivate(['A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result.map((w) => w.workflowId)).toEqual(['B', 'A']); + }); + + it('should activate C → B → A for a three-level chain', () => { + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'B')]), + makeWorkflow('B', [makeExecuteWorkflowNode('n2', 'C')]), + makeWorkflow('C'), + ]; + const toActivate = makeToActivate(['A', 'B', 'C']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result.map((w) => w.workflowId)).toEqual(['C', 'B', 'A']); + }); + + it('should activate both B and C before A when A calls both', () => { + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'B'), makeExecuteWorkflowNode('n2', 'C')]), + makeWorkflow('B'), + makeWorkflow('C'), + ]; + const toActivate = makeToActivate(['A', 'B', 'C']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + const ids = result.map((w) => w.workflowId); + expect(ids.indexOf('B')).toBeLessThan(ids.indexOf('A')); + expect(ids.indexOf('C')).toBeLessThan(ids.indexOf('A')); + expect(ids).toHaveLength(3); + }); + + it('should ignore referenced workflows not present in the activation batch', () => { + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'EXTERNAL')]), + makeWorkflow('B'), + ]; + const toActivate = makeToActivate(['A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result).toHaveLength(2); + expect(result.map((w) => w.workflowId)).toContain('A'); + expect(result.map((w) => w.workflowId)).toContain('B'); + }); + + it('should skip disabled executeWorkflow nodes when building the dependency graph', () => { + const disabledNode = { ...makeExecuteWorkflowNode('n1', 'B'), disabled: true }; + const workflows = [makeWorkflow('A', [disabledNode]), makeWorkflow('B')]; + const toActivate = makeToActivate(['A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + // A has no active dependencies on B, so order can be anything — just verify both present + expect(result).toHaveLength(2); + }); + + it('should handle resource-locator workflowId format in nodes', () => { + const node = makeNode('n1', 'n8n-nodes-base.executeWorkflow', { + workflowId: { __rl: true, value: 'B', mode: 'list' }, + }); + const workflows = [makeWorkflow('A', [node]), makeWorkflow('B')]; + const toActivate = makeToActivate(['A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result.map((w) => w.workflowId)).toEqual(['B', 'A']); + }); + + it('should return original order (fast path) when no workflow references another batch workflow', () => { + // Both workflows have executeWorkflow nodes, but they point to external IDs not in batch + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'EXTERNAL_1')]), + makeWorkflow('B', [makeExecuteWorkflowNode('n2', 'EXTERNAL_2')]), + ]; + const toActivate = makeToActivate(['A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + // Same reference → fast path returned the original array unchanged + expect(result).toBe(toActivate); + }); + + it('should return original order (fast path) when no workflows have executeWorkflow nodes', () => { + const workflows = [makeWorkflow('A'), makeWorkflow('B'), makeWorkflow('C')]; + const toActivate = makeToActivate(['A', 'B', 'C']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result).toBe(toActivate); + }); + + it('should append mutually-cyclic workflows (A↔B) and log a warning', () => { + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'B')]), + makeWorkflow('B', [makeExecuteWorkflowNode('n2', 'A')]), + ]; + const toActivate = makeToActivate(['A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result).toHaveLength(2); + expect(result.map((w) => w.workflowId)).toContain('A'); + expect(result.map((w) => w.workflowId)).toContain('B'); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('circular')); + }); + + it('should append all workflows in a three-way cycle (A→B→C→A) and log a warning', () => { + const workflows = [ + makeWorkflow('A', [makeExecuteWorkflowNode('n1', 'B')]), + makeWorkflow('B', [makeExecuteWorkflowNode('n2', 'C')]), + makeWorkflow('C', [makeExecuteWorkflowNode('n3', 'A')]), + ]; + const toActivate = makeToActivate(['A', 'B', 'C']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result).toHaveLength(3); + expect(result.map((w) => w.workflowId)).toContain('A'); + expect(result.map((w) => w.workflowId)).toContain('B'); + expect(result.map((w) => w.workflowId)).toContain('C'); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('circular')); + }); + + it('should sort non-cyclic workflows first and append cyclic workflows at the end', () => { + // D→E: normal dependency, no cycle + // A↔B: mutual cycle + const workflows = [ + makeWorkflow('D', [makeExecuteWorkflowNode('n1', 'E')]), + makeWorkflow('E'), + makeWorkflow('A', [makeExecuteWorkflowNode('n2', 'B')]), + makeWorkflow('B', [makeExecuteWorkflowNode('n3', 'A')]), + ]; + const toActivate = makeToActivate(['D', 'E', 'A', 'B']); + + // @ts-expect-error accessing private method for testing + const result = importService.sortWorkflowsForActivation(workflows, toActivate); + + expect(result).toHaveLength(4); + const ids = result.map((w) => w.workflowId); + // E must come before D (normal dependency order) + expect(ids.indexOf('E')).toBeLessThan(ids.indexOf('D')); + // A and B are cyclic — they appear after the sorted pair + expect(ids.indexOf('A')).toBeGreaterThan(ids.indexOf('D')); + expect(ids.indexOf('B')).toBeGreaterThan(ids.indexOf('D')); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('circular')); + }); + }); + describe('advanceIdentitySequences', () => { it('should run setval for each identity column on Postgres', async () => { // @ts-expect-error overriding for the test diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index 61fe0be7f37..fab5ef946c7 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -1,5 +1,5 @@ import { Logger, safeJoinPath } from '@n8n/backend-common'; -import type { TagEntity, ICredentialsDb } from '@n8n/db'; +import type { TagEntity, ICredentialsDb, User } from '@n8n/db'; import { Project, WorkflowEntity, @@ -7,15 +7,15 @@ import { WorkflowTagMapping, CredentialsRepository, TagRepository, + UserRepository, WorkflowHistory, - WorkflowPublishHistory, - WorkflowPublishHistoryRepository, - WorkflowRepository, } from '@n8n/db'; +import { Service } from '@n8n/di'; // eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import import { DataSource, EntityManager, In, type EntityMetadata } from '@n8n/typeorm'; import type { QueryDeepPartialEntity } from '@n8n/typeorm/query-builder/QueryPartialEntity'; -import { Service } from '@n8n/di'; +import { readdir, readFile } from 'fs/promises'; +import { Cipher } from 'n8n-core'; import { ensureError, type INode, @@ -23,23 +23,21 @@ import { type IWorkflowBase, } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; -import { readdir, readFile } from 'fs/promises'; - -import { replaceInvalidCredentials, validateWorkflowStructure } from '@/workflow-helpers'; -import { validateDbTypeForImportEntities } from '@/utils/validate-database-type'; -import { Cipher } from 'n8n-core'; -import { decompressFolder } from '@/utils/compression.util'; import { z } from 'zod'; -import { ActiveWorkflowManager } from '@/active-workflow-manager'; + import type { IWorkflowWithVersionMetadata } from '@/interfaces'; -import { WorkflowIndexService } from '@/modules/workflow-index/workflow-index.service'; -import { DataTableDDLService } from '@/modules/data-table/data-table-ddl.service'; import type { DataTableColumn } from '@/modules/data-table/data-table-column.entity'; +import { DataTableDDLService } from '@/modules/data-table/data-table-ddl.service'; import { normalizeUserRowValueForDatabase, quoteIdentifier, toTableName, } from '@/modules/data-table/utils/sql-utils'; +import { WorkflowIndexService } from '@/modules/workflow-index/workflow-index.service'; +import { decompressFolder } from '@/utils/compression.util'; +import { validateDbTypeForImportEntities } from '@/utils/validate-database-type'; +import { replaceInvalidCredentials, validateWorkflowStructure } from '@/workflow-helpers'; +import { WorkflowService } from '@/workflows/workflow.service'; const DATA_TABLE_ROWS_FILE_PREFIX = 'data_table_user_'; @@ -72,11 +70,10 @@ export class ImportService { private readonly tagRepository: TagRepository, private readonly dataSource: DataSource, private readonly cipher: Cipher, - private readonly activeWorkflowManager: ActiveWorkflowManager, private readonly workflowIndexService: WorkflowIndexService, private readonly dataTableDDLService: DataTableDDLService, - private readonly workflowRepository: WorkflowRepository, - private readonly workflowPublishHistoryRepository: WorkflowPublishHistoryRepository, + private readonly userRepository: UserRepository, + private readonly workflowService: WorkflowService, ) {} async initRecords() { @@ -87,10 +84,16 @@ export class ImportService { async importWorkflows( workflows: IWorkflowWithVersionMetadata[], projectId: string, - { activeState = 'false' }: { activeState?: 'false' | 'fromJson' } = {}, + userId: string, + { activeState = 'false' }: { activeState?: 'false' | 'fromJson' }, ) { await this.initRecords(); + const user = await this.userRepository.findOneOrFail({ + where: { id: userId }, + relations: ['role'], + }); + const { manager: dbManager } = this.credentialsRepository; // Check existence and active status of all workflows @@ -124,18 +127,16 @@ export class ImportService { if (hasInvalidCreds) await this.replaceInvalidCreds(workflow, projectId); validateWorkflowStructure(workflow); - // Remove workflows from ActiveWorkflowManager BEFORE transaction to prevent orphaned trigger listeners - // Only remove if the workflow already exists in the database and is active + // Deactivate BEFORE the transaction to prevent orphaned trigger listeners. + // Only applies to workflows that are currently active in the database. if (workflow.id && activeVersionIdByWorkflow.has(workflow.id)) { - await this.activeWorkflowManager.remove(workflow.id); + await this.workflowService.deactivateWorkflow(user, workflow.id, { source: 'import' }); } } const insertedWorkflows: IWorkflowWithVersionMetadata[] = []; const workflowsToActivate: Array<{ workflowId: string; versionId: string }> = []; await dbManager.transaction(async (tx) => { - const workflowsNeedingPublishHistory: Array<{ workflowId: string; versionId: string }> = []; - // Upsert all workflows for (const workflow of workflows) { // Always generate a new versionId on import to ensure proper history ordering @@ -163,11 +164,6 @@ export class ImportService { const workflowId = upsertResult.identifiers.at(0)?.id as string; insertedWorkflows.push({ ...workflow, id: workflowId }); // Collect inserted workflow with correct ID, for indexing later. - // Only add publish history if workflow was previously active - if (oldActiveVersionId) { - workflowsNeedingPublishHistory.push({ workflowId, versionId: oldActiveVersionId }); - } - if (shouldActivate) { workflowsToActivate.push({ workflowId, versionId: versionIdToActivate }); } @@ -210,20 +206,14 @@ export class ImportService { description: versionMetadata?.description ?? null, }); } - - // Add publish history records for workflows that were deactivated - for (const { workflowId, versionId } of workflowsNeedingPublishHistory) { - await tx.insert(WorkflowPublishHistory, { - workflowId, - versionId, - event: 'deactivated', - userId: null, - }); - } }); - for (const { workflowId, versionId } of workflowsToActivate) { - await this.activateWorkflow(workflowId, versionId); + const orderedWorkflowsToActivate = this.sortWorkflowsForActivation( + insertedWorkflows, + workflowsToActivate, + ); + for (const { workflowId, versionId } of orderedWorkflowsToActivate) { + await this.activateWorkflow(workflowId, versionId, user); } // Directly update the index for the important workflows, since they don't generate @@ -233,29 +223,101 @@ export class ImportService { } } - private async activateWorkflow(workflowId: string, versionIdToActivate: string): Promise { - let didActivate = false; - try { - await this.workflowRepository.update( - { id: workflowId }, - { activeVersionId: versionIdToActivate }, - ); - await this.workflowRepository.updateActiveState(workflowId, true); - await this.activeWorkflowManager.add(workflowId, 'activate'); - didActivate = true; - } catch (e) { - const error = ensureError(e); - this.logger.error(`Failed to activate workflow ${workflowId}`, { error }); - } finally { - if (didActivate) { - await this.workflowPublishHistoryRepository.addRecord({ - workflowId, - versionId: versionIdToActivate, - event: 'activated', - userId: null, - }); + /** + * Sorts workflows to activate in dependency order so that subworkflows are activated + * before the workflows that call them. Uses Kahn's topological sort algorithm. + */ + private sortWorkflowsForActivation( + allImportedWorkflows: IWorkflowWithVersionMetadata[], + toActivate: Array<{ workflowId: string; versionId: string }>, + ): Array<{ workflowId: string; versionId: string }> { + if (toActivate.length <= 1) return toActivate; + + const nodesByWorkflowId = new Map(allImportedWorkflows.map((w) => [w.id, w.nodes])); + const activateIds = new Set(toActivate.map((w) => w.workflowId)); + + // Fast path: skip the full graph build if no workflow in the batch references + // another batch workflow via an active executeWorkflow node. + const hasCrossReference = toActivate.some(({ workflowId }) => + (nodesByWorkflowId.get(workflowId) ?? []).some( + (node) => + !node.disabled && + node.type === 'n8n-nodes-base.executeWorkflow' && + activateIds.has(this.extractSubworkflowId(node) ?? ''), + ), + ); + if (!hasCrossReference) return toActivate; + + const toActivateByWorkflowId = new Map(toActivate.map((w) => [w.workflowId, w])); + // callee id → set of caller ids that depend on it being activated first + const dependents = new Map>( + toActivate.map(({ workflowId }) => [workflowId, new Set()]), + ); + // caller id → how many of its subworkflow dependencies in this batch are not yet activated + const unresolvedDepsCount = new Map( + toActivate.map(({ workflowId }) => [workflowId, 0]), + ); + + for (const { workflowId } of toActivate) { + for (const node of nodesByWorkflowId.get(workflowId) ?? []) { + if (node.disabled || node.type !== 'n8n-nodes-base.executeWorkflow') continue; + const calleeId = this.extractSubworkflowId(node); + if (!calleeId || !activateIds.has(calleeId) || calleeId === workflowId) continue; + dependents.get(calleeId)!.add(workflowId); + unresolvedDepsCount.set(workflowId, unresolvedDepsCount.get(workflowId)! + 1); } } + + const queue = toActivate.filter((w) => unresolvedDepsCount.get(w.workflowId) === 0); + const result: Array<{ workflowId: string; versionId: string }> = []; + + while (queue.length > 0) { + const item = queue.shift()!; + result.push(item); + for (const callerId of dependents.get(item.workflowId)!) { + const remaining = unresolvedDepsCount.get(callerId)! - 1; + unresolvedDepsCount.set(callerId, remaining); + if (remaining === 0) queue.push(toActivateByWorkflowId.get(callerId)!); + } + } + + if (result.length < toActivate.length) { + // Any workflow still with unresolvedDepsCount > 0 was never enqueued by the + // sort — it's part of a cycle (its dependency also waits on it). Append these + // so they still get activated rather than being silently dropped. + const cycleWorkflows = toActivate.filter( + (w) => Number(unresolvedDepsCount.get(w.workflowId)) > 0, + ); + this.logger.warn( + `Detected circular subworkflow references among workflows: [${cycleWorkflows.map((w) => w.workflowId).join(', ')}]. Activating them in original order.`, + ); + result.push(...cycleWorkflows); + } + + return result; + } + + private extractSubworkflowId(node: INode): string | undefined { + const source = node.parameters?.['source']; + if (source === 'parameter' || source === 'localFile' || source === 'url') return undefined; + const wfId = node.parameters?.['workflowId']; + const rawId = typeof wfId === 'string' ? wfId : (wfId as { value?: unknown } | null)?.value; + return typeof rawId === 'string' && !rawId.startsWith('=') ? rawId : undefined; + } + + private async activateWorkflow( + workflowId: string, + versionIdToActivate: string, + user: User, + ): Promise { + try { + await this.workflowService.activateWorkflow(user, workflowId, { + versionId: versionIdToActivate, + source: 'import', + }); + } catch (e) { + this.logger.error(`Failed to activate workflow ${workflowId}`, { error: ensureError(e) }); + } } async replaceInvalidCreds(workflow: IWorkflowBase, projectId: string) { diff --git a/packages/cli/test/integration/commands/import.cmd.test.ts b/packages/cli/test/integration/commands/import.cmd.test.ts index 94f9e540411..07fbd7e2598 100644 --- a/packages/cli/test/integration/commands/import.cmd.test.ts +++ b/packages/cli/test/integration/commands/import.cmd.test.ts @@ -8,12 +8,15 @@ import { import { GlobalConfig } from '@n8n/config'; import { WorkflowPublishHistoryRepository, WorkflowHistoryRepository } from '@n8n/db'; import { Container } from '@n8n/di'; +import type { INodeType } from 'n8n-workflow'; import { nanoid } from 'nanoid'; import '@/zod-alias-support'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; import { ImportWorkflowsCommand } from '@/commands/import/workflow'; import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; +import { NodeTypes } from '@/node-types'; +import { WorkflowService } from '@/workflows/workflow.service'; import { setupTestCommand } from '@test-integration/utils/test-command'; import { createMember, createOwner } from '../shared/db/users'; @@ -21,6 +24,7 @@ import { createMember, createOwner } from '../shared/db/users'; mockInstance(LoadNodesAndCredentials); mockInstance(ActiveWorkflowManager); mockInstance(WorkflowPublishHistoryRepository); +const mockNodeTypes = mockInstance(NodeTypes); const command = setupTestCommand(ImportWorkflowsCommand); @@ -385,6 +389,27 @@ describe('--activeState flag', () => { globalConfig.executions.mode = originalMode; }); + // TODO: fix this workaround being needed for these tests to run. + // It was introduced after refactoring the ImportService used by the import command + // from using the ActiveWorkflowManager to activate/deactivate workflows to the WorkflowService. + beforeEach(() => { + // Bypass webhook conflict detection to avoid infrastructure dependencies + // (getWorkflowExecutionData → VariablesService.getAllCached → CacheService/Redis) + jest + .spyOn(Container.get(WorkflowService) as any, '_findConflictingWebhooks') + .mockResolvedValue([]); + + mockNodeTypes.getByNameAndVersion.mockImplementation((nodeType) => { + if (nodeType === 'n8n-nodes-base.webhook') { + return { + description: { webhooks: undefined, properties: [] }, + webhook: jest.fn(), + } as unknown as INodeType; + } + return { description: { properties: [] } } as unknown as INodeType; + }); + }); + describe('fromJson', () => { it('should activate a workflow that is marked as active in the imported json', async () => { await createOwner(); @@ -410,7 +435,7 @@ describe('--activeState flag', () => { }); it('should deactivate the previously active version and activate the new version when importing a workflow json with an ID that already exists for an active workflow', async () => { - await createOwner(); + const owner = await createOwner(); await command.run([ '--input=./test/integration/commands/import-workflows/combined-with-update/original.json', @@ -441,12 +466,12 @@ describe('--activeState flag', () => { expect(activeWorkflowManager.add).toHaveBeenLastCalledWith('998', 'activate'); const publishHistoryRepo = Container.get(WorkflowPublishHistoryRepository); - expect(publishHistoryRepo.addRecord).toHaveBeenCalledTimes(2); + expect(publishHistoryRepo.addRecord).toHaveBeenCalledTimes(3); expect(publishHistoryRepo.addRecord).toHaveBeenLastCalledWith({ workflowId: '998', versionId: second.versionId, event: 'activated', - userId: null, + userId: owner.id, }); }); }); diff --git a/packages/cli/test/integration/import.service.test.ts b/packages/cli/test/integration/import.service.test.ts index 79e06a70b58..e0a8362f858 100644 --- a/packages/cli/test/integration/import.service.test.ts +++ b/packages/cli/test/integration/import.service.test.ts @@ -16,16 +16,16 @@ import { SharedWorkflowRepository, WorkflowRepository, WorkflowHistoryRepository, - WorkflowPublishHistoryRepository, + UserRepository, } from '@n8n/db'; import { Container } from '@n8n/di'; import { mock } from 'jest-mock-extended'; import type { INode } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; -import type { ActiveWorkflowManager } from '@/active-workflow-manager'; import type { WorkflowIndexService } from '@/modules/workflow-index/workflow-index.service'; import { ImportService } from '@/services/import.service'; +import type { WorkflowService } from '@/workflows/workflow.service'; import { createMember, createOwner } from './shared/db/users'; @@ -34,13 +34,12 @@ describe('ImportService', () => { let tagRepository: TagRepository; let owner: User; let ownerPersonalProject: Project; - let mockActiveWorkflowManager: ActiveWorkflowManager; + let mockWorkflowService: jest.Mocked; let mockWorkflowIndexService: WorkflowIndexService; let workflowRepository: WorkflowRepository; let sharedWorkflowRepository: SharedWorkflowRepository; let workflowHistoryRepository: WorkflowHistoryRepository; - let workflowPublishHistoryRepository: WorkflowPublishHistoryRepository; beforeAll(async () => { await testDb.init(); @@ -48,7 +47,6 @@ describe('ImportService', () => { workflowRepository = Container.get(WorkflowRepository); sharedWorkflowRepository = Container.get(SharedWorkflowRepository); workflowHistoryRepository = Container.get(WorkflowHistoryRepository); - workflowPublishHistoryRepository = Container.get(WorkflowPublishHistoryRepository); owner = await createOwner(); ownerPersonalProject = await getPersonalProject(owner); @@ -56,9 +54,9 @@ describe('ImportService', () => { tagRepository = Container.get(TagRepository); const credentialsRepository = Container.get(CredentialsRepository); + const userRepository = Container.get(UserRepository); - mockActiveWorkflowManager = mock(); - + mockWorkflowService = mock(); mockWorkflowIndexService = mock(); importService = new ImportService( @@ -67,11 +65,10 @@ describe('ImportService', () => { tagRepository, mock(), mock(), - mockActiveWorkflowManager, mockWorkflowIndexService, mock(), - workflowRepository, - workflowPublishHistoryRepository, + userRepository, + mockWorkflowService, ); }); @@ -93,7 +90,7 @@ describe('ImportService', () => { test('should import credless and tagless workflow', async () => { const workflowToImport = await createWorkflow(); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbWorkflow = await getWorkflowById(workflowToImport.id); @@ -106,7 +103,7 @@ describe('ImportService', () => { test('should make user owner of imported workflow', async () => { const workflowToImport = newWorkflow(); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbSharing = await sharedWorkflowRepository.findOneOrFail({ where: { @@ -124,7 +121,7 @@ describe('ImportService', () => { const memberPersonalProject = await getPersonalProject(member); const workflowToImport = await createWorkflow(undefined, owner); - await importService.importWorkflows([workflowToImport], memberPersonalProject.id); + await importService.importWorkflows([workflowToImport], memberPersonalProject.id, owner.id, {}); const sharings = await getAllSharedWorkflows(); @@ -140,7 +137,7 @@ describe('ImportService', () => { test('should deactivate imported workflow if active', async () => { const workflowToImport = await createActiveWorkflow(); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbWorkflow = await getWorkflowById(workflowToImport.id); @@ -169,7 +166,7 @@ describe('ImportService', () => { const workflowToImport = await createWorkflow({ nodes }); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbWorkflow = await getWorkflowById(workflowToImport.id); @@ -189,7 +186,7 @@ describe('ImportService', () => { const workflowToImport = await createWorkflow({ tags: [tag] }); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbWorkflow = await workflowRepository.findOneOrFail({ where: { id: workflowToImport.id }, @@ -210,7 +207,7 @@ describe('ImportService', () => { const workflowToImport = await createWorkflow({ tags: [tag] }); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbWorkflow = await workflowRepository.findOneOrFail({ where: { id: workflowToImport.id }, @@ -229,7 +226,7 @@ describe('ImportService', () => { const workflowToImport = await createWorkflow({ tags: [tag] }); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const dbWorkflow = await workflowRepository.findOneOrFail({ where: { id: workflowToImport.id }, @@ -245,17 +242,21 @@ describe('ImportService', () => { expect(dbTag.name).toBe(tag.name); // tag created }); - test('should remove workflow from ActiveWorkflowManager when workflow has ID', async () => { + test('should call WorkflowService.deactivateWorkflow when workflow has ID and is active', async () => { const workflowWithId = await createActiveWorkflow(); - await importService.importWorkflows([workflowWithId], ownerPersonalProject.id); + await importService.importWorkflows([workflowWithId], ownerPersonalProject.id, owner.id, {}); - expect(mockActiveWorkflowManager.remove).toHaveBeenCalledWith(workflowWithId.id); + expect(mockWorkflowService.deactivateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + workflowWithId.id, + { source: 'import' }, + ); }); test('should always create a record in workflow history', async () => { const workflowToImport = newWorkflow(); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const workflowHistoryRecords = await workflowHistoryRepository.find({ where: { @@ -277,7 +278,7 @@ describe('ImportService', () => { description: 'Historical workflow description', }; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); const workflowHistoryRecords = await workflowHistoryRepository.find({ where: { @@ -290,46 +291,31 @@ describe('ImportService', () => { expect(workflowHistoryRecords[0].description).toBe('Historical workflow description'); }); - test('should create a record in workflow publish history if active version exists', async () => { - // Create an existing active workflow in the database first + test('should call WorkflowService.deactivateWorkflow when re-importing an existing active workflow', async () => { const existingWorkflow = await createActiveWorkflow(); - const originalActiveVersionId = existingWorkflow.activeVersionId!; - // Now import it again (simulating re-import of an active workflow) const workflowToImport = await getWorkflowById(existingWorkflow.id); if (!workflowToImport) fail('Expected to find workflow'); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); - const publishHistoryRecords = await workflowPublishHistoryRepository.find({ - where: { - workflowId: existingWorkflow.id, - event: 'deactivated', - }, - }); - - // Should have publish history for deactivating the original active version - expect(publishHistoryRecords).toHaveLength(1); - expect(publishHistoryRecords[0].versionId).toBe(originalActiveVersionId); + expect(mockWorkflowService.deactivateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + existingWorkflow.id, + { source: 'import' }, + ); }); - test('should not create a record in workflow publish history for new workflows', async () => { + test('should not call WorkflowService.deactivateWorkflow for new (non-existing) workflows', async () => { + mockWorkflowService.deactivateWorkflow.mockClear(); + const workflowToImport = newWorkflow(); workflowToImport.active = true; workflowToImport.activeVersionId = 'some-version'; - if (!workflowToImport) fail('Expected to find workflow'); + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, {}); - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id); - - const publishHistoryRecords = await workflowPublishHistoryRepository.find({ - where: { - workflowId: workflowToImport.id, - event: 'deactivated', - }, - }); - - expect(publishHistoryRecords).toHaveLength(0); + expect(mockWorkflowService.deactivateWorkflow).not.toHaveBeenCalled(); }); test('should always generate a new versionId when importing, ensuring proper history ordering', async () => { @@ -340,7 +326,12 @@ describe('ImportService', () => { const workflowToReimport = await getWorkflowById(initialWorkflow.id); if (!workflowToReimport) fail('Expected to find workflow'); - await importService.importWorkflows([workflowToReimport], ownerPersonalProject.id); + await importService.importWorkflows( + [workflowToReimport], + ownerPersonalProject.id, + owner.id, + {}, + ); const historyRecords = await workflowHistoryRepository.find({ where: { workflowId: initialWorkflow.id }, @@ -361,20 +352,19 @@ describe('ImportService', () => { const workflowToImport = await createWorkflow(); workflowToImport.active = true; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, { + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, { activeState: 'fromJson', }); - const dbWorkflow = await getWorkflowById(workflowToImport.id); - if (!dbWorkflow) fail('Expected to find workflow'); - - expect(dbWorkflow.active).toBe(true); - expect(dbWorkflow.activeVersionId).toBe(dbWorkflow.versionId); - expect(mockActiveWorkflowManager.add).toHaveBeenCalledWith(workflowToImport.id, 'activate'); + expect(mockWorkflowService.activateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + workflowToImport.id, + expect.objectContaining({ source: 'import' }), + ); }); test('should deactivate imported workflow that is updating existing one when JSON has active=false', async () => { - jest.mocked(mockActiveWorkflowManager.add).mockClear(); + mockWorkflowService.activateWorkflow.mockClear(); const existingWorkflow = await createActiveWorkflow(); @@ -382,7 +372,7 @@ describe('ImportService', () => { if (!workflowToImport) fail('Expected to find workflow'); workflowToImport.active = false; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, { + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, { activeState: 'fromJson', }); @@ -391,16 +381,16 @@ describe('ImportService', () => { expect(dbWorkflow.active).toBe(false); expect(dbWorkflow.activeVersionId).toBeNull(); - expect(mockActiveWorkflowManager.add).not.toHaveBeenCalled(); + expect(mockWorkflowService.activateWorkflow).not.toHaveBeenCalled(); }); test('should leave imported workflow deactivated when JSON has active=false', async () => { - jest.mocked(mockActiveWorkflowManager.add).mockClear(); + mockWorkflowService.activateWorkflow.mockClear(); const workflowToImport = await createWorkflow(); workflowToImport.active = false; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, { + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, { activeState: 'fromJson', }); @@ -409,60 +399,58 @@ describe('ImportService', () => { expect(dbWorkflow.active).toBe(false); expect(dbWorkflow.activeVersionId).toBeNull(); - expect(mockActiveWorkflowManager.add).not.toHaveBeenCalled(); + expect(mockWorkflowService.activateWorkflow).not.toHaveBeenCalled(); }); - test('should record both deactivated (old) and activated (new) publish history when re-importing an active workflow', async () => { + test('should call both deactivateWorkflow and activateWorkflow when re-importing an active workflow', async () => { + mockWorkflowService.deactivateWorkflow.mockClear(); + mockWorkflowService.activateWorkflow.mockClear(); + const existingWorkflow = await createActiveWorkflow(); - const originalActiveVersionId = existingWorkflow.activeVersionId!; const workflowToImport = await getWorkflowById(existingWorkflow.id); if (!workflowToImport) fail('Expected to find workflow'); workflowToImport.active = true; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, { + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, { activeState: 'fromJson', }); - const dbWorkflow = await getWorkflowById(existingWorkflow.id); - if (!dbWorkflow) fail('Expected to find workflow'); - - const deactivatedRecords = await workflowPublishHistoryRepository.find({ - where: { workflowId: existingWorkflow.id, event: 'deactivated' }, - }); - const activatedForNewVersion = await workflowPublishHistoryRepository.find({ - where: { - workflowId: existingWorkflow.id, - event: 'activated', - versionId: dbWorkflow.versionId, - }, - }); - - expect(deactivatedRecords).toHaveLength(1); - expect(deactivatedRecords[0].versionId).toBe(originalActiveVersionId); - expect(activatedForNewVersion).toHaveLength(1); - expect(activatedForNewVersion[0].userId).toBeNull(); + expect(mockWorkflowService.deactivateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + existingWorkflow.id, + { source: 'import' }, + ); + expect(mockWorkflowService.activateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + existingWorkflow.id, + expect.objectContaining({ source: 'import' }), + ); }); - test('should not call ActiveWorkflowManager.remove for a brand-new active workflow', async () => { - jest.mocked(mockActiveWorkflowManager.remove).mockClear(); - jest.mocked(mockActiveWorkflowManager.add).mockClear(); + test('should not call WorkflowService.deactivateWorkflow for a brand-new active workflow', async () => { + mockWorkflowService.deactivateWorkflow.mockClear(); + mockWorkflowService.activateWorkflow.mockClear(); const workflowToImport = await createWorkflow(); workflowToImport.active = true; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, { + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, { activeState: 'fromJson', }); - expect(mockActiveWorkflowManager.remove).not.toHaveBeenCalled(); - expect(mockActiveWorkflowManager.add).toHaveBeenCalledTimes(1); - expect(mockActiveWorkflowManager.add).toHaveBeenCalledWith(workflowToImport.id, 'activate'); + expect(mockWorkflowService.deactivateWorkflow).not.toHaveBeenCalled(); + expect(mockWorkflowService.activateWorkflow).toHaveBeenCalledTimes(1); + expect(mockWorkflowService.activateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + workflowToImport.id, + expect.objectContaining({ source: 'import' }), + ); }); - test('should call ActiveWorkflowManager.remove exactly once when re-importing an active workflow', async () => { - jest.mocked(mockActiveWorkflowManager.remove).mockClear(); - jest.mocked(mockActiveWorkflowManager.add).mockClear(); + test('should call WorkflowService.deactivateWorkflow exactly once when re-importing an active workflow', async () => { + mockWorkflowService.deactivateWorkflow.mockClear(); + mockWorkflowService.activateWorkflow.mockClear(); const existingWorkflow = await createActiveWorkflow(); @@ -470,14 +458,22 @@ describe('ImportService', () => { if (!workflowToImport) fail('Expected to find workflow'); workflowToImport.active = true; - await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, { + await importService.importWorkflows([workflowToImport], ownerPersonalProject.id, owner.id, { activeState: 'fromJson', }); - expect(mockActiveWorkflowManager.remove).toHaveBeenCalledTimes(1); - expect(mockActiveWorkflowManager.remove).toHaveBeenCalledWith(existingWorkflow.id); - expect(mockActiveWorkflowManager.add).toHaveBeenCalledTimes(1); - expect(mockActiveWorkflowManager.add).toHaveBeenCalledWith(existingWorkflow.id, 'activate'); + expect(mockWorkflowService.deactivateWorkflow).toHaveBeenCalledTimes(1); + expect(mockWorkflowService.deactivateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + existingWorkflow.id, + { source: 'import' }, + ); + expect(mockWorkflowService.activateWorkflow).toHaveBeenCalledTimes(1); + expect(mockWorkflowService.activateWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ id: owner.id }), + existingWorkflow.id, + expect.objectContaining({ source: 'import' }), + ); }); }); });