Skip to content

fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on - #11430

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-11374-mysql-unbounded-string-index
Aug 23, 2026
Merged

fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on#11430
os-zhuang merged 3 commits into
mainfrom
claude/issue-11374-mysql-unbounded-string-index

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Part of #11374 — the mapping half. Deliberately not a closing reference: 12 of the 44 platform objects still fail schema-sync on MySQL, and the remainder needs a decision this PR does not take (see What is left).

The defect

createColumn mapped the whole text family (text / textarea / html / markdown) to an unbounded TEXT, ignoring the field's own declared maxLength. MySQL refuses a TEXT/BLOB column in a key without a prefix length, and the two halves of schema-sync fail separately: the CREATE TABLE succeeds, then ALTER TABLE … ADD [UNIQUE] INDEX fails with ER_BLOB_KEY_WITHOUT_LENGTH. The table lands on disk without the constraint it declared and the object stays registered-but-broken.

Reproduced on a live MySQL 8.0.46 (utf8mb4, InnoDB DYNAMIC, STRICT_TRANS_TABLES), feeding all 44 platform objects through syncSchema:

beforeafter
objects failing syncSchema36 / 4412 / 44
declared indexes physically present23 / 12889 / 128
Postgres 16.13 control0 failures0 failures

The shape, and why it is this one

The premise in the card's title is only partly right. The columns are not mostly unbounded — 78 of the 94 indexed text columns across platform objects already declare a maxLength (sys_user.phone_number declares maxLength: 32). The driver simply never read it. Only 16 are genuinely unbounded.

So the fix is not "impose a length where the spec declared none" — it is honour the bound the field already declared. Three things already agreed on that shape before this PR:

  • schema-drift.ts treats varchar(field.maxLength) as the expected physical shape of a bounded field — its widen_varchar / narrow_varchar ops say so in as many words. This is the emitter finally agreeing with the differ.
  • On MySQL columnInfo() reports maxLength: 65535 for a TEXT column, so every bounded text field already reported a permanent destructive narrow_varchar drift ("metadata caps at 32 chars but the column allows 65535") against a column the driver itself had created. That finding is now gone for new tables.
  • Field.string has always taken knex's varchar(255). A bounded text field is now less arbitrary than its string sibling, not more.

Why not a prefix index — measured, not reasoned

The card flagged the uniqueness gap as the trap, and the measurement inverts its direction. The expectation was that a prefix-unique index "silently accepts two values that differ only past the prefix". It does the opposite: it enforces uniqueness of the prefix, so it is stricter than the declared constraint and rejects two genuinely different values that share one.

On MySQL 8.0.46, UNIQUE KEY (token(191)) with two distinct 200+ character tokens sharing their first 191 characters:

insert v1 -> OK
insert v2 -> ER_DUP_ENTRY: Duplicate entry 'AAAA…' for key 't2.uniq_t2_token'
rows: 1 (two distinct tokens, one survived)

A true duplicate is still caught, so the index is not broken — it is a different constraint. On sys_session.token that is a valid sign-in refused with a duplicate-key error, data-dependent and silent. Disqualified for UNIQUE, and not adopted for ordinary indexes either, so the model stays one rule rather than two.

Scope — both bounds are load-bearing

  • Keyed only. A non-indexed Field.text({ maxLength: 65000 }) stays TEXT: varchar(65000) on utf8mb4 is 260000 bytes and blows MySQL's 65535-byte row limit, turning a working table into an un-creatable one.
  • Usable-as-a-key-part only.maxLength absent, or wider than 768 characters, leaves the column TEXT. 768 is measured, not read off a doc page: varchar(768) UNIQUE creates, varchar(769) UNIQUE is refused with ER_TOO_LONG_KEY: max key length is 3072 bytes.
  • All dialects, not isMysql. The alternative is one declaration with two enforcement answers — the same app refusing an over-length write on MySQL and accepting it on Postgres. Existing tables are untouched: schema-sync is additive and never rewrites a column already present, and on Postgres a text column reports maxLength: null, so the differ's varchar rule stays silent and no migration is provoked.

⚠️Graded minor, and the break is named: a keyed bounded text column now enforces its declared length where the dialect enforces varchar, so a write longer than maxLength that previously landed in an unbounded TEXT is refused (ER_DATA_TOO_LONG under strict mode). That is a previously-inert declaration becoming enforced, and it is what makes the column indexable — but it is a behaviour change.

When the column still cannot be keyed

The index is refused loudly, with a message naming the columns at fault and the declaration that fixes them, rather than silently substituting a weaker or different constraint. The boot still fails exactly where it failed before; it just says why.

The coverage hole

packages/drivers/driver-sql/src/sql-driver-keyed-text-mysql.test.tsno CI change needed. The required Temporal Conformance (live PG + MySQL) job already runs pnpm --filter @objectstack/driver-sql test with OS_TEST_MYSQL_URL set, and the per-file database ledger picks up a new *.test.ts in that directory automatically. The hole was never the job — it was that every suite in this package builds its tables with an explicit knex.string() (VARCHAR), so the live MySQL never met the TEXT mapping.

Six pins: the varchar(maxLength) emission and the two "stays TEXT" corners (SQLite, so Test Core carries them too); on live MySQL, that the declared indexes physically exist and are full-value, not prefixed (information_schema.statistics.sub_part IS NULL); that an unkeyable column is refused by name and leaves no substituted index; and the prefix-unique measurement itself, kept executable so the rejected route cannot be re-argued from intuition.

Reverse-verified. Reverting only the mapping (col = table.text(name)) reds 2 of the 6 — the SQLite emission pin and the live-MySQL index pin — and leaves the other 4 green, which is the correct direction. Mutation confirmed on disk before the run (anchor hit count 1, injected-text count 1 → 0, git diff --stat 2 insertions / 2 deletions); restore ran from an EXIT INT TERM trap. No rebuild was involved and none was needed: the suite imports ../src/index.js, and the red appeared with no build step, which is itself the proof it reads source rather than dist/.

Docs corrected in the same PR

content/docs/protocol/objectql/types.mdx, in three places — the drift bot flagged one, and the other two were the same claim in prose, which it could not see because it anchors on symbols:

  1. The Type Conversion Matrix row.| text | TEXT | TEXT | TEXT | → the row now covers text / textarea / html and carries a * to a footnote, since the condition does not fit a cell.
  2. The text section's prose, which asserted the opposite of the new behaviour outright: "maxLength is enforced by record validation, not by the column type — the DDL does not read it." Replaced, plus a callout explaining why the bound follows the index and what happens when a keyed column cannot be bounded.
  3. The textarea and html sections, which both carried a bare SQL driver: TEXT.

Both halves of the condition are stated wherever the claim appears — a declared maxLength of 768 or less and a declared index keying the column — because a reader who takes away only "bounded text becomes varchar" would be surprised by a non-indexed maxLength: 200 field staying TEXT. The company_name example already on that page is exactly such a field, so the prose names it.

The other VARCHAR(255) rows were checked by measurement, not assumption, as asked: email/url/phone, select/radio, lookup/master_detail/tree and autonumber all still emit varchar(255)even when keyed and even when declaring a maxLength — only the text family moved, so those rows stand. (Separately, and pre-existing: that they ignore maxLength at all is the defect filed as #11431 — not touched here.)

content/docs/releases/ is not touched. The drift bot also listed releases/v17.mdx; that page is release-owned, and I have flagged it in the report rather than editing it.

What is left, and why this PR stops here

12 objects still fail, in three groups that all need a decision about platform-object field declarations, not about the driver:

  1. 7 unbounded UNIQUE identity columnssys_account.{provider_id,account_id,issuer}, sys_api_key.key, sys_device_code.{device_code,user_code}, sys_session.token. These hold values produced by better-auth and by external IdPs (an OIDC sub, an issuer URL), so choosing a bound is a product decision with a real breakage mode.
  2. 3 maxLength: 1024UNIQUE token columnssys_oauth_access_token.token, sys_oauth_refresh_token.token, sys_oauth_resource.identifier. Bounded, but past the 768-character key ceiling. A full-value unique on a value that may legitimately be a multi-KB JWT is not expressible on utf8mb4 InnoDB at all; the options (an ascii charset on the column, a hashed shadow key like this driver's own _objectstack_sequences.key_hash, or a narrower declared bound) are architectural.
  3. 1 composite over budgetsys_metadata's 4-column unique is 3460 bytes against a 3072-byte ceiling even with every part bounded.

Guessing any of these would be writing speculative code into identity tables. They are reported for triage instead.

Verification

All at 0717830f5d, against live servers configured for CI parity (MySQL @@global.time_zone='+08:00', Postgres timezone='Asia/Shanghai', process TZ=America/New_York) — both were down on arrival and started for this work.

  • pnpm --filter @objectstack/driver-sql test against live PG + live MySQL125 files / 2511 tests passed.
  • pnpm --filter @objectstack/driver-sql typecheck — clean.
  • Gate union re-derived after the docs edit via node scripts/pm/dispatch-gates.mjs (no paths passed): the docs path grew it from 13 to 30 path-matched families. All 30 exit 0, plus the convention-triggered families, plus check:type-check-debt --re-measure re-run on this head ("33 ledger entr(ies) re-measured … 1897 raw tsc error(s) total, none above its recorded number").
  • pnpm lint (full repo, eslint . --no-inline-config) — exit 0.

Generated by Claude Code

os-project-managerand others added 2 commits August 23, 2026 17:30
…index keys on
MySQL refuses a TEXT/BLOB column in a key without a prefix length, so
CREATE TABLE succeeded while ALTER TABLE ... ADD [UNIQUE] INDEX failed and
the object stayed registered-but-broken. Measured on MySQL 8.0.46: 36 of 44
platform objects failed schema-sync this way; honouring the field's own
declared maxLength takes that to 12.
The bound is the field's own maxLength -- nothing is invented. schema-drift
already treats varchar(field.maxLength) as the expected physical shape (its
widen_varchar / narrow_varchar ops say so); this is the emitter agreeing
with the differ.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
…enforcement change)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
@github-actions

github-actionsBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 9 documentable anchor(s).

6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx(via SqlDriver (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json cccbe51bf7b0cde74e208f086d9593d3983c4238packageMentionDocs.

Which tree this was computed on

This run read content/docs from 978ab69c4d7f7a7d3f4fd82a779578d0d21d5a77 — the merge of head 0717830f5d4c6b6dade3a1a0f258b14f2ce10384 into base cccbe51bf7b0cde74e208f086d9593d3983c4238, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 978ab69c4d7f7a7d3f4fd82a779578d0d21d5a77 && git checkout 978ab69c4d7f7a7d3f4fd82a779578d0d21d5a77
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin cccbe51bf7b0cde74e208f086d9593d3983c4238 0717830f5d4c6b6dade3a1a0f258b14f2ce10384 && git checkout -B drift-repro cccbe51bf7b0cde74e208f086d9593d3983c4238 && git merge --no-ff 0717830f5d4c6b6dade3a1a0f258b14f2ce10384
node scripts/docs-audit/affected-docs.mjs --json cccbe51bf7b0cde74e208f086d9593d3983c4238

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs cccbe51bf7b0cde74e208f086d9593d3983c4238 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 23, 2026
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM note (engine seat) — CI still running; this is not the accept. Two corrections owed first, one of them mine.

⛔ My dispatch order stated a fact about prefix indexes that is WRONG. Correcting it here, where it was acted on.

I wrote, as the trap to watch for:

a prefix-unique index silently accepts two values that differ only past the prefix

Measured on live MySQL 8.0.46, that is backwards.UNIQUE KEY (token(191)) with two distinct 200+ character tokens sharing their first 191 characters:

insert v1 -> OK
insert v2 -> ER_DUP_ENTRY: Duplicate entry 'AAAA…' for key 't2.uniq_t2_token'
rows: 1 (two distinct tokens, one survived)

It enforces uniqueness of the prefix — so it is stricter than the declared constraint and rejects genuinely different values, rather than admitting duplicates. On sys_session.token that is a valid sign-in refused with a duplicate-key error: data-dependent, silent, and a live user impact.

The conclusion I asked for is unchanged — prefix indexes are disqualified for UNIQUE — but it was right for the opposite reason, and a dev reasoning from my sentence rather than from the server would have looked for the wrong failure and possibly concluded the route was safe for non-unique keys. Keeping that measurement executable as a pin, so "the rejected route cannot be re-argued from intuition", is exactly right.

This is the third claim of mine that measurement has overturned today (a region read from PR prose, a stale "the database is running", and now this). The pattern is the same each time: I stated something inherited or reasoned as though it were measured. The instruction to measure rather than reason is doing more work than my instructions are.

The card's own premise was also only partly right — and that reframes the fix

The title says unbounded string fields become TEXT. Measured: 78 of the 94 indexed text columns already declare a maxLength (sys_user.phone_number declares 32). Only 16 are genuinely unbounded. So this is not "impose a bound the spec never declared" — my option (A) — it is honour the bound the field already declared, which is a materially smaller and better-justified change.

Three independent things already agreed with that shape, and the PR found all three rather than asserting the choice:

  • schema-drift.ts already treats varchar(field.maxLength) as the expected physical shape (widen_varchar / narrow_varchar). The emitter is finally agreeing with the differ.
  • On MySQL, columnInfo() reports maxLength: 65535 for a TEXT column — so every bounded text field was already reporting a permanent destructive narrow_varchar drift against a column this driver itself created. That is a standing false alarm this change removes.
  • Field.string has always taken varchar(255); a bounded text field is now less arbitrary than its string sibling, not more.

The 768 ceiling is measured too (varchar(768) UNIQUE creates, varchar(769) is refused ER_TOO_LONG_KEY), and the two scope bounds are load-bearing: keyed-only (a non-indexed maxLength: 65000 would blow the 65535-byte row limit at utf8mb4) and usable-as-key-part-only.

Applying it on all dialects rather than isMysql is the right call for the reason given — one declaration with two enforcement answers would mean the same app refusing an over-length write on MySQL and accepting it on Postgres.

Stopping at 12 was correct

36/44 → 12/44 failing, indexes present 23/128 → 89/128. The remainder is three groups that need decisions about platform-object field declarations, not about the driver: 7 unbounded UNIQUE identity columns carrying better-auth and external-IdP values, 3 maxLength: 1024 UNIQUE token columns past the 768-character key ceiling, and sys_metadata's 4-column composite at 3460 bytes against a 3072-byte ceiling.

"Guessing any of these would be writing speculative code into identity tables" is the right instinct, and Part of rather than Fixes is the right marker. I will route the remainder as a decision after this lands — it is a product call (what bound is legitimate for an OIDC sub or an issuer URL) and an architectural one (ascii charset vs a hashed shadow key vs a narrower declared bound), which is the manual floor either way.


Generated by Claude Code

…clared index keys it
The Type Conversion Matrix claimed `text` -> TEXT unconditionally on all three
dialects, and the `text` section's prose said outright that "`maxLength` is
enforced by record validation, not by the column type -- the DDL does not read
it". Both are falsified by the keyed-text mapping.
Corrected in three places rather than one: the matrix row (with a footnote, since
the condition does not fit a cell), the `text` section's prose plus a callout
explaining why the bound follows the index, and the `textarea` / `html` sections
which carried the same unconditional claim and which the drift bot -- anchoring
on symbols, not prose -- did not flag.
Both halves of the condition are stated wherever the claim appears: a declared
maxLength of 768 or less AND a declared index keying the column. A reader taking
away only "bounded text becomes varchar" would be surprised by a non-indexed
maxLength: 200 field staying TEXT.
Measured, not assumed: the other VARCHAR(255) rows (email/url/phone,
select/radio, lookup/master_detail/tree, autonumber) are unchanged even when
keyed and even when declaring a maxLength, so those rows stay as they were.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-zhuang@os-project-manager