github-mirror/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2026-10-17 05:29:44 +02:00
Code Issues Actions 14 Packages Projects Releases Wiki Activity Graph

feat(core): Serve the migration rule detail from the finding table (#39974)

Browse Source 
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
mfsiega 2026-10-02 13:52:49 +00:00 committed by GitHub
parent 3fe4612d93
commit b8ff99f2cc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 128 additions and 195 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts
View File

@ -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<BreakingChangeService>;
let migrationService: MockProxy<BreakingChangeMigrationService>;
let syncService: MockProxy<MigrationFindingSyncService>;
let queryService: MockProxy<MigrationFindingQueryService>;
let ruleRegistry: MockProxy<RuleRegistry>;
let controller: BreakingChangesController;
beforeEach(() => {
service = mock<BreakingChangeService>();
migrationService = mock<BreakingChangeMigrationService>();
syncService = mock<MigrationFindingSyncService>();
queryService = mock<MigrationFindingQueryService>();
ruleRegistry = mock<RuleRegistry>();
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<IBreakingChangeRule>();
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();
});
});
});

123
packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.service.test.ts
View File

@ -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<WorkflowRepository>;
let workflowStatisticsRepository: Mocked<WorkflowStatisticsRepository>;
let ruleRegistry: RuleRegistry;
let cacheService: Mocked<CacheService>;
let errorReporter: Mocked<ErrorReporter>;
let service: BreakingChangeService;
@ -34,17 +30,8 @@ describe('BreakingChangeService', () => {
workflowRepository = mock<WorkflowRepository>();
workflowStatisticsRepository = mock<WorkflowStatisticsRepository>();
ruleRegistry = new RuleRegistry(logger);
cacheService = mock<CacheService>();
errorReporter = mock<ErrorReporter>();
// 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<IBreakingChangeRule['getMetadata']>;

33
packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts
View File

@ -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<BreakingChangeLightReportResult> {
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<BreakingChangeLightReportResult> {
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<BreakingChangeInstanceRuleResult | BreakingChangeWorkflowRuleResult> {
const result = await this.service.getDetectionReportForRule(ruleId);
if (!result) {
): Promise<BreakingChangeWorkflowRuleResult> {
// 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);
}
/**

80
packages/cli/src/modules/breaking-changes/breaking-changes.service.ts
View File

@ -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<BreakingChangeReportResult, 'shouldCache'> {
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<BreakingChangeReportResult>
>();
/** 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<BreakingChangeReportResult> {
await this.cacheService.delete(this.getCacheKey(targetVersion));
return await this.getDetectionResults(targetVersion);
}
async getDetectionResults(
targetVersion: BreakingChangeVersion,
): Promise<BreakingChangeReportResult> {
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<T>(
inFlight: Map<BreakingChangeVersion, Promise<T>>,
@ -334,7 +309,7 @@ export class BreakingChangeService {
): Promise<T> {
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<BreakingChangeReportResult> {
const cacheKey = this.getCacheKey(targetVersion);
const cachedResult = await this.cacheService.get<BreakingChangeReportResult>(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<BreakingChangeDetectionResult> {
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<BreakingChangeInstanceRuleResult | BreakingChangeWorkflowRuleResult | undefined> {
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[],

1
packages/cli/src/modules/breaking-changes/sync/__tests__/migration-finding-sync.service.test.ts
View File

@ -63,7 +63,6 @@ function detectionResult(
workflowResults,
},
totalWorkflows: 0,
shouldCache: false,
failedChecks,
};
}