diff --git a/.changeset/classify-index-failure-error-channel.md b/.changeset/classify-index-failure-error-channel.md new file mode 100644 index 0000000000..0eb4e394ce --- /dev/null +++ b/.changeset/classify-index-failure-error-channel.md @@ -0,0 +1,40 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): classify a failed index build from the ERROR, not its message (#6699) + +`classifyIndexFailure` — the function both runtime partial-index migrations in +this package classify a failed `CREATE UNIQUE INDEX` with — carried its own +private unique-violation vocabulary and answered from the **message channel +only**. That made it the fifth such copy in the repo, and the one #6250's +inventory missed: it lives in a package none of the other four touched, so it +was never in that table and none of the queued follow-ups covered it. + +The first arm now delegates to `@objectstack/types`' `isUniqueViolationError` +(#6250 / PR #6541) — the one named answer to "is this a unique-constraint +violation?" — and `probeThenReplaceIndex` passes it the **caught error object** +instead of `err.message`. A string-only swap would have compiled unchanged and +kept the defect: the point of the shared predicate is the `code` / `errno` / +`cause` channels, which unwrapping the message throws away. + +**What changes at runtime.** A driver that reports the conflict on `code` or +`errno` while giving unhelpful prose — SQLite's `SQLITE_CONSTRAINT_UNIQUE`, +MySQL's `ER_DUP_ENTRY` / errno `1062`, Postgres' SQLSTATE `23505`, or the +condition one step down `error.cause` behind a pooled wrapper's `Write failed` +— was classified `failed`. It is now `conflict`, which is the verdict that +produces the report ADR-0120 D4 requires: the key that is not enforced, the +query that lists the offending rows, and the pointer at `os migrate plan`. +Every message-channel verdict is unchanged — the shared predicate's message +limb covers all three shipped dialects' prose. + +**Two things deliberately preserved.** The arm order still checks the +duplicate-row question BEFORE the dialect question, because MySQL's duplicate +error mentions the key and some drivers wrap both facts in one string; and the +dialect arm (`unsupported`) is still this module's own message-based +vocabulary, since the shared predicate answers the first arm only and has no +opinion about dialect support. + +`classifyIndexFailure`'s parameter widens from `string` to `unknown`, so every +existing string call still compiles and is judged exactly as before. Callers +holding a caught error should pass it directly rather than `err.message`. diff --git a/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts index 304f715e9e..c5d85da225 100644 --- a/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts @@ -157,6 +157,97 @@ describe('probe-first partial index replacement (#6418)', () => { expect(classifyIndexFailure('disk I/O error')).toBe('failed'); }); + /** + * #6699 — the substance of the migration onto `@objectstack/types`' + * `isUniqueViolationError`: the conflict is judged on the channels the + * driver actually wrote it to, not on prose alone. + * + * Every message below is deliberately USELESS — none carries a word any + * unique-violation vocabulary's message limb looks for — so each case can + * pass ONLY by reading `code` / `errno`. The second assertion in each case + * proves that: run the same prose through the classifier on its own and the + * verdict is `failed`, which is what this module answered for all of them + * while it carried its own message-only regex. + */ + it.each([ + ['a SQLite extended result code', { code: 'SQLITE_CONSTRAINT_UNIQUE' }], + ["mysql2's symbolic name", { code: 'ER_DUP_ENTRY' }], + ['a bare MySQL errno', { errno: 1062 }], + ['a Postgres SQLSTATE', { code: '23505' }], + ])('reads a conflict off %s when the message says nothing (#6699)', (_label, channels) => { + const error = Object.assign(new Error('insert failed'), channels); + expect(classifyIndexFailure(error)).toBe('conflict'); + expect(classifyIndexFailure(error.message)).toBe('failed'); + }); + + it('follows a pooled wrapper down to the cause (#6699)', () => { + // Pool and query-builder layers re-throw with the original attached, so + // the only copy of the condition is one step down. + const wrapped = Object.assign(new Error('Write failed'), { + cause: Object.assign(new Error('insert failed'), { code: '23505' }), + }); + expect(classifyIndexFailure(wrapped)).toBe('conflict'); + expect(classifyIndexFailure(wrapped.message)).toBe('failed'); + }); + + it('keeps the data verdict ahead of the dialect verdict on the OBJECT channel too (#6699)', () => { + // The arm order, re-pinned where widening the input could have broken + // it: a duplicate reported on `code`, wrapped by a layer whose prose is + // a dialect refusal. Judged on the message alone this is `unsupported` + // — "this database cannot build this index" for a real data conflict, + // exactly the misreport the ordering exists to prevent. + const both = Object.assign(new Error('near "WHERE": syntax error'), { + code: 'SQLITE_CONSTRAINT_UNIQUE', + }); + expect(classifyIndexFailure(both)).toBe('conflict'); + expect(classifyIndexFailure(both.message)).toBe('unsupported'); + // …and on a single string carrying both facts, unchanged since #6418. + expect( + classifyIndexFailure('near "WHERE": syntax error — duplicate key value violates unique constraint'), + ).toBe('conflict'); + }); + + it('a dialect refusal carrying its own code is still `unsupported` (#6699)', () => { + // Widening the input from `string` to the error object must not blind + // the second arm: MySQL's parse error has `code` and `errno` too, and + // neither is a unique-violation signal, so the verdict has to come from + // the message exactly as before. + const parseError = Object.assign( + new Error("You have an error in your SQL syntax ... near 'WHERE state'"), + { code: 'ER_PARSE_ERROR', errno: 1064 }, + ); + expect(classifyIndexFailure(parseError)).toBe('unsupported'); + const io = Object.assign(new Error('disk I/O error'), { code: 'SQLITE_IOERR', errno: 10 }); + expect(classifyIndexFailure(io)).toBe('failed'); + }); + + it('the probe hands the ERROR to the classifier, not its message (#6699)', async () => { + // The threading pin, and the only test here that can see it: every + // assertion above still passes if `probeThenReplaceIndex` keeps + // unwrapping `err.message` before classifying. This one cannot — the + // verdict exists nowhere but on `code`. + const codeOnly: IndexExec = async (sql: string) => { + if (sql.startsWith('CREATE')) { + throw Object.assign(new Error('insert failed'), { code: 'SQLITE_CONSTRAINT_UNIQUE' }); + } + return db.exec(sql); + }; + + const outcome = await probeThenReplaceIndex(codeOnly, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + + expect(outcome.status).toBe('conflict'); + expect(outcome.failedAt).toBe('probe'); + // `detail` is unchanged — still the driver's own prose, for the operator. + expect(outcome.detail).toBe('insert failed'); + // The probe is what failed, so the previous index is untouched. + expect(indexDdl(REAL)).toEqual(EXISTING_DDL); + expect(indexDdl(PROBE)).toBeUndefined(); + }); + it('logProblem prefers error(), falls back to warn(), and tolerates neither', () => { const full = { warn: vi.fn(), error: vi.fn() }; logProblem(full, 'msg', 'detail'); diff --git a/packages/metadata-protocol/src/migrations/partial-index-probe.ts b/packages/metadata-protocol/src/migrations/partial-index-probe.ts index b407c37485..2c8a420a4b 100644 --- a/packages/metadata-protocol/src/migrations/partial-index-probe.ts +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.ts @@ -37,6 +37,8 @@ * a classified status plus the driver's own text and stays out of the way. */ +import { isUniqueViolationError } from '@objectstack/types'; + /** Raw-SQL seam. Mirrors `ensureOverlayIndex`: `raw()` first, `execute()` second. */ export type IndexExec = (sql: string) => Promise; @@ -86,7 +88,26 @@ export type PartialIndexStatus = | 'failed'; /** - * Classify a failed `CREATE UNIQUE INDEX`. + * The text the DIALECT arm judges, from a thrown value of any shape. + * + * `message` first, because that is the channel a driver writes its refusal on + * and the only one this arm has ever read; `String()` only as the last resort + * — which is what a bare string resolves to unchanged, so a caller holding + * nothing but prose is judged exactly as before. + */ +function indexFailureText(error: unknown): string { + if (typeof error === 'string') return error; + if (typeof error === 'object' && error !== null) { + const { message } = error as { message?: unknown }; + if (typeof message === 'string') return message; + } + return String(error); +} + +/** + * Classify a failed `CREATE UNIQUE INDEX`, from the thrown ERROR (#6699). + * + * ## The two arms, and why the order is load-bearing * * Duplicate-row wording is checked BEFORE dialect wording: MySQL's duplicate * error mentions the key, and some drivers wrap both facts in one string, so @@ -98,12 +119,35 @@ export type PartialIndexStatus = * split: no partial indexes at all, and (before 8.0.13 / on MariaDB) no * functional key parts for `COALESCE` parts. Both leave the same outcome — the * previous index stays — so one verdict is enough. + * + * ## Why the first arm is not this module's own regex any more + * + * "Is this a unique-constraint violation?" is one question, and #6250 gave it + * one named answer — `isUniqueViolationError` in `@objectstack/types`. This + * function carried a **fifth** private vocabulary for it (#6699, missed by that + * inventory because it lives in a package none of the other four touched), and + * the copy was strictly weaker in the way that inventory was about: it read the + * **message channel only**. A driver that reports the conflict on `code` / + * `errno` — SQLite's `SQLITE_CONSTRAINT_UNIQUE`, MySQL's `ER_DUP_ENTRY` / + * errno `1062`, Postgres' SQLSTATE `23505` — while giving unhelpful prose + * (`insert failed`, a pooled wrapper's `Write failed`, or the condition one step + * down `error.cause`) was classified `failed` here, where the shared predicate + * answers `true`. Same shape as the hole that made every MySQL conflict a 500 + * in `mapDataError` before #6541. + * + * The predicate answers the FIRST arm only. It has no opinion about dialect + * support, so the second arm stays this module's own — and stays second. + * + * ⚠️ Pass the **error**, not `err.message`. A string still works (the predicate + * reads it on the message channel, and so does {@link indexFailureText}), but a + * caller that unwraps first throws away the `code` / `errno` / `cause` channels + * that are the whole reason this reads the object. */ -export function classifyIndexFailure(message: string): PartialIndexStatus { - if (/unique constraint failed|duplicate entry|duplicate key value|violates unique/i.test(message)) { +export function classifyIndexFailure(error: unknown): PartialIndexStatus { + if (isUniqueViolationError(error)) { return 'conflict'; } - if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(message)) { + if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(indexFailureText(error))) { return 'unsupported'; } return 'failed'; @@ -169,9 +213,14 @@ export async function probeThenReplaceIndex( try { await exec(buildSql(probeIndexName)); } catch (err: unknown) { + // `detail` is the OPERATOR-facing text and stays the driver's own prose. + // The VERDICT is taken from the error object itself, so a conflict + // reported on `code` / `errno` / `cause` with unhelpful prose is still + // classified as one (#6699) — unwrapping first is exactly what the + // migration onto the shared predicate exists to stop. const detail = err instanceof Error ? err.message : String(err); await dropIndexQuietly(exec, probeIndexName); - return { status: classifyIndexFailure(detail), detail, failedAt: 'probe' }; + return { status: classifyIndexFailure(err), detail, failedAt: 'probe' }; } await dropIndexQuietly(exec, probeIndexName); diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts index 863f8a3fd1..af90d0400c 100644 --- a/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts @@ -469,6 +469,14 @@ describe('sys_view_definition active-row uniqueness (#5839) on a NULL-safe key ( // MariaDB's refusal of a functional key part, which #6417 introduces. expect(classifyIndexFailure('Functional index on a column is not supported')).toBe('unsupported'); expect(classifyIndexFailure('disk I/O error')).toBe('failed'); + // #6699: the same verdict off the `code` channel, with prose that + // carries no signal at all. Asserted through THIS module's re-export + // (the public `@objectstack/metadata-protocol` surface), because that is + // the export the classifier's own home is reached by — the full + // channel matrix lives in `partial-index-probe.test.ts`. + expect( + classifyIndexFailure(Object.assign(new Error('insert failed'), { code: 'ER_DUP_ENTRY' })), + ).toBe('conflict'); }); it('buildActiveIndexSql scopes rows AND spells the key NULL-safe', () => {