diff --git a/.changeset/17231-multi-value-column-storage-notnull.md b/.changeset/17231-multi-value-column-storage-notnull.md new file mode 100644 index 00000000000..23160665027 --- /dev/null +++ b/.changeset/17231-multi-value-column-storage-notnull.md @@ -0,0 +1,48 @@ +--- +'@objectstack/driver-sql': patch +--- + +`storage.notNull` now binds a multi-value column, as ADR-0113 says it does + +`SqlDriver.createColumn` decides the JSON column shape before its per-type +switch, and it `return`ed there — above the ADR-0113 nullability line and above +the column DEFAULT. So `storage: { notNull: true }` on a multi-valued field was +silently inert on the platform's own table, while both `os generate migration` +formats emitted the constraint from the same declaration: + +``` +{ d_multi_notnull: { type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true } } } + +field driver sqlgen tsgen +d_multi_notnull null=YES null=NO null=NO ← before +d_multi_notnull null=NO null=NO null=NO ← after +``` + +One declaration, two databases: an INSERT omitting the field was accepted by the +platform's own table and refused by every table built from a generated +migration. + +ADR-0113 P0 names this site verbatim — 「the physical constraint now keys off the +explicitly-authored `storage.notNull` at that same `#createColumn` site」 — and +carves out no field type. `storage.notNull`'s only declared exclusivity is +`requiredWhen`, at the parse seam, so `multiple: true` + `storage.notNull` is an +authorable declaration this site was dropping on the floor. The differ, the +ADR's other named consumer in this package, never had the gap: `fieldHasColumn` +answers the multi-value question first and the nullability comparison then runs, +so the platform reported DESTRUCTIVE `tighten_not_null` drift against tables it +had just created itself, with no rows in them. That self-inflicted report is +gone. + +⚠️ Not the destructive ceremony ADR-0113 routes around. `createColumn` runs on +`CREATE TABLE` and on `ALTER TABLE ADD COLUMN`, so the column constrained here +is always EMPTY — the same reason the string family's #11431 note gives for +sizing a `varchar` at this site. Imposing `NOT NULL` over an EXISTING column's +possibly-null data stays `tighten_not_null`, destructive category, behind +`os migrate apply --allow-destructive`, untouched. + +⛔ Not a widening, and nothing else acquired the constraint: `multiple: true` +alone still produces a nullable column, and `required: true` alone still does +too — it is the write-time contract the engine enforces, never the column +(ADR-0113). The column DEFAULT is still not emitted on this path either: the +multi-value shape has no scalar DDL form, and `os generate migration` skips it +for the same recorded reason, so the two producers already agreed there. diff --git a/packages/drivers/driver-sql/src/sql-driver-17231-multi-value-not-null.test.ts b/packages/drivers/driver-sql/src/sql-driver-17231-multi-value-not-null.test.ts new file mode 100644 index 00000000000..e2a845cedc8 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-17231-multi-value-not-null.test.ts @@ -0,0 +1,243 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17231] ADR-0113's column constraint reaches the MULTI-VALUE column too. + * + * ## The defect + * + * `SqlDriver.createColumn` decides the JSON column shape before its per-type + * switch and used to `return` right there — above BOTH the nullability line and + * the column DEFAULT. So `storage: { notNull: true }` on a multi-valued field + * was silently inert on the platform's own table while both `os generate + * migration` formats emitted the constraint (`"tags_nn" JSONB NOT NULL` / + * `table.jsonb('tags_nn').notNullable()`, pinned in + * `packages/cli/src/commands/generate-multiple-json-column.pin.test.ts`). An + * INSERT omitting the field was accepted by the platform's table and refused by + * both generated ones — one declaration, two databases. + * + * ## Which side moved, and why it is this one + * + * ADR-0113 P0 names the site verbatim — 「the physical constraint now keys off + * the explicitly-authored `storage.notNull` at that same `#createColumn` site」 — + * and lists `@objectstack/driver-sql` (`sql-driver.ts` column DDL, + * `schema-drift.ts`) as its consumer. The ADR carves out no type: `storage.notNull` + * is a field-level knob whose only declared exclusivity is `requiredWhen` + * (`FieldSchema.superRefine`), and the ENTRANCE test below shows the multi-value + * declaration parses green. So this was an implementation gap, not a decision: + * 补实现, never narrowing the declaration at the consumer. + * + * ⚠️ The nearest counter-reading, answered rather than stepped over: ADR-0113's + * Context row about the drift classifier records that 「imposing `NOT NULL` over + * possibly-null data is the classifier's `destructive` class」. That is about an + * EXISTING column acquiring a constraint — the `tighten_not_null` ceremony, + * untouched here. `createColumn` runs on `CREATE TABLE` and on `ALTER TABLE ADD + * COLUMN`, so the column it constrains is always empty, exactly as the #11431 + * note on the string-family arm already says for the varchar width. + * + * ## The parity that proves it internally + * + * ADR-0113's other named consumer in this package — `diffManagedTable` — already + * compares `storage.notNull` against the physical column for every field + * `fieldHasColumn` answers true for, and that includes multi-value columns + * (`isMultiValueField` is its first question). Before this repair the platform + * therefore reported DESTRUCTIVE `tighten_not_null` drift against a table it had + * just created itself, on a table with no rows in it — the same self-inflicted + * shape #11431 removed for `varchar` widths. The last test here is that parity, + * driven: read the columns back out of the database the driver just built and + * hand them to the differ. + * + * ## What this file deliberately does NOT assert + * + * A column DEFAULT on the multi-value path. Both generators skip it too + * (`declaredColumnDefault` in `generate.ts`: 「the `multiple` shape has no scalar + * DDL form」), so the producers already agree there and nothing diverges. + * + * @see SqlDriver.createColumn — the site ADR-0113 P0 names. + * @see packages/cli/src/commands/generate-multiple-json-column.pin.test.ts — the + * generator side of the same pair, which is the side that was already right. + * @see https://github.com/objectstack-ai/objectstack/issues/17231 + * @see https://github.com/objectstack-ai/objectstack/issues/17469 (the ONE + * definition of "multi-valued" this short-circuit now asks) + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { Knex } from 'knex'; +import { FieldSchema } from '@objectstack/spec/data'; +import { SqlDriver } from './sql-driver.js'; +import { diffManagedTable, type FieldDef, type PhysicalColumn, type SqlDialectName } from './schema-drift.js'; +import { + DIALECT_CELLS, + declareUnprovisionedCell, + type DialectCell, +} from './live-dialect-matrix.testkit.js'; + +/** Issue-prefixed: the live cells share one database with every other suite here. */ +const PROBE_OBJECT = 'os17231_multi_notnull'; + +/** + * The fixture, built as a 2×2 plus the harness control. + * + * multi_nn multi-valued + `storage.notNull` — THE SUBJECT + * multi_plain multi-valued, no constraint — the column must stay nullable + * multi_req multi-valued + `required` only — ADR-0113: the write contract + * never binds the column + * scalar_nn scalar + `storage.notNull` — the HARNESS control: proves + * this reader can observe a + * NOT NULL at all, so a green + * subject is a measurement + * scalar_plain scalar, no constraint — the column-TYPE control + */ +const PROBE_FIELDS: Record> = { + multi_nn: { type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true } }, + multi_plain: { type: 'lookup', reference: 'sys_user', multiple: true }, + multi_req: { type: 'lookup', reference: 'sys_user', multiple: true, required: true }, + scalar_nn: { type: 'string', storage: { notNull: true } }, + scalar_plain: { type: 'string' }, +}; + +/** The differ speaks dialect names, the matrix speaks cell ids. */ +const DIALECT_OF: Record = { sqlite: 'sqlite', pg: 'postgres', mysql: 'mysql' }; + +/** + * ONE behaviour, three dialect spellings — measured, not transcribed. + * + * The write omits the column (no field, no value), so each engine refuses it in + * its own vocabulary: SQLite and PostgreSQL substitute the column's implicit + * NULL and report the constraint, while MySQL in strict mode refuses the + * omission itself before a NULL is ever considered — `ER_NO_DEFAULT_FOR_FIELD`, + * "Field 'multi_nn' doesn't have a default value". ⛔ Not a per-dialect + * behaviour difference and ⛔ not a reason to widen one regex until it matches + * anything: all three REFUSE, which is the fact this cell exists to pin, and + * the row-count leg beside it states that dialect-independently. Keyed per cell + * so a refusal arriving for some OTHER reason — a connection fault, a tenancy + * error — still reddens this test instead of satisfying it. + * + * ⚠️ A `required: true` field would be refused earlier, by the record validator, + * with an ADR-0112 envelope. This declaration deliberately carries the storage + * constraint ALONE, which ADR-0113 says reaches the database as a raw driver + * error — so there is no envelope here to assert, and inventing one would pin a + * seam this card does not touch. + */ +const REFUSAL_BY_DIALECT: Record = { + sqlite: /NOT NULL constraint failed/i, + pg: /null value in column .* violates not-null constraint/i, + mysql: /doesn't have a default value|cannot be null/i, +}; + +describe('[#17231] the entrance — the declaration under repair is authorable', () => { + /** + * The reachability half. A rule about a VALUE (`storage.notNull === true`) + * means nothing if the spec refuses the declaration carrying it, so this is + * asserted as a full parse rather than as an absence of `unrecognized_keys`. + * ADR-0113 gives `storage.notNull` exactly one exclusivity — `requiredWhen` — + * and `multiple` is not it. + */ + it('`multiple: true` + `storage: { notNull: true }` parses green', () => { + const parsed = FieldSchema.safeParse({ + name: 'tags_nn', type: 'lookup', reference: 'sys_user', multiple: true, storage: { notNull: true }, + }); + expect(parsed.success, 'the card\'s own declaration must be authorable for this repair to have a population').toBe(true); + }); + + it('…and the one exclusivity ADR-0113 does declare is still refused — the discriminating control', () => { + const refused = FieldSchema.safeParse({ + name: 'tags_when', type: 'lookup', reference: 'sys_user', multiple: true, + requiredWhen: { field: 'stage', operator: '=', value: 'closed' }, + storage: { notNull: true }, + }); + expect(refused.success, '`storage.notNull` × `requiredWhen` is rejected at the parse seam (ADR-0113 Q1 rider)').toBe(false); + }); +}); + +for (const cell of DIALECT_CELLS) { + if (!cell.available) { + declareUnprovisionedCell(cell, '[#17231] multi-value column nullability'); + continue; + } + declareMultiValueNullability(cell); +} + +function declareMultiValueNullability(cell: DialectCell): void { + describe(`[#17231] SqlDriver.createColumn — storage.notNull on a JSON column (${cell.label})`, () => { + let driver: SqlDriver; + let knexInstance: Knex; + let info: Record; + + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + knexInstance = driver.getKnex(); + await knexInstance.schema.dropTableIfExists(PROBE_OBJECT); + await driver.initObjects([{ name: PROBE_OBJECT, fields: PROBE_FIELDS } as never]); + info = (await knexInstance(PROBE_OBJECT).columnInfo()) as typeof info; + }); + + afterAll(async () => { + await knexInstance?.schema.dropTableIfExists(PROBE_OBJECT).catch(() => {}); + await driver?.disconnect?.(); + }); + + it('control — the table exists and every probe column reached it', () => { + for (const name of Object.keys(PROBE_FIELDS)) { + expect(info, `${name} never became a column, so nothing below measures anything`).toHaveProperty(name); + } + }); + + it('control — this reader can see a NOT NULL, and sees NULL where nothing declared one', () => { + // Without this pair a green subject below is indistinguishable from a + // reader that reports `nullable: false` for everything (or for nothing). + expect(info.scalar_nn.nullable, 'the scalar half of ADR-0113 P0').toBe(false); + expect(info.scalar_plain.nullable).toBe(true); + }); + + it('the multi-value column honours `storage.notNull` — the repair', () => { + expect( + info.multi_nn.nullable, + 'createColumn returned at the multi-value short-circuit before reaching the ADR-0113 ' + + 'nullability line, so the platform left open a column both migration generators constrain', + ).toBe(false); + }); + + it('…and nothing else acquired the constraint: `multiple` and `required` still do not bind the column', () => { + expect(info.multi_plain.nullable, 'the flag is not a constraint').toBe(true); + expect(info.multi_req.nullable, 'ADR-0113: `required` is the write contract, not the column').toBe(true); + }); + + it('the column is still a JSON column — the constraint rides the short-circuit, it does not replace it', () => { + // The repair must not cost the type decision the short-circuit exists for. + expect(info.multi_nn.type, 'the constrained multi-value column changed shape').toBe(info.multi_plain.type); + expect(info.multi_nn.type).not.toBe(info.scalar_plain.type); + }); + + it('an INSERT omitting the field is refused — the divergence the card measured, closed', async () => { + const rows = async (): Promise => + Number(((await knexInstance(PROBE_OBJECT).count({ n: '*' })) as Array<{ n: unknown }>)[0].n); + const before = await rows(); + + await expect( + driver.create(PROBE_OBJECT, { scalar_nn: 'present' }), + ).rejects.toThrow(REFUSAL_BY_DIALECT[cell.id]); + + // The dialect-independent half: a rejection that still wrote the row would + // be the same green as a refusal, and it is the WRITE that this card is + // about — the platform's own table now refuses what every table built + // from a generated migration already refused. + expect(await rows(), 'the refused INSERT landed anyway').toBe(before); + }); + + it('the differ agrees with the writer — no nullability drift against a table the driver just built', () => { + const columns: PhysicalColumn[] = Object.entries(info).map(([name, c]) => ({ + name, type: String(c.type), nullable: c.nullable, + })); + const drift = diffManagedTable({ + table: PROBE_OBJECT, + fields: PROBE_FIELDS as unknown as Record, + columns, + dialect: DIALECT_OF[cell.id], + }).filter((d) => d.kind === 'nullability_mismatch'); + expect( + drift, + 'the platform reported drift against its own freshly-created, empty table — the #11431 shape', + ).toEqual([]); + }); + }); +} diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 3ca63ecc514..2a17fc14ab7 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -1788,6 +1788,28 @@ function isMultiValuedColumn(type: string, field: { multiple?: unknown } | null return isMultiValueField({ type, multiple: field?.multiple === true }); } +/** + * [#17231] ADR-0113's physical column constraint, asked once for every column + * {@link SqlDriver.createColumn} builds. + * + * It exists because `createColumn` has TWO exits: the multi-value short-circuit + * above its per-type switch, and the shared tail below it. The constraint used + * to be read only at the tail, so `storage: { notNull: true }` on a multi-valued + * field was silently inert on the platform's own table while BOTH + * `os generate migration` formats emitted it — one declaration, two databases, + * an INSERT omitting the field accepted by the platform's table and refused by + * every generated one. One exit acquiring the constraint and the other not is + * exactly how that happened, so the predicate is stated once here instead of + * being spelled at each exit. + * + * Truthiness rather than `=== true` deliberately: that is the test the tail has + * always applied, and this move is about WHERE the question is asked, never + * about which values answer it. + */ +function declaresColumnNotNull(field: unknown): boolean { + return Boolean((field as { storage?: { notNull?: boolean } } | null | undefined)?.storage?.notNull); +} + /** * [#16319] DDL-time defence: a field declaration with NO `type` gets no column * — it gets a refusal. @@ -17382,8 +17404,34 @@ export class SqlDriver implements IDataDriver { // reader now asks. A field whose `multiple` the spec does not recognise on // its type no longer gets a JSON column here — and `FieldSchema` refuses // that declaration at the authoring entrance in the same ruling. + // + // [#17231] The short-circuit decides the column TYPE; it does not decide + // the column's CONSTRAINTS, and returning here used to mean it silently + // did. ADR-0113 P0 names this site verbatim — 「the physical constraint now + // keys off the explicitly-authored `storage.notNull` at that same + // `#createColumn` site」 — and carves out no field type: `storage.notNull`'s + // only declared exclusivity is `requiredWhen` (`FieldSchema.superRefine`), + // so `multiple: true` + `storage: { notNull: true }` is an authorable + // declaration this site was dropping on the floor. The differ, ADR-0113's + // other named consumer in this package, never had the gap — `fieldHasColumn` + // answers multi-value first and the nullability comparison then runs — so + // the platform reported DESTRUCTIVE `tighten_not_null` drift against tables + // it had itself just created, with no rows in them. + // + // ⚠️ NOT the drift ceremony this ADR routes around: `createColumn` runs on + // `CREATE TABLE` and on `ALTER TABLE ADD COLUMN`, so the column constrained + // here is always EMPTY — the same reason the string family's #11431 note + // below gives for narrowing a varchar here. Imposing `NOT NULL` over + // possibly-null data stays `tighten_not_null`, destructive, behind + // `os migrate apply --allow-destructive`, untouched. + // + // The column DEFAULT is deliberately still NOT emitted on this path: the + // multi-value shape has no scalar DDL form, and `os generate migration` + // skips it for the same reason (`declaredColumnDefault`), so the two + // producers already agree. if (isMultiValuedColumn(String(field.type ?? ''), field)) { - this.jsonColumn(table, name); + const jsonCol = this.jsonColumn(table, name); + if (declaresColumnNotNull(field)) jsonCol.notNullable(); return; } @@ -17790,7 +17838,11 @@ export class SqlDriver implements IDataDriver { // its author wrote `storage: { notNull: true }`, and for no other // reason; a `required: true` field with no `storage` block gets a // nullable column, at every protocol floor, on every dialect. - if ((field as { storage?: { notNull?: boolean } }).storage?.notNull) col.notNullable(); + // + // [#17231] Asked through {@link declaresColumnNotNull} — the same + // question the multi-value short-circuit above now asks, stated once so + // this method's two exits cannot answer it differently again. + if (declaresColumnNotNull(field)) col.notNullable(); this.applyDeclaredColumnDefault(col, field, type); } }