Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): report a multi-value field left on a stale varchar/text column - #11720
Conversation
…ext column A field that gains `multiple: true` materialises as a `json` column on a fresh database, but `initObjects` is additive-only: on an existing database nothing is missing, so the old varchar/text column is kept and every array written to it is stored as the stringified literal and read back as a string. Measured on live Postgres 16.13 and MySQL 8.0.46 on the pre-fix tree, `detectManagedDrift()` returned `[]` for exactly that shape. Detection only. The column is not migrated: an `ALTER TABLE ... TYPE json USING` over existing rows plus an index rebuild is a destructive migration over shipped data, and whether the platform should perform it is a separate open decision. The new `manual_column_type_change` op has no reconciler arm by design. Severity `error`, category `needs_confirm` — measured, not chosen for tone: the artifact-pinned boot gate refuses a boot for `category === 'destructive'` and nothing else, and every database this describes is already serving. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn
…sting
`{ filters: [] } as any` was not merely untyped — `filters` is not a
DriverQuery key at all (`where` is), so the cast was hiding a wrong shape while
adding two sites to the query-options-erasure test-surface ratchet. An empty
query is the typed spelling of "all rows".
Part of #11535
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b09d1292792f2ab23dcd7431a364c4cfff063bfc && git checkout b09d1292792f2ab23dcd7431a364c4cfff063bfc
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b040ed3463ccf41a2c510437f80d00f4f6c27c93 ecd52f335e6a0ead04b5115de7eb87eac7b959f1 && git checkout -B drift-repro b040ed3463ccf41a2c510437f80d00f4f6c27c93 && git merge --no-ff ecd52f335e6a0ead04b5115de7eb87eac7b959f1
node scripts/docs-audit/affected-docs.mjs --json b040ed3463ccf41a2c510437f80d00f4f6c27c93
|
Uh oh!
There was an error while loading. Please reload this page.
… migration The operator-run half of #11535, ruled C on #11700: the platform warns and ships an explicit migration an operator invokes, and never runs it for them. The statement is the one driver-sql's `manual_column_type_change` finding prints (#11720, measured against live Postgres 16.13 / MySQL 8.0.46); the command refuses to execute anything the finding does not contain verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
Part of #11535
Detection half only. The column is not migrated — see "What this PR deliberately does not do".
The defect, measured
A field that gains
multiple: truematerialises as ajsoncolumn on a fresh database.initObjectsis additive-only, so on a database created while the field was single-value nothing is missing, therefore nothing is added and the oldvarchar/textcolumn is kept forever. The write path stringifies the array for a json field on every non-SQLite dialect; the read path relies on the driver's column-type-based decoding, which a stale textual column defeats.Reproduced end to end on live Postgres 16.13 (
timezone='Asia/Shanghai', processTZ=America/New_York) on the pre-fix tree:The reporter's root-cause paragraph is confirmed rather than assumed: with the FK left in place the insert failed with
Key (assignee)=(["user_A","user_B"]) is not present in table "sys_user", which is the write path's stringification caught in the act.The before-state is silence, so that is what the pins are built around. "A finding was produced" would be a weak assertion here; every case pins the finding on the right column with the right diagnosis, and each neighbouring shape that must stay silent is pinned as silent in the same breath.
Dialect scope is a measurement, not a convenience
Same fixture, three dialects, same driver:
character varyingjson'["x","y"]'(string)time_zone=+08:00)varcharjson'["x","y"]'(string)varcharjson['x','y'](array)SQLite's read path
JSON.parses regardless of what the column calls itself, so the mismatch is visible there but harmless. Reporting it anyway would put a permanenterrorfinding on every long-lived SQLite development database for a divergence that changes no value. A staleinteger/timestampcolumn is excluded for the mirror-image reason: the server already refuses that write loudly (22P02,ER_TRUNCATED_WRONG_VALUE), so there is no silence to break.The premise the dispatch named — measured, and it is HALF FALSE
The axis that gates a boot is
category, notseverity. Measured against the real consumers rather than inferred from the words:reconcileAndWarnDrift(ordinary boot)destructiveentry completed the bootrunArtifactBootMigrationGate(artifact-pinned boot,kernel:ready, before the socket opens)categoryonlydestructive→ok=false→ refuses the boot;needs_confirm→ok=truecategory === 'safe'needs_confirmSo
severity: 'error'is free — no gate reads it; it is the render weight (✗inos migrate plan). Butcategory: 'destructive'would have refused the boot, on the artifact-pinned path, for every affected deployment. Every database this finding describes is already serving — that is the premise of the user's report — so adestructivespelling would have turned the report of the corruption into the outage.Shipped as
severity: 'error'+category: 'needs_confirm', and that choice is pinned by a test with the measurement written into it, so nobody "corrects" it to match how bad it sounds.Residue, stated rather than hidden:
os migrate applyhands aneeds_confirmentry to the reconciler, which — having no arm for this op by design — declines it (applied=0, skipped=1) and logs it. Reported every time, applied never, which is the correct contract while the migration decision is open.What this PR deliberately does not do
No column-type migration. Not partially, not behind a flag. The new
manual_column_type_changeop has no reconciler arm, and its doc comment says so in as many words: whether ObjectStack should perform theALTER … TYPE json USING …itself is the other half of #11535 and a live maintainer decision, since it is a migration over existing rows plus an index drop/rebuild. The op is the absence of an automatic migration made explicit, not a placeholder for one.The remedy is executed, not merely printed
The finding names the table, the column, the declared type, the physical type, and the dialect-correct statement an operator runs by hand (including dropping any index first — a json column cannot carry a plain btree). The suite runs that emitted statement against both live servers, over every row state a stale column holds, and asserts the finding then clears:
'["x","y"]'["x","y"]["x","y"]'a'["a"]["a"]nullnullnullnullTwo deviations from the reporter's production SQL, both forced by measurement while writing this:
to_json(col)turns a legacy single value into the JSON scalar"a"under a field the metadata now declares multi-value — it read back withArray.isArray === false.json_build_arraymakes Postgres agree with MySQL'sJSON_ARRAY.json_build_array(NULL)is[null], so an explicitIS NULLarm was added; without it every NULL row silently gained a value. That arm exists because the version without it was run and produced[null].Also fixed — bounded, same defect class, declared
A multi-value field that also declared
maxLengthproduced, on the pre-fix tree,narrow_varcharat severityerror, category destructive on both enforcing dialects:That is a finding which refuses the artifact-pinned boot and invites
os migrate apply --allow-destructiveto rewrite the column tovarchar(50)— the exact opposite of the repair it needs, which isjson.createColumnreturns atif (field.multiple) { table.json(name); return; }beforemaxLengthis read, so the emitter provably never asks for that width; the differ no longer does either. Same file, same defect class, mechanical, and the correct shape is pinned by the emitter itself — the "two halves disagreeing about which declarations count" class #11431 exists to close. The single-value width branch is untouched and pinned as untouched.Verification
Union re-run on the final commit
ecd52f335e.pnpm lint(whole repo,eslint . --no-inline-config) — exit 0, 2m03s. No narrowing.pnpm --filter @objectstack/driver-sql typecheck(tsc --noEmit) — exit 0.--listFilesconfirms the new suite is inside the program (the tsconfigincludessrc/**/*with no test exclusion), so it is genuinely type-checked, not a phantom check.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack), all exit 0:check:changeset-gate-self-tests,check:driver-conformance,check:objectui-changeset,check:published-files,check:slot-lookup,check:test-source-alias,check:type-source-resolution,check-adr-0087-registration,check-changeset-no-major,check-empty-changeset,check-plugin-teardown-shape,release-rehearsal-clone --self-test,check-affected-docs,check:query-options-erasure,check:engine-double-contract,check:cross-package-test-inputs,check:where-matcher,check:type-check-coverage,check:nul-bytes.Ablation — the pin can fail, and both legs were proven on disk
Mutation
const declaresJsonColumn = field.multiple === true→false, applied under atrap … EXIT INT TERMrestore.0, injected-marker1detectManagedDrift()case and thenarrow_varcharregression case0, original anchor1, clean treeNo rebuild leg: the suite imports the subject by a relative specifier (
./schema-drift.js) from inside its own package, so vitest resolves it tosrc/schema-drift.ts. Nothing reaches a dependency'sexports→dist/, which is the condition that makes a rebuild load-bearing.One declared narrowing
check:type-check-debt --re-measurewas not run; it requires the whole workspace closure built. Measured justification: it re-runstscper ledger entry, anddriver-sqlappears in neither theDEBTnor theEXEMPTblock, so this change cannot move a ledgered count. The structural half (check:type-check-coverage) passed, and the package's owntsc --noEmitis green over a program that provably includes the new file. CI runs the farm regardless.Generated by Claude Code