Skip to content

Commit ebcc34e

Browse files
claude[bot]claude
andauthored
fix(driver-sql): name the real remedy when a bounded field sits over a stale TEXT column (#13073)
`explainUnkeyableTextColumn` rendered every ER_BLOB_KEY_WITHOUT_LENGTH / ER_TOO_LONG_KEY index refusal as "the field declares no `maxLength` — declare one". True at CREATE time; false in both halves on the UPGRADE path, where the additive sync never rewrites a column's type, so a field that HAS since declared a usable bound still sits over a TEXT column and the operator is told to redo what they already did — once per boot, in production. Adds a second branch selected per column on "physical column is TEXT AND keyableTextLength() would have emitted varchar(n)", using the columnInfo() read this method already performs and the driver's managedObjectFields. It names the declared bound, says re-declaring changes nothing, and gives the manual remedy in full: convert by hand, backup first, restate the FULL column definition on MySQL (MODIFY does not repeat NOT NULL and drops a DEFAULT it does not restate), then let the next boot create the index. The CREATE-path message is byte-identical, a bound past the 768-character key ceiling deliberately keeps it, the refusal stays loud, and the sync still never rewrites the column (that ALTER needs an exclusive metadata lock). Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry Co-authored-by: Claude <noreply@anthropic.com>
1 parent e25e839 commit ebcc34e

3 files changed

Lines changed: 398 additions & 2 deletions

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): an unkeyable TEXT column whose field ALREADY declares a bound now names the real remedy (#12999)
6+
7+
One message served two causes and was true of only one of them.
8+
9+
`explainUnkeyableTextColumn` turns MySQL's `ER_BLOB_KEY_WITHOUT_LENGTH` /
10+
`ER_TOO_LONG_KEY` index refusal into operator-readable advice. It rendered
11+
every such refusal as *"the field declares no `maxLength` … declare
12+
`maxLength` on the field(s)"*. That is correct at CREATE time. On the UPGRADE
13+
path both halves are false: the additive sync adds columns and indexes and
14+
deliberately never rewrites a column's type (#3728), so once a release adds a
15+
bound to a previously unbounded keyed field (#12978 did exactly that for five
16+
`sys_notification_*` objects), the field declares a perfectly usable
17+
`maxLength` while the physical column is still TEXT. The index is refused
18+
again on every boot and the message tells the operator to do the thing they
19+
already did — in production, once per boot, which reads as the release that
20+
shipped the fix being broken.
21+
22+
**What changed.** A second branch, selected per column on a criterion that
23+
needs both halves: the physical column is TEXT *and* `keyableTextLength` says
24+
a fresh create would have emitted `varchar(n)` for the field's declared bound.
25+
Both inputs were already in hand on the failure path — the `columnInfo()` read
26+
this method already performs, and the driver's `managedObjectFields`
27+
registration. That message names the column, the bound it already declares,
28+
that re-declaring changes nothing, and the remedy that does apply: convert the
29+
column to `varchar(n)` **by hand, with a backup taken first**, restating the
30+
FULL column definition on MySQL — `MODIFY` does not repeat a `NOT NULL` and
31+
silently drops a `DEFAULT` it does not restate — after which the next boot
32+
creates the index. A composite key that mixes a stale column with a genuinely
33+
unbounded one names both dispositions rather than sending the operator down
34+
one route for both.
35+
36+
**What deliberately did not change.**
37+
38+
- The CREATE-path message is **byte-identical**, and is what a field that
39+
really declares no usable bound still gets. A declared bound *wider* than a
40+
utf8mb4 key part can hold (768 characters) is not a stale column either — a
41+
fresh create emits TEXT for it too — so it keeps the CREATE message, whose
42+
768-character ceiling is the fact that operator needs.
43+
- The refusal stays **loud and stays a failure**. The index genuinely was not
44+
created and a declared uniqueness is genuinely unenforced; naming a better
45+
remedy is not a reason to downgrade or silence that.
46+
- The additive sync still does **not** rewrite the column itself. A widening
47+
`ALTER … MODIFY` takes an exclusive metadata lock on the table, which makes
48+
it a destructive, hard-to-roll-back action and a deliberate manual floor
49+
rather than something a boot may decide to do.
50+
51+
Diagnostic text only: no schema, DDL, wire or API surface moves.
Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #12999 — the refusal message for an unkeyable TEXT column has TWO causes, and
5+
* for one of them the advice it gave was a no-op.
6+
*
7+
* ## The defect
8+
*
9+
* `explainUnkeyableTextColumn` rendered every `ER_BLOB_KEY_WITHOUT_LENGTH` /
10+
* `ER_TOO_LONG_KEY` index refusal as "the field declares no `maxLength` …
11+
* declare `maxLength` on the field(s)". True at CREATE time. False on the
12+
* UPGRADE path, in both halves: once a release adds the bound (#12978 did
13+
* exactly that for five `sys_notification_*` objects), the field DOES declare
14+
* one — but the additive sync never rewrites a column's type, so the physical
15+
* column stays TEXT, the index is refused again on every boot, and the message
16+
* tells the operator to do the thing they already did. In production that reads
17+
* as the fix they just deployed being broken.
18+
*
19+
* ## Why the pins run HERE, on SQLite, and what that does NOT cover
20+
*
21+
* The branch is a message-rendering decision over two inputs the driver already
22+
* holds — the physical column type (`columnInfo()`) and the declared field
23+
* (`managedObjectFields`) — so it is dialect-independent; only the error CODE
24+
* that triggers it is MySQL's, and it is supplied here as the `cause` the real
25+
* call site passes through verbatim. What SQLite gives that a stub could not is
26+
* the FIXTURE: the stale column is produced by really booting the object twice,
27+
* unbounded then bounded, so the "additive sync never rewrites the column"
28+
* premise the whole card rests on is measured rather than assumed.
29+
*
30+
* ⚠️ Not covered here: the end-to-end MySQL boot in which the server itself
31+
* raises the refusal. The live cells in `sql-driver-keyed-text-mysql.test.ts`
32+
* own that path, and they pin the CREATE-path message — which this change must
33+
* leave byte-identical, and which the counter-pins below assert directly.
34+
*
35+
* ## Both directions, deliberately
36+
*
37+
* A suite that asserted only the new branch would stay green through a
38+
* regression that broke the CREATE message — the message that is still correct
39+
* for every deployment that never had the column. So each new-branch assertion
40+
* has a counter-pin: unbounded field, and bound past the key ceiling.
41+
*/
42+
43+
import { describe, it, expect, afterEach } from 'vitest';
44+
import { SqlDriver } from '../src/index.js';
45+
import { dialectCell } from './live-dialect-matrix.testkit.js';
46+
47+
/** The `cause` the real call site forwards — the server's own refusal object. */
48+
const BLOB_KEY_REFUSAL = {
49+
code: 'ER_BLOB_KEY_WITHOUT_LENGTH',
50+
message: "BLOB/TEXT column 'token' used in key specification without a key length",
51+
};
52+
53+
const TABLE = 'os12999_upgraded';
54+
const INDEX = 'idx_os12999_upgraded_token';
55+
56+
/** The release that shipped BEFORE anyone declared a bound. */
57+
const beforeTheBound = () => ({
58+
name: TABLE,
59+
fields: { token: { type: 'text' } },
60+
});
61+
62+
/** The release that adds it — the #12978 shape, and the one that must not lie. */
63+
const afterTheBound = () => ({
64+
name: TABLE,
65+
fields: { token: { type: 'text', maxLength: 64 } },
66+
indexes: [{ fields: ['token'], unique: false }],
67+
});
68+
69+
/** Never upgraded: the field genuinely declares nothing. The CREATE path. */
70+
const NEVER_BOUND_TABLE = 'os12999_never_bound';
71+
const neverBound = () => ({
72+
name: NEVER_BOUND_TABLE,
73+
fields: { token: { type: 'text' } },
74+
indexes: [{ fields: ['token'], unique: false }],
75+
});
76+
77+
/**
78+
* Bounded, but WIDER than a utf8mb4 key part can hold. ⚠️ The false-positive
79+
* this branch has to avoid: the field declares a `maxLength` and the column is
80+
* TEXT, yet the column is NOT stale — a fresh create emits TEXT for it too, and
81+
* converting it to `varchar(1024)` by hand would only trade
82+
* `ER_BLOB_KEY_WITHOUT_LENGTH` for `ER_TOO_LONG_KEY`. The 768-character ceiling
83+
* is what this operator needs to read, so this case keeps the CREATE message.
84+
*/
85+
const TOO_WIDE_TABLE = 'os12999_too_wide';
86+
const boundPastTheCeiling = () => ({
87+
name: TOO_WIDE_TABLE,
88+
fields: { token: { type: 'text', maxLength: 1024 } },
89+
indexes: [{ fields: ['token'], unique: false }],
90+
});
91+
92+
/** One stale column and one genuinely unbounded column in the SAME key. */
93+
const MIXED_TABLE = 'os12999_mixed';
94+
const mixedBefore = () => ({
95+
name: MIXED_TABLE,
96+
fields: { slug: { type: 'text' }, note: { type: 'text' } },
97+
});
98+
const mixedAfter = () => ({
99+
name: MIXED_TABLE,
100+
fields: { slug: { type: 'text', maxLength: 64 }, note: { type: 'text' } },
101+
indexes: [{ fields: ['slug', 'note'], unique: false }],
102+
});
103+
104+
const explain = (driver: SqlDriver, table: string, columns: string[], index = INDEX) =>
105+
(driver as any).explainUnkeyableTextColumn(table, index, columns, BLOB_KEY_REFUSAL) as Promise<
106+
string | null
107+
>;
108+
109+
const columnType = async (driver: SqlDriver, table: string, column: string) => {
110+
const info: Record<string, { type?: string }> = await (driver as any).knex(table).columnInfo();
111+
return String(info[column]?.type ?? '').toLowerCase();
112+
};
113+
114+
describe('unkeyable TEXT column: the upgrade path names the real remedy (#12999)', () => {
115+
let driver: SqlDriver;
116+
117+
afterEach(async () => {
118+
await driver?.disconnect().catch(() => {});
119+
});
120+
121+
it('produces the stale-column remedy for a bounded field over a TEXT column', async () => {
122+
driver = new SqlDriver(dialectCell('sqlite').config());
123+
124+
// The upgrade, performed rather than described: boot the old release, then
125+
// the new one on the same database.
126+
await driver.initObjects([beforeTheBound()]);
127+
expect(await columnType(driver, TABLE, 'token')).toBe('text');
128+
await driver.initObjects([afterTheBound()]);
129+
130+
// ⭐ The premise the whole card rests on, measured: the bound is declared
131+
// and the physical column is STILL TEXT. If the additive sync ever starts
132+
// rewriting the column, this assertion is the one that should fail first.
133+
expect(await columnType(driver, TABLE, 'token')).toBe('text');
134+
expect((driver as any).declaredFieldsFor(TABLE).token.maxLength).toBe(64);
135+
136+
const message = (await explain(driver, TABLE, ['token'])) ?? '';
137+
138+
// Names the column, the bound it already declares, and that re-declaring is
139+
// not the fix — the sentence whose absence is the reported defect.
140+
expect(message).toContain('"token"');
141+
expect(message).toContain('maxLength: 64');
142+
expect(message).toMatch(/ALREADY declares a usable `maxLength`/);
143+
expect(message).toMatch(/re-declaring `maxLength` changes nothing/);
144+
145+
// The remedy, in full. Each clause is separately load-bearing: an operator
146+
// who converts the column without restating NOT NULL / DEFAULT on MySQL
147+
// ends up WORSE off than the no-op, having silently dropped the default.
148+
expect(message).toMatch(/backup taken first/);
149+
expect(message).toMatch(/restating the FULL column definition on MySQL/);
150+
expect(message).toMatch(/MODIFY drops a NOT NULL or DEFAULT you do not repeat/);
151+
expect(message).toContain('varchar(64)');
152+
expect(message).toMatch(/next boot create this index/);
153+
154+
// ⛔ And it does NOT quietly become an instruction the driver will carry out
155+
// itself: the rewrite needs an exclusive metadata lock, so it stays manual.
156+
expect(message).toMatch(/does NOT rewrite the column for you/);
157+
expect(message).toMatch(/exclusive metadata lock/);
158+
159+
// ⛔ The CREATE-path advice must be GONE from this message — its presence is
160+
// the misdirection being fixed.
161+
expect(message).not.toMatch(/declares no `maxLength`/);
162+
expect(message).not.toMatch(/Declare `maxLength` on the field\(s\)/);
163+
});
164+
165+
it('keeps the refusal loud — the index is still absent and said to be', async () => {
166+
driver = new SqlDriver(dialectCell('sqlite').config());
167+
await driver.initObjects([beforeTheBound()]);
168+
await driver.initObjects([afterTheBound()]);
169+
170+
const message = (await explain(driver, TABLE, ['token'])) ?? '';
171+
172+
// ⛔ The card's hard fence: naming a better remedy must not soften the
173+
// report. The index genuinely was not created, and a declared uniqueness
174+
// that is not enforced is a real durability degradation.
175+
expect(message).toMatch(/^\[sql-driver\] cannot create index '.+' on ".+"/);
176+
expect(message).toMatch(/The table exists but this index does NOT/);
177+
expect(message).toMatch(/currently unenforced/);
178+
// The anti-workaround note survives too: a prefix index is still refused.
179+
expect(message).toMatch(/prefix index is deliberately not substituted/);
180+
});
181+
182+
// ── counter-pins: the CREATE path must be untouched ──────────────────────
183+
184+
it('COUNTER-PIN: an unbounded field still gets the original declare-maxLength message', async () => {
185+
driver = new SqlDriver(dialectCell('sqlite').config());
186+
await driver.initObjects([neverBound()]);
187+
expect(await columnType(driver, NEVER_BOUND_TABLE, 'token')).toBe('text');
188+
189+
const message = (await explain(driver, NEVER_BOUND_TABLE, ['token'])) ?? '';
190+
191+
expect(message).toMatch(/Column\(s\) "token" are stored as TEXT because the field declares no `maxLength`/);
192+
expect(message).toMatch(/Declare `maxLength` on the field\(s\) so the column is emitted as varchar\(n\)/);
193+
expect(message).toContain('#11374');
194+
// ⛔ The new branch must not reach this deployment: nothing here is stale.
195+
expect(message).not.toMatch(/ALREADY declares/);
196+
expect(message).not.toContain('#12999');
197+
});
198+
199+
it('COUNTER-PIN: a bound past the 768-character key ceiling is NOT a stale column', async () => {
200+
driver = new SqlDriver(dialectCell('sqlite').config());
201+
await driver.initObjects([boundPastTheCeiling()]);
202+
// A fresh create emits TEXT here too — so the column is current, not stale,
203+
// and hand-converting it to varchar(1024) would fix nothing.
204+
expect(await columnType(driver, TOO_WIDE_TABLE, 'token')).toBe('text');
205+
206+
const message = (await explain(driver, TOO_WIDE_TABLE, ['token'])) ?? '';
207+
208+
expect(message).toMatch(/wider than 768 characters/);
209+
expect(message).not.toMatch(/ALREADY declares/);
210+
expect(message).not.toContain('#12999');
211+
});
212+
213+
it('COUNTER-PIN: a table this driver holds no declaration for keeps the CREATE message', async () => {
214+
driver = new SqlDriver(dialectCell('sqlite').config());
215+
await driver.initObjects([neverBound()]);
216+
217+
// Never registered here (ADR-0015 external/federated objects land this way,
218+
// and so does a shard table, registered under its BASE name): the fields are
219+
// unavailable, so the branch must degrade rather than guess.
220+
expect((driver as any).declaredFieldsFor('os12999_unregistered')).toBeUndefined();
221+
const message = (await explain(driver, 'os12999_unregistered', ['token'])) ?? '';
222+
223+
expect(message).toMatch(/Declare `maxLength` on the field\(s\)/);
224+
expect(message).not.toContain('#12999');
225+
});
226+
227+
it('names BOTH dispositions when one key column is stale and another is unbounded', async () => {
228+
driver = new SqlDriver(dialectCell('sqlite').config());
229+
await driver.initObjects([mixedBefore()]);
230+
await driver.initObjects([mixedAfter()]);
231+
expect(await columnType(driver, MIXED_TABLE, 'slug')).toBe('text');
232+
expect(await columnType(driver, MIXED_TABLE, 'note')).toBe('text');
233+
234+
const message =
235+
(await explain(driver, MIXED_TABLE, ['slug', 'note'], 'idx_os12999_mixed_slug_note')) ?? '';
236+
237+
// The stale half gets the conversion remedy…
238+
expect(message).toMatch(/"slug" \(declares `maxLength: 64`\)/);
239+
expect(message).toMatch(/restating the FULL column definition on MySQL/);
240+
// …and the genuinely unbounded half is still told to declare a bound, so a
241+
// composite key does not send the operator down one route for both columns.
242+
expect(message).toMatch(/Column\(s\) "note" in the same key declare no usable bound and DO need `maxLength`/);
243+
expect(message).toContain('#11374');
244+
});
245+
246+
it('still declines to explain a failure that is not the TEXT-key refusal', async () => {
247+
driver = new SqlDriver(dialectCell('sqlite').config());
248+
await driver.initObjects([beforeTheBound()]);
249+
await driver.initObjects([afterTheBound()]);
250+
251+
// Unchanged gate: this helper speaks only for the two MySQL codes, and the
252+
// new branch sits behind that same gate rather than beside it.
253+
const other = await (driver as any).explainUnkeyableTextColumn(TABLE, INDEX, ['token'], {
254+
code: 'ER_DUP_ENTRY',
255+
});
256+
expect(other).toBeNull();
257+
});
258+
});

0 commit comments

Comments
 (0)