diff --git a/.changeset/plain-unique-index-duplicate-preflight.md b/.changeset/plain-unique-index-duplicate-preflight.md new file mode 100644 index 0000000000..174193bede --- /dev/null +++ b/.changeset/plain-unique-index-duplicate-preflight.md @@ -0,0 +1,17 @@ +--- +"@objectstack/driver-sql": patch +--- + +A plain unique index over existing duplicate rows no longer kills the boot with the database's raw error, and `os migrate plan` no longer calls that op `safe`. + +Declaring a column unique over a table that already holds duplicates had two very different outcomes depending on one branch in the SQL driver, and only one of them was survivable. + +- **An organization-scoped unique** (the `unique: 'organization'` default, materialised as the NULL-safe `COALESCE(organization_id, '__global__')` composite) kept the boot up: the driver logged at `error` naming the index, the constraint that is not enforced and the remedy, and the ADR-0120 D4 duplicate pre-flight reported the blocked `create_index` as `category: 'destructive'` / `severity: 'error'` with the conflicting key groups and their row counts. +- **A plain unique** — no organization key part at all, reached by an object with `tenancy: { enabled: false }` or by any explicit `unique: 'global'` — took the process down: `initObjects` threw the database's own error, which names the index and the column and no rows and no remedy, nothing reached the durability channel, and `detectManagedDrift` (what `os migrate plan` reports) classified the very same op `category: 'safe'`, `severity: 'warning'`, so `os migrate apply` and dev `autoMigrate: 'safe'` walked straight into the raw failure. + +The plain path now reaches the same posture as the scoped one: + +- **The boot survives and says what is not enforced.** `syncDeclaredIndexes` absorbs a uniqueness violation on a plain unique index the way it already absorbed one on the NULL-safe composite: the failure is logged on the durability channel (`error`) naming the index, the conflicting key groups with their row counts, the constraint that is NOT enforced, and `os migrate plan` as the way out. A non-unique index and any failure that is not a uniqueness violation still surface as before. +- **The duplicate pre-flight covers it.** The ADR-0120 D4 probe no longer skips ops whose NULL-safe column set is empty, so a plain unique `create_index` over dirty data is reported `destructive` / `error` with the same row report instead of `safe`. Nothing new probes it: the existing probe already groups by the bare columns when there is no NULL-safe key part, so both key shapes share one pre-flight rather than a second copy that can drift from the first. + +Consumers of the classification see the op move from the "Safe" group to "Destructive (requires --allow-destructive)" in `os migrate plan` and `os diff`; `os migrate apply` defers it instead of attempting it; the artifact boot gate refuses with a named destructive-drift refusal instead of crashing; and dev `autoMigrate: 'safe'` leaves it alone. Clean data is unaffected — the probe finds nothing and the index is created exactly as before. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index cb77f1e46a..e264ef7f21 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -680,9 +680,9 @@ occupancy on its own. | Category | Examples | Applied by | |----------|----------|------------| -| `safe` | relax `NOT NULL` → nullable, widen a `varchar`, create a declared index, replace a legacy installation-wide unique with its per-organization composite | `os migrate apply` (and dev auto-reconcile) | +| `safe` | relax `NOT NULL` → nullable, widen a `varchar`, create a declared index (a `UNIQUE` one only when its duplicate pre-flight comes back clean), replace a legacy installation-wide unique with its per-organization composite | `os migrate apply` (and dev auto-reconcile) | | `needs_confirm` | non-narrowing type change, rebuild a non-unique index whose columns changed | `os migrate apply` — except `manual_widen_varchar_to_text`, which nothing applies | -| `destructive` | drop an orphaned column or index, tighten `NOT NULL`, narrow a type, rebuild an index as `UNIQUE` | `os migrate apply --allow-destructive` | +| `destructive` | drop an orphaned column or index, tighten `NOT NULL`, narrow a type, rebuild an index as `UNIQUE`, create a `UNIQUE` index existing rows already violate | `os migrate apply --allow-destructive` | #### Index drift @@ -690,7 +690,7 @@ occupancy on its own. | Op | What it means | |----|---------------| -| `create_index` | Metadata declares an index the database does not have | +| `create_index` | Metadata declares an index the database does not have. A `UNIQUE` one runs the same duplicate pre-flight probe `recreate_index` does: rows that already violate it **block** the op with a report naming the conflicting key groups and their row counts, instead of failing a boot on the database's own error, and the declared constraint stays unenforced until they are resolved | | `replace_unique_index` | A field's `unique` used to be enforced installation-wide, but metadata now scopes it per organization — the legacy single-column index is swapped for the NULL-safe `(COALESCE(organization_id, '__global__'), field)` composite. A pure relaxation: it creates before it drops, and cannot fail | | `recreate_index` | An index exists under the declared name but with different columns/uniqueness. The additive sync skips it by name, so it must be dropped and rebuilt. This is also how a per-organization unique becomes NULL-safe: a **tightening**, so it runs a duplicate pre-flight probe first — rows the old NULL-distinct index wrongly admitted **block** the op with a report instead of failing a boot, and the old index stays in place until they are resolved | | `drop_index` | An index carrying ObjectStack's generated naming (`uniq_…` / `idx_…`) that metadata no longer declares | diff --git a/packages/drivers/driver-sql/src/sql-driver-14902-plain-unique-duplicate-preflight.test.ts b/packages/drivers/driver-sql/src/sql-driver-14902-plain-unique-duplicate-preflight.test.ts new file mode 100644 index 0000000000..f560851d90 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-14902-plain-unique-duplicate-preflight.test.ts @@ -0,0 +1,316 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +/** + * #14902 — a PLAIN unique index over existing duplicate rows. + * + * ## The defect, in the two shapes it was measured side by side + * + * Same fixture both times: a `crm_quote` table already holding two rows with + * the same `quote_number`, then `initObjects` declaring that column unique. + * + * - **Path A — the NULL-safe organization composite.** The boot CONTINUES: the + * driver logs at `error` naming the index, the unenforced constraint and the + * remedy, and the ADR-0120 D4 pre-flight reports the blocked `create_index` + * as `category: 'destructive'` / `severity: 'error'` with the conflicting key + * groups and their row counts. + * - **Path B — a plain unique, no organization key part.** The boot DIED: + * `initObjects` threw the database's own error, which names the index and the + * column and NO rows and NO remedy, nothing reached the durability channel, + * and `detectManagedDrift` — what `os migrate plan` reports — classified the + * very same op `category: 'safe'`, `severity: 'warning'`. + * + * Three properties stacked, and it is the combination that made it p1: the boot + * is DOWN rather than degraded; the message is unactionable; and the instrument + * an operator would reach for said `safe` about the op that was about to kill + * the boot. + * + * ## Reachability — precisely, because the wider framing is wrong + * + * ⛔ NOT "every autonumber field". Since #13894 an `autonumber` field that omits + * `unique` defaults to `unique: 'organization'`, and on an object that carries a + * tenant column that lands on path A. Path B is reached by an object with + * `tenancy: { enabled: false }` (no tenant column at all) or by any explicit + * `unique: 'global'`. Narrower — and live: it is the self-hosted upgrade path, + * a deployment with legacy duplicate rows and a tenancy-disabled object. + * + * ## What this suite pins + * + * Parity, in both halves, plus path A as the CONTROL. A change that quietly + * moved path A while making path B loud would otherwise read as success, so the + * last block asserts path A's message and classification are still their own — + * the `#5030` framing, which is precisely what the plain path must NOT claim + * (nothing ever admitted these rows; the constraint is simply newly declared + * over data that does not satisfy it). + * + * ⛔ Nothing here pins the DATABASE's raw error text: the `catch` shape is + * dialect-independent, that text is not. Assertions are on our own messages and + * on the drift classification. + */ +describe('#14902 plain unique index over duplicate rows', () => { + let driver: SqlDriver; + let logs: Array<{ level: 'warn' | 'info' | 'error'; msg: string }>; + + const makeDriver = (opts: any = {}) => + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + ...opts, + }); + + const attachSpy = () => { + logs = []; + (driver as any).logger = { + warn: (msg: string) => logs.push({ level: 'warn', msg }), + info: (msg: string) => logs.push({ level: 'info', msg }), + error: (msg: string) => logs.push({ level: 'error', msg }), + }; + }; + + beforeEach(() => { + driver = makeDriver(); + attachSpy(); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + /** + * A pre-existing table, no index on `quote_number` yet — the legacy database + * this card is about. `withOrg` selects which path the declaration lands on. + */ + const seed = async ( + withOrg: boolean, + rows: Array>, + ): Promise => { + const k = (driver as any).knex; + await k.schema.createTable('crm_quote', (t: any) => { + t.string('id').primary(); + t.timestamp('created_at'); + t.timestamp('updated_at'); + if (withOrg) t.string('organization_id'); + t.string('quote_number'); + }); + await k('crm_quote').insert(rows); + return k; + }; + + /** The non-PK index names physically present on the table. */ + const indexNames = async (table: string): Promise => { + const k = (driver as any).knex; + const list: any = await k.raw(`PRAGMA index_list(${table})`); + return list.filter((i: any) => i.origin !== 'pk').map((i: any) => i.name); + }; + + /** Tenancy OFF — the plain single-column unique. */ + const PLAIN_META = [ + { + name: 'crm_quote', + tenancy: { enabled: false }, + fields: { quote_number: { type: 'autonumber', unique: true } }, + }, + ] as any[]; + + /** Tenanted object, explicit platform-wide scope — also the plain shape. */ + const GLOBAL_META = [ + { + name: 'crm_quote', + fields: { + organization_id: { type: 'string' }, + quote_number: { type: 'autonumber', unique: 'global' }, + }, + }, + ] as any[]; + + const DUP_PLAIN = [ + { id: 'r1', quote_number: 'QUO-00009' }, + { id: 'r2', quote_number: 'QUO-00009' }, + ]; + + describe('the boot survives and says what is not enforced', () => { + it('does not throw the raw driver error — it logs on the durability channel, naming the rows and the remedy', async () => { + await seed(false, DUP_PLAIN); + + // Before this fix: `initObjects` rejected with + // "create unique index `uniq_crm_quote_quote_number` … UNIQUE constraint + // failed: crm_quote.quote_number" and the whole boot went down. + await expect(driver.initObjects(PLAIN_META)).resolves.not.toThrow(); + + const durability = logs.filter((l) => l.level === 'error'); + expect(durability).toHaveLength(1); + const msg = durability[0].msg; + // Names the index… + expect(msg).toContain("'uniq_crm_quote_quote_number'"); + // …the ROWS, which the database's own error never did… + expect(msg).toMatch(/Conflicting group\(s\)/); + expect(msg).toContain('quote_number="QUO-00009"'); + expect(msg).toMatch(/× 2 rows/); + // …that the constraint is NOT enforced… + expect(msg).toMatch(/NOT enforced/); + // …and the remedy. + expect(msg).toContain('os migrate plan'); + + // The index really is absent: the log is not covering for a silent + // success, and a duplicate write is still accepted (that is what "not + // enforced" MEANS, and it is why the message is on the durability + // channel rather than at `warn`). + expect(await indexNames('crm_quote')).not.toContain('uniq_crm_quote_quote_number'); + }); + + it("reaches the same disposition through an explicit unique: 'global' on a tenanted object", async () => { + await seed(true, [ + { id: 'r1', organization_id: 'org_x', quote_number: 'QUO-00009' }, + { id: 'r2', organization_id: 'org_y', quote_number: 'QUO-00009' }, + ]); + + await expect(driver.initObjects(GLOBAL_META)).resolves.not.toThrow(); + + const durability = logs.filter((l) => l.level === 'error'); + expect(durability).toHaveLength(1); + // The key is the bare column — the organization is NOT part of it, which + // is the whole point of `'global'`, and the two rows collide across + // organizations. + expect(durability[0].msg).toContain('quote_number="QUO-00009"'); + expect(durability[0].msg).not.toContain('organization_id'); + }); + + it('still creates the index, silently, when the data is clean', async () => { + await seed(false, [ + { id: 'r1', quote_number: 'QUO-00009' }, + { id: 'r2', quote_number: 'QUO-00010' }, + ]); + + await driver.initObjects(PLAIN_META); + + expect(await indexNames('crm_quote')).toContain('uniq_crm_quote_quote_number'); + expect(logs.filter((l) => l.level === 'error')).toHaveLength(0); + // Healthy database: converged, zero drift. The pre-flight probes and gets + // out of the way — it does not gate clean data. + expect(await driver.detectManagedDrift()).toHaveLength(0); + await expect(driver.create('crm_quote', { quote_number: 'QUO-00009' })).rejects.toThrow( + /UNIQUE constraint failed|duplicate key value/, + ); + }); + }); + + describe("os migrate plan stops calling the blocked op `safe`", () => { + it('classifies it destructive/error with the conflicting group and withdraws the safe claim', async () => { + await seed(false, DUP_PLAIN); + await driver.initObjects(PLAIN_META); + + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'create_index'); + expect(entry).toBeDefined(); + + // Before this fix: `category: 'safe'`, `severity: 'warning'`, message + // "…the database has no such index — run "os migrate apply" to create + // it." — an instrument saying nothing is wrong about the op that had just + // taken the boot down. + expect(entry!.category).toBe('destructive'); + expect(entry!.severity).toBe('error'); + expect(entry!.message).toMatch(/BLOCKED/); + expect(entry!.message).toContain('quote_number="QUO-00009"'); + expect(entry!.message).toMatch(/× 2 rows/); + expect(entry!.message).toMatch(/NOT enforced/); + expect(entry!.message).toContain('os migrate plan'); + // ⛔ And it must NOT borrow path A's story: no prior index admitted these + // rows, so #5030 is not what happened here. + expect(entry!.message).not.toContain('#5030'); + expect(entry!.message).not.toContain('NULL-safe'); + }); + + it('is not applied by a plain apply, and not created even under --allow-destructive', async () => { + await seed(false, DUP_PLAIN); + await driver.initObjects(PLAIN_META); + const entry = (await driver.detectManagedDrift()).find((d) => d.op.type === 'create_index')!; + + // `destructive` is what keeps `os migrate apply` (and the artifact boot + // gate, and dev autoMigrate) from walking into the raw failure. + const plain = await driver.applyMigrationEntries([entry], { allowDestructive: false }); + expect(plain.applied).toHaveLength(0); + expect(plain.skipped).toHaveLength(1); + + // …and forcing it does not produce a half-applied schema either: the + // create is refused by the data, reported skipped, and no index appears. + const forced = await driver.applyMigrationEntries([entry], { allowDestructive: true }); + expect(forced.applied).toHaveLength(0); + expect(forced.skipped).toHaveLength(1); + expect(await indexNames('crm_quote')).not.toContain('uniq_crm_quote_quote_number'); + }); + + it("is not auto-applied at boot under dev autoMigrate: 'safe'", async () => { + await driver.disconnect(); + driver = makeDriver({ autoMigrate: 'safe' }); + attachSpy(); + await seed(false, DUP_PLAIN); + + await expect(driver.initObjects(PLAIN_META)).resolves.not.toThrow(); + expect(await indexNames('crm_quote')).not.toContain('uniq_crm_quote_quote_number'); + expect(logs.some((l) => l.msg.includes('auto-reconciled'))).toBe(false); + }); + + it('unblocks once the duplicates are gone — the entry re-grades and a plain apply creates it', async () => { + const k = await seed(false, DUP_PLAIN); + await driver.initObjects(PLAIN_META); + expect( + (await driver.detectManagedDrift()).find((d) => d.op.type === 'create_index')!.category, + ).toBe('destructive'); + + await k('crm_quote').where({ id: 'r2' }).delete(); + + const entry = (await driver.detectManagedDrift()).find((d) => d.op.type === 'create_index')!; + expect(entry.category).toBe('safe'); + expect(entry.severity).toBe('warning'); + const res = await driver.applyMigrationEntries([entry], { allowDestructive: false }); + expect(res.applied).toHaveLength(1); + expect(await indexNames('crm_quote')).toContain('uniq_crm_quote_quote_number'); + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + }); + + describe('CONTROL — path A is untouched', () => { + it('still logs its own NULL-safe #5030 message and still reports destructive with the key groups', async () => { + await seed(true, [ + { id: 'r1', organization_id: null, quote_number: 'QUO-00009' }, + { id: 'r2', organization_id: null, quote_number: 'QUO-00009' }, + { id: 'r3', organization_id: 'org_x', quote_number: 'QUO-00010' }, + { id: 'r4', organization_id: 'org_x', quote_number: 'QUO-00010' }, + ]); + + // `unique: true` on a tenanted object == `unique: 'organization'` — the + // #13894 default, and the shape this card must not have disturbed. + await expect( + driver.initObjects([ + { + name: 'crm_quote', + fields: { + organization_id: { type: 'string' }, + quote_number: { type: 'autonumber', unique: true }, + }, + }, + ] as any[]), + ).resolves.not.toThrow(); + + const durability = logs.filter((l) => l.level === 'error'); + expect(durability).toHaveLength(1); + expect(durability[0].msg).toContain('NULL-safe unique index'); + expect(durability[0].msg).toContain('#5030'); + expect(durability[0].msg).toContain('ADR-0120 D4'); + expect(durability[0].msg).toContain("'organization_id, quote_number'"); + + const entry = (await driver.detectManagedDrift()).find((d) => d.op.type === 'create_index')!; + expect(entry.category).toBe('destructive'); + expect(entry.severity).toBe('error'); + expect(entry.message).toContain('#5030'); + expect(entry.message).toContain('__global__'); + // Both key groups, with counts — the NULL-organization bucket and a real + // organization, which is what makes the COALESCE key self-describing. + expect(entry.message).toContain('(organization_id="__global__", quote_number="QUO-00009")'); + expect(entry.message).toContain('(organization_id="org_x", quote_number="QUO-00010")'); + }); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts b/packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts index 38d18c1b41..5a0549eef9 100644 --- a/packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts @@ -1,10 +1,16 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * `syncDeclaredIndexes` judges "did existing rows violate the NULL-safe unique - * I just tried to create?" — the #5030 branch that keeps a dirty database - * BOOTING (the constraint is logged as not-enforced and reported by the - * ADR-0120 D4 drift pre-flight) instead of taking the process down. + * `syncDeclaredIndexes` judges "did existing rows violate the unique index I + * just tried to create?" — the #5030 branch that keeps a dirty database BOOTING + * (the constraint is logged as not-enforced and reported by the ADR-0120 D4 + * drift pre-flight) instead of taking the process down. + * + * ⚠️ Since #14902 that branch has two arms, not one: the NULL-safe organization + * composite AND the plain unique (`tenancy: { enabled: false }`, or an explicit + * `unique: 'global'`), which used to fall through to `throw e`. The + * discriminator below is what BOTH arms judge with, so its blind spots are now + * twice as expensive. * * It used to judge that with a private inline regex over the stringified * message — `unique constraint failed|duplicate entry|duplicate key value` — @@ -80,13 +86,19 @@ const NULL_SAFE_INDEX: DeclaredIndexInput = { nullSafeColumns: ['organization_id'], }; -/** The same index with no NULL-safe key part — the `nullSafe.size > 0` guard's false arm. */ +/** The same index with no NULL-safe key part — the plain single-column unique. */ const PLAIN_INDEX: DeclaredIndexInput = { name: 'uniq_product_code', fields: ['code'], unique: true, }; +/** Not unique at all — an ACCESS PATH. Its failures are nobody's #5030. */ +const NON_UNIQUE_INDEX: DeclaredIndexInput = { + name: 'idx_product_code', + fields: ['code'], +}; + const PHYSICAL_COLUMNS = new Set(['id', 'organization_id', 'code']); /** @@ -222,11 +234,64 @@ describe('syncDeclaredIndexes unique-violation discriminator (#6543)', () => { // ── The site's own business logic, untouched by the migration ───────────── - it('leaves the `nullSafe.size > 0` guard intact — a plain unique still fails the sync', async () => { + /** + * ⚠️ RETIRED PIN, re-authored — #14902. + * + * This block used to assert the opposite: 「leaves the `nullSafe.size > 0` + * guard intact — a plain unique still fails the sync」, on the reasoning that + * absorbing it 「would silently ship an unenforced constraint **the drift + * pre-flight was never told about**」. That reasoning was right, and its + * premise is exactly what #14902 removed: the ADR-0120 D4 pre-flight now + * probes the plain unique too, so the drift pass IS told, and `os migrate + * plan` reports the blocked op `destructive` with the offending rows instead + * of calling it `safe`. + * + * So the invariant the old pin was defending survives verbatim — a plain + * unique violation must never be absorbed SILENTLY — and only its + * disposition moved: from `throw` (the boot dies carrying the database's own + * error, naming no rows and no remedy) to the same loud, non-fatal posture + * the NULL-safe arm has had since #5030. What follows asserts the LOUD half, + * which is the half that was actually load-bearing. + */ + it('absorbs the plain arm too — but only LOUDLY, naming the rows and the remedy (#14902)', async () => { arm(postgresIndexBuildConflict()); + await realKnex('product').insert([ + { id: 'r1', organization_id: 'org_a', code: 'DUP' }, + { id: 'r2', organization_id: 'org_b', code: 'DUP' }, + ]); // The plain arm goes through knex's schema builder rather than the // overridden method, and `knex.schema` is a fresh builder on every access - // — so the failure is injected by standing in for `knex` itself. + // — so the failure is injected by standing in for `knex` itself. Only + // `schema` is stood in for: `ref`/`raw` and the query builder stay REAL, so + // the duplicate probe in the branch under test runs against the real table + // and the row report below is a measurement, not a fixture. + (driver as any).getExistingIndexNames = async () => new Set(); + (driver as any).knex = Object.assign((...args: any[]) => (realKnex as any)(...args), { + ref: (id: string) => realKnex.ref(id), + raw: (...args: any[]) => realKnex.raw(...args), + schema: { + alterTable: () => Promise.reject(postgresIndexBuildConflict()), + }, + }); + + await expect(sync([PLAIN_INDEX])).resolves.toBeUndefined(); + + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/cannot create unique index/); + expect(errors[0]).toMatch(/uniq_product_code/); + expect(errors[0]).toMatch(/NOT enforced/); + expect(errors[0]).toMatch(/os migrate plan/); + // The rows the database's own error never named. + expect(errors[0]).toContain('code="DUP"'); + expect(errors[0]).toMatch(/× 2 rows/); + // ⛔ And it does not borrow the NULL-safe arm's story: no earlier index + // admitted these rows, so #5030 is not what happened here. + expect(errors[0]).not.toMatch(/#5030/); + expect(errors[0]).not.toMatch(/NULL-safe/); + }); + + it('never absorbs a NON-unique index failure, however much the error reads like a conflict', async () => { + arm(postgresIndexBuildConflict()); (driver as any).getExistingIndexNames = async () => new Set(); (driver as any).knex = { schema: { @@ -234,10 +299,12 @@ describe('syncDeclaredIndexes unique-violation discriminator (#6543)', () => { }, }; - // A unique violation on a NON-NULL-safe index is not the #5030 case and - // must still surface: absorbing it would silently ship an unenforced - // constraint the drift pre-flight was never told about. - const rejected: any = await sync([PLAIN_INDEX]).then( + // A non-unique index exists for an ACCESS PATH — it cannot raise a + // uniqueness violation, so a failure that reads as one while creating it is + // something else entirely. #14902's `unique` limb is what keeps that + // failing loudly instead of being logged away as an unenforced constraint + // that was never declared in the first place. + const rejected: any = await sync([NON_UNIQUE_INDEX]).then( () => undefined, (e: unknown) => e, ); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 838137ba41..c42ef6d619 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -4162,6 +4162,26 @@ interface RowWidthContribution { bytes: number; } +/** + * Render the duplicate groups a unique pre-flight probe found, for an operator + * message: at most five groups, then a count of the rest. + * + * Module-local on purpose (#14902). The two sites that report a blocked unique + * — the drift entry and the boot-time durability log — must name the SAME rows + * in the SAME shape, and a second hand-rolled `.slice(0, 5).join('; ')` is + * exactly how the plain and the NULL-safe path drifted apart in the first + * place. Not a method, and not exported: `SqlDriver`'s `.d.ts` carries its + * protected members, so a new method there would move a published entry point + * for a string helper. + */ +function formatDuplicateGroups(duplicates: ReadonlyArray<{ key: string; rows: number }>): string { + const shown = duplicates + .slice(0, 5) + .map((g) => `(${g.key}) \u00d7 ${g.rows} rows`) + .join('; '); + return duplicates.length > 5 ? `${shown}; \u2026and ${duplicates.length - 5} more group(s)` : shown; +} + export class SqlDriver implements IDataDriver { // IDataDriver metadata public readonly name: string = 'com.objectstack.driver.sql'; @@ -10686,10 +10706,12 @@ export class SqlDriver implements IDataDriver { } /** - * ADR-0120 D4 — duplicate pre-flight for NULL-safe organization uniques. + * ADR-0120 D4 — duplicate pre-flight for unique index CREATEs. * - * Probes every index op that would CREATE a unique index whose organization - * key part is the NULL-safe COALESCE form, by grouping over that exact key: + * Probes every index op that would create a UNIQUE index over data that + * already violates it, grouping by the exact key the index will enforce — + * `COALESCE(, '__global__')` for a NULL-safe organization key part, the + * bare column otherwise: * * - `recreate_index` marked `tightenNullSafeOnly` (the bare composite * tightening into its COALESCE form — same identities, physical fully @@ -10697,10 +10719,26 @@ export class SqlDriver implements IDataDriver { * `autoMigrate: 'safe'` and a plain `os migrate apply` may apply it; a * dirty probe keeps it blocked (`destructive` + a re-probe refusal in * {@link applyIndexDriftOp}) and reports the offending rows. - * - `create_index` for a unique NULL-safe index: a dirty probe demotes the - * default `safe` to blocked with the same row report — the CREATE could - * only fail at apply time otherwise, with a raw driver error naming no - * rows. + * - `create_index` for a unique index: a dirty probe demotes the default + * `safe` to blocked with the same row report — the CREATE could only + * fail at apply time otherwise, with a raw driver error naming no rows. + * + * ⚠️ #14902 — the second bullet covers the PLAIN unique too: an index with no + * organization key part at all, reached by an object with + * `tenancy: { enabled: false }` or by any explicit `unique: 'global'`. The + * `nullSafeColumns.length > 0` guard used to exclude it, so `os migrate plan` + * classified the one op that was about to take the boot down `safe` — an + * instrument reporting nothing wrong about the thing that kills the boot. + * Nothing new was needed to probe it: + * {@link probeNullSafeUniqueDuplicates} groups by the bare columns when the + * NULL-safe set is empty, so the guard MOVED rather than a second copy of the + * check appearing beside the first — one pre-flight, two key shapes, no way + * for them to drift apart again. + * + * A PLAIN unique `recreate_index` stays unprobed: it has no + * `tightenNullSafeOnly` shape, and `diffManagedIndexes` already categorises a + * unique recreate `destructive`, so it never carried the `safe` claim this + * pre-flight exists to withdraw. * * `replace_unique_index` is deliberately NOT probed: the legacy index it * retires is a platform-wide unique, strictly stronger than the NULL-safe @@ -10710,15 +10748,20 @@ export class SqlDriver implements IDataDriver { for (const d of entries) { const op = d.op; if (op.type !== 'recreate_index' && op.type !== 'create_index') continue; - if (!op.unique || !op.nullSafeColumns || op.nullSafeColumns.length === 0) continue; + if (!op.unique) continue; + const nullSafeColumns = op.nullSafeColumns ?? []; + const nullSafeKey = nullSafeColumns.length > 0; const tighten = op.type === 'recreate_index' && op.tightenNullSafeOnly === true; // A generic unique recreate (columns differ beyond the key-part form) // keeps its pre-ADR-0120 semantics untouched. if (op.type === 'recreate_index' && !tighten) continue; + // …and a PLAIN unique has no tightening shape at all, so only its CREATE + // reaches the probe (#14902). + if (!nullSafeKey && op.type !== 'create_index') continue; let duplicates: Array<{ key: string; rows: number }>; try { - duplicates = await this.probeNullSafeUniqueDuplicates(op.table, op.columns, op.nullSafeColumns); + duplicates = await this.probeNullSafeUniqueDuplicates(op.table, op.columns, nullSafeColumns); } catch (e: any) { // Probe failure must fail SAFE: without evidence the data is clean the // op may not claim eligibility for auto-apply. @@ -10743,18 +10786,23 @@ export class SqlDriver implements IDataDriver { continue; } - const report = duplicates - .slice(0, 5) - .map((g) => `(${g.key}) × ${g.rows} rows`) - .join('; '); - const more = duplicates.length > 5 ? `; …and ${duplicates.length - 5} more group(s)` : ''; + const report = formatDuplicateGroups(duplicates); d.category = 'destructive'; d.severity = 'error'; - d.message = - `${op.table}: cannot ${tighten ? 'tighten' : 'create'} '${op.indexName}' as ${signature} — existing rows ` + - `already violate the NULL-safe unique constraint (duplicates the old index wrongly admitted, #5030): ` + - `${report}${more}. The op is BLOCKED: apply re-probes and refuses, and the existing index stays in place ` + - `(ADR-0120 D4). Deduplicate the listed rows, then re-run "os migrate plan".`; + d.message = nullSafeKey + ? `${op.table}: cannot ${tighten ? 'tighten' : 'create'} '${op.indexName}' as ${signature} — existing rows ` + + `already violate the NULL-safe unique constraint (duplicates the old index wrongly admitted, #5030): ` + + `${report}. The op is BLOCKED: apply re-probes and refuses, and the existing index stays in place ` + + `(ADR-0120 D4). Deduplicate the listed rows, then re-run "os migrate plan".` + : // #14902: the plain unique has no #5030 history behind it — nothing + // ever admitted these rows, the constraint is simply newly declared + // over data that does not satisfy it. So the message says what IS + // true, and above all withdraws the `safe` claim: this op is not + // applied by `os migrate apply` and not auto-applied at boot. + `${op.table}: cannot create '${op.indexName}' as ${signature} — existing rows already violate it: ` + + `${report}. The op is BLOCKED: neither "os migrate apply" nor dev autoMigrate: 'safe' will create it, ` + + `so the constraint is NOT enforced. Deduplicate the listed rows, then re-run "os migrate plan" ` + + `(ADR-0120 D4).`; } } @@ -11708,6 +11756,11 @@ export class SqlDriver implements IDataDriver { * at `error` (a declared constraint is not enforced — the * durability-degradation rule) and surfaces as drift with a row report via * the ADR-0120 D4 pre-flight, instead of failing the whole boot. + * - #14902: a PLAIN unique — no organization key part, i.e. + * `tenancy: { enabled: false }` or an explicit `unique: 'global'` — over + * data that already violates it gets the SAME disposition, where it used to + * throw the database's raw error and take the boot down naming no rows and + * no remedy. */ protected async syncDeclaredIndexes( tableName: string, @@ -11877,6 +11930,40 @@ export class SqlDriver implements IDataDriver { ); continue; } + if (unique && isUniqueViolationError(e)) { + // #14902 — the PLAIN unique: no organization key part at all, reached + // by `tenancy: { enabled: false }` or by an explicit + // `unique: 'global'`. It used to fall through to `throw e`, so the + // boot DIED carrying the database's own error, which names the index + // and the column and NO rows and NO remedy — while the arm above, + // one branch away, kept the boot up and told the operator exactly + // what to do. Same defect, same disposition: the declared constraint + // is not enforced, say so on the durability channel, name the + // conflicting groups, and let the boot continue. The D4 pre-flight + // reports the same rows in `os migrate plan`. + // + // ⚠️ The `unique` limb is load-bearing, not decoration. A NON-unique + // index cannot raise a uniqueness violation, so a failure that reads + // as one while creating a non-unique index is something else + // entirely and must keep failing loudly rather than being absorbed + // into a log line here. + let report = ''; + try { + const duplicates = await this.probeNullSafeUniqueDuplicates(tableName, columns, []); + if (duplicates.length > 0) { + report = ` Conflicting group(s): ${formatDuplicateGroups(duplicates)}.`; + } + } catch { + // The probe is a diagnostic; the report below stands without it. + } + this.logDurabilityFailure( + `[sql-driver] cannot create unique index '${name}' on "${tableName}" — existing rows violate ` + + `it.${report} The constraint '${columns.join(', ')}' is NOT enforced until the data is ` + + `deduplicated: run "os migrate plan" for the conflicting rows.`, + msg, + ); + continue; + } throw e; } }