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 627b367ca6a..f119b69e0d4 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 @@ -24,6 +24,7 @@ describe('BreakingChangeService', () => { let workflowStatisticsRepository: Mocked; let ruleRegistry: RuleRegistry; let cacheService: Mocked; + let errorReporter: Mocked; let service: BreakingChangeService; beforeEach(() => { @@ -33,6 +34,7 @@ describe('BreakingChangeService', () => { workflowStatisticsRepository = mock(); ruleRegistry = new RuleRegistry(logger); cacheService = mock(); + errorReporter = mock(); // Mock getHashValue to call refreshFn directly (bypass caching for tests) cacheService.getHashValue.mockImplementation(async (_key, _hashKey, options) => { @@ -52,7 +54,7 @@ describe('BreakingChangeService', () => { workflowStatisticsRepository, cacheService, logger, - mock(), + errorReporter, ); // Manually register only the rules we want to test with @@ -185,6 +187,39 @@ describe('BreakingChangeService', () => { expect(result2).toEqual(result3); }); + it('should skip a rule that throws for a workflow and keep the other results', async () => { + const { workflow } = createWorkflow('wf-1', 'Test Workflow', [ + createNode('Spontit Node', 'n8n-nodes-base.spontit'), + ]); + workflowRepository.find.mockResolvedValue([workflow as never]); + workflowRepository.count.mockResolvedValue(1); + + const throwingRule = ruleRegistry.getRule('file-access-restriction-v2') as FileAccessRule; + vi.spyOn(throwingRule, 'detectWorkflow').mockRejectedValue(new Error('boom')); + + const result = await service.getDetectionResults('v2'); + + const ruleIds = result.report.workflowResults.map((r) => r.ruleId); + expect(ruleIds).toContain('removed-nodes-v2'); + expect(ruleIds).not.toContain(throwingRule.id); + expect(errorReporter.error).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ extra: { ruleId: throwingRule.id, workflowId: 'wf-1' } }), + ); + }); + + it('should reject when detection fails and allow a later detection to run', async () => { + workflowRepository.count.mockRejectedValueOnce(new Error('db down')); + workflowRepository.count.mockResolvedValue(0); + workflowRepository.find.mockResolvedValue([]); + + await expect(service.getDetectionResults('v2')).rejects.toThrow('db down'); + + const detectSpy = vi.spyOn(service, 'detect'); + await expect(service.getDetectionResults('v2')).resolves.toBeDefined(); + expect(detectSpy).toHaveBeenCalledTimes(1); + }); + it('should clean up ongoing detection promise after completion', async () => { workflowRepository.find.mockResolvedValue([]); workflowRepository.count.mockResolvedValue(0); 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 52580401b2b..a5214fad468 100644 --- a/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts +++ b/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts @@ -23,6 +23,7 @@ import type { IBreakingChangeInstanceRule, IBreakingChangeRule, IBreakingChangeWorkflowRule, + WorkflowDetectionReport, } from './types'; import { N8N_VERSION } from '../../constants'; @@ -84,7 +85,6 @@ export class BreakingChangeService { }); } } catch (error) { - console.log('error', error); this.errorReporter.error(error, { shouldBeLogged: true }); } } @@ -227,7 +227,13 @@ export class BreakingChangeService { workflowMetadataMap.set(workflow.id, workflowMetadata); for (const rule of workflowLevelRules) { - const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + let result: WorkflowDetectionReport; + try { + result = await rule.detectWorkflow(workflow, nodesGroupedByType); + } catch (error) { + this.reportRuleError(error, rule.id, workflow.id); + continue; + } if (result.isAffected) { const affectedWorkflow: BreakingChangeAffectedWorkflow = { id: workflow.id, @@ -244,7 +250,11 @@ export class BreakingChangeService { } for (const rule of batchRules) { - await rule.collectWorkflowData(workflow, nodesGroupedByType); + try { + await rule.collectWorkflowData(workflow, nodesGroupedByType); + } catch (error) { + this.reportRuleError(error, rule.id, workflow.id); + } } } } @@ -275,40 +285,39 @@ export class BreakingChangeService { return await existingDetection; } - const cacheKey = `${BreakingChangeService.CACHE_KEY_PREFIX}_${targetVersion}`; - - // Start a new detection and store the promise - const detectionPromise: Promise = new Promise((resolve) => { - void (async () => { - // Check cache first - const cachedResult = await this.cacheService.get(cacheKey); - if (cachedResult) { - this.logger.debug('Using cached breaking change detection results', { - targetVersion, - }); - return resolve(cachedResult); - } - - // Perform detection - const detectionResult = await this.detect(targetVersion); - return resolve(detectionResult); - })(); - }); + const detectionPromise = this.detectWithCache(targetVersion); this.ongoingDetections.set(targetVersion, detectionPromise); try { - const result = await detectionPromise; - // Store in cache if detection took significant time - if (result.shouldCache) { - await this.cacheService.set(cacheKey, result); - } - return result; + return await detectionPromise; } finally { - // Clean up the promise after completion (success or failure) this.ongoingDetections.delete(targetVersion); } } + private async detectWithCache( + targetVersion: BreakingChangeVersion, + ): Promise { + const cacheKey = `${BreakingChangeService.CACHE_KEY_PREFIX}_${targetVersion}`; + + const cachedResult = await this.cacheService.get(cacheKey); + if (cachedResult) { + this.logger.debug('Using cached breaking change detection results', { targetVersion }); + return cachedResult; + } + + const result = await this.detect(targetVersion); + if (result.shouldCache) { + await this.cacheService.set(cacheKey, result); + } + return result; + } + + private reportRuleError(error: unknown, ruleId: string, workflowId: string) { + this.logger.warn('Breaking change rule failed for workflow, skipping', { ruleId, workflowId }); + this.errorReporter.error(error, { extra: { ruleId, workflowId } }); + } + private shouldCacheDetection(durationMs: number): boolean { return durationMs > BreakingChangeService.REPORT_DURATION_CACHE_THRESHOLD; } diff --git a/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/process-env-access.rule.test.ts b/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/process-env-access.rule.test.ts index d6b9143383e..fcb2f0722b6 100644 --- a/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/process-env-access.rule.test.ts +++ b/packages/cli/src/modules/breaking-changes/rules/v2/__tests__/process-env-access.rule.test.ts @@ -1,3 +1,5 @@ +import type { INode } from 'n8n-workflow'; + import { createNode, createWorkflow } from '../../../__tests__/test-helpers'; import { BreakingChangeCategory } from '../../../types'; import { ProcessEnvAccessRule } from '../process-env-access.rule'; @@ -10,6 +12,17 @@ describe('ProcessEnvAccessRule', () => { rule = new ProcessEnvAccessRule(); }); + it('should not be affected by a node that has no parameters', async () => { + const node = { ...createNode('Start', 'n8n-nodes-base.manualTrigger'), parameters: undefined }; + const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ + node as unknown as INode, + ]); + + const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + + expect(result.isAffected).toBe(false); + }); + describe('getMetadata()', () => { it('should return correct metadata', () => { const metadata = rule.getMetadata(); diff --git a/packages/cli/src/modules/breaking-changes/rules/v2/process-env-access.rule.ts b/packages/cli/src/modules/breaking-changes/rules/v2/process-env-access.rule.ts index f2620ceb1e3..7642e573dbb 100644 --- a/packages/cli/src/modules/breaking-changes/rules/v2/process-env-access.rule.ts +++ b/packages/cli/src/modules/breaking-changes/rules/v2/process-env-access.rule.ts @@ -54,7 +54,7 @@ export class ProcessEnvAccessRule implements IBreakingChangeWorkflowRule { } } else { // Check in expressions - const nodeJson = JSON.stringify(node.parameters); + const nodeJson = JSON.stringify(node.parameters ?? {}); if (processEnvPattern.test(nodeJson) && !affectedNodes.some((n) => n.nodeId === node.id)) { affectedNodes.push({ nodeId: node.id, nodeName: node.name }); } diff --git a/packages/cli/src/modules/breaking-changes/rules/v3/__tests__/get-paired-item.rule.test.ts b/packages/cli/src/modules/breaking-changes/rules/v3/__tests__/get-paired-item.rule.test.ts index bbbb9d08551..dba2db9595e 100644 --- a/packages/cli/src/modules/breaking-changes/rules/v3/__tests__/get-paired-item.rule.test.ts +++ b/packages/cli/src/modules/breaking-changes/rules/v3/__tests__/get-paired-item.rule.test.ts @@ -1,3 +1,5 @@ +import type { INode } from 'n8n-workflow'; + import { createNode, createWorkflow } from '../../../__tests__/test-helpers'; import { GetPairedItemRule } from '../get-paired-item.rule'; @@ -9,6 +11,20 @@ describe('GetPairedItemRule', () => { }); describe('detectWorkflow()', () => { + it('should not be affected by a node that has no parameters', async () => { + const node = { + ...createNode('Start', 'n8n-nodes-base.manualTrigger'), + parameters: undefined, + }; + const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ + node as unknown as INode, + ]); + + const result = await rule.detectWorkflow(workflow, nodesGroupedByType); + + expect(result.isAffected).toBe(false); + }); + it('should not be affected when no node uses $getPairedItem', async () => { const { workflow, nodesGroupedByType } = createWorkflow('wf-1', 'Test Workflow', [ createNode('Set', 'n8n-nodes-base.set', { value: '={{ $json.name }}' }), diff --git a/packages/cli/src/modules/breaking-changes/rules/v3/get-paired-item.rule.ts b/packages/cli/src/modules/breaking-changes/rules/v3/get-paired-item.rule.ts index 1d5ca3dea5d..63771f22d89 100644 --- a/packages/cli/src/modules/breaking-changes/rules/v3/get-paired-item.rule.ts +++ b/packages/cli/src/modules/breaking-changes/rules/v3/get-paired-item.rule.ts @@ -47,7 +47,7 @@ export class GetPairedItemRule implements IBreakingChangeWorkflowRule { _nodesGroupedByType: Map, ): Promise { const affectedNodes = workflow.nodes.filter((node) => - JSON.stringify(node.parameters).includes(HELPER_NAME), + JSON.stringify(node.parameters ?? {}).includes(HELPER_NAME), ); return reportAffectedNodes(affectedNodes, (node) => ({