From b5ea3b7afe0fb85b873bd75a06827b0eb5ac5aad Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 16:39:25 +0000 Subject: [PATCH 1/2] fix(driver-sql): a plain unique index over duplicate rows is loud and non-fatal, and `os migrate plan` stops calling it `safe` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Declaring a column unique over a table that already holds duplicates had two outcomes depending on one branch, and only one of them was survivable. An organization-scoped unique (the NULL-safe COALESCE composite) kept the boot up, logged at `error` naming the index, the unenforced constraint and the remedy, and the ADR-0120 D4 pre-flight reported the blocked `create_index` as `destructive`/`error` with the conflicting key groups and row counts. A PLAIN unique — no organization key part, reached by `tenancy: { enabled: false }` or an explicit `unique: 'global'` — took the process down: `initObjects` threw the database's own error, naming the index and the column and no rows and no remedy, nothing reached the durability channel, and `detectManagedDrift` classified the same op `safe`/`warning`, so `os migrate apply` and dev `autoMigrate: 'safe'` walked into the raw failure. The plain path now reaches parity: - `syncDeclaredIndexes` absorbs a uniqueness violation on a plain unique the way it already absorbed one on the NULL-safe composite — durability channel, conflicting groups with row counts, the constraint named as NOT enforced, and `os migrate plan` as the way out. The `unique` limb is load-bearing: a non-unique index cannot raise a uniqueness violation, so a failure that reads as one there is something else and keeps failing loudly. - The D4 pre-flight no longer skips ops with an empty NULL-safe column set, so a plain unique `create_index` over dirty data grades `destructive`/`error` with the same row report. Nothing new probes it: the existing probe already groups by the bare columns when there is no NULL-safe key part, so the guard MOVED rather than a second copy of the check appearing beside the first. Path A is pinned unchanged as the control. The retired assertion in `sql-driver-unique-violation-predicate.test.ts` is re-authored, not deleted: its reasoning was "absorbing it would silently ship an unenforced constraint the drift pre-flight was never told about", and the pre-flight is now told — so the invariant it defended (never absorb SILENTLY) is asserted on the loud half. Closes #14902 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .../plain-unique-index-duplicate-preflight.md | 17 + ...2-plain-unique-duplicate-preflight.test.ts | 316 ++++++++++++++++++ ...-driver-unique-violation-predicate.test.ts | 89 ++++- packages/drivers/driver-sql/src/sql-driver.ts | 125 +++++-- 4 files changed, 517 insertions(+), 30 deletions(-) create mode 100644 .changeset/plain-unique-index-duplicate-preflight.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-14902-plain-unique-duplicate-preflight.test.ts 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/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; } } From 5552ef35078589a05eece1c8bf12f5b6f2d4b82e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 17:25:47 +0000 Subject: [PATCH 2/2] docs(deployment): the op-classification table stops promising `safe` for a UNIQUE index create the data blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs-drift bot listed eight hand-written pages against this branch. Seven are noise — they match on the bare class name `SqlDriver` and carry no drift classification at all (0 hits for `create_index` between them) — and `content/docs/releases/v17.mdx` is release-owned and read-only; its `category` hits are `tool.category`, a spec key, not this one. `content/docs/deployment/cli.mdx` is genuinely falsified, and in the one place an operator reads before running the command: - The `safe` row of the category table listed "create a declared index" flat, so it promised auto-apply for exactly the case this branch now blocks. Carved out to the UNIQUE sub-case, in the row's existing voice. - `create_index`'s "what it means" row said only that the index is missing. It now owes the pre-flight sentence, and the page already had the right words one row down: `recreate_index` documents the tightening's probe as "block the op with a report instead of failing a boot". `create_index` is matched to that sentence rather than given a new shape — the doc edit is the same parity edit as the code. - The `destructive` row gains the counterpart example, because that row is where an operator looks to find out why the op they expected to be safe is not. The table is not restructured; three cells changed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- content/docs/deployment/cli.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 |