Skip to content

fix(driver-sql): honour a string field's declared maxLength in emitted DDL - #11564

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-11431-string-maxlength-varchar
Aug 24, 2026
Merged

fix(driver-sql): honour a string field's declared maxLength in emitted DDL#11564
os-zhuang merged 2 commits into
mainfrom
claude/issue-11431-string-maxlength-varchar

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Part of #11431

createColumn mapped the string family — string / email / url / phone / password — with a bare table.string(name), so every column took knex's default width of 255 and the field's own maxLength was never read.

Part of, not Fixes: the card enumerates lookup / user / auto_number alongside the five, and this PR deliberately does not change those three. The reasons are measured and below; the remaining question ("should maxLength mean anything on a lookup or an autonumber?") is a spec decision, not a driver one.

Measured, before and after — live MySQL 8.0.46 and Postgres 16

Through the driver's own initObjects, reading information_schema rather than the DDL emitted:

fieldbeforeafter (both dialects)
email({ maxLength: 400 })varchar(255)varchar(400)
url({ maxLength: 1024 })varchar(255)varchar(1024)
phone({ maxLength: 20 })varchar(255)varchar(20)
email() — no boundvarchar(255)varchar(255)
url({ maxLength: 100000 })varchar(255)text
lookup / user / autonumber / secret / selectvarchar(255)varchar(255)

The write the card reports, a 300-character value into the maxLength: 1024 column:

before MySQL ER_DATA_TOO_LONG
Postgres 22001 value too long for type character varying(255)
after MySQL ACCEPTED
Postgres ACCEPTED

Drift against the table the driver had just created, with no rows in it — 10 findings before, 5 after, and the 5 that remain are exactly the families this PR leaves alone:

before widen_varchar wide_email/wide_url/legacy_string/a_secret/a_select/huge_url [warning/safe]
narrow_varchar narrow_phone/a_lookup/a_user/an_autonumber [error/DESTRUCTIVE]
after widen_varchar a_secret, a_select [warning/safe]
narrow_varchar a_lookup, a_user, an_autonumber [error/destructive]

Narrowing — the fence, and why I read it differently than the dispatch did

The dispatch order fenced this to widening only, and asked to be corrected with a measurement rather than obeyed. Here is the measurement.

createColumn has exactly four call sites, and every one is a CREATE TABLE or an ALTER TABLE ADD COLUMN for a column that does not exist yet (sql-driver.ts 8054, 8063, 8508, 8525). The column it sizes is always empty. rebuildSqliteTablePatched — the one path that re-materialises a populated table — does not call it. So honouring maxLength: 20 at create time cannot truncate anything: there is nothing in the column.

Narrowing an existing populatedvarchar(255) is a different road, and this PR does not touch it: it is still the narrow_varchar drift op, category destructive, behind os migrate apply --allow-destructive. The differ's narrowing arm is unchanged in behaviour.

The trigger the order named — "would this newly make an existing deployment emit a truncating ALTER" — is therefore not met, in the strong sense: narrow_varchar for a maxLength: 20 field on a varchar(255) column is emitted on maintoday, identically, before this branch exists. The measurement is in the before-table above. What the change removes is the false half of that: a table the driver itself had just created reporting a destructive narrowing against its own empty columns.

If the fence was meant to hold regardless of that reasoning, the narrowing half is one condition in declaredVarcharLength and can be dropped — but it would leave maxLength: 20 inert and the self-drift in place.

Upper bound

Measured, not read off a doc page:

MySQL utf8mb4 varchar(16383) creates · varchar(16384) -> ERROR 1074 (max = 16383)
Postgres varchar(10485760) creates · 10485761 -> length for type varchar cannot exceed 10485760
SQLite records the declared type verbatim, enforces nothing

One ceiling for every dialect, and it is the lowest of the three (16383) — the alternative is one declaration with three physical shapes. Above it the column becomes TEXT rather than being clamped, because a clamp would reinstate the exact defect being fixed: a column narrower than the declaration, refusing writes the declaration permits. TEXT refuses nothing the author declared, and the bound is still enforced at the write seam by the record validator's max_length check — with a field-named ADR-0112 envelope instead of a raw ER_DATA_TOO_LONG.

⚠️One hazard this cannot see, reported rather than fixed. MySQL also caps the sum of a row's declared byte widths at 65535. Measured: 15 × varchar(1024) creates, 16 × varchar(1024) is ERROR 1118 Row size too large; 64 × varchar(255) (today's shape) creates and 65 does not. Postgres has no such limit (40 × varchar(4096) creates cleanly — it TOASTs). No object in this repo comes near it — the widest string-family bound declared anywhere here is 2000, and no object carries more than six such fields — but an authored app could, and createColumn is per-column so no decision inside it can see the budget. Filed separately rather than solved here, since wiring a diagnostic means wrapping the shared createTable call that sibling cards are on.

The three families left at 255, each for a measured reason

  • lookup / user — the column holds the referenced row's id, not the declared value. MySQL is content with a mismatched FK (varchar(20) child → varchar(255) parent id creates cleanly, and Postgres accepts it too), so the type system gives no warning — but a platform id is 26 characters, so INSERT … VALUES ('01JQ8XKZ9M4N7P2R5T6V8W0Y3B') into varchar(20) is ERROR 1406 Data too long. Honouring the bound here would make the column structurally incapable of holding any id. (My first hypothesis — that the FK itself would be refused — was wrong; measuring it is what found the real and worse failure.)
  • autonumber — the value is runtime-issued. maxLength has no write-time counterpart on this type (the record validator's textual branch does not cover it, and runtime-owned types never reach it), so binding the DDL to it creates a refusal with nothing declaring it.
  • the catch-allsecret persists an opaque sys_secret ref rather than the credential (ADR-0100); select / radio / code / tree store option machine names or ids. None of them stores the string the bound describes.

Why schema-drift.ts is in the diff

Two corrections, both required for this change to be coherent rather than optional cleanups — the emitter and the differ disagreeing about which declarations count is the defect class this card exists to close, and both of these would have re-opened it one case to the left.

  1. A malformed maxLength is no longer read as a bound.maxLength: 0 took the narrowing arm (0 > 255 is false) and planned varchar(0); maxLength: 12.5 planned varchar(12.5) — both at severity error, category destructive, i.e. as work --allow-destructive should go do. The differ now applies the same positive-integer predicate the emitter does.
  2. A MySQL TEXT column is no longer diffed as a varchar 65535 wide. Measured on the same two columns: MySQL reports character_maximum_length = 65535 for TEXT, Postgres reports NULL — so this was MySQL-only and invisible on Postgres. Every bounded unkeyed text field, the shape createColumn deliberately leaves as TEXT, diffed as "declared 4000, column allows 65535" and produced a narrow_varchar at severity error / category destructive against a freshly created empty table. Measured on live MySQL against sys_email's real field shapes:
narrow_varchar to_addresses varchar(65535) -> varchar(4000) [error/destructive]
narrow_varchar subject varchar(65535) -> varchar(998) [error/destructive]

Reverse verification

The pins were run against pre-fix source, rebuilt, with the mutation confirmed on disk before the run (an editing tool's exit code is not evidence) and a trap … EXIT INT TERM restoring both files:

declaredVarcharLength in sql-driver.ts : 0 (expect 0)
isCharacterColumn in schema-drift.ts : 0 (expect 0)
rebuilt dist; declaredVarcharLength in dist: 0 (expect 0)
Test Files 1 failed | 125 passed (126)
Tests 6 failed | 2515 passed (2521)
x emits varchar(maxLength) in BOTH directions, and 255 without a declaration
x falls back to TEXT past the varchar ceiling instead of clamping
x accepts a write the declaration allows — the whole defect, in one assertion (live mysql)
x reports no varchar drift against a table it just created (live mysql)
x accepts a write the declaration allows — the whole defect, in one assertion (live postgres)
x reports no varchar drift against a table it just created (live postgres)

The 4 pins that stay green pre-fix are the ones asserting unchanged behaviour — malformed bound → 255, the excluded families → 255, and the negative half on both dialects. That negative half is the one that would catch a fix that widened everything to silence the symptom: varchar(20) must really still refuse 300 characters, and it does.

Verification

All on bdf9b6da06, the final commit of this branch.

  • pnpm --filter @objectstack/driver-sql test with OS_TEST_MYSQL_URL and OS_TEST_POSTGRES_URL set — 126 files, 2521 tests passed, 10 of them new.
  • pnpm --filter @objectstack/driver-sql typecheck — clean.
  • 33 repo gates, derived from the actual diff via node scripts/pm/dispatch-gates.mjs (not from the dispatch list, which predated the docs edit) — all pass. check:query-options-erasure red first at "test surface grew 240 -> 241"; the new test's untyped options bag was replaced with a char_length read from the server, which is the fact being pinned anyway.
  • Ratchet families re-run on the final commit after the last edit: 13/13 pass on bdf9b6da06, working tree clean.

Docs

content/docs/protocol/objectql/types.mdx asserted the old shape in three places this diff falsifies — the email mapping bullet, the phone storage sentence, and the Type Conversion Matrix row. Corrected, plus a footnote stating the rule in both directions and the TEXT fallback, and naming the neighbouring rows that deliberately do not follow it.

Fences

⛔ Line 12728's keyed-text branch (col = keyable === null ? table.text(name) : table.string(name, keyable)) is not in this diff — #11374 is unruled and may reshape it. declaredVarcharLength deliberately mirrors keyableTextLengthwithout sharing code with it: the two answer different questions, and a shared helper would couple a settled decision to an unsettled one. toDateOnly, the aggregate lowering region, and introspectForeignKeys are untouched.

Generated by Claude Code


Generated by Claude Code

os-project-managerand others added 2 commits August 24, 2026 02:55
…d DDL
`createColumn` mapped the string family (string/email/url/phone/password) with
a bare `table.string(name)`, so every column took knex's default of 255 and the
field's `maxLength` was never read. Measured through the driver's own
`initObjects` on live MySQL 8.0.46 and Postgres 16: a `maxLength: 400` email, a
`maxLength: 1024` url and a `maxLength: 20` phone all landed as `varchar(255)`,
and a 300-character write the declaration plainly permits was refused by both
(`ER_DATA_TOO_LONG`; `22001 value too long for type character varying(255)`).
The bound now binds in both directions. Narrowing is not a destructive
migration: `createColumn`'s four call sites are all CREATE TABLE / ALTER ADD
COLUMN, so the column it sizes is always empty. Narrowing an existing populated
column stays where it was — the `narrow_varchar` drift op, behind
`os migrate apply --allow-destructive`.
Past the varchar ceiling (measured: MySQL refuses 16384 with ERROR 1074) the
column becomes TEXT rather than being clamped, since clamping would reinstate
the defect. lookup/user, autonumber and the catch-all keep 255 — none of them
stores the declared value.
schema-drift.ts is in the diff because the emitter and the differ must agree on
which declarations count: it now applies the same positive-integer predicate
(`maxLength: 0` planned `varchar(0)` at severity error/destructive), and no
longer reads a MySQL TEXT column as a varchar 65535 wide (measured: MySQL
reports character_maximum_length 65535 for TEXT, Postgres NULL — so every
bounded unkeyed text column reported a permanent destructive narrow_varchar on
MySQL only).
Part of #11431
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
The Type Conversion Matrix, the `email` mapping bullet and the `phone` storage
sentence all asserted VARCHAR(255) for the string family, which this branch's
diff falsifies. Corrected in place, with a footnote naming the rule in both
directions, the TEXT fallback above 16383, and — deliberately — the neighbouring
rows that do NOT follow it (select/radio, lookup/master_detail/tree, autonumber).
Also drops an untyped query-options bag the new test had introduced; the read-back
now asserts `char_length` from the server, which is the fact being pinned anyway.
Part of #11431
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y
@github-actions

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 ba8420b58d36077fe79ca8ce0f201bb31f8be2bcpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 24d66eda76aab4f201b39456cc16d45e1df66b9f — the merge of head bdf9b6da06e89cdd2c50d0bd3f7c44257177b35c into base ba8420b58d36077fe79ca8ce0f201bb31f8be2bc, 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 24d66eda76aab4f201b39456cc16d45e1df66b9f && git checkout 24d66eda76aab4f201b39456cc16d45e1df66b9f
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ba8420b58d36077fe79ca8ce0f201bb31f8be2bc bdf9b6da06e89cdd2c50d0bd3f7c44257177b35c && git checkout -B drift-repro ba8420b58d36077fe79ca8ce0f201bb31f8be2bc && git merge --no-ff bdf9b6da06e89cdd2c50d0bd3f7c44257177b35c
node scripts/docs-audit/affected-docs.mjs --json ba8420b58d36077fe79ca8ce0f201bb31f8be2bc

⚠️ 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 ba8420b58d36077fe79ca8ce0f201bb31f8be2bc → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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