Skip to content

Commit 32be735

Browse files
huangyiireneclaude
andauthored
fix(driver-sql): honour storage.notNull on the multi-value column, the ADR-0113 site it returned above (#18630)
Fixes #17231 Clause-②: no Honouring a declared constraint is narrowing back to the contract (ADR-0113), not widening one — 「拉回已声明契约不触条款②」. No accept set grows and no public surface enlarges: a column whose metadata declared it constrained becomes constrained. ## The falsification triage asked for, answered FIRST Triage flagged its own premise as falsify-first: *does ADR-0113 intend multi-value / JSON columns to be in scope at all, or did the short-circuit predate and survive P0 deliberately?* **It does not falsify.** Four readings, taken on this branch's base: 1. **ADR-0113 carves out no field type.** Its D2 is unconditional — `storage.notNull` "is what emits `col.notNullable()` and what schema-drift compares against the physical column" — and the word `multiple` (and any JSON/array wording) appears nowhere in the record. Its P0 note names the site verbatim: 「the physical constraint now keys off the explicitly-authored `storage.notNull` at that same `#createColumn` site」, with `@objectstack/driver-sql` (`sql-driver.ts` column DDL, `schema-drift.ts`) listed as the consumer. 2. **The declaration is authorable, measured.** `storage.notNull`'s only declared exclusivity is `requiredWhen` (`FieldSchema.superRefine`, ADR-0113 Q1 rider). `{ type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true } }` parses green, and the `requiredWhen` pair is still refused — both asserted in the new suite, the second as the discriminating control. 3. **ADR-0113 is already implemented as in-scope for JSON columns at its OTHER named consumer in this package.** `diffManagedTable` compares `storage.notNull` against the physical column for every field `fieldHasColumn` answers true for, and that predicate answers multi-value FIRST. So the platform reported `nullability_mismatch` / `tighten_not_null` / **destructive** against a table it had itself just created, with no rows in it. A reading in which JSON columns are out of ADR-0113's scope has to call the differ wrong too. 4. **The nearest counter-reading is about DRIFT, not about the create path.** ADR-0113's Context row — 「imposing `NOT NULL` over possibly-null data is the classifier's `destructive` class」 — is the `tighten_not_null` ceremony over an EXISTING column, untouched here. `createColumn` runs on `CREATE TABLE` and on `ALTER TABLE ADD COLUMN`, so the column it constrains is always EMPTY, which is the same reason the string family's #11431 note gives for sizing a `varchar` at this very site. ⛔ NOT MEASURED, and the conclusion does not rest on it: the historical half of the question ("did the short-circuit survive P0 deliberately?"). This checkout is shallow (500 commits) and both `git log -S` probes collapse onto the shallow boundary commit, so no date ordering was read. What replaces it is (3): had P0 exempted multi-value columns, the exemption would be in the differ, and it is not — nor is any exemption recorded in `createColumn`, a method that records every one it does have (virtual `formula`, the FILE family's deployment fact, `unique` deferred to the tenancy-aware path, `options[].default`). ## What changed `SqlDriver.createColumn` decides the JSON column shape before its per-type switch and `return`ed there — above the ADR-0113 nullability line and above the column DEFAULT. The short-circuit decides the column TYPE; returning made it silently decide the column's CONSTRAINTS as well. The constraint predicate is now stated once, as `declaresColumnNotNull`, and asked at BOTH of `createColumn`'s exits. Truthiness, not `=== true`: that is the test the tail has always applied, and this moves WHERE the question is asked, never which values answer it. ⛔ The pin was not touched and the generators were not relaxed: `packages/cli` has no file in this diff. The multi-value short-circuit still decides `multiple` before the type switch, so `generate-multiple-json-column.pin.test.ts`'s source-read of this method reads the same two landmarks it always did. The column DEFAULT is still NOT emitted on this path. `os generate migration` skips it for the same recorded reason (`declaredColumnDefault`: the multi-value shape has no scalar DDL form), so the two producers already agreed there and nothing diverges. ## Evidence — driven, with the control fired Server-free, on three of the card's own probe shapes plus two controls, through `initObjects` into an in-memory `better-sqlite3` database read back with knex `columnInfo()`. Before the repair, on base `21b7c12b4f` — 6 passed, 3 failed: ``` × the multi-value column honours `storage.notNull` expected true to be false × an INSERT omitting the field is refused promise resolved instead of rejecting × the differ agrees with the writer [ { kind: 'nullability_mismatch', column: 'multi_nn', expected: 'NOT NULL', actual: 'NULL', category: 'destructive', op: { type: 'tighten_not_null' } } ] ✓ control — this reader can see a NOT NULL, and sees NULL where nothing declared one ✓ …and nothing else acquired the constraint: `multiple` and `required` still do not bind the column ``` The control is what makes the green afterwards a reading: `scalar_nn` (a scalar carrying the same `storage.notNull`) comes back NOT NULL in BOTH runs, so the harness was never blind to the constraint. After the repair: **9 passed, 2 skipped**, the two skips named below. Also asserted, so the repair cannot be bought with the type decision: the constrained multi-value column's type still equals the unconstrained multi-value column's, and still differs from the scalar's. ## Verification Run through the shared verify lock; exit codes captured before any pipe. | what | reading | |---|---| | `pnpm --filter @objectstack/driver-sql test` | **178 files passed, 11 skipped · 2627 tests passed, 168 skipped** (exit 0), at merged HEAD `c20356f0e1` | | `pnpm --filter @objectstack/driver-sql typecheck` | exit 0. The new suite is really in that program: `tsc --noEmit --listFiles` names it, one occurrence | | gate families, derived not guessed | `scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` at `c20356f0e1` ⇒ **61 families**, each run with its exit code recorded, reconciled with `--ran`: **61 derived, 61 run, 0 NOT-MEASURED, 0 UNRUN** | | gate results | 60 green. 1 red — `check:cross-package-test-inputs` — is NOT from this diff; the control is below | | spec-generated artefacts after merging `main` | `pnpm --filter @objectstack/spec build && check:generated` ⇒ "All 15 generated artifacts are up to date" | **The narrowing declared, with its three pieces of evidence.** `pnpm lint` is a repo-scale scan CI owns; what ran here is `eslint --no-inline-config --format json` over the two changed source files ⇒ **2 files, 0 errors, 0 warnings**, at `c20356f0e1`. (i) The population comes from eslint's own config — the base block matches `**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}`, so the third changed path, a `.changeset/*.md`, is outside eslint's population entirely; (ii) the file count is the JSON reporter's own array length, 2; (iii) this repo's single `eslint.config.mjs` never enables type-aware linting for ANY file (no `parserOptions.project`, no typed `@typescript-eslint` rules — stated and measured in its own header), so this diff cannot move the verdict on a file it does not contain. ### ✅ Dialect cells — ALL THREE executed, after the patch round > ⚠️ **Corrected by the `domain:engine` seat (`session_01CqmCgU5RGDoJYhHUMVp2af`) at landing.** This section previously read 「Locally **only `sqlite` executed**」 and 「⛔ Nothing here claims a Postgres or MySQL reading」. That was **true at `c20356f0e1`** and is **false at `dd723b6dbc`**: the patch round provisioned both live servers and ran them. The earlier text was an accurate reading of an earlier head, ⛔ not an error — it is superseded, not retracted. Durable record: report comment `5713337146`. The patch round provisioned CI's own conditions locally — a private **PostgreSQL 16.13** (`initdb` + `pg_ctl`, `timezone='Asia/Shanghai'`, CI's step-4 setting) and a private **MySQL 8.0.46** (`default_time_zone='+08:00'`, database `conformance`), process `TZ=America/New_York`, `OS_EXPECT_LIVE_DIALECT_MATRIX=1`. - **Targeted, all three cells**: **23 passed | 0 skipped** — 2 entrance tests plus 7 per cell × 3 cells. - **The job's own population** (the full `driver-sql` suite against **both** live servers, under those conditions): **189 files passed | 0 failed**, **4017 passed | 1 skipped**, exit 0. The single skip is pre-existing and ⛔ not a dialect cell (`schema-drift.base-type-mismatch.test.ts`, sqlite). ⭐ **What the CI red actually was, and it was this suite, ⛔ not the product.** MySQL refuses the omitting INSERT in a **third vocabulary** the original assertion did not carry. The driver omits the column on every dialect; SQLite and PostgreSQL then substitute the implicit NULL and report the *constraint* (`NOT NULL constraint failed` / `null value in column … violates not-null constraint`), while MySQL in strict mode refuses the **omission** before a NULL is ever considered: `ER_NO_DEFAULT_FOR_FIELD`, 「Field 'multi_nn' doesn't have a default value」. **One behaviour, three spellings.** ⇒ `sql-driver.ts` is **byte-identical** to the commit CI graded red (sha256 `5de174652a3515e2` at both `c20356f0e1` and `dd723b6dbc`). ⛔ No product change was needed and none was made. ⭐ **The repair keys the refusal PER DIALECT (`REFUSAL_BY_DIALECT`) rather than widening one alternation until it matches.** A widened regex would also swallow a refusal arriving for some *other* reason — a connection fault, a tenancy error — and the test would stay green through it. A dialect-independent leg was added beside it: the table's row count is unchanged across the rejection, which is the fact the card is actually about. ⛔ Nothing was skipped, disabled or quarantined; the assertion that ran and failed now runs and passes on the server that failed it. ## Acceptance notes **`pnpm check:cross-package-test-inputs` is red on this branch and the red is not this diff's** — it is build-state dependent and reproduces on pristine `origin/main`. Two legs, same gate, same message, on a detached worktree at `origin/main` (`d5e64d8d9d`) carrying none of this branch's changes: - as checked out, with no `packages/spec/dist/` on disk ⇒ **exit 0**, "OK: 29 package(s) read outside themselves, all declared"; - after a bare `mkdir -p packages/spec/dist/chunks` and nothing else ⇒ **exit 1**, `@objectstack/cli descends a directory tree from packages/spec/dist/ … rooted in packages/cli/test/init-created-files-summary.e2e.test.ts`. The gate's walked-root limb reads the filesystem (`uncoveredWalkRadius` → `readdirSync`) and drops a folded walk root that names no real directory, so the same tree passes before a build and fails after one. This diff touches neither that test, nor the glob table, nor any build output. Filed as a finding for the seat rather than repaired here: it is outside this card's file surface and outside its subject. Dedupe words: `cross-package-test-inputs`, `uncoveredWalkRadius`, `walked root`, `packages/spec/dist`, `build-state dependent`. **Noted, not filed:** nothing else. `createColumn`'s other early exits were read while establishing the two-exit shape and none of them owes the nullability line — `formula` emits no column at all, and the string / text / file arms all fall through to the shared tail. <sub>Body section corrected at landing by the `domain:engine` seat, `session_01CqmCgU5RGDoJYhHUMVp2af`. The rest of the body is the delivering dev's and is unedited.</sub> --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent cb04f45 commit 32be735

3 files changed

Lines changed: 345 additions & 2 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
`storage.notNull` now binds a multi-value column, as ADR-0113 says it does
6+
7+
`SqlDriver.createColumn` decides the JSON column shape before its per-type
8+
switch, and it `return`ed there — above the ADR-0113 nullability line and above
9+
the column DEFAULT. So `storage: { notNull: true }` on a multi-valued field was
10+
silently inert on the platform's own table, while both `os generate migration`
11+
formats emitted the constraint from the same declaration:
12+
13+
```
14+
{ d_multi_notnull: { type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true } } }
15+
16+
field driver sqlgen tsgen
17+
d_multi_notnull null=YES null=NO null=NO ← before
18+
d_multi_notnull null=NO null=NO null=NO ← after
19+
```
20+
21+
One declaration, two databases: an INSERT omitting the field was accepted by the
22+
platform's own table and refused by every table built from a generated
23+
migration.
24+
25+
ADR-0113 P0 names this site verbatim — 「the physical constraint now keys off the
26+
explicitly-authored `storage.notNull` at that same `#createColumn` site」 — and
27+
carves out no field type. `storage.notNull`'s only declared exclusivity is
28+
`requiredWhen`, at the parse seam, so `multiple: true` + `storage.notNull` is an
29+
authorable declaration this site was dropping on the floor. The differ, the
30+
ADR's other named consumer in this package, never had the gap: `fieldHasColumn`
31+
answers the multi-value question first and the nullability comparison then runs,
32+
so the platform reported DESTRUCTIVE `tighten_not_null` drift against tables it
33+
had just created itself, with no rows in them. That self-inflicted report is
34+
gone.
35+
36+
⚠️ Not the destructive ceremony ADR-0113 routes around. `createColumn` runs on
37+
`CREATE TABLE` and on `ALTER TABLE ADD COLUMN`, so the column constrained here
38+
is always EMPTY — the same reason the string family's #11431 note gives for
39+
sizing a `varchar` at this site. Imposing `NOT NULL` over an EXISTING column's
40+
possibly-null data stays `tighten_not_null`, destructive category, behind
41+
`os migrate apply --allow-destructive`, untouched.
42+
43+
⛔ Not a widening, and nothing else acquired the constraint: `multiple: true`
44+
alone still produces a nullable column, and `required: true` alone still does
45+
too — it is the write-time contract the engine enforces, never the column
46+
(ADR-0113). The column DEFAULT is still not emitted on this path either: the
47+
multi-value shape has no scalar DDL form, and `os generate migration` skips it
48+
for the same recorded reason, so the two producers already agreed there.
Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,243 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#17231] ADR-0113's column constraint reaches the MULTI-VALUE column too.
5+
*
6+
* ## The defect
7+
*
8+
* `SqlDriver.createColumn` decides the JSON column shape before its per-type
9+
* switch and used to `return` right there — above BOTH the nullability line and
10+
* the column DEFAULT. So `storage: { notNull: true }` on a multi-valued field
11+
* was silently inert on the platform's own table while both `os generate
12+
* migration` formats emitted the constraint (`"tags_nn" JSONB NOT NULL` /
13+
* `table.jsonb('tags_nn').notNullable()`, pinned in
14+
* `packages/cli/src/commands/generate-multiple-json-column.pin.test.ts`). An
15+
* INSERT omitting the field was accepted by the platform's table and refused by
16+
* both generated ones — one declaration, two databases.
17+
*
18+
* ## Which side moved, and why it is this one
19+
*
20+
* ADR-0113 P0 names the site verbatim — 「the physical constraint now keys off
21+
* the explicitly-authored `storage.notNull` at that same `#createColumn` site」 —
22+
* and lists `@objectstack/driver-sql` (`sql-driver.ts` column DDL,
23+
* `schema-drift.ts`) as its consumer. The ADR carves out no type: `storage.notNull`
24+
* is a field-level knob whose only declared exclusivity is `requiredWhen`
25+
* (`FieldSchema.superRefine`), and the ENTRANCE test below shows the multi-value
26+
* declaration parses green. So this was an implementation gap, not a decision:
27+
* 补实现, never narrowing the declaration at the consumer.
28+
*
29+
* ⚠️ The nearest counter-reading, answered rather than stepped over: ADR-0113's
30+
* Context row about the drift classifier records that 「imposing `NOT NULL` over
31+
* possibly-null data is the classifier's `destructive` class」. That is about an
32+
* EXISTING column acquiring a constraint — the `tighten_not_null` ceremony,
33+
* untouched here. `createColumn` runs on `CREATE TABLE` and on `ALTER TABLE ADD
34+
* COLUMN`, so the column it constrains is always empty, exactly as the #11431
35+
* note on the string-family arm already says for the varchar width.
36+
*
37+
* ## The parity that proves it internally
38+
*
39+
* ADR-0113's other named consumer in this package — `diffManagedTable` — already
40+
* compares `storage.notNull` against the physical column for every field
41+
* `fieldHasColumn` answers true for, and that includes multi-value columns
42+
* (`isMultiValueField` is its first question). Before this repair the platform
43+
* therefore reported DESTRUCTIVE `tighten_not_null` drift against a table it had
44+
* just created itself, on a table with no rows in it — the same self-inflicted
45+
* shape #11431 removed for `varchar` widths. The last test here is that parity,
46+
* driven: read the columns back out of the database the driver just built and
47+
* hand them to the differ.
48+
*
49+
* ## What this file deliberately does NOT assert
50+
*
51+
* A column DEFAULT on the multi-value path. Both generators skip it too
52+
* (`declaredColumnDefault` in `generate.ts`: 「the `multiple` shape has no scalar
53+
* DDL form」), so the producers already agree there and nothing diverges.
54+
*
55+
* @see SqlDriver.createColumn — the site ADR-0113 P0 names.
56+
* @see packages/cli/src/commands/generate-multiple-json-column.pin.test.ts — the
57+
* generator side of the same pair, which is the side that was already right.
58+
* @see https://github.com/objectstack-ai/objectstack/issues/17231
59+
* @see https://github.com/objectstack-ai/objectstack/issues/17469 (the ONE
60+
* definition of "multi-valued" this short-circuit now asks)
61+
*/
62+
63+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
64+
import type { Knex } from 'knex';
65+
import { FieldSchema } from '@objectstack/spec/data';
66+
import { SqlDriver } from './sql-driver.js';
67+
import { diffManagedTable, type FieldDef, type PhysicalColumn, type SqlDialectName } from './schema-drift.js';
68+
import {
69+
DIALECT_CELLS,
70+
declareUnprovisionedCell,
71+
type DialectCell,
72+
} from './live-dialect-matrix.testkit.js';
73+
74+
/** Issue-prefixed: the live cells share one database with every other suite here. */
75+
const PROBE_OBJECT = 'os17231_multi_notnull';
76+
77+
/**
78+
* The fixture, built as a 2×2 plus the harness control.
79+
*
80+
* multi_nn multi-valued + `storage.notNull` — THE SUBJECT
81+
* multi_plain multi-valued, no constraint — the column must stay nullable
82+
* multi_req multi-valued + `required` only — ADR-0113: the write contract
83+
* never binds the column
84+
* scalar_nn scalar + `storage.notNull` — the HARNESS control: proves
85+
* this reader can observe a
86+
* NOT NULL at all, so a green
87+
* subject is a measurement
88+
* scalar_plain scalar, no constraint — the column-TYPE control
89+
*/
90+
const PROBE_FIELDS: Record<string, Record<string, unknown>> = {
91+
multi_nn: { type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true } },
92+
multi_plain: { type: 'lookup', reference: 'sys_user', multiple: true },
93+
multi_req: { type: 'lookup', reference: 'sys_user', multiple: true, required: true },
94+
scalar_nn: { type: 'string', storage: { notNull: true } },
95+
scalar_plain: { type: 'string' },
96+
};
97+
98+
/** The differ speaks dialect names, the matrix speaks cell ids. */
99+
const DIALECT_OF: Record<string, SqlDialectName> = { sqlite: 'sqlite', pg: 'postgres', mysql: 'mysql' };
100+
101+
/**
102+
* ONE behaviour, three dialect spellings — measured, not transcribed.
103+
*
104+
* The write omits the column (no field, no value), so each engine refuses it in
105+
* its own vocabulary: SQLite and PostgreSQL substitute the column's implicit
106+
* NULL and report the constraint, while MySQL in strict mode refuses the
107+
* omission itself before a NULL is ever considered — `ER_NO_DEFAULT_FOR_FIELD`,
108+
* "Field 'multi_nn' doesn't have a default value". ⛔ Not a per-dialect
109+
* behaviour difference and ⛔ not a reason to widen one regex until it matches
110+
* anything: all three REFUSE, which is the fact this cell exists to pin, and
111+
* the row-count leg beside it states that dialect-independently. Keyed per cell
112+
* so a refusal arriving for some OTHER reason — a connection fault, a tenancy
113+
* error — still reddens this test instead of satisfying it.
114+
*
115+
* ⚠️ A `required: true` field would be refused earlier, by the record validator,
116+
* with an ADR-0112 envelope. This declaration deliberately carries the storage
117+
* constraint ALONE, which ADR-0113 says reaches the database as a raw driver
118+
* error — so there is no envelope here to assert, and inventing one would pin a
119+
* seam this card does not touch.
120+
*/
121+
const REFUSAL_BY_DIALECT: Record<string, RegExp> = {
122+
sqlite: /NOT NULL constraint failed/i,
123+
pg: /null value in column .* violates not-null constraint/i,
124+
mysql: /doesn't have a default value|cannot be null/i,
125+
};
126+
127+
describe('[#17231] the entrance — the declaration under repair is authorable', () => {
128+
/**
129+
* The reachability half. A rule about a VALUE (`storage.notNull === true`)
130+
* means nothing if the spec refuses the declaration carrying it, so this is
131+
* asserted as a full parse rather than as an absence of `unrecognized_keys`.
132+
* ADR-0113 gives `storage.notNull` exactly one exclusivity — `requiredWhen` —
133+
* and `multiple` is not it.
134+
*/
135+
it('`multiple: true` + `storage: { notNull: true }` parses green', () => {
136+
const parsed = FieldSchema.safeParse({
137+
name: 'tags_nn', type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true },
138+
});
139+
expect(parsed.success, 'the card\'s own declaration must be authorable for this repair to have a population').toBe(true);
140+
});
141+
142+
it('…and the one exclusivity ADR-0113 does declare is still refused — the discriminating control', () => {
143+
const refused = FieldSchema.safeParse({
144+
name: 'tags_when', type: 'lookup', reference: 'sys_user', multiple: true,
145+
requiredWhen: { field: 'stage', operator: '=', value: 'closed' },
146+
storage: { notNull: true },
147+
});
148+
expect(refused.success, '`storage.notNull` × `requiredWhen` is rejected at the parse seam (ADR-0113 Q1 rider)').toBe(false);
149+
});
150+
});
151+
152+
for (const cell of DIALECT_CELLS) {
153+
if (!cell.available) {
154+
declareUnprovisionedCell(cell, '[#17231] multi-value column nullability');
155+
continue;
156+
}
157+
declareMultiValueNullability(cell);
158+
}
159+
160+
function declareMultiValueNullability(cell: DialectCell): void {
161+
describe(`[#17231] SqlDriver.createColumn — storage.notNull on a JSON column (${cell.label})`, () => {
162+
let driver: SqlDriver;
163+
let knexInstance: Knex;
164+
let info: Record<string, { nullable: boolean; type: string }>;
165+
166+
beforeAll(async () => {
167+
driver = new SqlDriver(cell.config());
168+
knexInstance = driver.getKnex();
169+
await knexInstance.schema.dropTableIfExists(PROBE_OBJECT);
170+
await driver.initObjects([{ name: PROBE_OBJECT, fields: PROBE_FIELDS } as never]);
171+
info = (await knexInstance(PROBE_OBJECT).columnInfo()) as typeof info;
172+
});
173+
174+
afterAll(async () => {
175+
await knexInstance?.schema.dropTableIfExists(PROBE_OBJECT).catch(() => {});
176+
await driver?.disconnect?.();
177+
});
178+
179+
it('control — the table exists and every probe column reached it', () => {
180+
for (const name of Object.keys(PROBE_FIELDS)) {
181+
expect(info, `${name} never became a column, so nothing below measures anything`).toHaveProperty(name);
182+
}
183+
});
184+
185+
it('control — this reader can see a NOT NULL, and sees NULL where nothing declared one', () => {
186+
// Without this pair a green subject below is indistinguishable from a
187+
// reader that reports `nullable: false` for everything (or for nothing).
188+
expect(info.scalar_nn.nullable, 'the scalar half of ADR-0113 P0').toBe(false);
189+
expect(info.scalar_plain.nullable).toBe(true);
190+
});
191+
192+
it('the multi-value column honours `storage.notNull` — the repair', () => {
193+
expect(
194+
info.multi_nn.nullable,
195+
'createColumn returned at the multi-value short-circuit before reaching the ADR-0113 ' +
196+
'nullability line, so the platform left open a column both migration generators constrain',
197+
).toBe(false);
198+
});
199+
200+
it('…and nothing else acquired the constraint: `multiple` and `required` still do not bind the column', () => {
201+
expect(info.multi_plain.nullable, 'the flag is not a constraint').toBe(true);
202+
expect(info.multi_req.nullable, 'ADR-0113: `required` is the write contract, not the column').toBe(true);
203+
});
204+
205+
it('the column is still a JSON column — the constraint rides the short-circuit, it does not replace it', () => {
206+
// The repair must not cost the type decision the short-circuit exists for.
207+
expect(info.multi_nn.type, 'the constrained multi-value column changed shape').toBe(info.multi_plain.type);
208+
expect(info.multi_nn.type).not.toBe(info.scalar_plain.type);
209+
});
210+
211+
it('an INSERT omitting the field is refused — the divergence the card measured, closed', async () => {
212+
const rows = async (): Promise<number> =>
213+
Number(((await knexInstance(PROBE_OBJECT).count({ n: '*' })) as Array<{ n: unknown }>)[0].n);
214+
const before = await rows();
215+
216+
await expect(
217+
driver.create(PROBE_OBJECT, { scalar_nn: 'present' }),
218+
).rejects.toThrow(REFUSAL_BY_DIALECT[cell.id]);
219+
220+
// The dialect-independent half: a rejection that still wrote the row would
221+
// be the same green as a refusal, and it is the WRITE that this card is
222+
// about — the platform's own table now refuses what every table built
223+
// from a generated migration already refused.
224+
expect(await rows(), 'the refused INSERT landed anyway').toBe(before);
225+
});
226+
227+
it('the differ agrees with the writer — no nullability drift against a table the driver just built', () => {
228+
const columns: PhysicalColumn[] = Object.entries(info).map(([name, c]) => ({
229+
name, type: String(c.type), nullable: c.nullable,
230+
}));
231+
const drift = diffManagedTable({
232+
table: PROBE_OBJECT,
233+
fields: PROBE_FIELDS as unknown as Record<string, FieldDef>,
234+
columns,
235+
dialect: DIALECT_OF[cell.id],
236+
}).filter((d) => d.kind === 'nullability_mismatch');
237+
expect(
238+
drift,
239+
'the platform reported drift against its own freshly-created, empty table — the #11431 shape',
240+
).toEqual([]);
241+
});
242+
});
243+
}

‎packages/drivers/driver-sql/src/sql-driver.ts‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1788,6 +1788,28 @@ function isMultiValuedColumn(type: string, field: { multiple?: unknown } | null
17881788
return isMultiValueField({ type, multiple: field?.multiple === true });
17891789
}
17901790

1791+
/**
1792+
* [#17231] ADR-0113's physical column constraint, asked once for every column
1793+
* {@link SqlDriver.createColumn} builds.
1794+
*
1795+
* It exists because `createColumn` has TWO exits: the multi-value short-circuit
1796+
* above its per-type switch, and the shared tail below it. The constraint used
1797+
* to be read only at the tail, so `storage: { notNull: true }` on a multi-valued
1798+
* field was silently inert on the platform's own table while BOTH
1799+
* `os generate migration` formats emitted it — one declaration, two databases,
1800+
* an INSERT omitting the field accepted by the platform's table and refused by
1801+
* every generated one. One exit acquiring the constraint and the other not is
1802+
* exactly how that happened, so the predicate is stated once here instead of
1803+
* being spelled at each exit.
1804+
*
1805+
* Truthiness rather than `=== true` deliberately: that is the test the tail has
1806+
* always applied, and this move is about WHERE the question is asked, never
1807+
* about which values answer it.
1808+
*/
1809+
function declaresColumnNotNull(field: unknown): boolean {
1810+
return Boolean((field as { storage?: { notNull?: boolean } } | null | undefined)?.storage?.notNull);
1811+
}
1812+
17911813
/**
17921814
* [#16319] DDL-time defence: a field declaration with NO `type` gets no column
17931815
* — it gets a refusal.
@@ -17382,8 +17404,34 @@ export class SqlDriver implements IDataDriver {
1738217404
// reader now asks. A field whose `multiple` the spec does not recognise on
1738317405
// its type no longer gets a JSON column here — and `FieldSchema` refuses
1738417406
// that declaration at the authoring entrance in the same ruling.
17407+
//
17408+
// [#17231] The short-circuit decides the column TYPE; it does not decide
17409+
// the column's CONSTRAINTS, and returning here used to mean it silently
17410+
// did. ADR-0113 P0 names this site verbatim — 「the physical constraint now
17411+
// keys off the explicitly-authored `storage.notNull` at that same
17412+
// `#createColumn` site」 — and carves out no field type: `storage.notNull`'s
17413+
// only declared exclusivity is `requiredWhen` (`FieldSchema.superRefine`),
17414+
// so `multiple: true` + `storage: { notNull: true }` is an authorable
17415+
// declaration this site was dropping on the floor. The differ, ADR-0113's
17416+
// other named consumer in this package, never had the gap — `fieldHasColumn`
17417+
// answers multi-value first and the nullability comparison then runs — so
17418+
// the platform reported DESTRUCTIVE `tighten_not_null` drift against tables
17419+
// it had itself just created, with no rows in them.
17420+
//
17421+
// ⚠️ NOT the drift ceremony this ADR routes around: `createColumn` runs on
17422+
// `CREATE TABLE` and on `ALTER TABLE ADD COLUMN`, so the column constrained
17423+
// here is always EMPTY — the same reason the string family's #11431 note
17424+
// below gives for narrowing a varchar here. Imposing `NOT NULL` over
17425+
// possibly-null data stays `tighten_not_null`, destructive, behind
17426+
// `os migrate apply --allow-destructive`, untouched.
17427+
//
17428+
// The column DEFAULT is deliberately still NOT emitted on this path: the
17429+
// multi-value shape has no scalar DDL form, and `os generate migration`
17430+
// skips it for the same reason (`declaredColumnDefault`), so the two
17431+
// producers already agree.
1738517432
if (isMultiValuedColumn(String(field.type ?? ''), field)) {
17386-
this.jsonColumn(table, name);
17433+
const jsonCol = this.jsonColumn(table, name);
17434+
if (declaresColumnNotNull(field)) jsonCol.notNullable();
1738717435
return;
1738817436
}
1738917437

@@ -17790,7 +17838,11 @@ export class SqlDriver implements IDataDriver {
1779017838
// its author wrote `storage: { notNull: true }`, and for no other
1779117839
// reason; a `required: true` field with no `storage` block gets a
1779217840
// nullable column, at every protocol floor, on every dialect.
17793-
if ((field as { storage?: { notNull?: boolean } }).storage?.notNull) col.notNullable();
17841+
//
17842+
// [#17231] Asked through {@link declaresColumnNotNull} — the same
17843+
// question the multi-value short-circuit above now asks, stated once so
17844+
// this method's two exits cannot answer it differently again.
17845+
if (declaresColumnNotNull(field)) col.notNullable();
1779417846
this.applyDeclaredColumnDefault(col, field, type);
1779517847
}
1779617848
}

0 commit comments

Comments
 (0)