From 61e8174a2315e36d59f16019b1a149588ed3e475 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 08:39:50 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(objectql):=20enforce=20the=20select=20?= =?UTF-8?q?idiom=20=E2=80=94=20the=20option=20marked=20`default:=20true`?= =?UTF-8?q?=20is=20the=20field=20default=20(#7246)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SelectOption.default` was authorable, spec-valid, and read by nothing on the insert path: `applyFieldDefaults` resolved `f.defaultValue` and never looked at `options`, so a create that omitted the field stored null instead of the marked option. The key's only consumer anywhere was lint's `isNullableField`, which concluded the column was always-valued — a build-breaking verdict resting on a declaration the engine did not honour. Executes the maintainer ruling on #7246 (ADR-0049 enforce leg): - `applyFieldDefaults` falls back to the marked option when the field declares no `defaultValue`; `defaultValue` wins when both are declared. - The fallback resolves inside the `defaultValue == null` arm, downstream of the token/envelope branches, so an option value is always a plain literal. - `multiple: true` assembles an array; a single-valued field takes the first marked option; the canonical `default` spelling only, no `type` gate — the same shape lint reads, so the two agree by construction. - No physical column DEFAULT for an option-default; the reasoning is recorded on `SqlDriver.applyDeclaredColumnDefault` and pinned by test. - `isNullableField`'s prose now records what grounds it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2 --- .changeset/select-option-default-enforced.md | 64 +++ .../src/sql-driver-option-default-ddl.test.ts | 124 ++++++ packages/drivers/driver-sql/src/sql-driver.ts | 35 ++ packages/lint/src/validate-expressions.ts | 12 + .../src/engine-select-option-default.test.ts | 369 ++++++++++++++++++ packages/objectql/src/engine.ts | 101 ++++- 6 files changed, 703 insertions(+), 2 deletions(-) create mode 100644 .changeset/select-option-default-enforced.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-option-default-ddl.test.ts create mode 100644 packages/objectql/src/engine-select-option-default.test.ts diff --git a/.changeset/select-option-default-enforced.md b/.changeset/select-option-default-enforced.md new file mode 100644 index 0000000000..95d1ba9f2b --- /dev/null +++ b/.changeset/select-option-default-enforced.md @@ -0,0 +1,64 @@ +--- +"@objectstack/objectql": minor +--- + +feat(objectql): the option marked `default: true` is now the field's default on insert (#7246) + +`SelectOption.default` has been authorable and spec-valid since the schema was +written, and nothing on the insert path read it. `ObjectQL.applyFieldDefaults` +resolved `f.defaultValue` — Expression envelopes, the `DEFAULT_VALUE_TOKENS` +family, then static literals — and never looked at `options`. So this, which +reads like a declaration of the initial value: + +```ts +status: Field.select({ + label: 'Status', + options: [ + { label: 'Draft', value: 'draft', default: true }, + { label: 'Approved', value: 'approved' }, + ], +}), +``` + +stored **null** on a create that omitted the field, not `draft`. + +The key's one consumer anywhere in the repo was lint's `isNullableField`, which +concluded from it that the column was **always valued** — and that verdict is +build-breaking. So the single place that read the key trusted it, while the +place that would have made it true ignored it: a predicate over such a field +could be silenced by a heuristic resting on a declaration nothing honoured. + +**After** (maintainer ruling on #7246, ADR-0049 enforce leg): a field that +declares no `defaultValue` falls back to the option marked `default: true`, on +every driver, resolved by the engine exactly as the token family is. + +- **`defaultValue` wins when both are declared** — the more specific + declaration. It names a value for *this* field; the option flag describes the + shared option list. When the two disagree the flag stays inert, as it was + everywhere before. +- **Presence is the engine's own `dv == null` test.** `defaultValue: ''` is a + real default and still wins; the fallback fires only when `defaultValue` is + absent by that test. +- **The fallback resolves in the `defaultValue == null` arm**, downstream of the + token and envelope branches, so an option value is always a plain literal — an + option spelled `current_user` stores those twelve characters rather than the + acting user's id. +- **`multiple: true` assembles an array** of every marked option in declaration + order, because that field stores an Array/JSON; a single-valued field with + several marked options takes the first. +- **No physical column DEFAULT** is emitted for an option-default. The engine is + the one place the two spellings are ranked, the multi-select shape has no + scalar DDL form, and emitting would give new databases a default that older + ones on identical metadata lack with nothing to report the divergence. The + reasoning is recorded on `SqlDriver.applyDeclaredColumnDefault` and pinned by + test. + +**Migration.** Metadata declaring an option `default: true` on a field with no +`defaultValue` changes insert behaviour: records that used to be born with a +null in that column are now born with the marked option. That is the behaviour +the declaration always described. In the shipped corpus this covers 30 fields +across the showcase, CRM and todo example apps and the downstream-contract QA +fixture — all of them status/stage/priority selects whose marked option is the +intended initial state. To keep the previous behaviour, drop the `default: true` +flag from the option; to make the value explicit, declare `defaultValue` +alongside it, which now outranks the flag. diff --git a/packages/drivers/driver-sql/src/sql-driver-option-default-ddl.test.ts b/packages/drivers/driver-sql/src/sql-driver-option-default-ddl.test.ts new file mode 100644 index 0000000000..83b8354d2d --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-option-default-ddl.test.ts @@ -0,0 +1,124 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * An option-level `default: true` gets NO physical column DEFAULT (#7246). + * + * The engine now honours the select idiom — `ObjectQL.applyFieldDefaults` falls + * back to the option marked `default: true` when the field declares no + * `defaultValue`. The DDL deliberately does NOT follow, and because that reads + * as an oversight next to case 4 of `applyDeclaredColumnDefault` (an ordinary + * literal IS emitted), it is pinned here rather than left to a comment. + * + * Why not emit, in short — the long form lives on + * `SqlDriver.applyDeclaredColumnDefault`: + * + * - `defaultValue` beats the option flag, and that precedence lives in ONE + * place, the engine. A column DEFAULT is a second resolver. + * - On `multiple: true` the default is an ARRAY; there is no scalar DDL form, + * so emitting needs a carve-out the author cannot see. + * - This method runs for fresh and re-materialized columns only, never a + * retrofit, so emitting would give new databases a DEFAULT that older ones + * on identical metadata lack — and `detectDrift`'s only `default_mismatch` + * producer is the #4560 runtime-token check, so nothing would report it. + * + * The #4560 discipline is NOT the reason: an option's `value` is a plain + * literal, so it could legally be emitted. This is a design decision about + * where a default is resolved, and the last test states the consequence that + * makes it safe — every ObjectStack write path stores the value regardless, + * because the engine, not the database, supplies it. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +describe('SqlDriver — an option `default: true` never becomes a column DEFAULT (#7246)', () => { + let knexInstance: any; + + const makeDriver = () => { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + knexInstance = (d as any).knex; + (d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }; + return d; + }; + + /** The raw `CREATE TABLE` SQLite stored — the only unambiguous view of a DEFAULT. */ + const tableSql = async (table: string): Promise => { + const row = await knexInstance.raw( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = ?", + [table], + ); + return String(row?.[0]?.sql ?? row?.sql ?? ''); + }; + + afterEach(async () => { + await knexInstance?.destroy(); + }); + + const optionZoo = [ + { + name: 'option_zoo', + fields: { + title: { type: 'string' }, + // Option-default only — the shape 30 fields in the shipped corpus use. + f_status: { + type: 'select', + options: [ + { label: 'Draft', value: 'draft', default: true }, + { label: 'Active', value: 'active' }, + ], + }, + // BOTH declared: the field-level literal is emitted (case 4), and it is + // also the value the engine resolves — the two agree, which is the + // property that matters when both sides can answer. + f_stage: { + type: 'select', + defaultValue: 'approved', + options: [ + { label: 'Draft', value: 'draft', default: true }, + { label: 'Approved', value: 'approved' }, + ], + }, + }, + }, + ]; + + it('creates an option-defaulted column with NO database default', async () => { + const driver = makeDriver(); + await driver.initObjects(optionZoo as any); + + const info = await knexInstance('option_zoo').columnInfo(); + expect(info.f_status.defaultValue ?? null).toBeNull(); + + const sql = await tableSql('option_zoo'); + expect(sql).not.toContain("DEFAULT 'draft'"); + }); + + it('REGRESSION: a field-level `defaultValue` on the SAME field is still emitted', async () => { + // The exclusion is scoped to the option flag. Losing case 4 here would be a + // silent behaviour change for every literal default in the platform. + const driver = makeDriver(); + await driver.initObjects(optionZoo as any); + const info = await knexInstance('option_zoo').columnInfo(); + expect(String(info.f_stage.defaultValue)).toContain('approved'); + // ...and never the option the field-level key outranks. + expect(String(info.f_stage.defaultValue)).not.toContain('draft'); + }); + + it('a driver-level insert that omits the field stores NULL — the engine, not the database, defaults it', async () => { + // The consequence of the decision, stated rather than left implicit: this + // is a RAW driver write, below the engine. Through `ObjectQL.insert` the + // same omission stores 'draft' (pinned in + // objectql/src/engine-select-option-default.test.ts), which is every + // ObjectStack write path. Only a writer bypassing the engine sees this + // NULL — and that writer is not reading `options` either. + const driver = makeDriver(); + await driver.initObjects(optionZoo as any); + await driver.create('option_zoo', { id: 'o1', title: 't' }, { bypassTenantAudit: true }); + const row = await knexInstance('option_zoo').where('id', 'o1').first(); + expect(row.f_status).toBeNull(); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index a72ef68f07..a2cc261aba 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -8936,6 +8936,41 @@ export class SqlDriver implements IDataDriver { * 3. **Objects** — Expression envelopes (`{ dialect, source }`), evaluated * app-side; never a column DEFAULT. * 4. **Everything else** — a real literal, emitted verbatim. + * + * # What this deliberately does NOT read: `options[].default` (#7246) + * + * A select field's option-level `default: true` is now a real initial value — + * `ObjectQL.applyFieldDefaults` falls back to the marked option when the field + * declares no `defaultValue`. It gets NO physical column DEFAULT here, and + * that asymmetry with case 4 is the decision, not an omission. + * + * The #4560 discipline does not forbid it: an option's `value` is a plain + * literal, not a runtime token, so it *could* be emitted. The reasons not to: + * + * - **One resolver owns the precedence.** `defaultValue` beats the option + * flag, and that ordering lives in the engine. A column DEFAULT is a + * second resolver that fires only where the engine did not, so keeping it + * out means there is exactly one place the question is answered. + * - **The shape has no scalar DDL form.** On `multiple: true` the default is + * an ARRAY of the marked options. Emitting would need a "…except for + * multi-selects" carve-out invisible to the author. + * - **It would divide deployments silently.** This method runs for a FRESH + * column ({@link createColumn}) and a re-materialized one + * ({@link rebuildSqliteTablePatched}) — never a retrofit of an existing + * table. Emitting would give new databases (and any SQLite rebuild) a + * DEFAULT that older databases on identical metadata lack, and + * `detectDrift`'s only `default_mismatch` producer is the #4560 token + * check — there is no general declared-literal-vs-physical comparison — so + * nothing would ever report the divergence. + * - **It would make SQL the odd driver out.** The engine fallback serves + * memory, mongodb, sql and turso identically from day one; a SQL-only + * second enforcement point is exactly the per-datasource split #4597 and + * #4560 were both about. + * + * The value the engine resolves is what every ObjectStack write path stores; + * only a writer bypassing the engine entirely sees NULL, and that writer is + * not reading `options` either. Pinned by + * `sql-driver-option-default-ddl.test.ts`. */ protected applyDeclaredColumnDefault(col: Knex.ColumnBuilder, field: any, type: string): void { const dv = field?.defaultValue; diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index c3f045f0c0..784dd2bb13 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -205,6 +205,18 @@ function fieldEntries(obj: AnyRec): Array<[string, AnyRec]> { * always-valued when it is `required`, carries a `defaultValue`, declares a * default option (`options: [{ …, default: true }]` — the select idiom), or is * an autonumber the platform populates. + * + * The select-idiom branch is grounded, not assumed (#7246). It was the only + * consumer of `SelectOption.default` anywhere in the repo while the insert path + * ignored the key entirely, so it concluded "always valued" about a column that + * really did store `null` — a build-breaking verdict resting on a declaration + * nothing honoured, and a predicate reading such a field could be silenced by + * it. `ObjectQL.applyFieldDefaults` now falls back to the marked option when + * the field declares no `defaultValue`, so the premise this branch always + * stated is true on the write path. The two sides are kept honest BY + * CONSTRUCTION: the engine reads the canonical `default` spelling, without a + * `type` test, exactly as this does — so there is no field this calls + * always-valued that the engine leaves empty. */ function isNullableField(def: AnyRec): boolean { if (def.required === true) return false; diff --git a/packages/objectql/src/engine-select-option-default.test.ts b/packages/objectql/src/engine-select-option-default.test.ts new file mode 100644 index 0000000000..8058347275 --- /dev/null +++ b/packages/objectql/src/engine-select-option-default.test.ts @@ -0,0 +1,369 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The select idiom is real on the insert path (#7246). + * + * `SelectOption.default` has been authorable and spec-valid since the schema was + * written, and nothing on the insert path read it: `applyFieldDefaults` resolved + * `f.defaultValue` and never `options`, so a create that omitted the field + * stored `null` — not the option marked `default: true`. The key's ONE consumer + * anywhere in the repo was lint's `isNullableField`, which concluded the column + * was always-valued on the strength of that declaration, and that verdict is + * **build-breaking**. So the single place the key was read trusted it, and the + * place that would have made it true ignored it. + * + * Maintainer ruling (2026-08-10, issue #7246): enforce. These tests pin the + * whole contract that ruling names, plus the three shapes it leaves to the + * implementation: + * + * - the fallback fires when the field declares no `defaultValue`; + * - `defaultValue` WINS when both are declared — the more specific + * declaration (the precedence pin the ruling asks for by name); + * - presence is the engine's own `dv == null` test, so `defaultValue: ''` is a + * real default that wins; + * - `multiple: true` assembles an ARRAY, because that field stores one; + * - an option value is a plain LITERAL — never routed through the + * `DEFAULT_VALUE_TOKENS` branches, which is why the fallback resolves in the + * `defaultValue == null` arm rather than upstream of the token tests. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { DEFAULT_VALUE_TOKEN_CURRENT_USER } from '@objectstack/spec/data'; + +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string) { return Array.from(storeFor(object).values()); }, + async findOne(object: string) { return storeFor(object).values().next().value ?? null; }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update() { return null; }, + async upsert(object: string, data: Record) { return this.create(object, data); }, + async delete() { return true; }, + async count(object: string) { return storeFor(object).size; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany() { return 0; }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +const sys = { context: { isSystem: true } } as any; + +describe('[#7246] the option marked `default: true` is the field default', () => { + let engine: ObjectQL; + + /** Mirrors `showcase_task`: a status select whose initial value is declared only on the option. */ + const task = { + name: 'opt_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { + name: 'status', label: 'Status', type: 'select' as const, + options: [ + { label: 'Backlog', value: 'backlog', default: true }, + { label: 'Active', value: 'active' }, + ], + }, + }, + }; + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(makeStubDriver().driver, true); + await engine.init(); + engine.registry.registerObject(task); + }); + + it('fills an OMITTED field with the marked option — the issue repro', async () => { + // Before this change the row stored `null`, while lint's `isNullableField` + // was already telling predicates the column could never be null. + const row: any = await engine.insert('opt_task', { title: 'A' }, sys); + expect(row.status).toBe('backlog'); + }); + + it('fills an EXPLICIT null too — the unpicked-control shape (#2706)', async () => { + // A form serializes an untouched select to `null`, not to omission. The + // insert path treats both as "not supplied", and the fallback rides the + // same test rather than inventing a second presence rule. + const row: any = await engine.insert('opt_task', { title: 'B', status: null }, sys); + expect(row.status).toBe('backlog'); + }); + + it('never overwrites a caller-supplied value', async () => { + const row: any = await engine.insert('opt_task', { title: 'C', status: 'active' }, sys); + expect(row.status).toBe('active'); + }); + + it('leaves the field alone when NO option is marked', async () => { + engine.registry.registerObject({ + ...task, + name: 'opt_unmarked', + fields: { + ...task.fields, + status: { + ...task.fields.status, + options: [{ label: 'Backlog', value: 'backlog' }, { label: 'Active', value: 'active' }], + }, + }, + } as any); + const row: any = await engine.insert('opt_unmarked', { title: 'D' }, sys); + expect(row.status).toBeUndefined(); + }); +}); + +describe('[#7246] precedence — `defaultValue` beats the option flag', () => { + let engine: ObjectQL; + + /** The ui#4047 reporter's shape: BOTH spellings on one field, and they DISAGREE. */ + const both = { + name: 'opt_both', + label: 'Both', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { + name: 'status', label: 'Status', type: 'select' as const, + defaultValue: 'approved', + options: [ + { label: 'Draft', value: 'draft', default: true }, + { label: 'Approved', value: 'approved' }, + ], + }, + }, + }; + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(makeStubDriver().driver, true); + await engine.init(); + engine.registry.registerObject(both); + }); + + it('PIN: the field-level `defaultValue` wins — the more specific declaration', async () => { + // The ruling's precedence rule. `defaultValue` names a value for THIS + // field; the option flag describes the shared option list, so the field + // wins and the flag is inert here exactly as it was everywhere before. + const row: any = await engine.insert('opt_both', { title: 'A' }, sys); + expect(row.status).toBe('approved'); + expect(row.status).not.toBe('draft'); + }); + + it("PIN: `defaultValue: ''` is a REAL default and still wins", async () => { + // Presence is the engine's own `dv == null` test — `'' == null` is false. + // Reading presence as truthiness instead would hand this field to the + // option fallback and silently store 'draft' where the author declared + // "empty". Same rule that makes a caller-supplied `''` a real value. + // + // Declared as `text`, not `select`, on purpose: `validateRecord` checks a + // single-valued `select`/`radio` value against `optionValues(def.options)`, + // and `''` is not among them — so a `select` with `defaultValue: ''` is + // rejected as `invalid_option` before this precedence is observable. That + // rejection is pre-existing and unrelated (it predates this change and is + // unmoved by it); this test is about which of the two DECLARATIONS the + // engine reads, so it uses a type whose values are free-form. + engine.registry.registerObject({ + ...both, + name: 'opt_empty', + fields: { + ...both.fields, + status: { ...both.fields.status, type: 'text' as const, defaultValue: '' }, + }, + } as any); + const row: any = await engine.insert('opt_empty', { title: 'B' }, sys); + expect(row.status).toBe(''); + }); + + it('an Expression-envelope `defaultValue` also wins, and still evaluates', async () => { + // The envelope resolves to `approved` — a DECLARED option, so the record + // stays spec-valid and the only thing under test is which declaration the + // engine read. (An expression yielding a non-option value is rejected by + // `validateRecord`'s `invalid_option` check, which is a separate contract + // this change neither touches nor relies on.) + engine.registry.registerObject({ + ...both, + name: 'opt_expr', + fields: { + ...both.fields, + status: { ...both.fields.status, defaultValue: { dialect: 'cel', source: "'approved'" } }, + }, + } as any); + const row: any = await engine.insert('opt_expr', { title: 'C' }, sys); + expect(row.status).toBe('approved'); + expect(row.status).not.toBe('draft'); + }); +}); + +describe('[#7246] an option value is a LITERAL, never a runtime token', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(makeStubDriver().driver, true); + await engine.init(); + }); + + it('an option spelled `current_user` stores the twelve characters, not the actor id', async () => { + // `current_user` is a spec-valid option value (a lowercase machine + // identifier), and as an OPTION it is a picklist entry — not the + // `DEFAULT_VALUE_TOKENS` instruction of the same spelling. This is why the + // fallback resolves inside the `defaultValue == null` arm: routing it + // through the token branches would silently store the acting user's id in + // a picklist column. Structural, not a name check. + engine.registry.registerObject({ + name: 'opt_tokenish', + label: 'Tokenish', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + assignee_kind: { + name: 'assignee_kind', label: 'Kind', type: 'select' as const, + options: [ + { label: 'Current user', value: DEFAULT_VALUE_TOKEN_CURRENT_USER, default: true }, + { label: 'Queue', value: 'queue' }, + ], + }, + }, + } as any); + const row: any = await engine.insert('opt_tokenish', {}, { context: { userId: 'usr_7' } } as any); + expect(row.assignee_kind).toBe(DEFAULT_VALUE_TOKEN_CURRENT_USER); + expect(row.assignee_kind).not.toBe('usr_7'); + }); +}); + +describe('[#7246] the default follows the FIELD shape, not the option count', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(makeStubDriver().driver, true); + await engine.init(); + }); + + const multi = (name: string, options: unknown[], extra: Record = {}) => ({ + name, + label: name, + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + tags: { name: 'tags', label: 'Tags', type: 'select' as const, options, ...extra }, + }, + }); + + it('`multiple: true` assembles an ARRAY of every marked option, in declaration order', async () => { + engine.registry.registerObject(multi('opt_multi', [ + { label: 'Important', value: 'important', default: true }, + { label: 'Quick', value: 'quick' }, + { label: 'Review', value: 'review', default: true }, + ], { multiple: true }) as any); + const row: any = await engine.insert('opt_multi', {}, sys); + expect(row.tags).toEqual(['important', 'review']); + }); + + it('`multiple: true` with ONE marked option is still an array — shape follows the field', async () => { + // Not a style preference: `validateRecord`'s multi-value branch REJECTS a + // non-array outright (`invalid_type_array`), so a bare scalar default here + // would fail the engine's own validator on the very insert it was meant to + // complete — and every driver persisting this column as Array/JSON would + // have stored the wrong shape if it got through. + engine.registry.registerObject(multi('opt_multi_one', [ + { label: 'Important', value: 'important', default: true }, + { label: 'Quick', value: 'quick' }, + ], { multiple: true }) as any); + const row: any = await engine.insert('opt_multi_one', {}, sys); + expect(row.tags).toEqual(['important']); + }); + + it('a SINGLE-valued field takes the FIRST marked option — one slot, declaration order decides', async () => { + // Neither `Field.select` nor `SelectOptionSchema` dedupes the flag, so this + // is reachable metadata. Refusing it would fail a spec-valid declaration at + // runtime; first-wins is deterministic and is what a picker preselects. + engine.registry.registerObject(multi('opt_multi_marked', [ + { label: 'Important', value: 'important', default: true }, + { label: 'Review', value: 'review', default: true }, + ]) as any); + const row: any = await engine.insert('opt_multi_marked', {}, sys); + expect(row.tags).toBe('important'); + }); + + it('an option marked default but carrying NO value declares nothing — the field is left unset', async () => { + engine.registry.registerObject(multi('opt_valueless', [ + { label: 'Broken', default: true }, + { label: 'Quick', value: 'quick' }, + ]) as any); + const row: any = await engine.insert('opt_valueless', {}, sys); + expect(row.tags).toBeUndefined(); + expect(row.tags).not.toBeNull(); + }); +}); + +describe('[#7246] the lint heuristic and the engine agree BY CONSTRUCTION', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(makeStubDriver().driver, true); + await engine.init(); + }); + + it('no `type` test — `isNullableField` has none either, so neither may have one', async () => { + // lint's `isNullableField` reads `options[].default` without asking the + // field's type. If the engine gated the fallback on `type === 'select'`, + // any other typed field carrying an option list would be called + // always-valued by a BUILD-BREAKING verdict while still storing null. + engine.registry.registerObject({ + name: 'opt_untyped', + label: 'Untyped', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + channel: { + name: 'channel', label: 'Channel', type: 'text' as const, + options: [{ label: 'Email', value: 'email', default: true }, { label: 'SMS', value: 'sms' }], + }, + }, + } as any); + const row: any = await engine.insert('opt_untyped', {}, sys); + expect(row.channel).toBe('email'); + }); + + it('only the CANONICAL `default` spelling is read — aliases are normalized before the engine', async () => { + // `isDefault` / `selected` are `SelectOptionSchema` authoring aliases, + // rewritten to `default` at parse time. Honouring them here would make the + // engine fill fields lint calls nullable — a disagreement in the harmless + // direction, but a disagreement, and it would move the alias contract out + // of the schema that owns it. + engine.registry.registerObject({ + name: 'opt_alias', + label: 'Alias', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + status: { + name: 'status', label: 'Status', type: 'select' as const, + options: [{ label: 'Draft', value: 'draft', isDefault: true }, { label: 'Done', value: 'done' }], + }, + }, + } as any); + const row: any = await engine.insert('opt_alias', {}, sys); + expect(row.status).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index f43be49db7..6cbce45379 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2464,6 +2464,78 @@ export class ObjectQL implements IObjectQLEngine { return new ScopedContext(safeCtx, this as unknown as IDataEngine); } + /** + * The initial value a field's OPTION LIST declares — the option marked + * `default: true` — or `undefined` when it declares none (#7246). + * + * `SelectOption.default` has been authorable and spec-valid since the schema + * was written, and until this method nothing on the insert path read it: a + * create that omitted the field stored `null`, not the marked option. The one + * consumer anywhere was lint's `isNullableField`, which concluded the column + * was always-valued — a **build-breaking** verdict resting on a declaration + * the engine did not honour. Enforcing it here is what makes that heuristic + * true rather than merely asserted. + * + * Deliberate decisions, each of which could reasonably have gone the other + * way: + * + * - **Precedence: `defaultValue` wins.** Not decided here — decided by the + * caller, which only reaches this method when `defaultValue == null`. The + * field-level key is the more specific declaration (it names a value for + * THIS field; the option flag describes the shared option list), and it is + * the spelling every other consumer already honours. When both are + * declared and disagree, the option flag is inert exactly as it is today. + * + * - **Type-agnostic, like the lint heuristic.** `options` is a field-level + * key on `FieldSchema`, not gated on `type: 'select'`, and + * `isNullableField` reads it without a type test. Matching that keeps the + * two sides honest BY CONSTRUCTION: there is no field the lint calls + * always-valued that this method leaves empty. + * + * - **`multiple: true` assembles an ARRAY.** That field stores an + * Array/JSON (`FieldSchema.multiple`: "Stores as Array/JSON"), so the + * shape of its default follows the field, not the number of marked + * options — one marked option on a multi-select defaults to a + * one-element array, never a bare scalar that the driver would then store + * with the wrong shape. Refusing (throwing) was rejected: the metadata is + * spec-valid, and a runtime throw on spec-valid input is a worse answer + * than a well-defined value. Ignoring it was rejected too — it would + * preserve, for multi-selects only, precisely the inertness this change + * removes, and an author cannot see that carve-out from the schema. + * + * - **Several options marked on a SINGLE-valued field: the FIRST wins.** + * There is one slot, so declaration order decides — deterministic, and + * the same option a picker preselects when it takes the first match. + * Neither the `Field.select` builder nor the schema dedupes the flag, so + * this case is reachable; the alternative (throw) again fails a + * spec-valid declaration at runtime. Nothing in the shipped corpus marks + * more than one. + * + * - **Only the canonical `default` spelling is read.** The authoring + * aliases (`isDefault`, `selected`) are normalized by + * `SelectOptionSchema`'s alias layer before metadata reaches the engine, + * and lint reads the canonical key only. Reading the aliases here would + * make the engine honour raw shapes lint calls nullable — the two would + * disagree in the direction that produces a false "always-valued". + */ + private resolveOptionDefault(field: { options?: unknown; multiple?: unknown }): unknown { + const options = field.options; + if (!Array.isArray(options)) return undefined; + const marked: unknown[] = []; + for (const o of options) { + if (!o || typeof o !== 'object') continue; + if ((o as { default?: unknown }).default !== true) continue; + const value = (o as { value?: unknown }).value; + // An option marked default but carrying no value declares nothing this + // method can apply — skipped rather than stored as `null`, which would + // be indistinguishable from "no default" downstream. + if (value === undefined || value === null) continue; + marked.push(value); + } + if (marked.length === 0) return undefined; + return field.multiple === true ? marked : marked[0]; + } + /** * Apply field defaults to an incoming insert payload. Defaults that are * Expression envelopes (e.g. `{ dialect: 'cel', source: 'today()' }`, @@ -2486,6 +2558,11 @@ export class ObjectQL implements IObjectQLEngine { * Implements ROADMAP §M9.9b — `defaultValue` accepts Expression so authors * can replace "write a hook to default to today/current-user" with a * declarative `defaultValue: cel\`today()\``. + * + * A field that declares NO `defaultValue` falls back to the option marked + * `default: true` ({@link resolveOptionDefault}, #7246) — the select idiom, + * which until then was authorable, spec-valid, and read by nothing on this + * path. */ private applyFieldDefaults( object: string, @@ -2497,7 +2574,9 @@ export class ObjectQL implements IObjectQLEngine { const fieldsRaw = (schema as any)?.fields; if (!fieldsRaw || typeof fieldsRaw !== 'object') return record; // `fields` may be a Record (canonical) or an array (legacy). - const fieldEntries: Array<{ name: string; type?: unknown; defaultValue?: unknown }> = Array.isArray(fieldsRaw) + const fieldEntries: Array<{ + name: string; type?: unknown; defaultValue?: unknown; options?: unknown; multiple?: unknown; + }> = Array.isArray(fieldsRaw) ? fieldsRaw : Object.entries(fieldsRaw).map(([name, def]) => ({ name, ...(def as object) })); const out = { ...record }; @@ -2508,7 +2587,25 @@ export class ObjectQL implements IObjectQLEngine { // real value (including `''`) is respected. Insert-only path, so an // intentional "set to null" on update is never touched here. if (out[f.name] != null) continue; - if (f.defaultValue == null) continue; + if (f.defaultValue == null) { + // No `defaultValue` — fall back to the option marked `default: true`. + // + // Reached ONLY through this branch, which is the whole point of putting + // it here (#7246): an option's `value` is a plain literal by + // construction (`SystemIdentifierSchema`), never one of the runtime + // TOKENS below, so it must not be handed to `isCurrentUserDefaultToken` + // / `isNowDefaultToken`. An option literally spelled `current_user` + // stores those twelve characters — it is a picklist value, not an + // instruction — and that is pinned by test. + // + // The `dv == null` gate above is the presence test that decides + // precedence: `defaultValue: ''` is a REAL default (`'' == null` is + // false), so it wins and this fallback never fires for it, exactly as + // `''` supplied by a caller is respected as a real value. + const fromOption = this.resolveOptionDefault(f); + if (fromOption !== undefined) out[f.name] = fromOption; + continue; + } const dv = f.defaultValue; if (typeof dv === 'object' && dv !== null && (dv as any).dialect && typeof (dv as any).source === 'string') { const result = ExpressionEngine.evaluate(dv as any, { From 67a08318633b7472631a0b79b97c82ddd28d5ada Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 09:50:19 +0000 Subject: [PATCH 2/2] fix(test): re-pin the two suites this change legitimately moves (#7246) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI round 1 went red twice on 61e8174, both genuine blast radius rather than flakes, and neither visible to the three package suites run locally. 1. examples/app-showcase hook-body-persisted-writes (#7258) — `showcase_inquiry` declares `status: { value: 'new', default: true }` with no `defaultValue`, so the engine now fills it. `applyFieldDefaults` runs at the top of the insert middleware, hence `status` is in `ctx.input` before the first `beforeInsert` body — exactly as a `defaultValue`-declared field always behaved. The priority-10 ordering the WITNESS case is about is untouched. The REVERSE case needed more than a value swap: `status` can no longer witness "the hook ran", since the engine writes the same 'new' either way. `source` is a plain `Field.text` with no options and no `defaultValue`, so 'web' still has exactly one producer — the vacuity pin moves onto it, and the engine-owned value is asserted separately for what it now is. 2. TypeScript Type Check — @objectstack/objectql's TEST_DEBT ratchet reported 356 against a recorded 355. Cause was in this PR's new test file: 12 `registerObject(def)` calls passing one argument where the signature is `registerObject(def, ownerId, ...)`. Invisible to the package's own `typecheck`, whose tsconfig excludes tests — which is precisely what TEST_DEBT measures. Fixed at the source by passing the owner id rather than by raising the ledger; re-measured with the gate's own method: 344 errors, 0 in this PR's files. Verified locally: app-showcase 16 files / 164 tests, app-crm 27, app-todo 105, qa/downstream-contract 14, objectql option-default 14 — all green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2 --- .../test/hook-body-persisted-writes.test.ts | 29 +++++++++++++++++-- .../src/engine-select-option-default.test.ts | 24 +++++++-------- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/examples/app-showcase/test/hook-body-persisted-writes.test.ts b/examples/app-showcase/test/hook-body-persisted-writes.test.ts index ec427dd55b..5116c3bce6 100644 --- a/examples/app-showcase/test/hook-body-persisted-writes.test.ts +++ b/examples/app-showcase/test/hook-body-persisted-writes.test.ts @@ -205,7 +205,17 @@ describe('#7258 — app-showcase sandboxed `body` hooks reach the persisted row' const stored = await readBack(engine, 'showcase_inquiry', String(created.id)); // The record's own fields are what a body enumerates... - expect(stored.company).toBe('KEYS[email,message,name] hasData=undefined'); + // + // `status` is in that list without any hook having run, and is not the + // caller's (#7246): `applyFieldDefaults` runs at the TOP of the insert + // middleware, so engine-resolved defaults are already on the record before + // the first `beforeInsert` body sees it. `showcase_inquiry.status` marks + // `{ value: 'new', default: true }`, and the engine now honours that option + // flag exactly as it has always honoured a `defaultValue`. The priority-10 + // ordering this case is really about is unaffected — `source` is absent + // here precisely because it is stamped by `StampInquiryDefaultsHook` at + // priority 50, which still has not run. + expect(stored.company).toBe('KEYS[email,message,name,status] hasData=undefined'); // ...and the write the probe made through that flat view landed in the row, // which is the second half of `installFlatInput`'s contract. expect(stored.status).toBe('new'); @@ -223,8 +233,23 @@ describe('#7258 — app-showcase sandboxed `body` hooks reach the persisted row' ctx as never, ); const storedInquiry = await readBack(engine, 'showcase_inquiry', String(inquiry.id)); - expect(storedInquiry.status == null).toBe(true); + // `source` is the whole vacuity witness for this hook now (#7246). + // + // It used to be that BOTH of the hook's stamps came back null with the hook + // unbound. `status` no longer does, and not because a hook ran: the field + // declares `{ value: 'new', default: true }`, and the ENGINE now applies + // that option default on insert. So `status` can no longer discriminate + // "the hook ran" from "the hook did not" — it reads `'new'` either way, and + // asserting `'new'` here would make this half of the reverse check assert + // the same fact as the forward case, i.e. nothing. + // + // `source` still can: `showcase_inquiry.source` is a plain `Field.text` + // with no options and no `defaultValue`, so `'web'` has exactly one + // producer — `StampInquiryDefaultsHook`. The pin therefore moves onto it + // rather than being softened, and the engine-owned value is asserted + // separately for what it now is. expect(storedInquiry.source == null).toBe(true); + expect(storedInquiry.status).toBe('new'); // engine option default, NOT the hook const account: any = await engine.insert( 'showcase_account', { name: 'Initech', status: 'active' }, ctx as never, diff --git a/packages/objectql/src/engine-select-option-default.test.ts b/packages/objectql/src/engine-select-option-default.test.ts index 8058347275..2f37912963 100644 --- a/packages/objectql/src/engine-select-option-default.test.ts +++ b/packages/objectql/src/engine-select-option-default.test.ts @@ -94,7 +94,7 @@ describe('[#7246] the option marked `default: true` is the field default', () => engine = new ObjectQL(); engine.registerDriver(makeStubDriver().driver, true); await engine.init(); - engine.registry.registerObject(task); + engine.registry.registerObject(task, 'test.issue7246'); }); it('fills an OMITTED field with the marked option — the issue repro', async () => { @@ -128,7 +128,7 @@ describe('[#7246] the option marked `default: true` is the field default', () => options: [{ label: 'Backlog', value: 'backlog' }, { label: 'Active', value: 'active' }], }, }, - } as any); + } as any, 'test.issue7246'); const row: any = await engine.insert('opt_unmarked', { title: 'D' }, sys); expect(row.status).toBeUndefined(); }); @@ -159,7 +159,7 @@ describe('[#7246] precedence — `defaultValue` beats the option flag', () => { engine = new ObjectQL(); engine.registerDriver(makeStubDriver().driver, true); await engine.init(); - engine.registry.registerObject(both); + engine.registry.registerObject(both, 'test.issue7246'); }); it('PIN: the field-level `defaultValue` wins — the more specific declaration', async () => { @@ -191,7 +191,7 @@ describe('[#7246] precedence — `defaultValue` beats the option flag', () => { ...both.fields, status: { ...both.fields.status, type: 'text' as const, defaultValue: '' }, }, - } as any); + } as any, 'test.issue7246'); const row: any = await engine.insert('opt_empty', { title: 'B' }, sys); expect(row.status).toBe(''); }); @@ -209,7 +209,7 @@ describe('[#7246] precedence — `defaultValue` beats the option flag', () => { ...both.fields, status: { ...both.fields.status, defaultValue: { dialect: 'cel', source: "'approved'" } }, }, - } as any); + } as any, 'test.issue7246'); const row: any = await engine.insert('opt_expr', { title: 'C' }, sys); expect(row.status).toBe('approved'); expect(row.status).not.toBe('draft'); @@ -245,7 +245,7 @@ describe('[#7246] an option value is a LITERAL, never a runtime token', () => { ], }, }, - } as any); + } as any, 'test.issue7246'); const row: any = await engine.insert('opt_tokenish', {}, { context: { userId: 'usr_7' } } as any); expect(row.assignee_kind).toBe(DEFAULT_VALUE_TOKEN_CURRENT_USER); expect(row.assignee_kind).not.toBe('usr_7'); @@ -275,7 +275,7 @@ describe('[#7246] the default follows the FIELD shape, not the option count', () { label: 'Important', value: 'important', default: true }, { label: 'Quick', value: 'quick' }, { label: 'Review', value: 'review', default: true }, - ], { multiple: true }) as any); + ], { multiple: true }) as any, 'test.issue7246'); const row: any = await engine.insert('opt_multi', {}, sys); expect(row.tags).toEqual(['important', 'review']); }); @@ -289,7 +289,7 @@ describe('[#7246] the default follows the FIELD shape, not the option count', () engine.registry.registerObject(multi('opt_multi_one', [ { label: 'Important', value: 'important', default: true }, { label: 'Quick', value: 'quick' }, - ], { multiple: true }) as any); + ], { multiple: true }) as any, 'test.issue7246'); const row: any = await engine.insert('opt_multi_one', {}, sys); expect(row.tags).toEqual(['important']); }); @@ -301,7 +301,7 @@ describe('[#7246] the default follows the FIELD shape, not the option count', () engine.registry.registerObject(multi('opt_multi_marked', [ { label: 'Important', value: 'important', default: true }, { label: 'Review', value: 'review', default: true }, - ]) as any); + ]) as any, 'test.issue7246'); const row: any = await engine.insert('opt_multi_marked', {}, sys); expect(row.tags).toBe('important'); }); @@ -310,7 +310,7 @@ describe('[#7246] the default follows the FIELD shape, not the option count', () engine.registry.registerObject(multi('opt_valueless', [ { label: 'Broken', default: true }, { label: 'Quick', value: 'quick' }, - ]) as any); + ]) as any, 'test.issue7246'); const row: any = await engine.insert('opt_valueless', {}, sys); expect(row.tags).toBeUndefined(); expect(row.tags).not.toBeNull(); @@ -341,7 +341,7 @@ describe('[#7246] the lint heuristic and the engine agree BY CONSTRUCTION', () = options: [{ label: 'Email', value: 'email', default: true }, { label: 'SMS', value: 'sms' }], }, }, - } as any); + } as any, 'test.issue7246'); const row: any = await engine.insert('opt_untyped', {}, sys); expect(row.channel).toBe('email'); }); @@ -362,7 +362,7 @@ describe('[#7246] the lint heuristic and the engine agree BY CONSTRUCTION', () = options: [{ label: 'Draft', value: 'draft', isDefault: true }, { label: 'Done', value: 'done' }], }, }, - } as any); + } as any, 'test.issue7246'); const row: any = await engine.insert('opt_alias', {}, sys); expect(row.status).toBeUndefined(); });