diff --git a/.changeset/index-failure-dialect-cause-walk.md b/.changeset/index-failure-dialect-cause-walk.md new file mode 100644 index 0000000000..33e5bf50cd --- /dev/null +++ b/.changeset/index-failure-dialect-cause-walk.md @@ -0,0 +1,46 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): the dialect arm of `classifyIndexFailure` walks `cause` to the same depth the conflict arm does (#6848) + +`classifyIndexFailure` had two arms reading two different wrap-depths. #6699 +moved the first arm onto `@objectstack/types`' `isUniqueViolationError`, which +follows `error.cause` four levels down because pool and query-builder layers +re-throw with the original attached. The second — the dialect arm — kept reading +`err.message` and stopping there. + +So a dialect refusal arriving behind a wrapper (outer prose `Write failed` or +`pool query failed`, the actual `near "WHERE": syntax error` one step down +`cause`) was graded `failed` instead of `unsupported`. The private +`indexFailureText` helper now collects the message channel of the thrown value +**and** of each `cause` below it, bounded at the same `MAX_CAUSE_DEPTH` of 4 the +predicate uses and counted the same way (the thrown value is depth 0). The +dialect vocabulary itself is unchanged — only the text fed to it. + +**Why the verdict matters beyond wording.** The two consumers dispose of +`unsupported` and `failed` differently. `view-definition-active-index.ts` treats +them the same (keep the previous index, report at `error`; only the wording +differs). But `ensureOverlayStateIndex` builds the composite **fallback lookup +index** on the `unsupported` branch and on no other — offered precisely because +a dialect that cannot take the partial form should still get the lookup. Under +a `failed` verdict that branch never ran, so `fallback` came back +`not-attempted` rather than `ensured` / `refused` and the degradation target was +silently never attempted. + +**Dormant, not a live regression.** No driver shipped today produces the wrapped +shape — each hands knex's error back with the dialect text on the outer message, +which is why every existing case matched on the first read. This closes an +asymmetry before a wrapping raw-SQL driver can land on it; it is also not a +regression from #6699, which only made the contrast visible by deepening the +first arm. + +Two details worth knowing if you touch this: the collected levels are joined +with a **newline**, never a space, because two of the dialect alternatives are +multi-word (`where clause`, `near "where"`) and a space would let a phrase be +synthesised across a wrapper boundary that no single driver wrote. And a looping +`cause` chain is **bounded rather than detected** — no visited set — which is +exactly what the predicate this mirrors does. + +Arm order is unchanged and still load-bearing: a conflict reported anywhere in +the chain still beats a dialect refusal in the outer prose. diff --git a/packages/metadata-protocol/src/migrations/overlay-index.test.ts b/packages/metadata-protocol/src/migrations/overlay-index.test.ts index cb06d28b7d..475bb91910 100644 --- a/packages/metadata-protocol/src/migrations/overlay-index.test.ts +++ b/packages/metadata-protocol/src/migrations/overlay-index.test.ts @@ -302,6 +302,44 @@ describe('sys_metadata overlay uniqueness (#6418)', () => { expect(logger.info).not.toHaveBeenCalled(); }); + /** + * #6848 — the same MariaDB refusal, behind a pooled wrapper. + * + * This is the consequence the classifier's wrap-depth actually decides, and + * the reason the dialect arm was given the `cause` walk rather than a doc + * comment. The fallback lookup index is built on the `unsupported` branch + * and on no other, so while the second arm stopped at the outer message this + * case graded `failed` and the degradation target was never attempted — the + * dialect's answer was present, one step down, and unread. + */ + it('a POOLED wrapper around the dialect refusal still reaches the fallback index (#6848)', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const pooled: IndexExec = async (sql: string) => { + if (/where/i.test(sql)) { + throw Object.assign(new Error('Write failed'), { + cause: new Error( + "You have an error in your SQL syntax; check the manual … near 'WHERE state = 'active''", + ), + }); + } + return db.exec(sql); + }; + + const result = await ensureOverlayStateIndex(pooled, 'active', logger); + + expect(result.status).toBe('unsupported'); + // The whole point: `not-attempted` here would mean the dialect that + // cannot take the partial form silently got no lookup index either. + expect(result.fallback).toBe('ensured'); + // `detail` stays the operator-facing OUTER prose, unchanged. + expect(result.detail).toBe('Write failed'); + // Nothing was downgraded — the declared UNIQUE index is byte-for-byte there. + expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL); + expect(insert('w1', 'view', 'lead.w', 'org1', 'pkg1', 'active').ok).toBe(true); + expect(insert('w2', 'view', 'lead.w', 'org1', 'pkg1', 'active').ok).toBe(false); + expect(String(logger.error.mock.calls[0]![0])).toContain('NOT enforced as specified on this dialect'); + }); + /** * MySQL proper, where the old code was safe only by ACCIDENT: it has * neither `DROP INDEX IF EXISTS` nor `CREATE INDEX IF NOT EXISTS`, so every 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 c5d85da225..3bcee65212 100644 --- a/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts @@ -248,6 +248,128 @@ describe('probe-first partial index replacement (#6418)', () => { expect(indexDdl(PROBE)).toBeUndefined(); }); + /* ────────────────────────────────────────────────────────────────────── * + * #6848 — the DIALECT arm walks `cause` to the same depth as the first + * ────────────────────────────────────────────────────────────────────── */ + + /** A neutral wrapper: matches NEITHER vocabulary, so it can only carry. */ + const wrap = (cause: unknown): Error => Object.assign(new Error('pool query failed'), { cause }); + + /** `leaf` behind `depth` neutral wrappers (`depth: 0` is the leaf itself). */ + const nest = (depth: number, leaf: unknown): unknown => + depth === 0 ? leaf : wrap(nest(depth - 1, leaf)); + + it('grades a WRAPPED dialect refusal `unsupported`, not `failed` (#6848)', () => { + // The shape a pooled or query-builder layer produces: useless outer + // prose, the dialect's actual answer one step down `cause`. Before this + // the second arm stopped at the outer message and returned `failed` — + // which costs `overlay-index` its fallback lookup index, because that + // branch is reached on `unsupported` and nowhere else. + const wrapped = Object.assign(new Error('Write failed'), { + cause: new Error('near "WHERE": syntax error'), + }); + expect(classifyIndexFailure(wrapped)).toBe('unsupported'); + // The control: the outer prose alone is still, correctly, `failed`. + expect(classifyIndexFailure('Write failed')).toBe('failed'); + + // …and the same for the functional-key-parts refusal, one layer deeper + // and on a plain object rather than an Error. + expect(classifyIndexFailure({ message: 'Write failed', cause: { cause: { message: 'Functional index on a column is not supported' } } })).toBe( + 'unsupported', + ); + }); + + it('keeps the data verdict ahead of the dialect verdict ACROSS the chain (#6848)', () => { + // The inversion risk of widening the second arm: both arms now walk, so + // the ordering has to hold at every depth, not just at the top. Outer + // prose is a dialect refusal; the real condition is a conflict reported + // on `code` two levels down. `conflict` must still win. + const misleading = Object.assign(new Error('near "WHERE": syntax error'), { + cause: wrap(Object.assign(new Error('insert failed'), { code: '23505' })), + }); + expect(classifyIndexFailure(misleading)).toBe('conflict'); + }); + + it('reads the dialect arm to exactly the depth the conflict arm reads (#6848)', () => { + // The card's whole point, pinned as a PARITY rather than as a number: + // whatever depth `isUniqueViolationError` reaches, this module's dialect + // arm reaches the same one. Expressed this way the assertion survives a + // deliberate change to the shared bound and still goes red the moment + // the two arms drift apart again. + const DEPTHS = [0, 1, 2, 3, 4, 5, 6]; + const dialectReach = DEPTHS.map( + (d) => classifyIndexFailure(nest(d, new Error('near "WHERE": syntax error'))) === 'unsupported', + ); + const conflictReach = DEPTHS.map( + (d) => + classifyIndexFailure(nest(d, Object.assign(new Error('insert failed'), { code: '23505' }))) === + 'conflict', + ); + + expect(dialectReach).toEqual(conflictReach); + // …and the shared profile is the predicate's `MAX_CAUSE_DEPTH` of 4 + // counted from the thrown value, so the equality above cannot pass by + // both arms reaching nothing (or everything). + expect(dialectReach).toEqual([true, true, true, true, true, false, false]); + }); + + it('joins the chain with a newline, so no phrase is synthesised across a wrapper (#6848)', () => { + // Two of the dialect alternatives are multi-word. Neither message below + // is a refusal on its own, and joining them with a SPACE would forge + // `where clause` out of text no layer ever wrote. + const spliced = Object.assign(new Error('rebuild attempt landed where'), { + cause: new Error('clause parsing completed'), + }); + expect(classifyIndexFailure('rebuild attempt landed where')).toBe('failed'); + expect(classifyIndexFailure('clause parsing completed')).toBe('failed'); + expect(classifyIndexFailure(spliced)).toBe('failed'); + }); + + it('terminates on a `cause` chain that loops, exactly as the predicate does (#6848)', () => { + // `isUniqueViolationError` bounds rather than detects cycles — it keeps + // no visited set — so this walk must not either, and the bound has to be + // what stops both. A self-referential cause must return a verdict rather + // than exhaust the stack. + const loop = new Error('disk I/O error') as Error & { cause?: unknown }; + loop.cause = loop; + expect(classifyIndexFailure(loop)).toBe('failed'); + + // …and a two-node cycle whose refusal is only on the INNER node is still + // found, because the bound is reached after the answer, not before it. + const outer = new Error('Write failed') as Error & { cause?: unknown }; + const inner = new Error('near "WHERE": syntax error') as Error & { cause?: unknown }; + outer.cause = inner; + inner.cause = outer; + expect(classifyIndexFailure(outer)).toBe('unsupported'); + }); + + it('the probe classifies a wrapped refusal, and leaves `detail` the OUTER prose (#6848)', async () => { + // End-to-end through `probeThenReplaceIndex`: the verdict widens, the + // operator-facing text does not. `detail` stays the driver's own outer + // message, which is the contract `probeThenReplaceIndex` already had. + const wrapping: IndexExec = async (sql: string) => { + if (sql.startsWith('CREATE')) { + throw Object.assign(new Error('Write failed'), { + cause: new Error('near "WHERE": syntax error'), + }); + } + return db.exec(sql); + }; + + const outcome = await probeThenReplaceIndex(wrapping, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + + expect(outcome.status).toBe('unsupported'); + expect(outcome.failedAt).toBe('probe'); + expect(outcome.detail).toBe('Write 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 2c8a420a4b..7f7e18ec91 100644 --- a/packages/metadata-protocol/src/migrations/partial-index-probe.ts +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.ts @@ -87,21 +87,70 @@ export type PartialIndexStatus = /** Anything else, best-effort. Previous index kept. */ | 'failed'; +/** + * How far to follow an `error.cause` chain — deliberately the same bound as + * `isUniqueViolationError`'s `MAX_CAUSE_DEPTH` in `@objectstack/types` (#6848). + * + * Counted the same way too: the thrown value itself is depth 0, so this admits + * the outer error plus four wrapper levels below it. The two arms of {@link + * classifyIndexFailure} reading the SAME depth is the whole point — see that + * function's "Why both arms walk" note. + * + * It is also the only cycle guard, again matching the predicate: a `cause` + * chain that loops back on itself is bounded rather than detected, because a + * bound terminates a cycle just as well as a visited-set does and the predicate + * this mirrors has no visited-set to mirror. + */ +const MAX_CAUSE_DEPTH = 4; + +/** + * Every message channel on one thrown value and its `cause` chain, in order. + * + * Deliberately a local walk. `@objectstack/types` owns the *conflict* question + * and exports a predicate for it, but it exposes no reusable message-collecting + * helper — its own chain walkers (`matchesUniqueViolation`, + * `findUniqueViolationColumn`) are private and each answers its own question + * rather than handing back text. Hoisting a shared collector there would widen + * that package's contract for a single consumer, so this stays here and stays + * pinned to the bound above. + */ +function collectIndexFailureText(error: unknown, depth: number, into: string[]): void { + if (error === null || error === undefined || depth > MAX_CAUSE_DEPTH) return; + if (typeof error === 'string') { + into.push(error); + return; + } + if (typeof error !== 'object') { + into.push(String(error)); + return; + } + const err = error as { message?: unknown; cause?: unknown }; + if (typeof err.message === 'string') into.push(err.message); + collectIndexFailureText(err.cause, depth + 1, into); +} + /** * 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. + * `message` first, because that is the channel a driver writes its refusal on; + * then the same channel one step at a time down `cause`, because pool and + * query-builder layers re-throw with the original attached and the refusal is + * then the ONLY copy of the dialect's answer (#6848). `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. + * + * ⚠️ The levels are joined with a NEWLINE, never a space. Two of the dialect + * vocabulary's alternatives are multi-word (`where clause`, `near "where"`), so + * a space would let a phrase be synthesised across a wrapper boundary that no + * single driver ever wrote — an outer message ending in `where` above a cause + * beginning with `clause` would read as a dialect refusal. A newline cannot + * match the literal space in those alternatives, so each level is still judged + * on text some layer actually emitted. */ 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); + const texts: string[] = []; + collectIndexFailureText(error, 0, texts); + return texts.length > 0 ? texts.join('\n') : String(error); } /** @@ -138,10 +187,34 @@ function indexFailureText(error: unknown): string { * 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. * + * ## Why both arms walk `cause` (#6848) + * + * They read the same depth because they are asked the same way. #6699 gave the + * first arm the shared predicate's four-level `cause` walk and left the second + * on the outer message alone; the two then disagreed about how deeply a driver + * is allowed to wrap. A dialect refusal arriving behind a pooled wrapper — + * outer prose `Write failed`, the real `near "WHERE": syntax error` one step + * down — was graded `failed` rather than `unsupported`. + * + * That gap is **not** a wording difference, which is why it was worth closing + * rather than documenting. `view-definition-active-index.ts` disposes of the + * two verdicts identically (keep the previous index, report at `error`), but + * `overlay-index.ts` builds the composite **fallback lookup index** on + * `unsupported` and only there — offered precisely because a dialect that + * cannot take the partial form should still get the lookup. Under a `failed` + * verdict that branch never runs and the fallback is reported `not-attempted` + * instead of `ensured` / `refused`, so the wrap depth silently decides whether + * the degradation target is built at all. + * + * No driver shipped today produces that shape — every one hands knex's error + * back with the dialect text on the outer message, which is why every case here + * matched on the first read. This closes a dormant asymmetry, not a live defect. + * * ⚠️ 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. + * that are the whole reason this reads the object — and, since #6848, the + * dialect answer too when a wrapper holds the useless half. */ export function classifyIndexFailure(error: unknown): PartialIndexStatus { if (isUniqueViolationError(error)) {