diff --git a/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.test.ts b/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.test.ts index 1e1c030d4e1..9cf1e91d504 100644 --- a/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.test.ts +++ b/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.test.ts @@ -140,14 +140,57 @@ describe('MigrationTimestampRule', () => { const violations = await rule.analyze(context([a, b, head])); - // `a` and `b` are below the new head — they violate ordering. - // This is intentional and expected for a multi-migration PR: each - // added migration must be greater than every other migration - // including the others added in the same PR. Authors should - // instead order them so each is strictly the head at the time - // of its conceptual insertion. - const violatingFiles = violations.map((v) => path.basename(v.file)).sort(); - expect(violatingFiles).toEqual(['1799000000001-A.ts', '1799000000002-B.ts']); + expect(violations).toEqual([]); + }); + + it('accepts multiple added migrations when no existing migrations exist', async () => { + const a = writeMigration(tmpDir, COMMON_DIR, '1777000000000-A.ts'); + const b = writeMigration(tmpDir, POSTGRES_DIR, '1777000000001-B.ts'); + const c = writeMigration(tmpDir, SQLITE_DIR, '1777000000002-C.ts'); + + const violations = await rule.analyze(context([c, a, b])); + + expect(violations).toEqual([]); + }); + + it('checks every added migration against the existing head', async () => { + writeMigration(tmpDir, COMMON_DIR, '1700000000000-Old.ts'); + writeMigration(tmpDir, POSTGRES_DIR, '1777000000000-Head.ts'); + const below = writeMigration(tmpDir, COMMON_DIR, '1750000000000-Below.ts'); + const above = writeMigration(tmpDir, SQLITE_DIR, '1777000000001-Above.ts'); + const head = writeMigration(tmpDir, COMMON_DIR, '1777000000002-NewHead.ts'); + + const violations = await rule.analyze(context([below, above, head])); + + expect(violations).toHaveLength(1); + expect(violations[0].file).toBe(path.join(tmpDir, below)); + expect(violations[0].message).toContain('1777000000000'); + }); + + it('does not use added migrations to extend the future timestamp ceiling', async () => { + writeMigration(tmpDir, COMMON_DIR, '1777000000000-Existing.ts'); + const a = writeMigration(tmpDir, COMMON_DIR, '1820000000000-A.ts'); + const b = writeMigration(tmpDir, COMMON_DIR, '1820000000001-B.ts'); + + const violations = await rule.analyze(context([a, b])); + + expect(violations).toHaveLength(2); + for (const violation of violations) { + expect(violation.message).toContain('future timestamp'); + } + }); + + it('checks all added migrations against the same ceiling buffer', async () => { + writeMigration(tmpDir, COMMON_DIR, `${NOW}-Existing.ts`); + const a = writeMigration(tmpDir, COMMON_DIR, `${NOW + 1}-A.ts`); + const b = writeMigration(tmpDir, COMMON_DIR, `${NOW + 1000}-B.ts`); + const above = writeMigration(tmpDir, COMMON_DIR, `${NOW + 1001}-Above.ts`); + + const violations = await rule.analyze(context([a, b, above])); + + expect(violations).toHaveLength(1); + expect(violations[0].file).toBe(path.join(tmpDir, above)); + expect(violations[0].message).toContain('future timestamp'); }); it('flags a newly-added migration that exactly ties the head', async () => { @@ -220,6 +263,20 @@ describe('MigrationTimestampRule', () => { expect(violations).toEqual([]); }); + it('keeps an existing dialect file as the floor for other added migrations', async () => { + writeMigration(tmpDir, COMMON_DIR, '1700000000000-Old.ts'); + writeMigration(tmpDir, COMMON_DIR, '1777000000000-Head.ts'); + const override = writeMigration(tmpDir, SQLITE_DIR, '1777000000000-Head.ts'); + const below = writeMigration(tmpDir, POSTGRES_DIR, '1750000000000-Below.ts'); + const above = writeMigration(tmpDir, COMMON_DIR, '1777000000001-Above.ts'); + + const violations = await rule.analyze(context([override, below, above])); + + expect(violations).toHaveLength(1); + expect(violations[0].file).toBe(path.join(tmpDir, below)); + expect(violations[0].message).toContain('1777000000000'); + }); + it('still flags two unrelated migrations colliding on the same timestamp', async () => { // Same timestamp + DIFFERENT suffix = genuine collision. Override // pairing must be by filename, not just by timestamp. @@ -231,12 +288,12 @@ describe('MigrationTimestampRule', () => { const files = violations.map((v) => path.basename(v.file)).sort(); expect(files).toEqual(['1777000000000-Bar.ts', '1777000000000-Foo.ts']); + for (const violation of violations) { + expect(violation.message).toContain('shares its timestamp'); + } }); it('still flags a sub-floor override file', async () => { - // An override pair below the head is still out of order — the - // floor is computed from non-self slots, so the pair shares one - // slot at the override timestamp but is still below the head. writeMigration(tmpDir, COMMON_DIR, '1777000000000-Head.ts'); const commonLate = writeMigration(tmpDir, COMMON_DIR, '1700000000000-Override.ts'); const sqliteLate = writeMigration(tmpDir, SQLITE_DIR, '1700000000000-Override.ts'); @@ -273,17 +330,19 @@ describe('MigrationTimestampRule', () => { }); it('scans every configured migration directory', async () => { + writeMigration(tmpDir, COMMON_DIR, '1777000000000-Existing.ts'); const common = writeMigration(tmpDir, COMMON_DIR, '1700000000000-Common.ts'); const postgres = writeMigration(tmpDir, POSTGRES_DIR, '1750000000000-Postgres.ts'); const sqlite = writeMigration(tmpDir, SQLITE_DIR, '1755000000000-Sqlite.ts'); const violations = await rule.analyze(context([common, postgres, sqlite])); - // Common and Postgres are below Sqlite's head — both trip - // ordering, proving every directory is scanned and addedFiles - // matches across directories. const files = violations.map((v) => path.basename(v.file)).sort(); - expect(files).toEqual(['1700000000000-Common.ts', '1750000000000-Postgres.ts']); + expect(files).toEqual([ + '1700000000000-Common.ts', + '1750000000000-Postgres.ts', + '1755000000000-Sqlite.ts', + ]); }); }); }); diff --git a/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.ts b/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.ts index 24f19dd13ce..89b437e5f5c 100644 --- a/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.ts +++ b/packages/quality/policy/code-health/src/rules/migration-timestamp.rule.ts @@ -79,31 +79,39 @@ export class MigrationTimestampRule extends BaseRule { ? new Set(addedFiles.map((p) => path.normalize(p))) : undefined; - // Dedupe to ordering slots: a `common/-.ts` paired with a - // `postgresdb|sqlite/-.ts` override is one logical migration - // (same TypeORM class name, only one runs per dialect) and occupies a - // single slot. Two files at the same timestamp with *different* - // suffixes remain distinct slots and still trip ordering. - const slotByKey = new Map(); + // Dialect overrides with the same filename share one ordering slot. + const slotByKey = new Map< + string, + { timestamp: number; slotKey: string; isExisting: boolean } + >(); + const slotsPerTimestamp = new Map(); for (const m of parsed) { - if (!slotByKey.has(m.slotKey)) { - slotByKey.set(m.slotKey, { timestamp: m.timestamp, slotKey: m.slotKey }); + const isExisting = !addedFilesSet?.has(path.normalize(m.relativePath)); + const slot = slotByKey.get(m.slotKey); + if (slot) { + // Keep the slot as existing if any dialect file predates the PR. + slot.isExisting ||= isExisting; + } else { + slotByKey.set(m.slotKey, { timestamp: m.timestamp, slotKey: m.slotKey, isExisting }); + slotsPerTimestamp.set(m.timestamp, (slotsPerTimestamp.get(m.timestamp) ?? 0) + 1); } } - const slotsDesc = Array.from(slotByKey.values()).sort((a, b) => { - if (b.timestamp !== a.timestamp) return b.timestamp - a.timestamp; - return a.slotKey.localeCompare(b.slotKey); - }); - const globalMaxSlot = slotsDesc[0]; - const secondMaxSlot = slotsDesc[1]; - const globalMax = globalMaxSlot?.timestamp ?? 0; + const existingSlotsDesc = Array.from(slotByKey.values()) + .filter((slot) => slot.isExisting) + .sort((a, b) => { + if (b.timestamp !== a.timestamp) return b.timestamp - a.timestamp; + return a.slotKey.localeCompare(b.slotKey); + }); + const existingMaxSlot = existingSlotsDesc[0]; + const secondMaxSlot = existingSlotsDesc[1]; + const existingMax = existingMaxSlot?.timestamp ?? 0; const secondMax = secondMaxSlot?.timestamp ?? 0; const violations: Violation[] = []; for (const migration of parsed) { const { filePath, relativePath, fileName, timestamp, slotKey } = migration; - const isHeadSlot = globalMaxSlot?.slotKey === slotKey; - const floor = isHeadSlot ? secondMax : globalMax; + const isHeadSlot = existingMaxSlot?.slotKey === slotKey; + const floor = isHeadSlot ? secondMax : existingMax; const ceiling = Math.max(now, floor + ceilingBuffer); const isAdded = addedFilesSet?.has(path.normalize(relativePath)) ?? false; @@ -121,6 +129,19 @@ export class MigrationTimestampRule extends BaseRule { continue; } + if (isAdded && (slotsPerTimestamp.get(timestamp) ?? 0) > 1) { + violations.push( + this.createViolation( + filePath, + 1, + 1, + `${fileName} shares its timestamp (${timestamp}) with a different migration. New migrations must use distinct timestamps.`, + `Rename the file using an unused timestamp between ${floor + 1} and ${ceiling}.`, + ), + ); + continue; + } + if (timestamp > ceiling) { violations.push( this.createViolation(