Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/sql-driver-field-string-comment-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
---
"@objectstack/driver-sql": patch
---

fix(driver-sql): correct two comments in `createColumn` that cited a `Field.string` builder that has never existed (#12593)

Two comments in the keyed/bounded text-family branch of `createColumn`
(`signature` / `qrcode` / `richtext` / `code`) asserted a `Field.string`
builder exists and that it "has always taken knex's `varchar(255)`." It does
not exist and never has: `Field` has 37 keys and none is `string`,
`FieldType.options` (49 entries) does not list it, and
`FieldSchema.safeParse({ type: 'string', … })` fails at `[type]` — all three
reproduced fresh on this branch, plus `git log -S` confirming no commit ever
added such a builder key to `field.zod.ts`.

The name is not arbitrary: `'string'` is knex's own column-builder method
name (`table.string(name)`), reused internally by this driver as its
*untyped* default (`field?.type || 'string'`) — a storage-side spelling that
collides with, but is not, an authoring-side one. The corrected comments now
state only checkable facts: knex's bare `table.string(name)` (no length) is
`varchar(255)`; this branch never calls it bare, it calls
`table.string(name, keyable)` with the field's own declared `maxLength`
(up to `MAX_KEYABLE_VARCHAR_CHARS`, 768 chars); and the nearest *authorable*
spelling that reaches this exact `varchar(n)` shape is `Field.text({
maxLength: n })` on a keyed column — exactly what this switch arm already
serves.

No code changed — `git diff` is comment-only lines inside `sql-driver.ts`.
Nothing here alters DDL, column widths, or any runtime behavior; the two
pins that already exercise this exact branch behaviorally
(`sql-driver-11565-row-byte-budget.test.ts`'s "agrees with createColumn about
every FieldType" mirror, and `sql-driver-keyed-text-mysql.test.ts`'s
"emits varchar(maxLength) for a keyed bounded field") both stay green,
unmodified.

**Grade: `patch`, and deliberately no higher.** This is documentation
embedded in source, not an exported symbol, a spec key, or any authorable or
runtime surface — there is nothing here for a consumer to migrate. `patch`
is the correct floor for a fix that changes only what the driver's own
source *says*, matching the sibling `builtin-column-delivery-id-type.md`
changeset (#12131) that corrected the same false `Field.string` premise in
an adjacent file. Not a declared-breaking changeset, so no ADR-0087
disposition marker applies.
18 changes: 13 additions & 5 deletions packages/drivers/driver-sql/src/sql-driver.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -14636,17 +14636,25 @@ export class SqlDriver implements IDataDriver {
// `schema-drift.ts` already 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. `Field.string` has always taken
// knex's `varchar(255)`, so a bounded text field is now LESS arbitrary
// than its string sibling, not more.
// finally agreeing with the differ. There is no `Field.string`
// builder: none of `Field`'s keys is named `string`, `FieldType.options`
// omits it too, and `FieldSchema.safeParse({ type: 'string', … })`
// fails at `[type]` (#12593). What IS true of knex: its bare
// `table.string(name)` call (no length argument) is `varchar(255)`.
// This branch never calls it bare — `table.string(name, keyable)`,
// where `keyable` is the field's own declared `maxLength`, up to
// `MAX_KEYABLE_VARCHAR_CHARS` (768 chars, wider than knex's
// 255-char default).
//
// Applied on every dialect rather than under `isMysql`, deliberately:
// the alternative is one declaration with two enforcement answers, so
// the same app would refuse an over-length write on MySQL and accept it
// on Postgres. Dialect-divergent enforcement of one declared bound is
// the defect class this repo's conformance matrices exist to close, and
// a `varchar(n)` is exactly what the SQLite and Postgres columns would
// have been had the field been declared `Field.string`.
// a `varchar(n)` is exactly what the SQLite and Postgres columns land
// as for a field declared `Field.text({ maxLength: n })` on a keyed
// column — the nearest authorable spelling to this shape; there is no
// `Field.string` (#12593).
//
// ⚠️ Scope, both halves load-bearing:
// - KEYED only. A non-indexed `Field.text({ maxLength: 65000 })` stays
Expand Down
Loading