Skip to content

Commit c733ae8

Browse files
os-zhuangclaude
andauthored
fix(metadata-protocol): dialect arm walks cause to the conflict arm's depth (#6848) (#6989)
`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 dialect arm kept reading `err.message` and stopping there, so a refusal behind a wrapper — outer prose `Write failed`, the real `near "WHERE": syntax error` one step down — graded `failed` instead of `unsupported`. That is not only a wording difference: `ensureOverlayStateIndex` builds the composite fallback lookup index on the `unsupported` branch and on no other, so under a `failed` verdict the degradation target was never attempted and `fallback` came back `not-attempted`. `indexFailureText` now collects the message channel of the thrown value and of each `cause` below it, bounded at the same `MAX_CAUSE_DEPTH` of 4 and counted the same way. The dialect regex is unchanged — only the text fed to it. Levels join with a newline, never a space, so a multi-word alternative cannot be synthesised across a wrapper boundary. A looping chain is bounded rather than detected, matching the predicate, which keeps no visited set either. Dormant, not a live regression: no shipped driver produces the wrapped shape. Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw Co-authored-by: Claude <noreply@anthropic.com>
1 parent d3e53f2 commit c733ae8

4 files changed

Lines changed: 290 additions & 11 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
fix(metadata-protocol): the dialect arm of `classifyIndexFailure` walks `cause` to the same depth the conflict arm does (#6848)
6+
7+
`classifyIndexFailure` had two arms reading two different wrap-depths. #6699
8+
moved the first arm onto `@objectstack/types`' `isUniqueViolationError`, which
9+
follows `error.cause` four levels down because pool and query-builder layers
10+
re-throw with the original attached. The second — the dialect arm — kept reading
11+
`err.message` and stopping there.
12+
13+
So a dialect refusal arriving behind a wrapper (outer prose `Write failed` or
14+
`pool query failed`, the actual `near "WHERE": syntax error` one step down
15+
`cause`) was graded `failed` instead of `unsupported`. The private
16+
`indexFailureText` helper now collects the message channel of the thrown value
17+
**and** of each `cause` below it, bounded at the same `MAX_CAUSE_DEPTH` of 4 the
18+
predicate uses and counted the same way (the thrown value is depth 0). The
19+
dialect vocabulary itself is unchanged — only the text fed to it.
20+
21+
**Why the verdict matters beyond wording.** The two consumers dispose of
22+
`unsupported` and `failed` differently. `view-definition-active-index.ts` treats
23+
them the same (keep the previous index, report at `error`; only the wording
24+
differs). But `ensureOverlayStateIndex` builds the composite **fallback lookup
25+
index** on the `unsupported` branch and on no other — offered precisely because
26+
a dialect that cannot take the partial form should still get the lookup. Under
27+
a `failed` verdict that branch never ran, so `fallback` came back
28+
`not-attempted` rather than `ensured` / `refused` and the degradation target was
29+
silently never attempted.
30+
31+
**Dormant, not a live regression.** No driver shipped today produces the wrapped
32+
shape — each hands knex's error back with the dialect text on the outer message,
33+
which is why every existing case matched on the first read. This closes an
34+
asymmetry before a wrapping raw-SQL driver can land on it; it is also not a
35+
regression from #6699, which only made the contrast visible by deepening the
36+
first arm.
37+
38+
Two details worth knowing if you touch this: the collected levels are joined
39+
with a **newline**, never a space, because two of the dialect alternatives are
40+
multi-word (`where clause`, `near "where"`) and a space would let a phrase be
41+
synthesised across a wrapper boundary that no single driver wrote. And a looping
42+
`cause` chain is **bounded rather than detected** — no visited set — which is
43+
exactly what the predicate this mirrors does.
44+
45+
Arm order is unchanged and still load-bearing: a conflict reported anywhere in
46+
the chain still beats a dialect refusal in the outer prose.

packages/metadata-protocol/src/migrations/overlay-index.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,44 @@ describe('sys_metadata overlay uniqueness (#6418)', () => {
302302
expect(logger.info).not.toHaveBeenCalled();
303303
});
304304

305+
/**
306+
* #6848 — the same MariaDB refusal, behind a pooled wrapper.
307+
*
308+
* This is the consequence the classifier's wrap-depth actually decides, and
309+
* the reason the dialect arm was given the `cause` walk rather than a doc
310+
* comment. The fallback lookup index is built on the `unsupported` branch
311+
* and on no other, so while the second arm stopped at the outer message this
312+
* case graded `failed` and the degradation target was never attempted — the
313+
* dialect's answer was present, one step down, and unread.
314+
*/
315+
it('a POOLED wrapper around the dialect refusal still reaches the fallback index (#6848)', async () => {
316+
const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() };
317+
const pooled: IndexExec = async (sql: string) => {
318+
if (/where/i.test(sql)) {
319+
throw Object.assign(new Error('Write failed'), {
320+
cause: new Error(
321+
"You have an error in your SQL syntax; check the manual … near 'WHERE state = 'active''",
322+
),
323+
});
324+
}
325+
return db.exec(sql);
326+
};
327+
328+
const result = await ensureOverlayStateIndex(pooled, 'active', logger);
329+
330+
expect(result.status).toBe('unsupported');
331+
// The whole point: `not-attempted` here would mean the dialect that
332+
// cannot take the partial form silently got no lookup index either.
333+
expect(result.fallback).toBe('ensured');
334+
// `detail` stays the operator-facing OUTER prose, unchanged.
335+
expect(result.detail).toBe('Write failed');
336+
// Nothing was downgraded — the declared UNIQUE index is byte-for-byte there.
337+
expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL);
338+
expect(insert('w1', 'view', 'lead.w', 'org1', 'pkg1', 'active').ok).toBe(true);
339+
expect(insert('w2', 'view', 'lead.w', 'org1', 'pkg1', 'active').ok).toBe(false);
340+
expect(String(logger.error.mock.calls[0]![0])).toContain('NOT enforced as specified on this dialect');
341+
});
342+
305343
/**
306344
* MySQL proper, where the old code was safe only by ACCIDENT: it has
307345
* neither `DROP INDEX IF EXISTS` nor `CREATE INDEX IF NOT EXISTS`, so every

packages/metadata-protocol/src/migrations/partial-index-probe.test.ts

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,128 @@ describe('probe-first partial index replacement (#6418)', () => {
248248
expect(indexDdl(PROBE)).toBeUndefined();
249249
});
250250

251+
/* ────────────────────────────────────────────────────────────────────── *
252+
* #6848 — the DIALECT arm walks `cause` to the same depth as the first
253+
* ────────────────────────────────────────────────────────────────────── */
254+
255+
/** A neutral wrapper: matches NEITHER vocabulary, so it can only carry. */
256+
const wrap = (cause: unknown): Error => Object.assign(new Error('pool query failed'), { cause });
257+
258+
/** `leaf` behind `depth` neutral wrappers (`depth: 0` is the leaf itself). */
259+
const nest = (depth: number, leaf: unknown): unknown =>
260+
depth === 0 ? leaf : wrap(nest(depth - 1, leaf));
261+
262+
it('grades a WRAPPED dialect refusal `unsupported`, not `failed` (#6848)', () => {
263+
// The shape a pooled or query-builder layer produces: useless outer
264+
// prose, the dialect's actual answer one step down `cause`. Before this
265+
// the second arm stopped at the outer message and returned `failed` —
266+
// which costs `overlay-index` its fallback lookup index, because that
267+
// branch is reached on `unsupported` and nowhere else.
268+
const wrapped = Object.assign(new Error('Write failed'), {
269+
cause: new Error('near "WHERE": syntax error'),
270+
});
271+
expect(classifyIndexFailure(wrapped)).toBe('unsupported');
272+
// The control: the outer prose alone is still, correctly, `failed`.
273+
expect(classifyIndexFailure('Write failed')).toBe('failed');
274+
275+
// …and the same for the functional-key-parts refusal, one layer deeper
276+
// and on a plain object rather than an Error.
277+
expect(classifyIndexFailure({ message: 'Write failed', cause: { cause: { message: 'Functional index on a column is not supported' } } })).toBe(
278+
'unsupported',
279+
);
280+
});
281+
282+
it('keeps the data verdict ahead of the dialect verdict ACROSS the chain (#6848)', () => {
283+
// The inversion risk of widening the second arm: both arms now walk, so
284+
// the ordering has to hold at every depth, not just at the top. Outer
285+
// prose is a dialect refusal; the real condition is a conflict reported
286+
// on `code` two levels down. `conflict` must still win.
287+
const misleading = Object.assign(new Error('near "WHERE": syntax error'), {
288+
cause: wrap(Object.assign(new Error('insert failed'), { code: '23505' })),
289+
});
290+
expect(classifyIndexFailure(misleading)).toBe('conflict');
291+
});
292+
293+
it('reads the dialect arm to exactly the depth the conflict arm reads (#6848)', () => {
294+
// The card's whole point, pinned as a PARITY rather than as a number:
295+
// whatever depth `isUniqueViolationError` reaches, this module's dialect
296+
// arm reaches the same one. Expressed this way the assertion survives a
297+
// deliberate change to the shared bound and still goes red the moment
298+
// the two arms drift apart again.
299+
const DEPTHS = [0, 1, 2, 3, 4, 5, 6];
300+
const dialectReach = DEPTHS.map(
301+
(d) => classifyIndexFailure(nest(d, new Error('near "WHERE": syntax error'))) === 'unsupported',
302+
);
303+
const conflictReach = DEPTHS.map(
304+
(d) =>
305+
classifyIndexFailure(nest(d, Object.assign(new Error('insert failed'), { code: '23505' }))) ===
306+
'conflict',
307+
);
308+
309+
expect(dialectReach).toEqual(conflictReach);
310+
// …and the shared profile is the predicate's `MAX_CAUSE_DEPTH` of 4
311+
// counted from the thrown value, so the equality above cannot pass by
312+
// both arms reaching nothing (or everything).
313+
expect(dialectReach).toEqual([true, true, true, true, true, false, false]);
314+
});
315+
316+
it('joins the chain with a newline, so no phrase is synthesised across a wrapper (#6848)', () => {
317+
// Two of the dialect alternatives are multi-word. Neither message below
318+
// is a refusal on its own, and joining them with a SPACE would forge
319+
// `where clause` out of text no layer ever wrote.
320+
const spliced = Object.assign(new Error('rebuild attempt landed where'), {
321+
cause: new Error('clause parsing completed'),
322+
});
323+
expect(classifyIndexFailure('rebuild attempt landed where')).toBe('failed');
324+
expect(classifyIndexFailure('clause parsing completed')).toBe('failed');
325+
expect(classifyIndexFailure(spliced)).toBe('failed');
326+
});
327+
328+
it('terminates on a `cause` chain that loops, exactly as the predicate does (#6848)', () => {
329+
// `isUniqueViolationError` bounds rather than detects cycles — it keeps
330+
// no visited set — so this walk must not either, and the bound has to be
331+
// what stops both. A self-referential cause must return a verdict rather
332+
// than exhaust the stack.
333+
const loop = new Error('disk I/O error') as Error & { cause?: unknown };
334+
loop.cause = loop;
335+
expect(classifyIndexFailure(loop)).toBe('failed');
336+
337+
// …and a two-node cycle whose refusal is only on the INNER node is still
338+
// found, because the bound is reached after the answer, not before it.
339+
const outer = new Error('Write failed') as Error & { cause?: unknown };
340+
const inner = new Error('near "WHERE": syntax error') as Error & { cause?: unknown };
341+
outer.cause = inner;
342+
inner.cause = outer;
343+
expect(classifyIndexFailure(outer)).toBe('unsupported');
344+
});
345+
346+
it('the probe classifies a wrapped refusal, and leaves `detail` the OUTER prose (#6848)', async () => {
347+
// End-to-end through `probeThenReplaceIndex`: the verdict widens, the
348+
// operator-facing text does not. `detail` stays the driver's own outer
349+
// message, which is the contract `probeThenReplaceIndex` already had.
350+
const wrapping: IndexExec = async (sql: string) => {
351+
if (sql.startsWith('CREATE')) {
352+
throw Object.assign(new Error('Write failed'), {
353+
cause: new Error('near "WHERE": syntax error'),
354+
});
355+
}
356+
return db.exec(sql);
357+
};
358+
359+
const outcome = await probeThenReplaceIndex(wrapping, {
360+
indexName: REAL,
361+
probeIndexName: PROBE,
362+
buildSql,
363+
});
364+
365+
expect(outcome.status).toBe('unsupported');
366+
expect(outcome.failedAt).toBe('probe');
367+
expect(outcome.detail).toBe('Write failed');
368+
// The probe is what failed, so the previous index is untouched.
369+
expect(indexDdl(REAL)).toEqual(EXISTING_DDL);
370+
expect(indexDdl(PROBE)).toBeUndefined();
371+
});
372+
251373
it('logProblem prefers error(), falls back to warn(), and tolerates neither', () => {
252374
const full = { warn: vi.fn(), error: vi.fn() };
253375
logProblem(full, 'msg', 'detail');

packages/metadata-protocol/src/migrations/partial-index-probe.ts

Lines changed: 84 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -87,21 +87,70 @@ export type PartialIndexStatus =
8787
/** Anything else, best-effort. Previous index kept. */
8888
| 'failed';
8989

90+
/**
91+
* How far to follow an `error.cause` chain — deliberately the same bound as
92+
* `isUniqueViolationError`'s `MAX_CAUSE_DEPTH` in `@objectstack/types` (#6848).
93+
*
94+
* Counted the same way too: the thrown value itself is depth 0, so this admits
95+
* the outer error plus four wrapper levels below it. The two arms of {@link
96+
* classifyIndexFailure} reading the SAME depth is the whole point — see that
97+
* function's "Why both arms walk" note.
98+
*
99+
* It is also the only cycle guard, again matching the predicate: a `cause`
100+
* chain that loops back on itself is bounded rather than detected, because a
101+
* bound terminates a cycle just as well as a visited-set does and the predicate
102+
* this mirrors has no visited-set to mirror.
103+
*/
104+
const MAX_CAUSE_DEPTH = 4;
105+
106+
/**
107+
* Every message channel on one thrown value and its `cause` chain, in order.
108+
*
109+
* Deliberately a local walk. `@objectstack/types` owns the *conflict* question
110+
* and exports a predicate for it, but it exposes no reusable message-collecting
111+
* helper — its own chain walkers (`matchesUniqueViolation`,
112+
* `findUniqueViolationColumn`) are private and each answers its own question
113+
* rather than handing back text. Hoisting a shared collector there would widen
114+
* that package's contract for a single consumer, so this stays here and stays
115+
* pinned to the bound above.
116+
*/
117+
function collectIndexFailureText(error: unknown, depth: number, into: string[]): void {
118+
if (error === null || error === undefined || depth > MAX_CAUSE_DEPTH) return;
119+
if (typeof error === 'string') {
120+
into.push(error);
121+
return;
122+
}
123+
if (typeof error !== 'object') {
124+
into.push(String(error));
125+
return;
126+
}
127+
const err = error as { message?: unknown; cause?: unknown };
128+
if (typeof err.message === 'string') into.push(err.message);
129+
collectIndexFailureText(err.cause, depth + 1, into);
130+
}
131+
90132
/**
91133
* The text the DIALECT arm judges, from a thrown value of any shape.
92134
*
93-
* `message` first, because that is the channel a driver writes its refusal on
94-
* and the only one this arm has ever read; `String()` only as the last resort
95-
* — which is what a bare string resolves to unchanged, so a caller holding
96-
* nothing but prose is judged exactly as before.
135+
* `message` first, because that is the channel a driver writes its refusal on;
136+
* then the same channel one step at a time down `cause`, because pool and
137+
* query-builder layers re-throw with the original attached and the refusal is
138+
* then the ONLY copy of the dialect's answer (#6848). `String()` only as the
139+
* last resort — which is what a bare string resolves to unchanged, so a caller
140+
* holding nothing but prose is judged exactly as before.
141+
*
142+
* ⚠️ The levels are joined with a NEWLINE, never a space. Two of the dialect
143+
* vocabulary's alternatives are multi-word (`where clause`, `near "where"`), so
144+
* a space would let a phrase be synthesised across a wrapper boundary that no
145+
* single driver ever wrote — an outer message ending in `where` above a cause
146+
* beginning with `clause` would read as a dialect refusal. A newline cannot
147+
* match the literal space in those alternatives, so each level is still judged
148+
* on text some layer actually emitted.
97149
*/
98150
function indexFailureText(error: unknown): string {
99-
if (typeof error === 'string') return error;
100-
if (typeof error === 'object' && error !== null) {
101-
const { message } = error as { message?: unknown };
102-
if (typeof message === 'string') return message;
103-
}
104-
return String(error);
151+
const texts: string[] = [];
152+
collectIndexFailureText(error, 0, texts);
153+
return texts.length > 0 ? texts.join('\n') : String(error);
105154
}
106155

107156
/**
@@ -138,10 +187,34 @@ function indexFailureText(error: unknown): string {
138187
* The predicate answers the FIRST arm only. It has no opinion about dialect
139188
* support, so the second arm stays this module's own — and stays second.
140189
*
190+
* ## Why both arms walk `cause` (#6848)
191+
*
192+
* They read the same depth because they are asked the same way. #6699 gave the
193+
* first arm the shared predicate's four-level `cause` walk and left the second
194+
* on the outer message alone; the two then disagreed about how deeply a driver
195+
* is allowed to wrap. A dialect refusal arriving behind a pooled wrapper —
196+
* outer prose `Write failed`, the real `near "WHERE": syntax error` one step
197+
* down — was graded `failed` rather than `unsupported`.
198+
*
199+
* That gap is **not** a wording difference, which is why it was worth closing
200+
* rather than documenting. `view-definition-active-index.ts` disposes of the
201+
* two verdicts identically (keep the previous index, report at `error`), but
202+
* `overlay-index.ts` builds the composite **fallback lookup index** on
203+
* `unsupported` and only there — offered precisely because a dialect that
204+
* cannot take the partial form should still get the lookup. Under a `failed`
205+
* verdict that branch never runs and the fallback is reported `not-attempted`
206+
* instead of `ensured` / `refused`, so the wrap depth silently decides whether
207+
* the degradation target is built at all.
208+
*
209+
* No driver shipped today produces that shape — every one hands knex's error
210+
* back with the dialect text on the outer message, which is why every case here
211+
* matched on the first read. This closes a dormant asymmetry, not a live defect.
212+
*
141213
* ⚠️ Pass the **error**, not `err.message`. A string still works (the predicate
142214
* reads it on the message channel, and so does {@link indexFailureText}), but a
143215
* caller that unwraps first throws away the `code` / `errno` / `cause` channels
144-
* that are the whole reason this reads the object.
216+
* that are the whole reason this reads the object — and, since #6848, the
217+
* dialect answer too when a wrapper holds the useless half.
145218
*/
146219
export function classifyIndexFailure(error: unknown): PartialIndexStatus {
147220
if (isUniqueViolationError(error)) {

0 commit comments

Comments
 (0)