diff --git a/.changeset/driver-sql-unique-violation-predicate.md b/.changeset/driver-sql-unique-violation-predicate.md new file mode 100644 index 0000000000..3c168f17f8 --- /dev/null +++ b/.changeset/driver-sql-unique-violation-predicate.md @@ -0,0 +1,49 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): judge unique violations with the shared predicate, so a Postgres index build over dirty data no longer takes the boot down (#6543) + +`syncDeclaredIndexes` has a branch whose whole job is to keep a database +BOOTING when existing rows violate a NULL-safe unique it was asked to create +(the #5030 defect made data): the constraint is logged at `error` as not +enforced, and the ADR-0120 D4 drift pre-flight reports the exact conflicting +rows. Taking the process down instead would brick the deployment. + +It decided whether it was looking at that case with a private inline regex over +the stringified message — `unique constraint failed|duplicate entry|duplicate +key value`, the fourth hand-written spelling of this question #6250 +inventoried. That read one of the two channels drivers use, and on the DDL path +the missing channel is the whole answer for one shipped dialect: + +| dialect | `CREATE UNIQUE INDEX` over duplicate rows says | old regex | +|:---|:---|:---| +| SQLite | `UNIQUE constraint failed: product.code` | matched | +| MySQL | `ER_DUP_ENTRY: Duplicate entry 'DUP' for key 'uniq_…'` | matched | +| Postgres | `could not create unique index "uniq_…"`, SQLSTATE 23505 | **missed** | + +Postgres does not reuse its DML phrasing for an index build: `duplicate key +value violates unique constraint` is what a conflicting INSERT says, while a +conflicting index BUILD says `could not create unique index "…"` and puts the +verdict on `error.code` (SQLSTATE `23505`) with the offending tuple on +`error.detail`. None of the three message limbs appear in it — so on Postgres +the branch never fired, and a database with legacy duplicates failed to start +rather than booting with the constraint reported as unenforced. + +Both discriminators in this file now call `isUniqueViolationError` from +`@objectstack/types`, passing the **error object** rather than a pre-stringified +message, so `code`, `errno` and the `cause` chain are read alongside `message`: + +- the #5030 boot-survival branch above; +- the negative limb of the MySQL functional-key-part fallback in + `createNullSafeUniqueIndex`, which used a bare `/duplicate/i` to avoid + degrading a conflict into a "this server rejects functional key parts" + verdict — a message-only exclusion that did not fire on the `errno`-only + shape mysql2 can hand back. + +`patch` rather than `minor`: no API changes, and the message spellings that +were recognised before are a strict subset of what the predicate recognises, so +nothing that was absorbed before is absorbed differently now. The site's own +business logic — the `nullSafe.size > 0` guard that keeps this absorption +scoped to the NULL-safe case, and the "already exists" race arm that runs ahead +of it — is unchanged. 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 new file mode 100644 index 0000000000..38d18c1b41 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-unique-violation-predicate.test.ts @@ -0,0 +1,272 @@ +// 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. + * + * It used to judge that with a private inline regex over the stringified + * message — `unique constraint failed|duplicate entry|duplicate key value` — + * the fourth hand-written vocabulary #6250 inventoried. #6543 migrates it onto + * `@objectstack/types`' `isUniqueViolationError`, passing the ERROR OBJECT so + * the `code` / `errno` channels are read at all. + * + * ## Why this is a live defect and not only a structural one + * + * The issue graded the migration `finding`, on the reasoning that "on the three + * dialects the repo ships, the message channel happens to carry the words". + * That holds for the DML path (a duplicate INSERT), which is what this + * package's other tests exercise. It does not hold for the DDL path this + * branch is in: + * + * | dialect | `CREATE UNIQUE INDEX` over duplicate rows says | old regex | + * |:---|:---|:---| + * | SQLite | `UNIQUE constraint failed: product.code` | ✅ matched | + * | MySQL | `ER_DUP_ENTRY: Duplicate entry 'DUP' for key 'uniq_…'` | ✅ matched | + * | Postgres | `could not create unique index "uniq_…"`, SQLSTATE 23505 | ❌ **missed** | + * + * Postgres does not reuse its DML phrasing here: `duplicate key value violates + * unique constraint` is what a conflicting INSERT says, while a conflicting + * index BUILD says `could not create unique index "…"` and carries the verdict + * on `error.code` (SQLSTATE `23505`, `ERRCODE_UNIQUE_VIOLATION`) with the + * offending tuple on `error.detail`. None of the three old message limbs + * appear in it — so on Postgres, the one dialect where the boot-survival + * branch was needed most, it never fired and `throw e` took the boot down. + * Postgres is a first-class shipped dialect for this package + * (`description: "… Supports PostgreSQL, MySQL, SQLite via Knex"`). + * + * The failures are injected rather than driven through a live Postgres because + * this package's unit suite boots SQLite only; the shapes below are the wire + * shapes `pg`/`mysql2` hand knex, message prefix included. + * + * ## Why this package's OTHER tests keep their own spelling + * + * `sql-driver-schema.test.ts`, `sql-driver-unique-tenancy.test.ts` and + * `adr0120-three-posture-conformance.test.ts` assert on + * `/UNIQUE constraint failed|duplicate key value/`. #6543 asked for a decision + * on those rather than leaving them to the next reader. **They stay as they + * are, deliberately.** + * + * They are not discriminators — they are assertions on what a real driver + * actually emitted when a real duplicate INSERT was refused, and their job is + * to prove the constraint EXISTS in the database. Routing them through + * `isUniqueViolationError` would make them strictly weaker in two ways: + * + * 1. The predicate is deliberately broad (four message limbs, three codes, an + * errno, and a `cause` walk). An assertion through it can no longer + * distinguish "SQLite refused this row on a unique index" from "some error + * the predicate happens to accept", which is the whole content of those + * tests. + * 2. A test that judges with the same predicate the production path judges + * with shares that predicate's blind spots — the two stop being + * independent, and a wrong predicate passes its own tests. That + * independence is exactly what caught the Postgres hole above. + * + * The narrow spelling is therefore the right one THERE, and the shared + * predicate the right one in `src/sql-driver.ts`. The rule that reconciles + * them: **judge with the predicate, assert on the literal.** + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import type { DeclaredIndexInput } from '../src/index.js'; + +/** The NULL-safe unique of the #5030 scenario: `COALESCE(organization_id), code`. */ +const NULL_SAFE_INDEX: DeclaredIndexInput = { + name: 'uniq_product_organization_id_code', + fields: ['organization_id', 'code'], + unique: 'organization', + nullSafeColumns: ['organization_id'], +}; + +/** The same index with no NULL-safe key part — the `nullSafe.size > 0` guard's false arm. */ +const PLAIN_INDEX: DeclaredIndexInput = { + name: 'uniq_product_code', + fields: ['code'], + unique: true, +}; + +const PHYSICAL_COLUMNS = new Set(['id', 'organization_id', 'code']); + +/** + * What `pg` hands knex when `CREATE UNIQUE INDEX` finds duplicate rows. + * knex prefixes the failing statement onto the message; the primary message is + * `could not create unique index "…"` and the tuple lives on `detail`. + */ +function postgresIndexBuildConflict(): Error { + const err = new Error( + `create unique index "uniq_product_organization_id_code" on "product" ` + + `(COALESCE("organization_id", '__global__'), "code") - ` + + `could not create unique index "uniq_product_organization_id_code"`, + ); + Object.assign(err, { + code: '23505', + detail: `Key (COALESCE(organization_id, '__global__'::text), code)=(__global__, DUP) is duplicated.`, + severity: 'ERROR', + routine: '_bt_check_unique', + }); + return err; +} + +/** mysql2's numeric channel with prose the old regex could not read. */ +function mysqlErrnoOnlyConflict(): Error { + const err = new Error('alter table `product` add unique `uniq_product_organization_id_code` - ER_DUP_ENTRY'); + Object.assign(err, { errno: 1062, sqlState: '23000' }); + return err; +} + +/** A failure that is NOT a unique violation and must keep taking the boot down. */ +function unrelatedDdlFailure(): Error { + const err = new Error('create unique index "uniq_product_organization_id_code" - permission denied for table product'); + Object.assign(err, { code: '42501' }); + return err; +} + +describe('syncDeclaredIndexes unique-violation discriminator (#6543)', () => { + let driver: SqlDriver; + let realKnex: any; + let errors: string[]; + let warns: string[]; + + /** Make the NULL-safe index creation fail with `err`, and capture the log. */ + function arm(err: Error): void { + (driver as any).createNullSafeUniqueIndex = async () => { + throw err; + }; + errors = []; + warns = []; + (driver as any).logger = { + warn: (msg: string) => warns.push(String(msg)), + error: (msg: string) => errors.push(String(msg)), + }; + } + + /** Drive the branch under test directly — `initObjects` is not needed to reach it. */ + function sync(indexes: DeclaredIndexInput[]): Promise { + return (driver as any).syncDeclaredIndexes('product', indexes, PHYSICAL_COLUMNS, 'organization_id'); + } + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + realKnex = (driver as any).knex; + await realKnex.schema.createTable('product', (t: any) => { + t.string('id').primary(); + t.string('organization_id'); + t.string('code'); + }); + }); + + afterEach(async () => { + // One test stands in for `knex`; put the real one back so teardown closes it. + (driver as any).knex = realKnex; + await driver.disconnect(); + }); + + // ── The channels the old message-only read could not see ────────────────── + + it('absorbs a Postgres index-build conflict that names the verdict only on `code` (SQLSTATE 23505)', async () => { + arm(postgresIndexBuildConflict()); + + // Before #6543 this REJECTED: none of `unique constraint failed`, + // `duplicate entry`, `duplicate key value` appears in Postgres' DDL + // phrasing, so the branch fell through to `throw e` and the boot died on + // exactly the dirty database it exists to survive. + await expect(sync([NULL_SAFE_INDEX])).resolves.toBeUndefined(); + + // Absorbed the way the branch promises: the durability-degradation + // channel, naming the constraint that is NOT enforced and the way out. + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/cannot create NULL-safe unique index/); + expect(errors[0]).toMatch(/uniq_product_organization_id_code/); + expect(errors[0]).toMatch(/NOT enforced/); + expect(errors[0]).toMatch(/#5030/); + expect(errors[0]).toMatch(/ADR-0120 D4/); + }); + + it('absorbs a MySQL conflict carried only on `errno` (1062)', async () => { + arm(mysqlErrnoOnlyConflict()); + + await expect(sync([NULL_SAFE_INDEX])).resolves.toBeUndefined(); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/#5030/); + }); + + it('reads the violation through a driver `cause` wrapper', async () => { + const wrapped = new Error('index sync failed'); + Object.assign(wrapped, { cause: postgresIndexBuildConflict() }); + arm(wrapped); + + await expect(sync([NULL_SAFE_INDEX])).resolves.toBeUndefined(); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/#5030/); + }); + + // ── Nothing the old regex caught may be narrowed ────────────────────────── + + it.each([ + ['sqlite', 'UNIQUE constraint failed: product.organization_id, product.code'], + ['mysql', "ER_DUP_ENTRY: Duplicate entry 'DUP' for key 'uniq_product_organization_id_code'"], + ['postgres dml', 'duplicate key value violates unique constraint "uniq_product_organization_id_code"'], + ])('still absorbs the %s message spelling the inline regex used to match', async (_dialect, message) => { + arm(new Error(message)); + + await expect(sync([NULL_SAFE_INDEX])).resolves.toBeUndefined(); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/#5030/); + }); + + // ── 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 () => { + arm(postgresIndexBuildConflict()); + // 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. + (driver as any).getExistingIndexNames = async () => new Set(); + (driver as any).knex = { + schema: { + alterTable: () => Promise.reject(postgresIndexBuildConflict()), + }, + }; + + // 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( + () => undefined, + (e: unknown) => e, + ); + expect(rejected).toBeInstanceOf(Error); + expect(rejected.code).toBe('23505'); + expect(errors).toHaveLength(0); + }); + + it('rethrows a failure that is not a unique violation, identity preserved', async () => { + const original = unrelatedDdlFailure(); + arm(original); + + const rejected: any = await sync([NULL_SAFE_INDEX]).then( + () => undefined, + (e: unknown) => e, + ); + expect(rejected).toBe(original); + expect(rejected.code).toBe('42501'); + expect(errors).toHaveLength(0); + }); + + it('still treats an "already exists" race as benign, ahead of the conflict branch', async () => { + const race = new Error('create unique index - index "uniq_product_organization_id_code" already exists'); + Object.assign(race, { code: '42P07' }); + arm(race); + + await expect(sync([NULL_SAFE_INDEX])).resolves.toBeUndefined(); + // Benign: absorbed WITHOUT the durability-degradation log, because the + // constraint IS enforced — a different outcome from the #5030 branch. + expect(errors).toHaveLength(0); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index fc3a44a838..918fdd7695 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -40,7 +40,7 @@ import type { DriverQuery, IDataDriver } from '@objectstack/spec/contracts'; import { StandardErrorCode } from '@objectstack/spec/api'; import { StorageNameMapping } from '@objectstack/spec/system'; import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; -import { resolveTenancyPosture } from '@objectstack/types'; +import { isUniqueViolationError, resolveTenancyPosture } from '@objectstack/types'; import { postureEnforcesWall } from '@objectstack/spec/security'; import { nextUtcCalendarDay } from '@objectstack/core'; import { @@ -6348,7 +6348,17 @@ export class SqlDriver implements IDataDriver { // different name can race us here — both are benign for our intent // (the index exists). Anything else is a real failure. if (/already exists|duplicate key name|exists/i.test(msg)) continue; - if (nullSafe.size > 0 && /unique constraint failed|duplicate entry|duplicate key value/i.test(msg)) { + // The ERROR OBJECT, not `msg` (#6543). This used to be a private + // inline regex over the message alone, which is the only channel the + // SQLite family reliably fills — but Postgres answers this exact + // failure with `could not create unique index "…"` and puts the + // verdict on `code` (SQLSTATE 23505) instead, so a message-only read + // missed the dialect entirely and took the boot down on the very case + // the branch below exists to absorb. The shared predicate reads + // `code` / `errno` / `message` / `cause`; see + // `@objectstack/types`' `unique-violation.ts` for why it is the one + // name for this question. + if (nullSafe.size > 0 && isUniqueViolationError(e)) { // Existing rows violate the NULL-safe unique — the #5030 defect made // visible. Do not take the boot down: the declared constraint is not // enforced yet, say so at `error` (from the outside everything looks @@ -6398,8 +6408,16 @@ export class SqlDriver implements IDataDriver { await this.knex.raw(sql); } catch (e: any) { const msg = String(e?.message ?? e); + // The positive limb is this site's own question — "does this server + // reject functional key parts?" — and stays a message test, because + // that is the only channel the answer is on. The NEGATIVE limb was a + // seventh spelling of the unique-violation vocabulary (`/duplicate/i`) + // and is now the shared predicate (#6543): a conflict must never be + // read as a syntax rejection and silently degraded to the bare + // composite, and on the `errno`-only shape mysql2 can hand back, a + // message-only exclusion did not fire. const functionalUnsupported = - this.isMysql && /syntax|functional|not supported|near '\(/i.test(msg) && !/duplicate/i.test(msg); + this.isMysql && /syntax|functional|not supported|near '\(/i.test(msg) && !isUniqueViolationError(e); if (!functionalUnsupported) throw e; (this.logger.error ?? this.logger.warn)( `[sql-driver] this MySQL/MariaDB server rejects functional key parts — created '${name}' on ` +