From efb97604ee236c52d6fc842a73020542d7b08718 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 02:32:32 +0000 Subject: [PATCH] fix(driver-sql): name the real remedy when a bounded field sits over a stale TEXT column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .../driver-sql-stale-text-column-remedy.md | 51 ++++ ...sql-driver-12999-stale-text-remedy.test.ts | 258 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 91 +++++- 3 files changed, 398 insertions(+), 2 deletions(-) create mode 100644 .changeset/driver-sql-stale-text-column-remedy.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-12999-stale-text-remedy.test.ts diff --git a/.changeset/driver-sql-stale-text-column-remedy.md b/.changeset/driver-sql-stale-text-column-remedy.md new file mode 100644 index 0000000000..c29e4d6566 --- /dev/null +++ b/.changeset/driver-sql-stale-text-column-remedy.md @@ -0,0 +1,51 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): an unkeyable TEXT column whose field ALREADY declares a bound now names the real remedy (#12999) + +One message served two causes and was true of only one of them. + +`explainUnkeyableTextColumn` turns MySQL's `ER_BLOB_KEY_WITHOUT_LENGTH` / +`ER_TOO_LONG_KEY` index refusal into operator-readable advice. It rendered +every such refusal as *"the field declares no `maxLength` … declare +`maxLength` on the field(s)"*. That is correct at CREATE time. On the UPGRADE +path both halves are false: the additive sync adds columns and indexes and +deliberately never rewrites a column's type (#3728), so once a release adds a +bound to a previously unbounded keyed field (#12978 did exactly that for five +`sys_notification_*` objects), the field declares a perfectly usable +`maxLength` while the physical column is still TEXT. The index is refused +again on every boot and the message tells the operator to do the thing they +already did — in production, once per boot, which reads as the release that +shipped the fix being broken. + +**What changed.** A second branch, selected per column on a criterion that +needs both halves: the physical column is TEXT *and* `keyableTextLength` says +a fresh create would have emitted `varchar(n)` for the field's declared bound. +Both inputs were already in hand on the failure path — the `columnInfo()` read +this method already performs, and the driver's `managedObjectFields` +registration. That message names the column, the bound it already declares, +that re-declaring changes nothing, and the remedy that does apply: convert the +column to `varchar(n)` **by hand, with a backup taken first**, restating the +FULL column definition on MySQL — `MODIFY` does not repeat a `NOT NULL` and +silently drops a `DEFAULT` it does not restate — after which the next boot +creates the index. A composite key that mixes a stale column with a genuinely +unbounded one names both dispositions rather than sending the operator down +one route for both. + +**What deliberately did not change.** + +- The CREATE-path message is **byte-identical**, and is what a field that + really declares no usable bound still gets. A declared bound *wider* than a + utf8mb4 key part can hold (768 characters) is not a stale column either — a + fresh create emits TEXT for it too — so it keeps the CREATE message, whose + 768-character ceiling is the fact that operator needs. +- The refusal stays **loud and stays a failure**. The index genuinely was not + created and a declared uniqueness is genuinely unenforced; naming a better + remedy is not a reason to downgrade or silence that. +- The additive sync still does **not** rewrite the column itself. A widening + `ALTER … MODIFY` takes an exclusive metadata lock on the table, which makes + it a destructive, hard-to-roll-back action and a deliberate manual floor + rather than something a boot may decide to do. + +Diagnostic text only: no schema, DDL, wire or API surface moves. diff --git a/packages/drivers/driver-sql/src/sql-driver-12999-stale-text-remedy.test.ts b/packages/drivers/driver-sql/src/sql-driver-12999-stale-text-remedy.test.ts new file mode 100644 index 0000000000..c549b3c352 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-12999-stale-text-remedy.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12999 — the refusal message for an unkeyable TEXT column has TWO causes, and + * for one of them the advice it gave was a no-op. + * + * ## The defect + * + * `explainUnkeyableTextColumn` rendered every `ER_BLOB_KEY_WITHOUT_LENGTH` / + * `ER_TOO_LONG_KEY` index refusal as "the field declares no `maxLength` … + * declare `maxLength` on the field(s)". True at CREATE time. False on the + * UPGRADE path, in both halves: once a release adds the bound (#12978 did + * exactly that for five `sys_notification_*` objects), the field DOES declare + * one — but the additive sync never rewrites a column's type, so the physical + * column stays TEXT, the index is refused again on every boot, and the message + * tells the operator to do the thing they already did. In production that reads + * as the fix they just deployed being broken. + * + * ## Why the pins run HERE, on SQLite, and what that does NOT cover + * + * The branch is a message-rendering decision over two inputs the driver already + * holds — the physical column type (`columnInfo()`) and the declared field + * (`managedObjectFields`) — so it is dialect-independent; only the error CODE + * that triggers it is MySQL's, and it is supplied here as the `cause` the real + * call site passes through verbatim. What SQLite gives that a stub could not is + * the FIXTURE: the stale column is produced by really booting the object twice, + * unbounded then bounded, so the "additive sync never rewrites the column" + * premise the whole card rests on is measured rather than assumed. + * + * ⚠️ Not covered here: the end-to-end MySQL boot in which the server itself + * raises the refusal. The live cells in `sql-driver-keyed-text-mysql.test.ts` + * own that path, and they pin the CREATE-path message — which this change must + * leave byte-identical, and which the counter-pins below assert directly. + * + * ## Both directions, deliberately + * + * A suite that asserted only the new branch would stay green through a + * regression that broke the CREATE message — the message that is still correct + * for every deployment that never had the column. So each new-branch assertion + * has a counter-pin: unbounded field, and bound past the key ceiling. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { dialectCell } from './live-dialect-matrix.testkit.js'; + +/** The `cause` the real call site forwards — the server's own refusal object. */ +const BLOB_KEY_REFUSAL = { + code: 'ER_BLOB_KEY_WITHOUT_LENGTH', + message: "BLOB/TEXT column 'token' used in key specification without a key length", +}; + +const TABLE = 'os12999_upgraded'; +const INDEX = 'idx_os12999_upgraded_token'; + +/** The release that shipped BEFORE anyone declared a bound. */ +const beforeTheBound = () => ({ + name: TABLE, + fields: { token: { type: 'text' } }, +}); + +/** The release that adds it — the #12978 shape, and the one that must not lie. */ +const afterTheBound = () => ({ + name: TABLE, + fields: { token: { type: 'text', maxLength: 64 } }, + indexes: [{ fields: ['token'], unique: false }], +}); + +/** Never upgraded: the field genuinely declares nothing. The CREATE path. */ +const NEVER_BOUND_TABLE = 'os12999_never_bound'; +const neverBound = () => ({ + name: NEVER_BOUND_TABLE, + fields: { token: { type: 'text' } }, + indexes: [{ fields: ['token'], unique: false }], +}); + +/** + * Bounded, but WIDER than a utf8mb4 key part can hold. ⚠️ The false-positive + * this branch has to avoid: the field declares a `maxLength` and the column is + * TEXT, yet the column is NOT stale — a fresh create emits TEXT for it too, and + * converting it to `varchar(1024)` by hand would only trade + * `ER_BLOB_KEY_WITHOUT_LENGTH` for `ER_TOO_LONG_KEY`. The 768-character ceiling + * is what this operator needs to read, so this case keeps the CREATE message. + */ +const TOO_WIDE_TABLE = 'os12999_too_wide'; +const boundPastTheCeiling = () => ({ + name: TOO_WIDE_TABLE, + fields: { token: { type: 'text', maxLength: 1024 } }, + indexes: [{ fields: ['token'], unique: false }], +}); + +/** One stale column and one genuinely unbounded column in the SAME key. */ +const MIXED_TABLE = 'os12999_mixed'; +const mixedBefore = () => ({ + name: MIXED_TABLE, + fields: { slug: { type: 'text' }, note: { type: 'text' } }, +}); +const mixedAfter = () => ({ + name: MIXED_TABLE, + fields: { slug: { type: 'text', maxLength: 64 }, note: { type: 'text' } }, + indexes: [{ fields: ['slug', 'note'], unique: false }], +}); + +const explain = (driver: SqlDriver, table: string, columns: string[], index = INDEX) => + (driver as any).explainUnkeyableTextColumn(table, index, columns, BLOB_KEY_REFUSAL) as Promise< + string | null + >; + +const columnType = async (driver: SqlDriver, table: string, column: string) => { + const info: Record = await (driver as any).knex(table).columnInfo(); + return String(info[column]?.type ?? '').toLowerCase(); +}; + +describe('unkeyable TEXT column: the upgrade path names the real remedy (#12999)', () => { + let driver: SqlDriver; + + afterEach(async () => { + await driver?.disconnect().catch(() => {}); + }); + + it('produces the stale-column remedy for a bounded field over a TEXT column', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + + // The upgrade, performed rather than described: boot the old release, then + // the new one on the same database. + await driver.initObjects([beforeTheBound()]); + expect(await columnType(driver, TABLE, 'token')).toBe('text'); + await driver.initObjects([afterTheBound()]); + + // ⭐ The premise the whole card rests on, measured: the bound is declared + // and the physical column is STILL TEXT. If the additive sync ever starts + // rewriting the column, this assertion is the one that should fail first. + expect(await columnType(driver, TABLE, 'token')).toBe('text'); + expect((driver as any).declaredFieldsFor(TABLE).token.maxLength).toBe(64); + + const message = (await explain(driver, TABLE, ['token'])) ?? ''; + + // Names the column, the bound it already declares, and that re-declaring is + // not the fix — the sentence whose absence is the reported defect. + expect(message).toContain('"token"'); + expect(message).toContain('maxLength: 64'); + expect(message).toMatch(/ALREADY declares a usable `maxLength`/); + expect(message).toMatch(/re-declaring `maxLength` changes nothing/); + + // The remedy, in full. Each clause is separately load-bearing: an operator + // who converts the column without restating NOT NULL / DEFAULT on MySQL + // ends up WORSE off than the no-op, having silently dropped the default. + expect(message).toMatch(/backup taken first/); + expect(message).toMatch(/restating the FULL column definition on MySQL/); + expect(message).toMatch(/MODIFY drops a NOT NULL or DEFAULT you do not repeat/); + expect(message).toContain('varchar(64)'); + expect(message).toMatch(/next boot create this index/); + + // ⛔ And it does NOT quietly become an instruction the driver will carry out + // itself: the rewrite needs an exclusive metadata lock, so it stays manual. + expect(message).toMatch(/does NOT rewrite the column for you/); + expect(message).toMatch(/exclusive metadata lock/); + + // ⛔ The CREATE-path advice must be GONE from this message — its presence is + // the misdirection being fixed. + expect(message).not.toMatch(/declares no `maxLength`/); + expect(message).not.toMatch(/Declare `maxLength` on the field\(s\)/); + }); + + it('keeps the refusal loud — the index is still absent and said to be', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + await driver.initObjects([beforeTheBound()]); + await driver.initObjects([afterTheBound()]); + + const message = (await explain(driver, TABLE, ['token'])) ?? ''; + + // ⛔ The card's hard fence: naming a better remedy must not soften the + // report. The index genuinely was not created, and a declared uniqueness + // that is not enforced is a real durability degradation. + expect(message).toMatch(/^\[sql-driver\] cannot create index '.+' on ".+"/); + expect(message).toMatch(/The table exists but this index does NOT/); + expect(message).toMatch(/currently unenforced/); + // The anti-workaround note survives too: a prefix index is still refused. + expect(message).toMatch(/prefix index is deliberately not substituted/); + }); + + // ── counter-pins: the CREATE path must be untouched ────────────────────── + + it('COUNTER-PIN: an unbounded field still gets the original declare-maxLength message', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + await driver.initObjects([neverBound()]); + expect(await columnType(driver, NEVER_BOUND_TABLE, 'token')).toBe('text'); + + const message = (await explain(driver, NEVER_BOUND_TABLE, ['token'])) ?? ''; + + expect(message).toMatch(/Column\(s\) "token" are stored as TEXT because the field declares no `maxLength`/); + expect(message).toMatch(/Declare `maxLength` on the field\(s\) so the column is emitted as varchar\(n\)/); + expect(message).toContain('#11374'); + // ⛔ The new branch must not reach this deployment: nothing here is stale. + expect(message).not.toMatch(/ALREADY declares/); + expect(message).not.toContain('#12999'); + }); + + it('COUNTER-PIN: a bound past the 768-character key ceiling is NOT a stale column', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + await driver.initObjects([boundPastTheCeiling()]); + // A fresh create emits TEXT here too — so the column is current, not stale, + // and hand-converting it to varchar(1024) would fix nothing. + expect(await columnType(driver, TOO_WIDE_TABLE, 'token')).toBe('text'); + + const message = (await explain(driver, TOO_WIDE_TABLE, ['token'])) ?? ''; + + expect(message).toMatch(/wider than 768 characters/); + expect(message).not.toMatch(/ALREADY declares/); + expect(message).not.toContain('#12999'); + }); + + it('COUNTER-PIN: a table this driver holds no declaration for keeps the CREATE message', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + await driver.initObjects([neverBound()]); + + // Never registered here (ADR-0015 external/federated objects land this way, + // and so does a shard table, registered under its BASE name): the fields are + // unavailable, so the branch must degrade rather than guess. + expect((driver as any).declaredFieldsFor('os12999_unregistered')).toBeUndefined(); + const message = (await explain(driver, 'os12999_unregistered', ['token'])) ?? ''; + + expect(message).toMatch(/Declare `maxLength` on the field\(s\)/); + expect(message).not.toContain('#12999'); + }); + + it('names BOTH dispositions when one key column is stale and another is unbounded', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + await driver.initObjects([mixedBefore()]); + await driver.initObjects([mixedAfter()]); + expect(await columnType(driver, MIXED_TABLE, 'slug')).toBe('text'); + expect(await columnType(driver, MIXED_TABLE, 'note')).toBe('text'); + + const message = + (await explain(driver, MIXED_TABLE, ['slug', 'note'], 'idx_os12999_mixed_slug_note')) ?? ''; + + // The stale half gets the conversion remedy… + expect(message).toMatch(/"slug" \(declares `maxLength: 64`\)/); + expect(message).toMatch(/restating the FULL column definition on MySQL/); + // …and the genuinely unbounded half is still told to declare a bound, so a + // composite key does not send the operator down one route for both columns. + expect(message).toMatch(/Column\(s\) "note" in the same key declare no usable bound and DO need `maxLength`/); + expect(message).toContain('#11374'); + }); + + it('still declines to explain a failure that is not the TEXT-key refusal', async () => { + driver = new SqlDriver(dialectCell('sqlite').config()); + await driver.initObjects([beforeTheBound()]); + await driver.initObjects([afterTheBound()]); + + // Unchanged gate: this helper speaks only for the two MySQL codes, and the + // new branch sits behind that same gate rather than beside it. + const other = await (driver as any).explainUnkeyableTextColumn(TABLE, INDEX, ['token'], { + code: 'ER_DUP_ENTRY', + }); + expect(other).toBeNull(); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 5d47a18032..40fa55994c 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -14021,8 +14021,26 @@ export class SqlDriver implements IDataDriver { /** * Turn MySQL's `ER_BLOB_KEY_WITHOUT_LENGTH` / `ER_TOO_LONG_KEY` into a message - * that names the columns at fault and the declaration that fixes them - * (#11374). + * that names the columns at fault and the remedy that actually applies + * (#11374, #12999). + * + * ## TWO remedies, because the refusal has two causes + * + * | path | the fact | what fixes it | + * |---|---|---| + * | **create** | the field declares no usable `maxLength` | declare one — the next create emits `varchar(n)` | + * | **upgrade** | the field declares one; the COLUMN predates it and is still TEXT | convert the column by hand | + * + * One message served both until #12999, and on the upgrade path both of its + * halves were false: it diagnosed a missing declaration that was present, and + * prescribed a declaration that was already made. That is not merely + * imprecise — it fires once per boot on a production upgrade and reads as the + * release that added the bound being broken, so the operator's next move is to + * doubt a correct fix. The second cause exists at all because the additive + * sync is additive: it adds columns and indexes and never rewrites a column's + * type (#3728), by design — the rewrite needs an exclusive metadata lock + * (see the `lock_wait_timeout` note this file opens with), which is a manual + * floor rather than something a boot may decide to do. * * Worth the extra `columnInfo()` read because it only happens on the failure * path, and because the raw server error is actively misleading about where @@ -14063,6 +14081,75 @@ export class SqlDriver implements IDataDriver { offenders.length > 0 ? `Column(s) ${offenders.map((c) => `"${c}"`).join(', ')} are stored as TEXT` : 'One or more of its key columns is stored as TEXT'; + + // #12999: the SAME refusal has two causes, and the message below is true of + // only one of them. On CREATE the field really declares no usable bound, so + // "declare `maxLength`" is the fix. On an UPGRADE the bound IS declared — + // the additive sync never rewrites a column's type (#3728), so a column + // created before the declaration stays TEXT forever — and that same sentence + // tells the operator to do what they already did, once per boot, in + // production, which reads as the release they just deployed being broken. + // + // The discriminator is per COLUMN and needs BOTH halves, which is why it is + // computed here rather than inferred from the error code: a bounded + // declaration alone does not imply a stale column, because `ER_TOO_LONG_KEY` + // can refuse a COMPOSITE whose parts are each individually keyable (see + // {@link MAX_KEYABLE_VARCHAR_CHARS} — the bound is per KEY, not per column). + // So the branch fires only where the physical column is TEXT *and* + // {@link keyableTextLength} says a fresh create would have emitted + // `varchar(n)` for it. When `columnInfo()` fails, `offenders` is empty and + // this map stays empty, so the CREATE-path message stands — the honest + // reading, since without introspection the two causes are indistinguishable. + // + // `declaredFieldsFor` returns `undefined` for a table this driver does not + // own the column set of, and misses a SHARD table (registered under the base + // name). Both degrade to the message below rather than guessing. + const declaredFields = this.declaredFieldsFor(tableName); + const staleBounds = new Map(); + for (const c of offenders) { + const field = declaredFields?.[c]; + const bound = field ? this.keyableTextLength(field) : null; + if (bound !== null) staleBounds.set(c, bound); + } + + if (staleBounds.size > 0) { + const staleNamed = [...staleBounds] + .map(([c, n]) => `"${c}" (declares \`maxLength: ${n}\`)`) + .join(', '); + const [firstColumn, firstBound] = [...staleBounds][0]; + // Named in full, deliberately. The mirror case in `schema-drift.ts` (the + // unbounded field over a stale `varchar`) already had to derive this, and + // the two MySQL `MODIFY` hazards it names are recorded twice more in this + // file, on the datetime and time widenings: MODIFY does NOT repeat a + // `NOT NULL`, and it DROPS a `DEFAULT` it does not restate. An operator + // who follows an abbreviated version of this remedy ends up worse off than + // the no-op they started with — a column that lost its default. + const others = + offenders.length > staleBounds.size + ? ` Column(s) ${offenders + .filter((c) => !staleBounds.has(c)) + .map((c) => `"${c}"`) + .join(', ')} in the same key declare no usable bound and DO need \`maxLength\` ` + + `declared on the field (#11374).` + : ''; + return ( + `[sql-driver] cannot create index '${indexName}' on "${tableName}" — MySQL refuses a TEXT/BLOB ` + + `column in a key without a key length. Column(s) ${staleNamed} are stored as TEXT even though the ` + + `field ALREADY declares a usable \`maxLength\` — the column was created before that bound was ` + + `declared, and the additive sync never rewrites a column's type, so re-declaring \`maxLength\` ` + + `changes nothing here (#12999). REMEDY: convert the column(s) to varchar(n) by hand, with a backup ` + + `taken first, restating the FULL column definition on MySQL (MODIFY drops a NOT NULL or DEFAULT you ` + + `do not repeat) — e.g. ALTER TABLE \`${tableName}\` MODIFY \`${firstColumn}\` ` + + `varchar(${firstBound}) plus that column's existing NOT NULL / DEFAULT clauses, restated verbatim; ` + + `— and let the next boot create this index. ObjectStack does NOT rewrite the column for you: that ` + + `ALTER needs an exclusive metadata lock on the table, which makes it a destructive, hard-to-roll-back ` + + `action and a deliberate manual floor rather than a boot-time one.${others} The table exists but this ` + + `index does NOT, so any uniqueness it declared is currently unenforced. A prefix index is deliberately ` + + `not substituted: on a UNIQUE index it constrains the prefix rather than the value, and rejects two ` + + `different values that share one.` + ); + } + return ( `[sql-driver] cannot create index '${indexName}' on "${tableName}" — MySQL refuses a TEXT/BLOB ` + `column in a key without a key length. ${named} because the field declares no \`maxLength\` (or one ` +