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 a212a5e816..33328ff96b 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -14040,8 +14040,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 @@ -14082,6 +14100,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 ` +