From b8ff99f2cc66c64c8347d97d803fb1ed63c13547 Mon Sep 17 00:00:00 2001 From: mfsiega <93014743+mfsiega@users.noreply.github.com> Date: Fri, 2 Oct 2026 13:52:49 +0000 Subject: [PATCH] feat(core): Serve the migration rule detail from the finding table (#39974) Co-authored-by: Claude Fable 5.1 --- .../breaking-changes.controller.test.ts | 86 ++++++++++-- .../breaking-changes.service.test.ts | 123 ++++-------------- .../breaking-changes.controller.ts | 33 +++-- .../breaking-changes.service.ts | 80 +----------- .../migration-finding-sync.service.test.ts | 1 - 5 files changed, 128 insertions(+), 195 deletions(-) diff --git a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts index 0835def6838..ef3e05413fe 100644 --- a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts +++ b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts @@ -1,11 +1,17 @@ -import type { BreakingChangeLightReportResult, BreakingChangeReportQueryDto } from '@n8n/api-types'; +import type { + BreakingChangeLightReportResult, + BreakingChangeReportQueryDto, + BreakingChangeWorkflowRuleResult, +} from '@n8n/api-types'; import type { AuthenticatedRequest } from '@n8n/db'; +import { NotFoundError } from '@n8n/errors'; import type { Response } from 'express'; import { mock, type MockProxy } from 'vitest-mock-extended'; import type { BreakingChangeMigrationService } from '../breaking-changes.migration.service'; -import type { BreakingChangeService } from '../breaking-changes.service'; import { BreakingChangesController } from '../breaking-changes.controller'; +import type { RuleRegistry } from '../breaking-changes.rule-registry.service'; +import type { IBreakingChangeRule } from '../types'; import type { MigrationFindingQueryService } from '../query/migration-finding-query.service'; import type { MigrationFindingSyncService } from '../sync/migration-finding-sync.service'; @@ -27,23 +33,36 @@ function lightReport(generatedAt: Date): BreakingChangeLightReportResult { }; } +function ruleResult(ruleId: string): BreakingChangeWorkflowRuleResult { + return { + ruleId, + ruleTitle: 'Title', + ruleDescription: 'Description', + ruleImpact: 'behaviorChanges', + ruleDocumentationUrl: 'https://docs.n8n.io', + affectedWorkflows: [], + recommendations: [], + migratable: false, + }; +} + describe('BreakingChangesController', () => { - let service: MockProxy; let migrationService: MockProxy; let syncService: MockProxy; let queryService: MockProxy; + let ruleRegistry: MockProxy; let controller: BreakingChangesController; beforeEach(() => { - service = mock(); migrationService = mock(); syncService = mock(); queryService = mock(); + ruleRegistry = mock(); controller = new BreakingChangesController( - service, migrationService, syncService, queryService, + ruleRegistry, ); }); @@ -67,7 +86,6 @@ describe('BreakingChangesController', () => { expect(queryService.getLightReport).toHaveBeenCalledWith('v3'); expect(callOrder).toEqual(['syncIfStale', 'getLightReport']); expect(syncService.sync).not.toHaveBeenCalled(); - expect(service.getDetectionResults).not.toHaveBeenCalled(); }); it('defaults the target version to v2', async () => { @@ -92,23 +110,73 @@ describe('BreakingChangesController', () => { return expected; }); - const result = await controller.refreshCache(req, res, { version: 'v3' }); + const result = await controller.regenerate(req, res, { version: 'v3' }); expect(result).toBe(expected); expect(syncService.sync).toHaveBeenCalledWith('v3'); expect(queryService.getLightReport).toHaveBeenCalledWith('v3'); expect(callOrder).toEqual(['sync', 'getLightReport']); expect(syncService.syncIfStale).not.toHaveBeenCalled(); - expect(service.refreshDetectionResults).not.toHaveBeenCalled(); }); it('defaults the target version to v2', async () => { queryService.getLightReport.mockResolvedValue(lightReport(new Date())); - await controller.refreshCache(req, res, {}); + await controller.regenerate(req, res, {}); expect(syncService.sync).toHaveBeenCalledWith('v2'); expect(queryService.getLightReport).toHaveBeenCalledWith('v2'); }); }); + + describe('GET /report/:ruleId', () => { + function registerRule(ruleId: string, version: 'v2' | 'v3') { + const rule = mock(); + rule.getMetadata.mockReturnValue({ version } as ReturnType< + IBreakingChangeRule['getMetadata'] + >); + ruleRegistry.getRule.calledWith(ruleId).mockReturnValue(rule); + } + + it("syncs the rule's version when stale, then returns the query service result unchanged", async () => { + registerRule('removed-nodes-v3', 'v3'); + const expected = ruleResult('removed-nodes-v3'); + const callOrder: string[] = []; + syncService.syncIfStale.mockImplementation(async () => { + callOrder.push('syncIfStale'); + }); + queryService.getRuleFindings.mockImplementation(async () => { + callOrder.push('getRuleFindings'); + return expected; + }); + + const result = await controller.getDetectionReportForRule(req, res, 'removed-nodes-v3'); + + expect(result).toBe(expected); + expect(syncService.syncIfStale).toHaveBeenCalledWith('v3'); + expect(queryService.getRuleFindings).toHaveBeenCalledWith('v3', 'removed-nodes-v3'); + expect(callOrder).toEqual(['syncIfStale', 'getRuleFindings']); + expect(syncService.sync).not.toHaveBeenCalled(); + }); + + it('uses v2 for a v2 rule', async () => { + registerRule('removed-nodes-v2', 'v2'); + queryService.getRuleFindings.mockResolvedValue(ruleResult('removed-nodes-v2')); + + await controller.getDetectionReportForRule(req, res, 'removed-nodes-v2'); + + expect(syncService.syncIfStale).toHaveBeenCalledWith('v2'); + expect(queryService.getRuleFindings).toHaveBeenCalledWith('v2', 'removed-nodes-v2'); + }); + + it('rejects an unknown rule with not-found before syncing or reading', async () => { + ruleRegistry.getRule.mockReturnValue(undefined); + + await expect( + controller.getDetectionReportForRule(req, res, 'unknown'), + ).rejects.toBeInstanceOf(NotFoundError); + expect(syncService.syncIfStale).not.toHaveBeenCalled(); + expect(queryService.getRuleFindings).not.toHaveBeenCalled(); + }); + }); }); 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 f4d7551daaa..6283e54b857 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 @@ -1,12 +1,9 @@ -import type { BreakingChangeWorkflowRuleResult } from '@n8n/api-types'; import { mockLogger } from '@n8n/backend-test-utils'; import type { WorkflowRepository, WorkflowStatisticsRepository } from '@n8n/db'; import type { ErrorReporter } from 'n8n-core'; import type { Mocked } from 'vitest'; import { mock } from 'vitest-mock-extended'; -import type { CacheService } from '@n8n/backend-services'; - import { N8N_VERSION } from '../../../constants'; import { MigrationRegistry } from '../breaking-changes.migration-registry.service'; import { RuleRegistry } from '../breaking-changes.rule-registry.service'; @@ -24,7 +21,6 @@ describe('BreakingChangeService', () => { let workflowRepository: Mocked; let workflowStatisticsRepository: Mocked; let ruleRegistry: RuleRegistry; - let cacheService: Mocked; let errorReporter: Mocked; let service: BreakingChangeService; @@ -34,17 +30,8 @@ describe('BreakingChangeService', () => { workflowRepository = mock(); 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) => { - if (options?.refreshFn) { - return await options.refreshFn(_key); - } - return undefined; - }); - // Mock statistics repository to return empty array (tests focus on breaking change detection, not statistics) workflowStatisticsRepository.find.mockResolvedValue([]); @@ -53,7 +40,6 @@ describe('BreakingChangeService', () => { new MigrationRegistry(logger), workflowRepository, workflowStatisticsRepository, - cacheService, logger, errorReporter, ); @@ -164,38 +150,31 @@ describe('BreakingChangeService', () => { }); }); - describe('getDetectionResults()', () => { - it('should protect against concurrent detection operations', async () => { - workflowRepository.find.mockResolvedValue([]); - workflowRepository.count.mockResolvedValue(0); - - // Create a spy on the detect method to track how many times it's called - const detectSpy = vi.spyOn(service, 'detect'); - - // Simulate multiple concurrent requests for the same version - const promise1 = service.getDetectionResults('v2'); - const promise2 = service.getDetectionResults('v2'); - const promise3 = service.getDetectionResults('v2'); - - // Wait for all promises to resolve - const [result1, result2, result3] = await Promise.all([promise1, promise2, promise3]); - - // Verify that detect was only called once (not three times) - expect(detectSpy).toHaveBeenCalledTimes(1); - - // Verify all three requests received the same result - expect(result1).toEqual(result2); - expect(result2).toEqual(result3); - }); - - it('should share one scan between a report request and a direct detect call', async () => { + describe('detect() concurrency and failures', () => { + it('should share one scan between concurrent calls for the same version', async () => { workflowRepository.find.mockResolvedValue([]); workflowRepository.count.mockResolvedValue(0); // `count` runs once per scan, so it tells how many scans really ran. - await Promise.all([service.getDetectionResults('v2'), service.detect('v2')]); + const results = await Promise.all([ + service.detect('v2'), + service.detect('v2'), + service.detect('v2'), + ]); expect(workflowRepository.count).toHaveBeenCalledTimes(1); + expect(results[0]).toBe(results[1]); + expect(results[1]).toBe(results[2]); + }); + + it('should run a new scan once the previous one has completed', async () => { + workflowRepository.find.mockResolvedValue([]); + workflowRepository.count.mockResolvedValue(0); + + await service.detect('v2'); + await service.detect('v2'); + + expect(workflowRepository.count).toHaveBeenCalledTimes(2); }); it('should skip a rule that throws for a workflow and keep the other results', async () => { @@ -208,7 +187,7 @@ describe('BreakingChangeService', () => { 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 result = await service.detect('v2'); const ruleIds = result.report.workflowResults.map((r) => r.ruleId); expect(ruleIds).toContain('removed-nodes-v2'); @@ -219,7 +198,7 @@ describe('BreakingChangeService', () => { ); }); - it('should list the rule checks that threw and keep them out of the report result', async () => { + it('should list the rule checks that threw', async () => { const { workflow } = createWorkflow('wf-1', 'Test Workflow', [ createNode('Spontit Node', 'n8n-nodes-base.spontit'), ]); @@ -230,46 +209,19 @@ describe('BreakingChangeService', () => { vi.spyOn(throwingRule, 'detectWorkflow').mockRejectedValue(new Error('boom')); const scan = await service.detect('v2'); - expect(scan.failedChecks).toEqual([{ ruleId: throwingRule.id, workflowId: 'wf-1' }]); - const result = await service.getDetectionResults('v2'); - expect(result).not.toHaveProperty('failedChecks'); + expect(scan.failedChecks).toEqual([{ ruleId: throwingRule.id, workflowId: 'wf-1' }]); }); - it('should reject when detection fails and allow a later detection to run', async () => { + it('should reject when the scan fails and allow a later scan 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'); + await expect(service.detect('v2')).rejects.toThrow('db down'); - const detectSpy = vi.spyOn(service, 'detect'); - await expect(service.getDetectionResults('v2')).resolves.toBeDefined(); - expect(detectSpy).toHaveBeenCalledTimes(1); - }); - - it('should key the cache on the n8n version and the target version', async () => { - workflowRepository.find.mockResolvedValue([]); - workflowRepository.count.mockResolvedValue(0); - - await service.getDetectionResults('v2'); - - expect(cacheService.get).toHaveBeenCalledWith(`breaking-changes:results:${N8N_VERSION}:v2`); - }); - - it('should clean up ongoing detection promise after completion', async () => { - workflowRepository.find.mockResolvedValue([]); - workflowRepository.count.mockResolvedValue(0); - - const detectSpy = vi.spyOn(service, 'detect'); - - // First detection - await service.getDetectionResults('v2'); - expect(detectSpy).toHaveBeenCalledTimes(1); - - // Second detection after the first completes should trigger a new detect call - await service.getDetectionResults('v2'); - expect(detectSpy).toHaveBeenCalledTimes(2); + await expect(service.detect('v2')).resolves.toBeDefined(); + expect(workflowRepository.count).toHaveBeenCalledTimes(2); }); }); @@ -391,29 +343,6 @@ describe('BreakingChangeService', () => { }); }); - describe('getDetectionReportForRule()', () => { - it('should return undefined for unknown rule ID', async () => { - const result = await service.getDetectionReportForRule('unknown-rule-id'); - expect(result).toBeUndefined(); - }); - - it('should return correct report for a known workflow-level rule', 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 result = (await service.getDetectionReportForRule( - 'removed-nodes-v2', - )) as BreakingChangeWorkflowRuleResult; - expect(result).toBeDefined(); - expect(result?.ruleId).toBe('removed-nodes-v2'); - expect(result?.affectedWorkflows).toHaveLength(1); - }); - }); - describe('detectWorkflowHits()', () => { const v2Metadata = () => ({ version: 'v2' }) as ReturnType; diff --git a/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts b/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts index 9550f9c19c3..128aa88fab8 100644 --- a/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts +++ b/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts @@ -1,28 +1,30 @@ import { - BreakingChangeInstanceRuleResult, BreakingChangeLightReportResult, BreakingChangeReportQueryDto, + BreakingChangeVersion, BreakingChangeWorkflowRuleResult, WorkflowMigrationResult, } from '@n8n/api-types'; import { AuthenticatedRequest } from '@n8n/db'; import { Get, RestController, GlobalScope, Query, Post, Param } from '@n8n/decorators'; +import { NotFoundError } from '@n8n/errors'; import { Response } from 'express'; -import { NotFoundError } from '@n8n/errors'; - import { BreakingChangeMigrationService } from './breaking-changes.migration.service'; -import { BreakingChangeService } from './breaking-changes.service'; +import { RuleRegistry } from './breaking-changes.rule-registry.service'; import { MigrationFindingQueryService } from './query/migration-finding-query.service'; import { MigrationFindingSyncService } from './sync/migration-finding-sync.service'; +/** The version the report targets when the request names none. */ +const DEFAULT_TARGET_VERSION: BreakingChangeVersion = 'v2'; + @RestController('/breaking-changes') export class BreakingChangesController { constructor( - private readonly service: BreakingChangeService, private readonly migrationService: BreakingChangeMigrationService, private readonly syncService: MigrationFindingSyncService, private readonly queryService: MigrationFindingQueryService, + private readonly ruleRegistry: RuleRegistry, ) {} /** @@ -36,7 +38,7 @@ export class BreakingChangesController { _res: Response, @Query query: BreakingChangeReportQueryDto, ): Promise { - const version = query.version ?? 'v2'; + const version = query.version ?? DEFAULT_TARGET_VERSION; await this.syncService.syncIfStale(version); return await this.queryService.getLightReport(version); } @@ -44,18 +46,20 @@ export class BreakingChangesController { /** Re-scans every workflow, updates the finding table, and returns the fresh overview. */ @Post('/report/refresh') @GlobalScope('breakingChanges:list') - async refreshCache( + async regenerate( _req: AuthenticatedRequest, _res: Response, @Query query: BreakingChangeReportQueryDto, ): Promise { - const version = query.version ?? 'v2'; + const version = query.version ?? DEFAULT_TARGET_VERSION; await this.syncService.sync(version); return await this.queryService.getLightReport(version); } /** - * Get specific breaking change rules + * The detail of one workflow rule, read from the finding table like the + * overview. The same stale check runs first, so a deep link on a fresh + * instance is not empty. */ @Get('/report/:ruleId') @GlobalScope('breakingChanges:list') @@ -63,12 +67,15 @@ export class BreakingChangesController { _req: AuthenticatedRequest, _res: Response, @Param('ruleId') ruleId: string, - ): Promise { - const result = await this.service.getDetectionReportForRule(ruleId); - if (!result) { + ): Promise { + // The page names the rule but not the version, so the rule decides. + const rule = this.ruleRegistry.getRule(ruleId); + if (!rule) { throw new NotFoundError(`Breaking change rule with ID '${ruleId}' not found.`); } - return result; + const version = rule.getMetadata().version; + await this.syncService.syncIfStale(version); + return await this.queryService.getRuleFindings(version, ruleId); } /** 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 6a96aa38b21..6628adb4723 100644 --- a/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts +++ b/packages/cli/src/modules/breaking-changes/breaking-changes.service.ts @@ -6,7 +6,6 @@ import { BreakingChangeWorkflowRuleResult, } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; -import { Time } from '@n8n/constants'; import { WorkflowRepository, WorkflowStatisticsRepository, type WorkflowEntity } from '@n8n/db'; import { BreakingChangeRuleMetadata } from '@n8n/decorators'; import { Container, Service } from '@n8n/di'; @@ -14,8 +13,6 @@ import { In } from '@n8n/typeorm'; import { ErrorReporter } from 'n8n-core'; import type { INode } from 'n8n-workflow'; -import { CacheService } from '@n8n/backend-services'; - import { MigrationRegistry } from './breaking-changes.migration-registry.service'; import { RuleRegistry } from './breaking-changes.rule-registry.service'; import { groupNodesByType } from './group-nodes-by-type'; @@ -37,9 +34,11 @@ export interface FailedRuleCheck { /** * Result of one full scan. `failedChecks` lists the rule checks that threw, so the report - * is incomplete for those pairs. The public report type does not carry this field. + * is incomplete for those pairs. The public report type does not carry this field, and the + * scan does not decide caching: the finding table replaced the report cache. */ -export interface BreakingChangeDetectionResult extends BreakingChangeReportResult { +export interface BreakingChangeDetectionResult + extends Omit { failedChecks: FailedRuleCheck[]; } @@ -71,12 +70,6 @@ interface WorkflowMetadata { @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, - Promise - >(); /** In-flight full scans per version, so concurrent callers share one run. */ private readonly ongoingScans = new Map< BreakingChangeVersion, @@ -93,7 +86,6 @@ export class BreakingChangeService { private readonly migrationRegistry: MigrationRegistry, private readonly workflowRepository: WorkflowRepository, private readonly workflowStatisticsRepository: WorkflowStatisticsRepository, - private readonly cacheService: CacheService, private readonly logger: Logger, private readonly errorReporter: ErrorReporter, ) { @@ -309,23 +301,6 @@ export class BreakingChangeService { return { results: regularResults.concat(batchResults), failedChecks }; } - async refreshDetectionResults( - targetVersion: BreakingChangeVersion, - ): Promise { - await this.cacheService.delete(this.getCacheKey(targetVersion)); - return await this.getDetectionResults(targetVersion); - } - - async getDetectionResults( - targetVersion: BreakingChangeVersion, - ): Promise { - return await this.shareInFlight( - this.ongoingDetections, - targetVersion, - async () => await this.detectWithCache(targetVersion), - ); - } - /** Joins the promise already in `inFlight` for the version, or starts and registers a new one. */ private async shareInFlight( inFlight: Map>, @@ -334,7 +309,7 @@ export class BreakingChangeService { ): Promise { const existing = inFlight.get(targetVersion); if (existing) { - this.logger.debug('Reusing ongoing detection', { targetVersion }); + this.logger.debug('Reusing ongoing scan', { targetVersion }); return await existing; } @@ -347,32 +322,6 @@ export class BreakingChangeService { } } - // The rule set changes with every release, so a report from an older build is stale. - // The n8n version in the key rotates the entry on deploy; the old key ages out by TTL. - private getCacheKey(targetVersion: BreakingChangeVersion): string { - return `${BreakingChangeService.CACHE_KEY_PREFIX}${N8N_VERSION}:${targetVersion}`; - } - - private async detectWithCache( - targetVersion: BreakingChangeVersion, - ): Promise { - const cacheKey = this.getCacheKey(targetVersion); - - const cachedResult = await this.cacheService.get(cacheKey); - if (cachedResult) { - this.logger.debug('Using cached breaking change detection results', { targetVersion }); - return cachedResult; - } - - // The public report type has no `failedChecks`, so the field is dropped here. - const { report, totalWorkflows, shouldCache } = await this.detect(targetVersion); - const result: BreakingChangeReportResult = { report, totalWorkflows, shouldCache }; - if (result.shouldCache) { - await this.cacheService.set(cacheKey, result); - } - return result; - } - /** Runs one rule on one workflow. A check that throws is reported and added to `failedChecks`. */ private async runWorkflowRule( rule: IBreakingChangeWorkflowRule, @@ -424,10 +373,6 @@ export class BreakingChangeService { return { hits, failedChecks }; } - private shouldCacheDetection(durationMs: number): boolean { - return durationMs > BreakingChangeService.REPORT_DURATION_CACHE_THRESHOLD; - } - /** Runs a full, uncached scan. Concurrent calls for one version share a single scan. */ async detect(targetVersion: BreakingChangeVersion): Promise { return await this.shareInFlight( @@ -530,25 +475,10 @@ export class BreakingChangeService { return { report, totalWorkflows, - shouldCache: this.shouldCacheDetection(duration), failedChecks: workflowScan.failedChecks, }; } - async getDetectionReportForRule( - ruleId: string, - ): Promise { - const rule = this.ruleRegistry.getRule(ruleId); - if (!rule) { - return undefined; - } - - if ('detectWorkflow' in rule || 'collectWorkflowData' in rule) { - return await this.detectRule(rule.getMetadata().version, rule); - } - return (await this.getAllInstanceRulesResults([rule]))[0]; - } - private createDetectionReport( targetVersion: BreakingChangeVersion, instanceResults: BreakingChangeInstanceRuleResult[], diff --git a/packages/cli/src/modules/breaking-changes/sync/__tests__/migration-finding-sync.service.test.ts b/packages/cli/src/modules/breaking-changes/sync/__tests__/migration-finding-sync.service.test.ts index cc76a193544..b2068f4ac78 100644 --- a/packages/cli/src/modules/breaking-changes/sync/__tests__/migration-finding-sync.service.test.ts +++ b/packages/cli/src/modules/breaking-changes/sync/__tests__/migration-finding-sync.service.test.ts @@ -63,7 +63,6 @@ function detectionResult( workflowResults, }, totalWorkflows: 0, - shouldCache: false, failedChecks, }; }