Skip to content

Commit 34d3011

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): report an unbounded text-family field left on a pre-existing varchar column (#12121) (#12733)
* wip(driver-sql): report an unbounded text-family field over a pre-existing varchar * test(driver-sql): pin the unbounded text-family drift op, its silences, and the set * chore(changeset): unbounded text-family column drift report * docs(cli): correct the drift-op superlative this PR falsifies (#12121) `manual_widen_varchar_to_text` is, by design, a second drift op `os migrate apply` never applies — schema-drift.ts's own comments say so. Two sites in cli.mdx claimed it was the only one: - The `os migrate` command table (:546) said `apply` never reconciles "the one drift op" that `multi-value-columns` migrates. - The `#### os migrate multi-value-columns` section opener (:670) made the same "the one drift op" claim. Both now say "one of two" and name the new op next to `manual_column_type_change`, without documenting it at length here — that belongs to the driver, not this command's doc section. Also reworded the `needs_confirm` category table's "Applied by" cell (:635): unlike `manual_column_type_change` (applied by `os migrate multi-value-columns --apply`), `manual_widen_varchar_to_text` has no applier at all — nothing in the CLI references it. Left silently as `os migrate apply` it would read as false for this one entry. PM rework request: PR #12733 comment 5441049227. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 366f895 commit 34d3011

4 files changed

Lines changed: 542 additions & 3 deletions

File tree

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
---
2+
'@objectstack/driver-sql': minor
3+
---
4+
5+
Report an **unbounded** text-family field left on a pre-existing `varchar`
6+
column, instead of leaving the operator with a refused write and no diagnostic
7+
8+
After #11875/#12119 a **newly created** `signature` / `qrcode` column is TEXT and
9+
holds a data URI correctly. `initObjects` is additive-only, so on a database
10+
created by an earlier release nothing is missing, nothing is added, and the old
11+
`varchar(255)` column is kept forever — the boundary #12119's own changeset
12+
states in as many words. What was not stated is what the drift reporter did
13+
about it, and the answer was **nothing**.
14+
15+
The varchar differ's entire branch required `declaredMaxLength !== undefined`, so
16+
on a pre-existing table it split the text family by whether its author had
17+
written a number:
18+
19+
```
20+
Field.signature({ maxLength: 4096 }) over varchar(255) -> widen_varchar reported
21+
Field.signature() — no bound over varchar(255) -> (nothing) silent
22+
```
23+
24+
The second row is the common case. Measured on the pre-fix tree, one
25+
`diffManagedTable` call per type on dialect `postgres` against a `varchar(255)`
26+
column: `text` / `textarea` / `html` / `markdown` / `richtext` / `code` /
27+
`signature` / `qrcode` with no `maxLength` each returned **zero** entries, while
28+
`{ type: 'signature', maxLength: 4096 }` over the same column returned exactly
29+
one `widen_varchar` in the same run — so the differ was working and this shape
30+
was simply invisible to it. An upgrading deployment therefore saw no change and
31+
no diagnostic, while the server kept refusing the same write; and the refusal is
32+
a poor substitute for a report, because the live probe behind objectql's
33+
`driver-fault-redaction.ts` measured Postgres's `22001` as identifier-only and
34+
naming the **type** rather than the column (`value too long for type character
35+
varying(255)`).
36+
37+
The divergence is now **detected and reported** under a new report-only
38+
`manual_widen_varchar_to_text` op, naming the declared type, the physical width,
39+
the consequence, and both operator routes. Same `declared ≠ enforced` shape as
40+
the #11374 / #11431 / #11875 family, closed one door further along — at the
41+
migration seam rather than the authoring or write seam.
42+
43+
**Nothing is migrated for you, and nothing new is refused.** There is no
44+
reconciler arm: `os migrate apply` reports the entry as skipped, exactly as it
45+
does for `manual_column_type_change`. The entry is `category: 'needs_confirm'`,
46+
so the artifact-pinned boot gate — which refuses a boot for `destructive` and
47+
nothing else — is unaffected: a deployment that merely refuses over-long values
48+
must not become a crash-loop on its next restart. Dev auto-reconcile takes
49+
`safe` only, so it never applies this unattended either. SQLite is excluded: it
50+
enforces no declared width, so there is no divergence to report.
51+
52+
`manual_widen_varchar_to_text` is a **distinct** op rather than a second use of
53+
`manual_column_type_change`, for a measured reason: `os migrate
54+
multi-value-columns` selects its entire population by
55+
`op.type === 'manual_column_type_change'` and recovers the dialect by matching
56+
the message against `manualJsonConversionSql`, so sharing the op would hand this
57+
finding to a command whose remedy makes the column `json` — and, the message
58+
carrying no json statement, have it refused as `remedy_not_recognized` on every
59+
run.
60+
61+
Graded `minor` rather than `patch` on two counts, matching the sibling drift-op
62+
addition that shipped for #11535: `detectManagedDrift` emits a finding on
63+
existing deployments where it previously emitted none (visible in `os migrate
64+
plan`, in `os migrate apply`'s skipped count and in the boot-time
65+
`[schema-drift]` warn), and the exported `DriftOp` union gains a member, which is
66+
additive for producers but widens a type any consumer switching exhaustively
67+
over it must account for. Nothing is removed, renamed or newly rejected, so it is
68+
not a breaking change.

content/docs/deployment/cli.mdx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,7 @@ diverges from the live schema, and the physical column wins at write time.
543543
|---------|-------------|
544544
| `os migrate plan` | Dry-run: show how the database has drifted from metadata, categorised safe / needs-confirm / destructive (no changes applied) |
545545
| `os migrate apply` | Reconcile the database to metadata. Applies loosening changes; destructive ones require `--allow-destructive` |
546-
| `os migrate multi-value-columns` | Migrate a stale `varchar`/`text` column to `json` where the field declares `multiple: true`the one drift op `apply` never reconciles for you. Dry run by default; `--apply` runs the statement the finding prints |
546+
| `os migrate multi-value-columns` | Migrate a stale `varchar`/`text` column to `json` where the field declares `multiple: true` — one of two drift ops `apply` never reconciles for you. Dry run by default; `--apply` runs the statement the finding prints |
547547

548548
```bash
549549
os migrate plan # Preview drift (no changes)
@@ -632,7 +632,7 @@ occupancy on its own.
632632
| Category | Examples | Applied by |
633633
|----------|----------|------------|
634634
| `safe` | relax `NOT NULL` → nullable, widen a `varchar`, create a declared index, replace a legacy installation-wide unique with its per-organization composite | `os migrate apply` (and dev auto-reconcile) |
635-
| `needs_confirm` | non-narrowing type change, rebuild a non-unique index whose columns changed | `os migrate apply` |
635+
| `needs_confirm` | non-narrowing type change, rebuild a non-unique index whose columns changed | `os migrate apply` — except `manual_widen_varchar_to_text`, which nothing applies |
636636
| `destructive` | drop an orphaned column or index, tighten `NOT NULL`, narrow a type, rebuild an index as `UNIQUE` | `os migrate apply --allow-destructive` |
637637

638638
#### Index drift
@@ -667,7 +667,7 @@ it reconciles via a table rebuild (copy → swap) that preserves your data.
667667

668668
#### `os migrate multi-value-columns`
669669

670-
The one drift op `os migrate apply` will **never** apply for you.
670+
`os migrate apply` will **never** apply this drift op — and it isn't the only one: `manual_widen_varchar_to_text` (an unbounded text-family field left on a pre-existing `varchar` column) is also never applied, but has no `os migrate` subcommand of its own. This section covers the op that does.
671671

672672
A field that gains `multiple: true` over a database that already exists keeps
673673
its old `varchar` / `text` column: the additive sync adds columns, and never

packages/drivers/driver-sql/src/schema-drift.ts

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,38 @@ export type DriftOp =
171171
* can show the divergence without re-deriving it from the message.
172172
*/
173173
| { type: 'manual_column_type_change'; table: string; column: string; to: string; from: string }
174+
/**
175+
* REPORT ONLY (#12121). The column is a `varchar(n)` under a TEXT-family
176+
* field that declared NO usable bound — the shape `createColumn` emits as
177+
* TEXT — so the column caps writes the declaration allows, and **the platform
178+
* deliberately does not change it**.
179+
*
180+
* ⛔ A DISTINCT op rather than a second use of `manual_column_type_change`,
181+
* for a measured reason and not a stylistic one: `os migrate
182+
* multi-value-columns` selects its ENTIRE population by
183+
* `op.type === 'manual_column_type_change'` (`isStaleMultiValueColumn`, "the
184+
* only op this command touches") and then recovers the dialect by matching the
185+
* finding's message against {@link manualJsonConversionSql}. Reusing that op
186+
* would hand this finding to a command whose remedy converts the column to
187+
* **json** — the wrong column type for a signature — and, since this message
188+
* embeds no json statement, the command would file it under
189+
* `remedy_not_recognized`: one refusal line per finding, on every run, for a
190+
* divergence that command has no business with.
191+
*
192+
* ⛔ There is NO reconciler arm, deliberately. `applyDriftOpInPlace` matches
193+
* no case, so `applyMigrationEntries` reports the entry as skipped and logs
194+
* it. Whether ObjectStack should run the `ALTER` itself is a separate decision
195+
* with hazards this differ must not pre-empt: on MySQL a `MODIFY` restates the
196+
* WHOLE column definition (silently dropping a NOT NULL or DEFAULT that is not
197+
* repeated) and a TEXT column cannot carry a plain index without a prefix
198+
* length, so the conversion can turn a working table into one whose declared
199+
* index no longer exists.
200+
*
201+
* `from` is the physical type word and `to` is always `'text'`, spelled the way
202+
* `manual_column_type_change` spells them so a renderer showing `from → to`
203+
* needs no new arm.
204+
*/
205+
| { type: 'manual_widen_varchar_to_text'; table: string; column: string; to: 'text'; from: string }
174206
/**
175207
* Retire the legacy platform-wide UNIQUE index on a now-tenant-scoped field
176208
* and put the composite `(tenantField, field)` in its place (#3696). The two
@@ -483,6 +515,46 @@ function acceptsStringifiedJson(type: string | undefined): boolean {
483515
return /char|text/i.test(String(type ?? ''));
484516
}
485517

518+
/**
519+
* The field types whose column is TEXT whenever the field declares no usable
520+
* `maxLength` — `createColumn`'s text-family case (#11794 / #11875).
521+
*
522+
* ⚠️ Read the scope precisely: these are the types for which the emitter's
523+
* answer is TEXT **regardless of whether an index keys the column**. That
524+
* independence is what licenses the #12121 branch below to report on this shape
525+
* without being told which columns are keyed, and it is not an assumption — it
526+
* falls out of the emitter's own expression, `keyable = keyed ?
527+
* keyableTextLength(field) : null`, whose keyed arm returns `null` for a field
528+
* with no positive-integer bound. A text-family field that DID declare a keyable
529+
* bound takes `varchar(maxLength)` when keyed, which is exactly why that branch
530+
* is gated on the declaration being ABSENT and never fires for it.
531+
*
532+
* ⛔ NOT derived from the spec's `BOUNDED_STRING_FIELD_TYPES`, for the reason
533+
* `sql-driver-12017-bounded-string-spec-parity.test.ts` argues in full: that set
534+
* answers "may this type declare a bound?" and carries no varchar/TEXT
535+
* partition, so deriving would have to INVENT an answer for every future member
536+
* at the one seam where the maintainer has actually ruled per type. It is a
537+
* hand-written list and is therefore PINNED rather than trusted —
538+
* `schema-drift.unbounded-text-column.test.ts` probes the driver's OWN
539+
* dispatch (`varcharColumnChars`) over every `FieldType` the spec declares and
540+
* asserts set equality in both directions, plus the keyed-and-unkeyed `null`
541+
* above for every member. A type entering or leaving `createColumn`'s text
542+
* family reds there by name.
543+
*
544+
* ⚠️ It lives here rather than being imported from `sql-driver.ts` because the
545+
* dependency runs the other way — `sql-driver.ts` imports this module — so an
546+
* import would be a cycle. The pin is what stands in for a shared constant.
547+
*
548+
* ⛔ Module-exported so this package's own suites can pin it, and deliberately
549+
* NOT added to `index.ts` — the same call {@link MULTI_VALUE_COLUMN_REMEDY_COMMAND}
550+
* makes: nothing outside this package has a question this set answers.
551+
*/
552+
export const UNBOUNDED_TEXT_FIELD_TYPES: ReadonlySet<string> = new Set([
553+
'text', 'textarea', 'html', 'markdown', 'richtext', 'code',
554+
// #11875 — joined the family once the write seam enforced their declared bound.
555+
'signature', 'qrcode',
556+
]);
557+
486558
/**
487559
* Does a multi-value field's JSON column carry its type on THIS dialect — i.e.
488560
* does a stale textual column silently corrupt the value (#11535)?
@@ -863,6 +935,93 @@ export function diffManagedTable(args: {
863935
});
864936
}
865937
}
938+
939+
// ── an UNBOUNDED text-family field over a pre-existing varchar (#12121) ──
940+
//
941+
// The exact COMPLEMENT of the branch above: that one REQUIRES
942+
// `declaredMaxLength !== undefined`, so on a pre-existing table the two
943+
// partition the text family by whether its author wrote a number.
944+
//
945+
// Until this branch existed the undeclared half was reported by NOTHING, and
946+
// that half is the common case. Measured on the pre-fix tree, one
947+
// `diffManagedTable` call per type: a `text` / `textarea` / `html` /
948+
// `markdown` / `richtext` / `code` / `signature` / `qrcode` field with no
949+
// `maxLength` over a `character varying(255)` column returned **zero**
950+
// entries on both enforcing dialects, while `{ type: 'signature', maxLength:
951+
// 4096 }` over the same column returned `widen_varchar` in the same run — so
952+
// the differ was working and this shape was simply invisible to it.
953+
//
954+
// What that silence costs: after #11875/#12119 a NEWLY created column for
955+
// these types is TEXT and holds a data URI correctly, but the additive sync
956+
// never revisits an existing column, so a deployment upgrading into that
957+
// release gets no change AND no diagnostic. The server keeps refusing the
958+
// same write, and the refusal is a poor substitute for a report: the live
959+
// probe behind `objectql`'s `driver-fault-redaction.ts` measured Postgres's
960+
// `22001` as identifier-only and naming the TYPE rather than the column
961+
// (`value too long for type character varying(255)`), MySQL's `1406` as
962+
// `Data too long for column 'label' at row 1`. Meanwhile every
963+
// drift-reporting road in the platform — `os migrate plan`, `os migrate
964+
// apply`, the artifact-pinned boot gate, the boot-time `[schema-drift]` warn
965+
// — reads THIS function, so the one place that could have named the column
966+
// and the cause named nothing at all.
967+
//
968+
// ## Why this needs no keyed-column input
969+
//
970+
// `createColumn` sizes a text-family column as `keyed ?
971+
// keyableTextLength(field) : null`, and `keyableTextLength` returns `null`
972+
// for a field with no positive-integer bound. So for the fields this branch
973+
// SELECTS the emitter answers TEXT whether or not an index keys them: the
974+
// differ does not have to know, and cannot be wrong about it. Pinned as such
975+
// — see {@link UNBOUNDED_TEXT_FIELD_TYPES}.
976+
//
977+
// ## Severity `error`, category `needs_confirm` — and the category is the
978+
// ## load-bearing half, exactly as it is for the base-type branch above
979+
//
980+
// ⛔ Do NOT "correct" `needs_confirm` to `destructive` to match how bad it
981+
// sounds. `runArtifactBootMigrationGate` refuses a boot for `category ===
982+
// 'destructive'` and for nothing else, and every database this finding
983+
// describes is ALREADY SERVING — that is the premise of the report. A
984+
// `destructive` spelling would convert a deployment that merely refuses
985+
// over-long values into a crash-loop on its next restart. `safe` is wrong in
986+
// the other direction: dev auto-reconcile applies `safe` entries unattended
987+
// and there is no arm to apply.
988+
//
989+
// `severity` is read by NO gate — it is render weight — and `error` is the
990+
// honest weight for the same reason the base-type branch takes it: there is
991+
// no automatic repair, so the operator has to act.
992+
if (
993+
enforcesVarcharLength(dialect) &&
994+
!declaresJsonColumn &&
995+
declaredMaxLength === undefined &&
996+
UNBOUNDED_TEXT_FIELD_TYPES.has(field.type || 'string') &&
997+
isCharacterColumn(col.type) &&
998+
typeof col.maxLength === 'number'
999+
) {
1000+
out.push({
1001+
kind: 'type_mismatch',
1002+
remoteName: table,
1003+
table,
1004+
column: fieldName,
1005+
expected: 'text',
1006+
actual: `varchar(${col.maxLength})`,
1007+
severity: 'error',
1008+
category: 'needs_confirm',
1009+
op: { type: 'manual_widen_varchar_to_text', table, column: fieldName, to: 'text', from: col.type },
1010+
message:
1011+
`${table}.${fieldName}: metadata declares \`${field.type || 'string'}\` with no ` +
1012+
`\`maxLength\`, so ObjectStack creates this column as TEXT — but the existing column is ` +
1013+
`\`varchar(${col.maxLength})\` and the additive sync never changes a column's type. The ` +
1014+
`column still caps at ${col.maxLength} characters, so the server refuses longer values the ` +
1015+
`declaration ALLOWS (Postgres 22001, MySQL ER_DATA_TOO_LONG) — a data URI in a ` +
1016+
`\`signature\`/\`qrcode\` field, or an ordinary rich-text body, is routinely past it ` +
1017+
`(#12121). ObjectStack does NOT migrate this column: "os migrate apply" reports this entry ` +
1018+
`as skipped. Two operator routes — declare a \`maxLength\` this dialect can express, which ` +
1019+
`turns this into the widen op "os migrate apply" performs; or convert the column to TEXT by ` +
1020+
`hand, with a backup taken first, restating the FULL column definition on MySQL (MODIFY ` +
1021+
`drops a NOT NULL or DEFAULT you do not repeat) and dropping any index that keys the column ` +
1022+
`first, since MySQL cannot key a TEXT column without a prefix length.`,
1023+
});
1024+
}
8661025
}
8671026

8681027
// ── orphaned columns (physical column, no metadata field) ──────────

0 commit comments

Comments
 (0)