From 895d55979f255bb0c5da6469553f9095e86cb0a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 11:47:57 +0000 Subject: [PATCH] fix(driver-sql): the hash-shadow arm survives a plain unique over duplicate rows `syncDeclaredIndexes`'s `catch` handles a refused unique on two arms: the direct one, and the hash-shadow one MySQL takes when a key part exceeds the 768-char utf8mb4 ceiling. #14902 brought the direct arm to the ADR-0120 D4 disposition -- a uniqueness violation over existing rows is a durability degradation, not a fatal. The shadow arm still required a NULL-safe organization key part as well, so a PLAIN unique matched neither branch, fell through to the unkeyable-column refusal and took the boot down. Measured on live MySQL 8.0.46: the boot died with ER_BLOB_KEY_WITHOUT_LENGTH, advising a `maxLength` the field already declared, naming neither the two duplicate rows nor a remedy. Not a bare guard widening. The surviving branch's message says the rows violate the NULL-safe key and duplicate what the previous void constraint admitted (#5030); neither clause is true of a plain unique. The two arms are split so the NULL-safe one keeps its wording and the plain one carries the direct arm's reviewed sentence. Verified on live MySQL 8.0.46 through a new opt-in cell, with the two existing shadow cells as the firing control (green before and after). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- ...iver-sql-shadow-plain-unique-duplicates.md | 13 + ...479-shadow-plain-unique-duplicates.test.ts | 246 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 56 ++++ 3 files changed, 315 insertions(+) create mode 100644 .changeset/driver-sql-shadow-plain-unique-duplicates.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-15479-shadow-plain-unique-duplicates.test.ts diff --git a/.changeset/driver-sql-shadow-plain-unique-duplicates.md b/.changeset/driver-sql-shadow-plain-unique-duplicates.md new file mode 100644 index 0000000000..cc580381f4 --- /dev/null +++ b/.changeset/driver-sql-shadow-plain-unique-duplicates.md @@ -0,0 +1,13 @@ +--- +"@objectstack/driver-sql": patch +--- + +MySQL: a plain unique index over existing duplicate rows no longer takes the boot down when the index has to be carried by a hash shadow. + +`syncDeclaredIndexes` handles a declared unique that the database refuses in one `catch`, and that `catch` has two arms: the DIRECT one, and the hash-shadow one MySQL takes when a key part is wider than the 768-char utf8mb4 ceiling. #14902 taught the direct arm that a uniqueness violation over existing rows is a durability degradation rather than a fatal — log it, name the conflicting rows and the remedy, let the boot continue. The shadow arm kept the older guard, which also required a NULL-safe organization key part, so a PLAIN unique (`tenancy: { enabled: false }`, or an explicit `unique: 'global'`) matched neither branch. + +Measured on live MySQL 8.0.46: the boot died carrying `ER_BLOB_KEY_WITHOUT_LENGTH` — a refusal about an unkeyable TEXT column, telling the operator to declare a `maxLength` the field already declared — while the real cause was two duplicate rows it never mentioned. It named no rows and no remedy. + +The two arms now agree, and they say different things because they mean different things. The NULL-safe arm keeps its wording (existing rows violate the NULL-safe key, duplicating what the previous void constraint admitted); the plain arm gets the direct arm's reviewed sentence, because neither of those clauses is true of a plain unique — nothing admitted the rows, and there is no NULL-safe key. Widening the guard alone would have shipped a factually false durability log, which is worse than the throw it replaces. + +`os migrate plan` already reported this operation as `destructive` with the row report and is unchanged. diff --git a/packages/drivers/driver-sql/src/sql-driver-15479-shadow-plain-unique-duplicates.test.ts b/packages/drivers/driver-sql/src/sql-driver-15479-shadow-plain-unique-duplicates.test.ts new file mode 100644 index 0000000000..e79de98437 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-15479-shadow-plain-unique-duplicates.test.ts @@ -0,0 +1,246 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15479 — the HASH-SHADOW arm of `syncDeclaredIndexes`'s `catch`, on a PLAIN + * unique over rows that already violate it. + * + * ## The defect + * + * #14902 (PR #15477) brought the DIRECT arm to parity: a plain unique over + * existing duplicates logs on the durability channel and lets the boot + * continue, instead of throwing the database's raw error. The hash-shadow arm + * — one branch above it in the SAME `catch`, taken when MySQL refuses to key + * the column directly — still asked `nullSafe.size > 0 && isUniqueViolation…`, + * so with no organization key part the branch did not fire, the code fell + * through to `logDurabilityFailure(unkeyable, msg)` + `throw`, and the boot + * died carrying a message about an UNKEYABLE TEXT COLUMN while the actual + * cause was duplicate rows — naming neither the conflicting rows nor a remedy. + * + * ## Why this file is a live cell and could not be a SQLite one + * + * The shadow route exists only because MySQL refuses a key part over the + * 768-char utf8mb4 ceiling; SQLite and Postgres never refuse, so they never + * reach this arm. `maxLength: 1024` is what selects it. The card that filed + * this said the branch was unreachable in the dispatch container — measured + * false: MySQL 8.0 installs from the distro archive and this suite drives it. + * + * ## The two-arm message split, which is the half that is NOT a guard widening + * + * The surviving branch's message said "existing rows violate the NULL-safe key + * (duplicates the previous void constraint admitted, #5030)". Neither clause is + * true of a plain unique: nothing admitted those rows, and there is no NULL-safe + * key. Widening the guard and leaving one message would ship a FACTUALLY FALSE + * durability log — worse than the throw it replaces, because it sends the + * operator hunting for a NULL-distinct index that was never there. So the + * NULL-safe branch keeps its wording (asserted below as the CONTROL) and the + * plain branch gets the direct arm's reviewed sentence. + * + * Opt-in, like every live cell in this package: + * + * OS_TEST_MYSQL_URL=mysql://root:root@127.0.0.1:3306/conformance \ + * pnpm --filter @objectstack/driver-sql test + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { isHashShadowColumn } from './schema-drift.js'; +import { MYSQL_CELL, declareDialectCell } from './live-dialect-matrix.testkit.js'; + +/** 900 chars — comfortably over the 768-char keyable ceiling, so `v` is TEXT. */ +const LONG = 'p'.repeat(900); + +/** + * A tenancy-DISABLED object whose one long text field carries a PLAIN unique. + * `maxLength: 1024` exceeds the keyable ceiling, so MySQL refuses the direct + * index and the sync takes the shadow route; `tenancy: { enabled: false }` is + * one of the two shapes #14902 identified as reaching the plain path (the other + * is an explicit `unique: 'global'`, exercised by `globalUniqueOn` below). + */ +const plainUniqueOn = (name: string) => ({ + name, + tenancy: { enabled: false }, + fields: { v: { type: 'text', maxLength: 1024 } }, + indexes: [{ fields: ['v'], unique: true as const, name: `uniq_${name}_v` }], +}); + +/** The second shape of the same path: tenanted object, explicit global scope. */ +const globalUniqueOn = (name: string) => ({ + name, + fields: { + organization_id: { type: 'string' }, + v: { type: 'text', maxLength: 1024 }, + }, + indexes: [{ fields: ['v'], unique: 'global' as const, name: `uniq_${name}_v` }], +}); + +/** The CONTROL shape: an organization-scoped unique, i.e. the NULL-safe arm. */ +const orgUniqueOn = (name: string) => ({ + name, + fields: { + organization_id: { type: 'string' }, + v: { type: 'text', maxLength: 1024 }, + }, + indexes: [{ fields: ['v'], unique: 'organization' as const, name: `uniq_${name}_v` }], +}); + +declareDialectCell(MYSQL_CELL, 'hash-shadow plain unique over duplicates (#15479)', (cell) => { + describe('hash-shadow arm, plain unique over duplicate rows on live MySQL (#15479)', () => { + let driver: SqlDriver; + afterEach(async () => { + await driver?.disconnect().catch(() => {}); + }); + + /** Physical truth, read back from the catalog rather than from our DDL. */ + const catalog = async (table: string) => { + const knex = (driver as any).knex; + const cols = await knex + .select('COLUMN_NAME', 'DATA_TYPE', 'GENERATION_EXPRESSION') + .from('information_schema.COLUMNS') + .where({ TABLE_SCHEMA: knex.client.database(), TABLE_NAME: table }); + const idx = await knex + .select('INDEX_NAME', 'NON_UNIQUE', 'COLUMN_NAME', 'SUB_PART') + .from('information_schema.STATISTICS') + .where({ TABLE_SCHEMA: knex.client.database(), TABLE_NAME: table }); + return { cols, idx }; + }; + + /** Collect this driver's log lines rather than letting them reach stdout. */ + const spy = (): string[] => { + const logs: string[] = []; + (driver as any).logger = { + warn: (msg: string) => logs.push(String(msg)), + info: (msg: string) => logs.push(String(msg)), + error: (msg: string) => logs.push(String(msg)), + }; + return logs; + }; + + /** + * Boot the object WITHOUT its unique index, accumulate two rows that + * violate it, then re-register WITH the index — the legacy-upgrade shape. + */ + const seedDuplicatesThenDeclare = async ( + meta: { name: string; indexes: unknown[] }, + row: Record, + ): Promise<{ logs: string[]; err: unknown }> => { + driver = new SqlDriver(cell.config()); + const logs = spy(); + await driver.initObjects([{ ...meta, indexes: [] }] as any); + const knex = (driver as any).knex; + await knex(meta.name).insert([ + { id: 'a', ...row }, + { id: 'b', ...row }, + ]); + const err: unknown = await driver.initObjects([meta] as any).then( + () => null, + (e) => e, + ); + return { logs, err }; + }; + + // ── Why each it() carries an explicit 60_000 budget (#13902) ── + // Each block constructs a FRESH `new SqlDriver(...)` against this cell's + // live server inside its own body, so the connect cycle plus the schema-sync + // DDL and catalog read-back are paid PER TEST rather than once in a + // beforeAll. Sized like this package's siblings; not a claim that these are + // normally anywhere near that slow. + + /** + * THE CARD'S THREE ACCEPTANCE CONDITIONS, on the `tenancy: { enabled: false }` + * shape: the boot survives, the durability log names the conflicting group + * and the remedy, and the index is absent afterwards. + */ + it('survives the boot and diagnoses duplicates under a tenancy-disabled plain unique', async () => { + const { logs, err } = await seedDuplicatesThenDeclare(plainUniqueOn('os15479_plain'), { + v: LONG, + }); + + expect(err, 'the boot must NOT die: this is a degradation, not a fatal').toBeNull(); + + const diagnosis = logs.find((l) => l.includes("cannot create hash-shadow unique index 'uniq_os15479_plain_v'")); + expect(diagnosis, 'the degradation must reach the durability channel').toBeTruthy(); + expect(diagnosis).toMatch(/Conflicting group\(s\): \(v="p+"\) × 2 rows/); + expect(diagnosis).toMatch(/os migrate plan/); + expect(diagnosis).toContain("The constraint 'v' is NOT enforced"); + + // ⛔ And it must NOT carry the NULL-safe arm's framing, which is false + // here: nothing admitted these rows and there is no NULL-safe key. + expect(diagnosis).not.toContain('#5030'); + expect(diagnosis).not.toContain('NULL-safe'); + expect(diagnosis).not.toContain('COALESCE'); + + // The constraint is honestly ABSENT, and the atomic ALTER left no + // orphaned shadow column behind. + const { cols, idx } = await catalog('os15479_plain'); + expect(idx.some((i: any) => i.INDEX_NAME === 'uniq_os15479_plain_v')).toBe(false); + expect(cols.filter((c: any) => isHashShadowColumn(c.COLUMN_NAME))).toEqual([]); + }, 60_000); + + /** + * The same disposition on the OTHER shape that reaches the plain path — an + * explicit `unique: 'global'` on a tenanted object. Two shapes because + * #14902 measured both, and a guard keyed on the wrong one would pass here + * and fail in production. + */ + it("survives the boot under an explicit unique: 'global' on a tenanted object", async () => { + const { logs, err } = await seedDuplicatesThenDeclare(globalUniqueOn('os15479_global'), { + v: LONG, + organization_id: 'org_a', + }); + + expect(err, 'the boot must NOT die').toBeNull(); + const diagnosis = logs.find((l) => l.includes("cannot create hash-shadow unique index 'uniq_os15479_global_v'")); + expect(diagnosis, 'the degradation must reach the durability channel').toBeTruthy(); + expect(diagnosis).toMatch(/Conflicting group\(s\):/); + expect(diagnosis).not.toContain('#5030'); + expect(diagnosis).not.toContain('COALESCE'); + + const { idx } = await catalog('os15479_global'); + expect(idx.some((i: any) => i.INDEX_NAME === 'uniq_os15479_global_v')).toBe(false); + }, 60_000); + + /** + * ⛔ THE CONTROL for the message split: the NULL-safe arm keeps its OWN + * wording. A one-character fix that widened the guard and left a single + * message would pass the two blocks above and fail exactly here — and the + * inverse mistake, rewriting both arms into the plain sentence, fails here + * too. + */ + it('leaves the NULL-safe arm saying the NULL-safe thing', async () => { + const { logs, err } = await seedDuplicatesThenDeclare(orgUniqueOn('os15479_org'), { + v: LONG, + organization_id: null, + }); + + expect(err, 'the NULL-safe arm already survived the boot').toBeNull(); + const diagnosis = logs.find((l) => l.includes("cannot create hash-shadow unique index 'uniq_os15479_org_v'")); + expect(diagnosis, 'the NULL-safe degradation must still be logged').toBeTruthy(); + expect(diagnosis).toContain('#5030'); + expect(diagnosis).toContain('NULL-safe'); + expect(diagnosis).toContain("COALESCE(organization_id, '__global__')"); + }, 60_000); + + /** + * The positive control that the widened guard did not turn the shadow route + * off: over CLEAN data the plain unique is still CREATED, on the shadow + * column, and still enforces. + */ + it('still creates and enforces the plain shadow unique over clean data', async () => { + driver = new SqlDriver(cell.config()); + spy(); + await driver.initObjects([plainUniqueOn('os15479_clean')] as any); + + const { cols, idx } = await catalog('os15479_clean'); + const shadow = cols.find((c: any) => isHashShadowColumn(c.COLUMN_NAME)); + expect(shadow, 'a shadow column must exist').toBeTruthy(); + const carried = idx.filter((i: any) => isHashShadowColumn(i.COLUMN_NAME)); + expect(carried.length).toBe(1); + expect(Number(carried[0].NON_UNIQUE)).toBe(0); + + const knex = (driver as any).knex; + await knex('os15479_clean').insert({ id: 'a', v: LONG }); + await expect(knex('os15479_clean').insert({ id: 'b', v: LONG })).rejects.toThrow(/duplicate/i); + await knex('os15479_clean').insert({ id: 'c', v: 'q'.repeat(900) }); + }, 60_000); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index adf67e552d..393a83f1a5 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -11816,6 +11816,12 @@ export class SqlDriver implements IDataDriver { * 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. + * - #15479: the same disposition on the HASH-SHADOW route (the MySQL arm + * #11627 added, taken when the server refuses to key the column directly). + * Its guard asked `nullSafe.size > 0` as well, so a plain unique reached + * neither branch and took the boot down carrying the UNKEYABLE-COLUMN + * refusal instead of the duplicate rows. Measured on live MySQL 8.0.46 in + * `sql-driver-15479-shadow-plain-unique-duplicates.test.ts`. */ protected async syncDeclaredIndexes( tableName: string, @@ -11954,6 +11960,56 @@ export class SqlDriver implements IDataDriver { ); continue; } + if (isUniqueViolationError(shadowErr)) { + // #15479 — the PLAIN unique on the SHADOW route: no + // organization key part at all (`tenancy: { enabled: false }` + // or an explicit `unique: 'global'`), reached here because + // MySQL refused to key the column directly. The uniqueness + // limb is supplied by the enclosing `if (unique)` a few lines + // above, which is why this asks only the violation question -- + // the same load-bearing role the explicit `unique &&` limb + // plays on the direct arm below, where no enclosing block + // supplies it. + // + // This branch used to be part of the NULL-safe one above, + // behind `nullSafe.size > 0`, so with no organization key part + // NOTHING fired: the code fell through to + // `logDurabilityFailure(unkeyable, msg)` + `throw` and the boot + // DIED carrying `ER_BLOB_KEY_WITHOUT_LENGTH` -- a message about + // an unkeyable TEXT column, telling the operator to declare a + // `maxLength` the field already declares, naming NO rows and NO + // remedy -- while the actual cause was duplicate rows. #14902 + // fixed exactly that on the direct arm; the two arms of one + // `catch` then disagreed about one question. + // + // The message is deliberately NOT the NULL-safe one above. + // Neither of its clauses is true here: nothing "admitted" these + // rows (there was no previous void constraint, #5030), and + // there is no NULL-safe key. Widening the guard while keeping + // one message would ship a factually FALSE durability log -- + // worse than the throw it replaces, because it sends the + // operator hunting a NULL-distinct index that never existed. + // The sentence below is the direct arm's, verbatim; only the + // route noun ("hash-shadow") is this arm's own, because it is + // what honestly names which physical route was attempted, and + // the NULL-safe branch above already spells it that way. + 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 hash-shadow 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.`, + shadowMsg, + ); + continue; + } // Fall through to the named refusal, which is still the honest // outcome — but say that the shadow route was tried and why it // did not land, so this does not read as never having been