From 3c896fcdeaec0d6fca26e34e1096f179ff1c833f Mon Sep 17 00:00:00 2001 From: mfsiega <93014743+mfsiega@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:10:53 +0000 Subject: [PATCH] feat(core): Add migration finding repositories (no-changelog) (#39550) Co-authored-by: Claude Fable 5.1 --- .../src/modules/breaking-changes/README.md | 1 + ...ion-finding.repository.integration.test.ts | 321 ++++++++++++++++++ .../migration-finding-sync.repository.ts | 33 ++ .../migration-finding.repository.ts | 93 +++++ 4 files changed, 448 insertions(+) create mode 100644 packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts create mode 100644 packages/cli/src/modules/breaking-changes/database/repositories/migration-finding-sync.repository.ts create mode 100644 packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts diff --git a/packages/cli/src/modules/breaking-changes/README.md b/packages/cli/src/modules/breaking-changes/README.md index 733b65ad70a..861cbd6fab5 100644 --- a/packages/cli/src/modules/breaking-changes/README.md +++ b/packages/cli/src/modules/breaking-changes/README.md @@ -23,6 +23,7 @@ breaking-changes/ index.ts # Side-effect imports for all rules database/ entities/ # migration_finding + migration_finding_sync tables + repositories/ # Use-case-named DB access (BaseRepository + OperationContext) breaking-changes.service.ts # Detection orchestration breaking-changes.rule-registry.service.ts # Rule management breaking-changes.controller.ts # REST API diff --git a/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts b/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts new file mode 100644 index 00000000000..1119fa0c009 --- /dev/null +++ b/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts @@ -0,0 +1,321 @@ +import { createWorkflow, testDb, testModules } from '@n8n/backend-test-utils'; +import { isUniqueConstraintError, TransactionRunner, WorkflowRepository } from '@n8n/db'; +import { Container } from '@n8n/di'; + +import { MigrationFindingSyncRepository } from '../database/repositories/migration-finding-sync.repository'; +import { + MigrationFindingRepository, + type NewMigrationFinding, +} from '../database/repositories/migration-finding.repository'; + +// Runs against the real database: the unique index, the check constraints and +// the cascade delete live in the schema, so a mocked manager would not exercise them. + +const ctx = {}; + +let findingRepository: MigrationFindingRepository; +let syncRepository: MigrationFindingSyncRepository; + +const finding = ( + workflowId: string, + ruleId = 'removed-nodes-v3', + targetVersion: NewMigrationFinding['targetVersion'] = 'v3', +): NewMigrationFinding => ({ targetVersion, ruleId, workflowId }); + +/** A fixed past timestamp, so a moved `statusChangedAt` is distinguishable from a kept one. */ +const PAST = new Date('2026-01-01T00:00:00.000Z'); + +/** Inserts one open finding and pins its `statusChangedAt`; returns the id. */ +async function insertWithStatusChangedAt(workflowId: string, statusChangedAt: Date) { + await findingRepository.insertMany([finding(workflowId)], ctx); + const [row] = await findingRepository.listForWorkflows('v3', [workflowId], ctx); + await findingRepository.update({ id: row.id }, { statusChangedAt }); + return row.id; +} + +beforeAll(async () => { + await testModules.loadModules(['breaking-changes']); + await testDb.init(); + findingRepository = Container.get(MigrationFindingRepository); + syncRepository = Container.get(MigrationFindingSyncRepository); +}); + +beforeEach(async () => { + await findingRepository.delete({}); + await syncRepository.delete({}); + await testDb.truncate(['WorkflowEntity']); +}); + +afterAll(async () => { + await testDb.terminate(); +}); + +describe('MigrationFindingRepository', () => { + describe('insertMany and listForWorkflows', () => { + test('inserts findings as open and lists them for the given workflows only', async () => { + const [first, second, other] = await Promise.all([ + createWorkflow(), + createWorkflow(), + createWorkflow(), + ]); + await findingRepository.insertMany( + [finding(first.id), finding(second.id), finding(other.id)], + ctx, + ); + + const listed = await findingRepository.listForWorkflows('v3', [first.id, second.id], ctx); + + expect(listed.map((f) => f.workflowId).sort()).toEqual([first.id, second.id].sort()); + for (const row of listed) { + expect(row.status).toBe('open'); + expect(row.note).toBeNull(); + expect(row.notifiedAt).toBeNull(); + expect(row.statusChangedAt).toBeInstanceOf(Date); + expect(row.id).toEqual(expect.any(Number)); + } + }); + + test('lists only findings for the requested target version', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany( + [finding(workflow.id, 'rule-a', 'v2'), finding(workflow.id, 'rule-a', 'v3')], + ctx, + ); + + const listed = await findingRepository.listForWorkflows('v2', [workflow.id], ctx); + + expect(listed).toHaveLength(1); + expect(listed[0].targetVersion).toBe('v2'); + }); + + test('returns an empty list for an empty id array', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id)], ctx); + + expect(await findingRepository.listForWorkflows('v3', [], ctx)).toEqual([]); + }); + + test('does nothing for an empty finding array', async () => { + await findingRepository.insertMany([], ctx); + + expect(await findingRepository.count()).toBe(0); + }); + + test('rejects a second finding for the same workflow, rule and target version', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id)], ctx); + + await expect(findingRepository.insertMany([finding(workflow.id)], ctx)).rejects.toSatisfy( + isUniqueConstraintError, + ); + }); + + test('accepts the same rule and workflow for another target version', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id, 'rule-a', 'v2')], ctx); + + await findingRepository.insertMany([finding(workflow.id, 'rule-a', 'v3')], ctx); + + expect(await findingRepository.count()).toBe(2); + }); + + test('rolls back the insert when the surrounding transaction fails', async () => { + const workflow = await createWorkflow(); + + await expect( + Container.get(TransactionRunner).run(ctx, async (trxCtx) => { + await findingRepository.insertMany([finding(workflow.id)], trxCtx); + throw new Error('abort'); + }), + ).rejects.toThrow('abort'); + + expect(await findingRepository.count()).toBe(0); + }); + }); + + describe('updateStatusForIds', () => { + test('updates status, note and statusChangedAt for the given ids only', async () => { + const [target, untouched] = await Promise.all([createWorkflow(), createWorkflow()]); + await findingRepository.insertMany([finding(target.id), finding(untouched.id)], ctx); + const [before] = await findingRepository.listForWorkflows('v3', [target.id], ctx); + + await findingRepository.updateStatusForIds([before.id], 'wont_fix', 'Legacy flow', ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [target.id], ctx); + expect(after.status).toBe('wont_fix'); + expect(after.note).toBe('Legacy flow'); + expect(after.statusChangedAt.getTime()).toBeGreaterThanOrEqual( + before.statusChangedAt.getTime(), + ); + + const [other] = await findingRepository.listForWorkflows('v3', [untouched.id], ctx); + expect(other.status).toBe('open'); + expect(other.note).toBeNull(); + }); + + test('keeps the existing note when note is undefined', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id)], ctx); + const [row] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + await findingRepository.updateStatusForIds([row.id], 'wont_fix', 'Keep me', ctx); + + await findingRepository.updateStatusForIds([row.id], 'notified', undefined, ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.status).toBe('notified'); + expect(after.note).toBe('Keep me'); + }); + + test('moves statusChangedAt on a real status transition', async () => { + const workflow = await createWorkflow(); + const id = await insertWithStatusChangedAt(workflow.id, PAST); + + await findingRepository.updateStatusForIds([id], 'wont_fix', undefined, ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.statusChangedAt.getTime()).toBeGreaterThan(PAST.getTime()); + }); + + test('keeps statusChangedAt when the status does not change', async () => { + const workflow = await createWorkflow(); + const id = await insertWithStatusChangedAt(workflow.id, PAST); + + await findingRepository.updateStatusForIds([id], 'open', undefined, ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.status).toBe('open'); + expect(after.statusChangedAt.getTime()).toBe(PAST.getTime()); + }); + + test('keeps statusChangedAt on a note-only edit', async () => { + const workflow = await createWorkflow(); + const id = await insertWithStatusChangedAt(workflow.id, PAST); + + await findingRepository.updateStatusForIds([id], 'open', 'Edited note', ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.note).toBe('Edited note'); + expect(after.statusChangedAt.getTime()).toBe(PAST.getTime()); + }); + + test('does nothing for an empty id array', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id)], ctx); + + await findingRepository.updateStatusForIds([], 'wont_fix', 'ignored', ctx); + + const [row] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(row.status).toBe('open'); + }); + }); + + describe('markFixedForIds', () => { + test('sets the status to fixed and keeps the note', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id)], ctx); + const [row] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + await findingRepository.updateStatusForIds([row.id], 'notified', 'Owner pinged', ctx); + + await findingRepository.markFixedForIds([row.id], ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.status).toBe('fixed'); + expect(after.note).toBe('Owner pinged'); + }); + }); + + describe('markNotifiedForIds', () => { + test('sets the status to notified and records notifiedAt', async () => { + const workflow = await createWorkflow(); + const id = await insertWithStatusChangedAt(workflow.id, PAST); + + await findingRepository.markNotifiedForIds([id], ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.status).toBe('notified'); + expect(after.notifiedAt?.getTime()).toBeGreaterThan(PAST.getTime()); + expect(after.statusChangedAt.getTime()).toBeGreaterThan(PAST.getTime()); + }); + + test('bumps notifiedAt on a reminder without moving statusChangedAt', async () => { + const workflow = await createWorkflow(); + const id = await insertWithStatusChangedAt(workflow.id, PAST); + await findingRepository.markNotifiedForIds([id], ctx); + await findingRepository.update({ id }, { statusChangedAt: PAST, notifiedAt: PAST }); + + await findingRepository.markNotifiedForIds([id], ctx); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.status).toBe('notified'); + expect(after.notifiedAt?.getTime()).toBeGreaterThan(PAST.getTime()); + expect(after.statusChangedAt.getTime()).toBe(PAST.getTime()); + }); + + test('does nothing for an empty id array', async () => { + const workflow = await createWorkflow(); + await findingRepository.insertMany([finding(workflow.id)], ctx); + + await findingRepository.markNotifiedForIds([], ctx); + + const [row] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(row.status).toBe('open'); + expect(row.notifiedAt).toBeNull(); + }); + }); + + describe('cascade delete', () => { + test('deletes the findings of a deleted workflow and keeps the others', async () => { + const [deleted, kept] = await Promise.all([createWorkflow(), createWorkflow()]); + await findingRepository.insertMany([finding(deleted.id), finding(kept.id)], ctx); + + await Container.get(WorkflowRepository).delete({ id: deleted.id }); + + expect(await findingRepository.findBy({ workflowId: deleted.id })).toEqual([]); + expect(await findingRepository.findBy({ workflowId: kept.id })).toHaveLength(1); + }); + }); +}); + +describe('MigrationFindingSyncRepository', () => { + test('getForVersion returns null when no scan ran for the version', async () => { + expect(await syncRepository.getForVersion('v3', ctx)).toBeNull(); + }); + + test('upsertForVersion inserts a record and then overwrites it', async () => { + const firstSync = new Date('2026-01-01T00:00:00.000Z'); + await syncRepository.upsertForVersion( + { targetVersion: 'v3', syncedAt: firstSync, ruleSetFingerprint: 'fp-1' }, + ctx, + ); + + const inserted = await syncRepository.getForVersion('v3', ctx); + expect(inserted?.syncedAt.getTime()).toBe(firstSync.getTime()); + expect(inserted?.ruleSetFingerprint).toBe('fp-1'); + + const secondSync = new Date('2026-02-01T00:00:00.000Z'); + await syncRepository.upsertForVersion( + { targetVersion: 'v3', syncedAt: secondSync, ruleSetFingerprint: 'fp-2' }, + ctx, + ); + + const updated = await syncRepository.getForVersion('v3', ctx); + expect(updated?.syncedAt.getTime()).toBe(secondSync.getTime()); + expect(updated?.ruleSetFingerprint).toBe('fp-2'); + expect(await syncRepository.count()).toBe(1); + }); + + test('keeps the records of different target versions apart', async () => { + const syncedAt = new Date('2026-01-01T00:00:00.000Z'); + await syncRepository.upsertForVersion( + { targetVersion: 'v2', syncedAt, ruleSetFingerprint: 'fp-v2' }, + ctx, + ); + await syncRepository.upsertForVersion( + { targetVersion: 'v3', syncedAt, ruleSetFingerprint: 'fp-v3' }, + ctx, + ); + + expect((await syncRepository.getForVersion('v2', ctx))?.ruleSetFingerprint).toBe('fp-v2'); + expect((await syncRepository.getForVersion('v3', ctx))?.ruleSetFingerprint).toBe('fp-v3'); + }); +}); diff --git a/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding-sync.repository.ts b/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding-sync.repository.ts new file mode 100644 index 00000000000..d306570f1c8 --- /dev/null +++ b/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding-sync.repository.ts @@ -0,0 +1,33 @@ +import type { BreakingChangeVersion } from '@n8n/api-types'; +import { BaseRepository, type OperationContext, TransactionRunner } from '@n8n/db'; +import { Service } from '@n8n/di'; +import { DataSource } from '@n8n/typeorm'; + +import { MigrationFindingSync } from '../entities/migration-finding-sync.entity'; + +export type MigrationFindingSyncRecord = Pick< + MigrationFindingSync, + 'targetVersion' | 'syncedAt' | 'ruleSetFingerprint' +>; + +@Service() +export class MigrationFindingSyncRepository extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(MigrationFindingSync, dataSource.manager, transactionRunner); + } + + async getForVersion( + targetVersion: BreakingChangeVersion, + ctx: OperationContext, + ): Promise { + return await this.managerFor(ctx).findOneBy(MigrationFindingSync, { targetVersion }); + } + + /** + * Inserts the record for the version, or overwrites it when one exists. + * Not named `upsert`, because that would shadow TypeORM's `Repository.upsert`. + */ + async upsertForVersion(record: MigrationFindingSyncRecord, ctx: OperationContext): Promise { + await this.managerFor(ctx).upsert(MigrationFindingSync, record, ['targetVersion']); + } +} diff --git a/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts b/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts new file mode 100644 index 00000000000..940d429b307 --- /dev/null +++ b/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts @@ -0,0 +1,93 @@ +import type { BreakingChangeVersion, MigrationFindingStatus } from '@n8n/api-types'; +import { BaseRepository, type OperationContext, TransactionRunner } from '@n8n/db'; +import { Service } from '@n8n/di'; +import { DataSource, In, Not, type EntityManager } from '@n8n/typeorm'; + +import { MigrationFinding } from '../entities/migration-finding.entity'; + +/** A finding the scan detected. New findings always start as `open`. */ +export type NewMigrationFinding = Pick; + +@Service() +export class MigrationFindingRepository extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(MigrationFinding, dataSource.manager, transactionRunner); + } + + /** + * All findings for the given workflows and target version. The sync loads + * workflows in batches, so this takes the batch of ids rather than one id. + */ + async listForWorkflows( + targetVersion: BreakingChangeVersion, + workflowIds: string[], + ctx: OperationContext, + ): Promise { + if (workflowIds.length === 0) return []; + + return await this.managerFor(ctx).find(MigrationFinding, { + where: { targetVersion, workflowId: In(workflowIds) }, + order: { id: 'ASC' }, + }); + } + + async insertMany(findings: NewMigrationFinding[], ctx: OperationContext): Promise { + if (findings.length === 0) return; + + const statusChangedAt = new Date(); + await this.managerFor(ctx).insert( + MigrationFinding, + findings.map((finding) => ({ ...finding, status: 'open' as const, statusChangedAt })), + ); + } + + /** + * Sets the triage status. Pass `note` to replace the note; leave it `undefined` to keep it. + * `statusChangedAt` moves only for rows whose status actually changes, so a note edit + * or a repeated sync does not falsify it. + */ + async updateStatusForIds( + ids: number[], + status: MigrationFindingStatus, + note: string | undefined, + ctx: OperationContext, + ): Promise { + if (ids.length === 0) return; + + const manager = this.managerFor(ctx); + await this.transitionStatus(manager, ids, status); + if (note !== undefined) { + await manager.update(MigrationFinding, { id: In(ids) }, { note }); + } + } + + /** Updates only the rows not already in `status`, so `statusChangedAt` marks a real transition. */ + private async transitionStatus( + manager: EntityManager, + ids: number[], + status: MigrationFindingStatus, + ): Promise { + await manager.update( + MigrationFinding, + { id: In(ids), status: Not(status) }, + { status, statusChangedAt: new Date() }, + ); + } + + /** Called by the sync when a re-scan no longer detects the finding. */ + async markFixedForIds(ids: number[], ctx: OperationContext): Promise { + await this.updateStatusForIds(ids, 'fixed', undefined, ctx); + } + + /** + * Records a notification. `notifiedAt` is bumped for every id, so a reminder + * updates it, while `statusChangedAt` moves only on the first transition. + */ + async markNotifiedForIds(ids: number[], ctx: OperationContext): Promise { + if (ids.length === 0) return; + + const manager = this.managerFor(ctx); + await this.transitionStatus(manager, ids, 'notified'); + await manager.update(MigrationFinding, { id: In(ids) }, { notifiedAt: new Date() }); + } +}