diff --git a/packages/@n8n/api-types/src/schemas/breaking-changes.schema.ts b/packages/@n8n/api-types/src/schemas/breaking-changes.schema.ts index 5ed9df5005a..bc6403eb863 100644 --- a/packages/@n8n/api-types/src/schemas/breaking-changes.schema.ts +++ b/packages/@n8n/api-types/src/schemas/breaking-changes.schema.ts @@ -35,7 +35,7 @@ const affectedWorkflowSchema = z.object({ active: z.boolean(), numberOfExecutions: z.number(), lastUpdatedAt: z.date(), - lastExecutedAt: z.date().nullable(), + lastExecutedAt: z.date().optional(), issues: z.array(workflowIssueSchema), }); export type BreakingChangeAffectedWorkflow = z.infer; diff --git a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.service.test.ts b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.service.test.ts index a8fc2193139..4b140cb81a2 100644 --- a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.service.test.ts +++ b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.service.test.ts @@ -13,6 +13,7 @@ import { createNode, createWorkflow } from './test-helpers'; import { FileAccessRule } from '../rules/v2/file-access.rule'; import { ProcessEnvAccessRule } from '../rules/v2/process-env-access.rule'; import { RemovedNodesRule } from '../rules/v2/removed-nodes.rule'; +import { WaitNodeSubworkflowRule } from '../rules/v2/wait-node-subworkflow.rule'; describe('BreakingChangeService', () => { const logger = mockLogger(); @@ -120,6 +121,38 @@ describe('BreakingChangeService', () => { expect(report.report).toHaveProperty('workflowResults'); expect(Array.isArray(report.report.workflowResults)).toBe(true); }); + + it('should aggregate results from batch rules', async () => { + // Register the batch rule + const waitNodeRule = new WaitNodeSubworkflowRule(); + ruleRegistry.registerAll([waitNodeRule]); + + // Create a sub-workflow with ExecuteWorkflowTrigger and Wait node + const { workflow: subWorkflow } = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('Execute Workflow Trigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + // Create a parent workflow that calls the sub-workflow + const { workflow: parentWorkflow } = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('Execute Workflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: 'sub-wf-1', + }), + ]); + + workflowRepository.find.mockResolvedValue([subWorkflow, parentWorkflow] as never); + workflowRepository.count.mockResolvedValue(2); + + const report = await service.detect('v2'); + + const waitNodeResult = report.report.workflowResults.find( + (r) => r.ruleId === 'wait-node-subworkflow-v2', + ); + expect(waitNodeResult).toBeDefined(); + expect(waitNodeResult?.affectedWorkflows).toHaveLength(1); + expect(waitNodeResult?.affectedWorkflows[0].id).toBe('parent-wf-1'); + }); }); describe('getDetectionResults()', () => { diff --git a/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts b/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts index 4828390cd8b..ab29347485c 100644 --- a/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts +++ b/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts @@ -1,5 +1,5 @@ +import type { BreakingChangeAffectedWorkflow } from '@n8n/api-types'; import { - BreakingChangeAffectedWorkflow, BreakingChangeInstanceRuleResult, BreakingChangeReportResult, BreakingChangeVersion, @@ -10,18 +10,31 @@ import { Time } from '@n8n/constants'; import { WorkflowRepository } from '@n8n/db'; import { Container, Service } from '@n8n/di'; import { ErrorReporter } from 'n8n-core'; -import { INode } from 'n8n-workflow'; +import type { INode } from 'n8n-workflow'; import { RuleRegistry } from './breaking-changes.rule-registry.service'; import { allRules, RuleInstances } from './rules'; -import type { IBreakingChangeWorkflowRule, IBreakingChangeInstanceRule } from './types'; +import type { + IBreakingChangeBatchWorkflowRule, + IBreakingChangeInstanceRule, + IBreakingChangeWorkflowRule, +} from './types'; import { N8N_VERSION } from '../../constants'; import { CacheService } from '@/services/cache/cache.service'; +interface WorkflowMetadata { + name: string; + active: boolean; + numberOfExecutions: number; + lastExecutedAt?: Date; + lastUpdatedAt: Date; +} + @Service() export class BreakingChangeService { private readonly batchSize = 100; + private static readonly REPORT_DURATION_CACHE_THRESHOLD = Time.seconds.toMilliseconds * 2; private static readonly CACHE_KEY_PREFIX = 'breaking-changes:results:'; private readonly ongoingDetections = new Map< BreakingChangeVersion, @@ -72,77 +85,28 @@ export class BreakingChangeService { return instanceLevelResults; } - private async getAllWorkflowRulesResults( - workflowLevelRules: IBreakingChangeWorkflowRule[], - totalWorkflows: number, - ): Promise { - const allAffectedWorkflowsByRule: Map = new Map(); - const allResults: BreakingChangeWorkflowRuleResult[] = []; - - this.logger.debug('Processing workflows in batches', { - totalWorkflows, - batchSize: this.batchSize, - }); - - // Process workflows in batches - for (let skip = 0; skip < totalWorkflows; skip += this.batchSize) { - const workflows = await this.workflowRepository.find({ - select: ['id', 'name', 'active', 'activeVersionId', 'nodes', 'updatedAt', 'statistics'], - skip, - take: this.batchSize, - order: { id: 'ASC' }, - relations: { - statistics: true, - }, - }); - - this.logger.debug('Processing batch', { - skip, - workflowsInBatch: workflows.length, - }); - - for (const workflow of workflows) { - const nodesGroupedByType: Map = new Map(); - for (const node of workflow.nodes) { - if (!nodesGroupedByType.has(node.type)) { - nodesGroupedByType.set(node.type, []); - } - nodesGroupedByType.get(node.type)!.push(node); - } - for (const rule of workflowLevelRules) { - const workflowDetectionResult = await rule.detectWorkflow(workflow, nodesGroupedByType); - if (workflowDetectionResult.isAffected) { - const affectedWorkflow: BreakingChangeAffectedWorkflow = { - id: workflow.id, - name: workflow.name, - active: !!workflow.activeVersionId, - issues: workflowDetectionResult.issues, - numberOfExecutions: workflow.statistics.reduce( - (acc, cur) => acc + (cur.count || 0), - 0, - ), - lastExecutedAt: workflow.statistics.sort( - (a, b) => b.latestEvent.getTime() - a.latestEvent.getTime(), - )[0]?.latestEvent, - lastUpdatedAt: workflow.updatedAt, - }; - if (!allAffectedWorkflowsByRule.has(rule.id)) { - allAffectedWorkflowsByRule.set(rule.id, [affectedWorkflow]); - } else { - allAffectedWorkflowsByRule.get(rule.id)!.push(affectedWorkflow); - } - } - } + private groupNodesByType(nodes: INode[]): Map { + const nodesGroupedByType: Map = new Map(); + for (const node of nodes) { + if (!nodesGroupedByType.has(node.type)) { + nodesGroupedByType.set(node.type, []); } + nodesGroupedByType.get(node.type)!.push(node); } + return nodesGroupedByType; + } - // Aggregate results + private async aggregateRegularRuleResults( + workflowLevelRules: IBreakingChangeWorkflowRule[], + allAffectedWorkflowsByRule: Map, + ): Promise { + const results: BreakingChangeWorkflowRuleResult[] = []; for (const rule of workflowLevelRules) { - const workflowResults = allAffectedWorkflowsByRule.get(rule.id) || []; + const workflowResults = allAffectedWorkflowsByRule.get(rule.id) ?? []; const isAffected = workflowResults.some((wr) => wr.issues.length > 0); if (isAffected) { - allResults.push({ + results.push({ ruleId: rule.id, ruleTitle: rule.getMetadata().title, ruleDescription: rule.getMetadata().description, @@ -153,7 +117,130 @@ export class BreakingChangeService { }); } } - return allResults; + return results; + } + + private async aggregateBatchRuleResults( + batchRules: IBreakingChangeBatchWorkflowRule[], + workflowMetadataMap: Map, + ): Promise { + const results: BreakingChangeWorkflowRuleResult[] = []; + for (const rule of batchRules) { + const batchReport = await rule.produceReport(); + if (batchReport.affectedWorkflows.length === 0) { + continue; + } + + const affectedWorkflows: BreakingChangeAffectedWorkflow[] = []; + for (const affected of batchReport.affectedWorkflows) { + const metadata = workflowMetadataMap.get(affected.workflowId); + if (!metadata) { + this.logger.warn('Workflow metadata not found for batch rule result', { + workflowId: affected.workflowId, + ruleId: rule.id, + }); + continue; + } + + affectedWorkflows.push({ + id: affected.workflowId, + name: metadata.name, + active: metadata.active, + issues: affected.issues, + numberOfExecutions: metadata.numberOfExecutions, + lastExecutedAt: metadata.lastExecutedAt, + lastUpdatedAt: metadata.lastUpdatedAt, + }); + } + + if (affectedWorkflows.length > 0) { + results.push({ + ruleId: rule.id, + ruleTitle: rule.getMetadata().title, + ruleDescription: rule.getMetadata().description, + ruleSeverity: rule.getMetadata().severity, + ruleDocumentationUrl: rule.getMetadata().documentationUrl, + affectedWorkflows, + recommendations: await rule.getRecommendations(affectedWorkflows), + }); + } + } + return results; + } + + private async getAllWorkflowRulesResults( + workflowLevelRules: IBreakingChangeWorkflowRule[], + batchRules: IBreakingChangeBatchWorkflowRule[], + totalWorkflows: number, + ): Promise { + const allAffectedWorkflowsByRule: Map = new Map(); + const workflowMetadataMap: Map = new Map(); + + // Reset batch rules internal state before processing + batchRules.forEach((rule) => rule.reset()); + + this.logger.debug('Processing workflows in batches', { + totalWorkflows, + batchSize: this.batchSize, + regularRulesCount: workflowLevelRules.length, + batchRulesCount: batchRules.length, + }); + + for (let skip = 0; skip < totalWorkflows; skip += this.batchSize) { + const workflows = await this.workflowRepository.find({ + select: ['id', 'name', 'active', 'activeVersionId', 'nodes', 'updatedAt', 'statistics'], + skip, + take: this.batchSize, + order: { id: 'ASC' }, + relations: { statistics: true }, + }); + + this.logger.debug('Processing batch', { skip, workflowsInBatch: workflows.length }); + + for (const workflow of workflows) { + const nodesGroupedByType = this.groupNodesByType(workflow.nodes); + + const workflowMetadata: WorkflowMetadata = { + name: workflow.name, + active: !!workflow.activeVersionId, + numberOfExecutions: workflow.statistics.reduce((acc, cur) => acc + (cur.count || 0), 0), + lastExecutedAt: workflow.statistics.sort( + (a, b) => b.latestEvent.getTime() - a.latestEvent.getTime(), + )[0]?.latestEvent, + lastUpdatedAt: workflow.updatedAt, + }; + workflowMetadataMap.set(workflow.id, workflowMetadata); + + for (const rule of workflowLevelRules) { + const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + if (result.isAffected) { + const affectedWorkflow: BreakingChangeAffectedWorkflow = { + id: workflow.id, + issues: result.issues, + ...workflowMetadata, + }; + const existing = allAffectedWorkflowsByRule.get(rule.id); + if (existing) { + existing.push(affectedWorkflow); + } else { + allAffectedWorkflowsByRule.set(rule.id, [affectedWorkflow]); + } + } + } + + for (const rule of batchRules) { + await rule.collectWorkflowData(workflow, nodesGroupedByType); + } + } + } + + const regularResults = await this.aggregateRegularRuleResults( + workflowLevelRules, + allAffectedWorkflowsByRule, + ); + const batchResults = await this.aggregateBatchRuleResults(batchRules, workflowMetadataMap); + + return regularResults.concat(batchResults); } async refreshDetectionResults( @@ -208,7 +295,7 @@ export class BreakingChangeService { } private shouldCacheDetection(durationMs: number): boolean { - return durationMs > Time.seconds.toMilliseconds * 10; + return durationMs > BreakingChangeService.REPORT_DURATION_CACHE_THRESHOLD; } async detect(targetVersion: BreakingChangeVersion): Promise { @@ -217,14 +304,21 @@ export class BreakingChangeService { const rules = this.ruleRegistry.getRules(targetVersion); - const workflowLevelRules = rules.filter((rule) => 'detectWorkflow' in rule); - const instanceLevelRules = rules.filter((rule) => 'detect' in rule); + const workflowLevelRules = rules.filter( + (rule): rule is IBreakingChangeWorkflowRule => 'detectWorkflow' in rule, + ); + const batchWorkflowRules = rules.filter( + (rule): rule is IBreakingChangeBatchWorkflowRule => 'collectWorkflowData' in rule, + ); + const instanceLevelRules = rules.filter( + (rule): rule is IBreakingChangeInstanceRule => 'detect' in rule, + ); const totalWorkflows = await this.workflowRepository.count(); const [instanceLevelResults, workflowLevelResults] = await Promise.all([ this.getAllInstanceRulesResults(instanceLevelRules), - this.getAllWorkflowRulesResults(workflowLevelRules, totalWorkflows), + this.getAllWorkflowRulesResults(workflowLevelRules, batchWorkflowRules, totalWorkflows), ]); const report = this.createDetectionReport( @@ -253,9 +347,13 @@ export class BreakingChangeService { return undefined; } + const totalWorkflows = await this.workflowRepository.count(); + if ('detectWorkflow' in rule) { - const totalWorkflows = await this.workflowRepository.count(); - return (await this.getAllWorkflowRulesResults([rule], totalWorkflows))[0]; + return (await this.getAllWorkflowRulesResults([rule], [], totalWorkflows))[0]; + } + if ('collectWorkflowData' in rule) { + return (await this.getAllWorkflowRulesResults([], [rule], totalWorkflows))[0]; } return (await this.getAllInstanceRulesResults([rule]))[0]; } diff --git a/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/wait-node-subworkflow.rule.test.ts b/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/wait-node-subworkflow.rule.test.ts index 8eba1cf2755..7819bb8538f 100644 --- a/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/wait-node-subworkflow.rule.test.ts +++ b/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/wait-node-subworkflow.rule.test.ts @@ -8,6 +8,7 @@ describe('WaitNodeSubworkflowRule', () => { beforeEach(() => { jest.clearAllMocks(); rule = new WaitNodeSubworkflowRule(); + rule.reset(); }); describe('getMetadata()', () => { @@ -16,9 +17,8 @@ describe('WaitNodeSubworkflowRule', () => { expect(metadata).toMatchObject({ version: 'v2', - title: 'Waiting node behavior change in sub-workflows', - description: - 'Waiting nodes (Wait, Form, and HITL nodes) in sub-workflows now return data from the last node instead of the node before the waiting node', + title: 'Sub-workflow waiting node output behavior change', + description: expect.stringContaining('Parent workflows calling sub-workflows'), category: BreakingChangeCategory.workflow, severity: 'medium', }); @@ -29,252 +29,393 @@ describe('WaitNodeSubworkflowRule', () => { it('should return recommendations', async () => { const recommendations = await rule.getRecommendations([]); - expect(recommendations).toHaveLength(3); - expect(recommendations).toEqual([ - { - action: 'Review sub-workflow output handling', - description: - 'Check workflows that use Execute Workflow node to call sub-workflows containing waiting nodes (Wait, Form, or HITL nodes). The output data structure may have changed.', - }, - { - action: 'Update downstream logic', - description: - 'Adjust any logic in parent workflows that depends on the data returned from sub-workflows with waiting nodes, as it now returns the last node data instead of the node before the waiting node.', - }, - { - action: 'Test affected workflows', - description: - 'Test all workflows with Execute Workflow nodes calling sub-workflows that contain waiting nodes to ensure the new behavior works as expected.', - }, - ]); + expect(recommendations).toHaveLength(2); + expect(recommendations[0].action).toBe('Review Execute Workflow node outputs'); + expect(recommendations[1].action).toBe('Test affected parent workflows'); }); }); - describe('detectWorkflow()', () => { - it.each([ - { - description: 'workflow has no waiting nodes', - nodes: [ - createNode('HTTP', 'n8n-nodes-base.httpRequest'), - createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - ], - }, - { - description: 'workflow has waiting nodes but no Execute Workflow Trigger', - nodes: [ - createNode('HTTP', 'n8n-nodes-base.httpRequest'), - createNode('Wait', 'n8n-nodes-base.wait'), - ], - }, - { - description: 'workflow has Execute Workflow Trigger but no waiting nodes', - nodes: [ - createNode('HTTP', 'n8n-nodes-base.httpRequest'), - createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - ], - }, - ])('should return no issues when $description', async ({ nodes }) => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', nodes); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); - - expect(result).toEqual({ - isAffected: false, - issues: [], - }); - }); - - it('should detect sub-workflow with Wait nodes', async () => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ + describe('batch workflow detection', () => { + it('should flag parent workflow when it calls a sub-workflow with waiting nodes', async () => { + // Create a sub-workflow with waiting nodes + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), createNode('Wait', 'n8n-nodes-base.wait'), createNode('HTTP', 'n8n-nodes-base.httpRequest'), ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); - - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(1); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe('Wait'); - }); - - it('should detect sub-workflow with multiple Wait nodes', async () => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ - createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - createNode('Wait1', 'n8n-nodes-base.wait'), - createNode('Wait2', 'n8n-nodes-base.wait'), - createNode('HTTP', 'n8n-nodes-base.httpRequest'), + // Create a parent workflow that calls the sub-workflow + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('Start', 'n8n-nodes-base.manualTrigger'), + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + // Collect data from both workflows + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(2); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe('Wait1'); - expect(result.issues[1].nodeId).toBeDefined(); - expect(result.issues[1].nodeName).toBe('Wait2'); + // Produce report + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); + expect(report.affectedWorkflows[0].workflowId).toBe('parent-wf-1'); + expect(report.affectedWorkflows[0].issues).toHaveLength(1); + expect(report.affectedWorkflows[0].issues[0].nodeName).toBe('ExecuteWorkflow'); + expect(report.affectedWorkflows[0].issues[0].description).toContain('sub-wf-1'); }); - it('should detect complex sub-workflow with Wait among other nodes', async () => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ + it('should NOT flag sub-workflow itself', async () => { + // Create a sub-workflow with waiting nodes (no parent calling it) + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + // Sub-workflow should NOT be in the affected list - only parents should be flagged + expect(report.affectedWorkflows).toHaveLength(0); + }); + + it('should NOT flag parent when waitForSubWorkflow is false', async () => { + // Create a sub-workflow with waiting nodes + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + // Create a parent workflow that does NOT wait for sub-workflow completion + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('Start', 'n8n-nodes-base.manualTrigger'), + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + options: { waitForSubWorkflow: false }, + }), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(0); + }); + + it('should flag parent when waitForSubWorkflow is true (explicit)', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + options: { waitForSubWorkflow: true }, + }), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); + expect(report.affectedWorkflows[0].workflowId).toBe('parent-wf-1'); + }); + + it('should flag parent when waitForSubWorkflow is not set (defaults to true)', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + // No options set - waitForSubWorkflow defaults to true + }), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); + }); + + it('should flag parent when waitForSubWorkflow is an expression (treated as true)', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + options: { waitForSubWorkflow: '={{ $json.shouldWait }}' }, + }), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + // Expression is treated as true to avoid false negatives + expect(report.affectedWorkflows).toHaveLength(1); + expect(report.affectedWorkflows[0].workflowId).toBe('parent-wf-1'); + }); + + it('should NOT flag parent when sub-workflow has no waiting nodes', async () => { + // Create a sub-workflow WITHOUT waiting nodes + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), createNode('HTTP', 'n8n-nodes-base.httpRequest'), - createNode('Wait', 'n8n-nodes-base.wait'), - createNode('Code', 'n8n-nodes-base.code'), - createNode('Set', 'n8n-nodes-base.set'), ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); - - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(1); - expect(result.issues[0].level).toBe('warning'); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe('Wait'); - }); - - it('should not detect regular workflow with Wait and Execute Workflow nodes', async () => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ - createNode('HTTP', 'n8n-nodes-base.httpRequest'), - createNode('Wait', 'n8n-nodes-base.wait'), - createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow'), + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); - expect(result.isAffected).toBe(false); - expect(result.issues).toHaveLength(0); + expect(report.affectedWorkflows).toHaveLength(0); }); - it('should detect sub-workflow with Form node', async () => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ + it('should NOT flag parent when sub-workflow has waiting nodes but no ExecuteWorkflowTrigger', async () => { + // A workflow with waiting nodes but NOT a sub-workflow (no trigger) + const regularWorkflow = createWorkflow('regular-wf', 'Regular Workflow', [ + createNode('ManualTrigger', 'n8n-nodes-base.manualTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'regular-wf' }, + }), + ]); + + await rule.collectWorkflowData(regularWorkflow.workflow, regularWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(0); + }); + + it('should handle workflowId as string', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: 'sub-wf-1', // String instead of object + }), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); + }); + + it('should flag when workflowId is an expression (dynamic call)', async () => { + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: '={{ $json.workflowId }}', // Expression - can't evaluate statically + }), + ]); + + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + // Should flag with "may call" warning since we can't determine the workflow ID + expect(report.affectedWorkflows).toHaveLength(1); + expect(report.affectedWorkflows[0].workflowId).toBe('parent-wf-1'); + expect(report.affectedWorkflows[0].issues[0].title).toContain('may call'); + expect(report.affectedWorkflows[0].issues[0].description).toContain('dynamically'); + }); + + it('should flag when source is not database (dynamic call)', async () => { + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'parameter', // JSON parameter source + workflowJson: '{}', + }), + ]); + + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + // Should flag with "may call" warning since we can't determine the workflow ID + expect(report.affectedWorkflows).toHaveLength(1); + expect(report.affectedWorkflows[0].workflowId).toBe('parent-wf-1'); + expect(report.affectedWorkflows[0].issues[0].title).toContain('may call'); + }); + + it('should flag parent when sub-workflow has Form node', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), createNode('Form', 'n8n-nodes-base.form'), - createNode('HTTP', 'n8n-nodes-base.httpRequest'), ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + ]); - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(1); - expect(result.issues[0].description).toContain('form'); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe('Form'); + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); }); - it.each([ - { - nodeName: 'Slack', - nodeType: 'n8n-nodes-base.slack', - operation: 'sendAndWait', - expectedInDescription: 'slack', - }, - ])( - 'should detect sub-workflow with $nodeName HITL node using $operation operation', - async ({ nodeName, nodeType, operation, expectedInDescription }) => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ - createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - createNode(nodeName, nodeType, { operation }), - createNode('HTTP', 'n8n-nodes-base.httpRequest'), - ]); - - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); - - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(1); - expect(result.issues[0].description).toContain(expectedInDescription); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe(nodeName); - }, - ); - - it.each([ - { - nodeName: 'Slack', - nodeType: 'n8n-nodes-base.slack', - nonWaitingOperation: 'sendMessage', - }, - { - nodeName: 'Telegram', - nodeType: 'n8n-nodes-base.telegram', - nonWaitingOperation: 'sendMessage', - }, - { - nodeName: 'GitHub', - nodeType: 'n8n-nodes-base.github', - nonWaitingOperation: 'getIssue', - }, - ])( - 'should NOT detect sub-workflow with $nodeName node without $waitingOperation operation', - async ({ nodeName, nodeType, nonWaitingOperation }) => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ - createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - createNode(nodeName, nodeType, { operation: nonWaitingOperation }), - createNode('HTTP', 'n8n-nodes-base.httpRequest'), - ]); - - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); - - expect(result.isAffected).toBe(false); - expect(result.issues).toHaveLength(0); - }, - ); - - it('should detect sub-workflow with multiple HITL node types', async () => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ + it('should flag parent when sub-workflow has HITL node with sendAndWait operation', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - createNode('Wait', 'n8n-nodes-base.wait'), - createNode('Form', 'n8n-nodes-base.form'), createNode('Slack', 'n8n-nodes-base.slack', { operation: 'sendAndWait' }), - createNode('HTTP', 'n8n-nodes-base.httpRequest'), ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + ]); - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(3); - expect(result.issues[0].description).toContain('wait'); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe('Wait'); - expect(result.issues[1].description).toContain('form'); - expect(result.issues[1].nodeId).toBeDefined(); - expect(result.issues[1].nodeName).toBe('Form'); - expect(result.issues[2].description).toContain('slack'); - expect(result.issues[2].nodeId).toBeDefined(); - expect(result.issues[2].nodeName).toBe('Slack'); + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); }); - it.each([ - { nodeName: 'Telegram', nodeType: 'n8n-nodes-base.telegram', operation: 'sendAndWait' }, - { nodeName: 'EmailSend', nodeType: 'n8n-nodes-base.emailSend', operation: 'sendAndWait' }, - { - nodeName: 'MicrosoftTeams', - nodeType: 'n8n-nodes-base.microsoftTeams', - operation: 'sendAndWait', - }, - { - nodeName: 'MicrosoftOutlook', - nodeType: 'n8n-nodes-base.microsoftOutlook', - operation: 'sendAndWait', - }, - { nodeName: 'Discord', nodeType: 'n8n-nodes-base.discord', operation: 'sendAndWait' }, - { nodeName: 'GitHub', nodeType: 'n8n-nodes-base.github', operation: 'dispatchAndWait' }, - ])( - 'should detect sub-workflow with $nodeName HITL node using $operation', - async ({ nodeName, nodeType, operation }) => { - const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ - createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), - createNode(nodeName, nodeType, { operation }), - ]); + it('should NOT flag when HITL node does not use waiting operation', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Slack', 'n8n-nodes-base.slack', { operation: 'sendMessage' }), // Not sendAndWait + ]); - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + ]); - expect(result.isAffected).toBe(true); - expect(result.issues).toHaveLength(1); - expect(result.issues[0].nodeId).toBeDefined(); - expect(result.issues[0].nodeName).toBe(nodeName); - }, - ); + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(0); + }); + + it('should flag parent calling multiple affected sub-workflows', async () => { + const subWorkflow1 = createWorkflow('sub-wf-1', 'Sub Workflow 1', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const subWorkflow2 = createWorkflow('sub-wf-2', 'Sub Workflow 2', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Form', 'n8n-nodes-base.form'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow1', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + createNode('ExecuteWorkflow2', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-2' }, + }), + ]); + + await rule.collectWorkflowData(subWorkflow1.workflow, subWorkflow1.nodesGroupedByType); + await rule.collectWorkflowData(subWorkflow2.workflow, subWorkflow2.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(1); + expect(report.affectedWorkflows[0].workflowId).toBe('parent-wf-1'); + expect(report.affectedWorkflows[0].issues).toHaveLength(2); + }); + + it('should flag multiple parents calling the same affected sub-workflow', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow1 = createWorkflow('parent-wf-1', 'Parent Workflow 1', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + ]); + + const parentWorkflow2 = createWorkflow('parent-wf-2', 'Parent Workflow 2', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + ]); + + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow1.workflow, parentWorkflow1.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow2.workflow, parentWorkflow2.nodesGroupedByType); + const report = await rule.produceReport(); + + expect(report.affectedWorkflows).toHaveLength(2); + const workflowIds = report.affectedWorkflows.map((w) => w.workflowId); + expect(workflowIds).toContain('parent-wf-1'); + expect(workflowIds).toContain('parent-wf-2'); + }); + + it('should reset state correctly', async () => { + const subWorkflow = createWorkflow('sub-wf-1', 'Sub Workflow', [ + createNode('ExecuteWorkflowTrigger', 'n8n-nodes-base.executeWorkflowTrigger'), + createNode('Wait', 'n8n-nodes-base.wait'), + ]); + + const parentWorkflow = createWorkflow('parent-wf-1', 'Parent Workflow', [ + createNode('ExecuteWorkflow', 'n8n-nodes-base.executeWorkflow', { + source: 'database', + workflowId: { value: 'sub-wf-1' }, + }), + ]); + + // First run + await rule.collectWorkflowData(subWorkflow.workflow, subWorkflow.nodesGroupedByType); + await rule.collectWorkflowData(parentWorkflow.workflow, parentWorkflow.nodesGroupedByType); + const report1 = await rule.produceReport(); + expect(report1.affectedWorkflows).toHaveLength(1); + + // Reset + rule.reset(); + + // Second run should be empty without collecting data again + const report2 = await rule.produceReport(); + expect(report2.affectedWorkflows).toHaveLength(0); + }); }); }); diff --git a/packages/cli/src/modules/breaking-changes/rules/v2/wait-node-subworkflow.rule.ts b/packages/cli/src/modules/breaking-changes/rules/v2/wait-node-subworkflow.rule.ts index 2574eadbaae..5bee49c6066 100644 --- a/packages/cli/src/modules/breaking-changes/rules/v2/wait-node-subworkflow.rule.ts +++ b/packages/cli/src/modules/breaking-changes/rules/v2/wait-node-subworkflow.rule.ts @@ -1,20 +1,34 @@ -import type { BreakingChangeAffectedWorkflow, BreakingChangeRecommendation } from '@n8n/api-types'; +import type { + BreakingChangeAffectedWorkflow, + BreakingChangeRecommendation, + BreakingChangeWorkflowIssue, +} from '@n8n/api-types'; import type { WorkflowEntity } from '@n8n/db'; import { Service } from '@n8n/di'; -import type { INode } from 'n8n-workflow'; +import type { INode, INodeParameters } from 'n8n-workflow'; import { SEND_AND_WAIT_OPERATION } from 'n8n-workflow'; import type { + BatchWorkflowDetectionReport, BreakingChangeRuleMetadata, - IBreakingChangeWorkflowRule, - WorkflowDetectionReport, + IBreakingChangeBatchWorkflowRule, } from '../../types'; import { BreakingChangeCategory } from '../../types'; +interface ParentWorkflowInfo { + parentWorkflowId: string; + executeWorkflowNode: INode; + calledWorkflowId?: string; +} + @Service() -export class WaitNodeSubworkflowRule implements IBreakingChangeWorkflowRule { +export class WaitNodeSubworkflowRule implements IBreakingChangeBatchWorkflowRule { id: string = 'wait-node-subworkflow-v2'; + // Internal state for batch processing + private subWorkflowsWithWaitingNodes: Map = new Map(); // workflowId -> workflowName + private parentWorkflowsCalling: ParentWorkflowInfo[] = []; + // Configuration for node types and their waiting conditions private readonly waitingNodeConfig: Array<{ nodeTypes: string[]; operation?: string }> = [ { @@ -51,13 +65,13 @@ export class WaitNodeSubworkflowRule implements IBreakingChangeWorkflowRule { getMetadata(): BreakingChangeRuleMetadata { return { version: 'v2', - title: 'Waiting node behavior change in sub-workflows', + title: 'Sub-workflow waiting node output behavior change', description: - 'Waiting nodes (Wait, Form, and HITL nodes) in sub-workflows now return data from the last node instead of the node before the waiting node', + 'Parent workflows calling sub-workflows with waiting nodes (Wait, Form, HITL) now receive correct data. Previously, incorrect results were returned when the sub-workflow entered a waiting state.', category: BreakingChangeCategory.workflow, severity: 'medium', documentationUrl: - 'https://docs.n8n.io/2-0-breaking-changes/#return-expected-sub-workflow-data-when-it-contains-a-wait-node', + 'https://docs.n8n.io/2-0-breaking-changes/#return-expected-sub-workflow-data-when-the-sub-workflow-resumes-from-waiting-waiting-for-webhook-forms-hitl-etc', }; } @@ -66,80 +80,155 @@ export class WaitNodeSubworkflowRule implements IBreakingChangeWorkflowRule { ): Promise { return [ { - action: 'Review sub-workflow output handling', + action: 'Review Execute Workflow node outputs', description: - 'Check workflows that use Execute Workflow node to call sub-workflows containing waiting nodes (Wait, Form, or HITL nodes). The output data structure may have changed.', + 'Check the Execute Workflow nodes flagged above. If your workflow logic depends on the specific data returned from sub-workflows containing waiting nodes, verify the data structure is correct.', }, { - action: 'Update downstream logic', + action: 'Test affected parent workflows', description: - 'Adjust any logic in parent workflows that depends on the data returned from sub-workflows with waiting nodes, as it now returns the last node data instead of the node before the waiting node.', - }, - { - action: 'Test affected workflows', - description: - 'Test all workflows with Execute Workflow nodes calling sub-workflows that contain waiting nodes to ensure the new behavior works as expected.', + 'Run the affected parent workflows to verify the data returned from sub-workflows with waiting nodes is now correct and matches your expectations.', }, ]; } - private hasWaitingOperation(node: INode, requiredOperation: string): boolean { - const operation = node.parameters.operation; - return operation === requiredOperation; + reset(): void { + this.subWorkflowsWithWaitingNodes.clear(); + this.parentWorkflowsCalling = []; } - async detectWorkflow( - _workflow: WorkflowEntity, + async collectWorkflowData( + workflow: WorkflowEntity, nodesGroupedByType: Map, - ): Promise { - // Check if the workflow contains any waiting nodes (Wait, Form, or HITL nodes) - const foundWaitingNodes: Array<{ node: INode; nodeTypeName: string }> = []; + ): Promise { + // Check if this workflow IS a sub-workflow with waiting nodes + const hasExecuteWorkflowTrigger = + (nodesGroupedByType.get('n8n-nodes-base.executeWorkflowTrigger')?.length ?? 0) > 0; + const hasWaitingNodes = this.findWaitingNodes(nodesGroupedByType).length > 0; + + if (hasExecuteWorkflowTrigger && hasWaitingNodes) { + this.subWorkflowsWithWaitingNodes.set(workflow.id, workflow.name); + } + + // Check if this workflow CALLS sub-workflows with waitForSubWorkflow enabled + const executeWorkflowNodes = nodesGroupedByType.get('n8n-nodes-base.executeWorkflow') ?? []; + for (const node of executeWorkflowNodes) { + // Check if waitForSubWorkflow is enabled (default is true) + const options = node.parameters.options as INodeParameters | undefined; + const waitForSubWorkflowValue = options?.waitForSubWorkflow; + // If waitForSubWorkflow is explicitly false, skip. Otherwise treat as true (default). + // Expressions (strings starting with =) are treated as true to avoid false negatives. + const isExplicitlyFalse = waitForSubWorkflowValue === false; + if (isExplicitlyFalse) { + continue; // Skip if not waiting for sub-workflow completion + } + + const calledWorkflowId = this.extractCalledWorkflowId(node); + + this.parentWorkflowsCalling.push({ + parentWorkflowId: workflow.id, + executeWorkflowNode: node, + calledWorkflowId, + }); + } + } + + async produceReport(): Promise { + // Group issues by parent workflow ID (a parent may call multiple affected sub-workflows) + const issuesByParentWorkflow = new Map(); + + // For each parent workflow calling a sub-workflow, check if it calls an affected sub-workflow + for (const parent of this.parentWorkflowsCalling) { + const isUnknownWorkflow = parent.calledWorkflowId === undefined; + const subWorkflowName = + parent.calledWorkflowId !== undefined + ? this.subWorkflowsWithWaitingNodes.get(parent.calledWorkflowId) + : undefined; + const isKnownAffectedWorkflow = subWorkflowName !== undefined; + + if (!isUnknownWorkflow && !isKnownAffectedWorkflow) { + continue; + } + + const issue: BreakingChangeWorkflowIssue = { + title: isUnknownWorkflow + ? 'Execute Workflow node may call sub-workflow with changed output behavior' + : 'Execute Workflow node calls sub-workflow with changed output behavior', + description: isUnknownWorkflow + ? `The "${parent.executeWorkflowNode.name}" node calls a sub-workflow dynamically (via expression or parameter). If the called sub-workflow contains waiting nodes (Wait, Form, Human-in-the-loop), the data returned has changed in v2 - it now returns the correct data instead of the previously incorrect results.` + : `The "${parent.executeWorkflowNode.name}" node calls sub-workflow "${subWorkflowName}" (ID: ${parent.calledWorkflowId}) which contains waiting nodes. The data returned from this sub-workflow has changed in v2 - it now returns the correct data instead of the previously incorrect results.`, + level: 'warning', + nodeId: parent.executeWorkflowNode.id, + nodeName: parent.executeWorkflowNode.name, + }; + + const existingIssues = issuesByParentWorkflow.get(parent.parentWorkflowId); + if (existingIssues) { + existingIssues.push(issue); + } else { + issuesByParentWorkflow.set(parent.parentWorkflowId, [issue]); + } + } + + // Convert to the expected format + const affectedWorkflows: BatchWorkflowDetectionReport['affectedWorkflows'] = []; + for (const [workflowId, issues] of issuesByParentWorkflow) { + affectedWorkflows.push({ workflowId, issues }); + } + + return { affectedWorkflows }; + } + + private findWaitingNodes(nodesGroupedByType: Map): INode[] { + const waitingNodes: INode[] = []; - // Check all configured node types for (const { nodeTypes, operation } of this.waitingNodeConfig) { for (const nodeType of nodeTypes) { const nodes = nodesGroupedByType.get(nodeType) ?? []; // If no operation is specified, all nodes of this type wait // Otherwise, filter for nodes with the specific operation - const waitingNodes = operation - ? nodes.filter((node) => this.hasWaitingOperation(node, operation)) + const matchingNodes = operation + ? nodes.filter((node) => node.parameters.operation === operation) : nodes; - for (const node of waitingNodes) { - const nodeTypeName = nodeType.split('.').pop() ?? nodeType; - foundWaitingNodes.push({ node, nodeTypeName }); - } + waitingNodes.push(...matchingNodes); } } - if (foundWaitingNodes.length === 0) { - return { isAffected: false, issues: [] }; + return waitingNodes; + } + + private extractCalledWorkflowId(node: INode): string | undefined { + const source = node.parameters.source as string | undefined; + + // Only handle database source - for other sources we can't determine the workflow ID statically + if (source !== 'database' && source !== undefined) { + return undefined; } - // Check if this workflow IS a subworkflow by looking for Execute Workflow Trigger - const executeWorkflowTriggerNodes = - nodesGroupedByType.get('n8n-nodes-base.executeWorkflowTrigger') ?? []; + // Default source is 'database', so also handle when source is undefined + const workflowId = node.parameters.workflowId; - if (executeWorkflowTriggerNodes.length === 0) { - return { isAffected: false, issues: [] }; + if (typeof workflowId === 'string') { + // Check if it's an expression (starts with =) + if (workflowId.startsWith('=')) { + return undefined; // Can't evaluate expressions statically + } + return workflowId; } - // This workflow is a subworkflow (has Execute Workflow Trigger) and contains waiting nodes - // The output behavior has changed - // Create one issue per waiting node + if (typeof workflowId === 'object' && workflowId !== null && 'value' in workflowId) { + const value = workflowId.value; + if (typeof value === 'string') { + // Check if it's an expression (starts with =) + if (value.startsWith('=')) { + return undefined; // Can't evaluate expressions statically + } + return value; + } + } - const issues = foundWaitingNodes.map(({ node, nodeTypeName }) => ({ - title: 'Sub-workflow with waiting node has changed output behavior', - description: `This workflow is a sub-workflow (contains Execute Workflow Trigger) with a waiting node (${nodeTypeName}). The data returned to the parent workflow from sub-workflows containing waiting nodes has changed. Previously, the child workflow returned data from the node before the waiting node. Now they return data from the last node in the workflow.`, - level: 'warning' as const, - nodeId: node.id, - nodeName: node.name, - })); - - return { - isAffected: true, - issues, - }; + return undefined; } } diff --git a/packages/cli/src/modules/breaking-changes/types/detection.types.ts b/packages/cli/src/modules/breaking-changes/types/detection.types.ts index 6561c425fd5..162eb9a55b2 100644 --- a/packages/cli/src/modules/breaking-changes/types/detection.types.ts +++ b/packages/cli/src/modules/breaking-changes/types/detection.types.ts @@ -14,3 +14,14 @@ export interface InstanceDetectionReport { instanceIssues: BreakingChangeInstanceIssue[]; recommendations: BreakingChangeRecommendation[]; } + +/** + * Report returned by batch workflow rules after processing all workflows. + * Used when a rule needs to correlate data across multiple workflows before producing results. + */ +export interface BatchWorkflowDetectionReport { + affectedWorkflows: Array<{ + workflowId: string; + issues: BreakingChangeWorkflowIssue[]; + }>; +} diff --git a/packages/cli/src/modules/breaking-changes/types/rule.types.ts b/packages/cli/src/modules/breaking-changes/types/rule.types.ts index 1bcc4827253..276cf13e7c5 100644 --- a/packages/cli/src/modules/breaking-changes/types/rule.types.ts +++ b/packages/cli/src/modules/breaking-changes/types/rule.types.ts @@ -7,7 +7,11 @@ import type { import type { WorkflowEntity } from '@n8n/db'; import type { INode } from 'n8n-workflow'; -import type { InstanceDetectionReport, WorkflowDetectionReport } from './detection.types'; +import type { + BatchWorkflowDetectionReport, + InstanceDetectionReport, + WorkflowDetectionReport, +} from './detection.types'; export const enum BreakingChangeCategory { workflow = 'workflow', @@ -45,4 +49,43 @@ export interface IBreakingChangeWorkflowRule { ): Promise; } -export type IBreakingChangeRule = IBreakingChangeInstanceRule | IBreakingChangeWorkflowRule; +/** + * Interface for batch-based workflow rules that need to correlate data across multiple workflows. + * Unlike IBreakingChangeWorkflowRule which processes each workflow independently, + * batch rules collect data from all workflows first, then produce a final report. + * + * Use case example: Detecting parent workflows that call sub-workflows with specific characteristics, + * where the rule needs to identify both the sub-workflows and their callers before reporting. + */ +export interface IBreakingChangeBatchWorkflowRule { + id: string; + getMetadata(): BreakingChangeRuleMetadata; + getRecommendations( + workflowResults: BreakingChangeAffectedWorkflow[], + ): Promise; + + /** + * Called for each workflow during the scanning phase. + * The rule should collect and store any relevant data internally. + */ + collectWorkflowData( + workflow: WorkflowEntity, + nodesGroupedByType: Map, + ): Promise; + + /** + * Called after all workflows have been scanned to produce the final report. + * The rule should correlate the collected data and return the affected workflows. + */ + produceReport(): Promise; + + /** + * Called to reset internal state before a new detection run. + */ + reset(): void; +} + +export type IBreakingChangeRule = + | IBreakingChangeInstanceRule + | IBreakingChangeWorkflowRule + | IBreakingChangeBatchWorkflowRule; diff --git a/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts b/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts index 062ed663785..ebb11b05669 100644 --- a/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts @@ -40,7 +40,6 @@ const mockWorkflowWithMultipleNodes = { active: false, numberOfExecutions: 50, lastUpdatedAt: new Date('2024-01-10'), - lastExecutedAt: null, issues: [ { nodeId: 'node-2',