Skip to content

Commit 1788e19

Browse files
os-zhuangclaude
andauthored
feat(objectql): enforce the select idiom — the option marked default: true is the field default (#7246) (#7388)
* feat(objectql): enforce the select idiom — the option marked `default: true` is the field default (#7246) `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2 * fix(test): re-pin the two suites this change legitimately moves (#7246) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 73f69dc commit 1788e19

7 files changed

Lines changed: 730 additions & 4 deletions

File tree

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
feat(objectql): the option marked `default: true` is now the field's default on insert (#7246)
6+
7+
`SelectOption.default` has been authorable and spec-valid since the schema was
8+
written, and nothing on the insert path read it. `ObjectQL.applyFieldDefaults`
9+
resolved `f.defaultValue` — Expression envelopes, the `DEFAULT_VALUE_TOKENS`
10+
family, then static literals — and never looked at `options`. So this, which
11+
reads like a declaration of the initial value:
12+
13+
```ts
14+
status: Field.select({
15+
label: 'Status',
16+
options: [
17+
{ label: 'Draft', value: 'draft', default: true },
18+
{ label: 'Approved', value: 'approved' },
19+
],
20+
}),
21+
```
22+
23+
stored **null** on a create that omitted the field, not `draft`.
24+
25+
The key's one consumer anywhere in the repo was lint's `isNullableField`, which
26+
concluded from it that the column was **always valued** — and that verdict is
27+
build-breaking. So the single place that read the key trusted it, while the
28+
place that would have made it true ignored it: a predicate over such a field
29+
could be silenced by a heuristic resting on a declaration nothing honoured.
30+
31+
**After** (maintainer ruling on #7246, ADR-0049 enforce leg): a field that
32+
declares no `defaultValue` falls back to the option marked `default: true`, on
33+
every driver, resolved by the engine exactly as the token family is.
34+
35+
- **`defaultValue` wins when both are declared** — the more specific
36+
declaration. It names a value for *this* field; the option flag describes the
37+
shared option list. When the two disagree the flag stays inert, as it was
38+
everywhere before.
39+
- **Presence is the engine's own `dv == null` test.** `defaultValue: ''` is a
40+
real default and still wins; the fallback fires only when `defaultValue` is
41+
absent by that test.
42+
- **The fallback resolves in the `defaultValue == null` arm**, downstream of the
43+
token and envelope branches, so an option value is always a plain literal — an
44+
option spelled `current_user` stores those twelve characters rather than the
45+
acting user's id.
46+
- **`multiple: true` assembles an array** of every marked option in declaration
47+
order, because that field stores an Array/JSON; a single-valued field with
48+
several marked options takes the first.
49+
- **No physical column DEFAULT** is emitted for an option-default. The engine is
50+
the one place the two spellings are ranked, the multi-select shape has no
51+
scalar DDL form, and emitting would give new databases a default that older
52+
ones on identical metadata lack with nothing to report the divergence. The
53+
reasoning is recorded on `SqlDriver.applyDeclaredColumnDefault` and pinned by
54+
test.
55+
56+
**Migration.** Metadata declaring an option `default: true` on a field with no
57+
`defaultValue` changes insert behaviour: records that used to be born with a
58+
null in that column are now born with the marked option. That is the behaviour
59+
the declaration always described. In the shipped corpus this covers 30 fields
60+
across the showcase, CRM and todo example apps and the downstream-contract QA
61+
fixture — all of them status/stage/priority selects whose marked option is the
62+
intended initial state. To keep the previous behaviour, drop the `default: true`
63+
flag from the option; to make the value explicit, declare `defaultValue`
64+
alongside it, which now outranks the flag.

examples/app-showcase/test/hook-body-persisted-writes.test.ts

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,17 @@ describe('#7258 — app-showcase sandboxed `body` hooks reach the persisted row'
205205

206206
const stored = await readBack(engine, 'showcase_inquiry', String(created.id));
207207
// The record's own fields are what a body enumerates...
208-
expect(stored.company).toBe('KEYS[email,message,name] hasData=undefined');
208+
//
209+
// `status` is in that list without any hook having run, and is not the
210+
// caller's (#7246): `applyFieldDefaults` runs at the TOP of the insert
211+
// middleware, so engine-resolved defaults are already on the record before
212+
// the first `beforeInsert` body sees it. `showcase_inquiry.status` marks
213+
// `{ value: 'new', default: true }`, and the engine now honours that option
214+
// flag exactly as it has always honoured a `defaultValue`. The priority-10
215+
// ordering this case is really about is unaffected — `source` is absent
216+
// here precisely because it is stamped by `StampInquiryDefaultsHook` at
217+
// priority 50, which still has not run.
218+
expect(stored.company).toBe('KEYS[email,message,name,status] hasData=undefined');
209219
// ...and the write the probe made through that flat view landed in the row,
210220
// which is the second half of `installFlatInput`'s contract.
211221
expect(stored.status).toBe('new');
@@ -223,8 +233,23 @@ describe('#7258 — app-showcase sandboxed `body` hooks reach the persisted row'
223233
ctx as never,
224234
);
225235
const storedInquiry = await readBack(engine, 'showcase_inquiry', String(inquiry.id));
226-
expect(storedInquiry.status == null).toBe(true);
236+
// `source` is the whole vacuity witness for this hook now (#7246).
237+
//
238+
// It used to be that BOTH of the hook's stamps came back null with the hook
239+
// unbound. `status` no longer does, and not because a hook ran: the field
240+
// declares `{ value: 'new', default: true }`, and the ENGINE now applies
241+
// that option default on insert. So `status` can no longer discriminate
242+
// "the hook ran" from "the hook did not" — it reads `'new'` either way, and
243+
// asserting `'new'` here would make this half of the reverse check assert
244+
// the same fact as the forward case, i.e. nothing.
245+
//
246+
// `source` still can: `showcase_inquiry.source` is a plain `Field.text`
247+
// with no options and no `defaultValue`, so `'web'` has exactly one
248+
// producer — `StampInquiryDefaultsHook`. The pin therefore moves onto it
249+
// rather than being softened, and the engine-owned value is asserted
250+
// separately for what it now is.
227251
expect(storedInquiry.source == null).toBe(true);
252+
expect(storedInquiry.status).toBe('new'); // engine option default, NOT the hook
228253

229254
const account: any = await engine.insert(
230255
'showcase_account', { name: 'Initech', status: 'active' }, ctx as never,
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* An option-level `default: true` gets NO physical column DEFAULT (#7246).
5+
*
6+
* The engine now honours the select idiom — `ObjectQL.applyFieldDefaults` falls
7+
* back to the option marked `default: true` when the field declares no
8+
* `defaultValue`. The DDL deliberately does NOT follow, and because that reads
9+
* as an oversight next to case 4 of `applyDeclaredColumnDefault` (an ordinary
10+
* literal IS emitted), it is pinned here rather than left to a comment.
11+
*
12+
* Why not emit, in short — the long form lives on
13+
* `SqlDriver.applyDeclaredColumnDefault`:
14+
*
15+
* - `defaultValue` beats the option flag, and that precedence lives in ONE
16+
* place, the engine. A column DEFAULT is a second resolver.
17+
* - On `multiple: true` the default is an ARRAY; there is no scalar DDL form,
18+
* so emitting needs a carve-out the author cannot see.
19+
* - This method runs for fresh and re-materialized columns only, never a
20+
* retrofit, so emitting would give new databases a DEFAULT that older ones
21+
* on identical metadata lack — and `detectDrift`'s only `default_mismatch`
22+
* producer is the #4560 runtime-token check, so nothing would report it.
23+
*
24+
* The #4560 discipline is NOT the reason: an option's `value` is a plain
25+
* literal, so it could legally be emitted. This is a design decision about
26+
* where a default is resolved, and the last test states the consequence that
27+
* makes it safe — every ObjectStack write path stores the value regardless,
28+
* because the engine, not the database, supplies it.
29+
*/
30+
31+
import { describe, it, expect, afterEach, vi } from 'vitest';
32+
import { SqlDriver } from '../src/index.js';
33+
34+
describe('SqlDriver — an option `default: true` never becomes a column DEFAULT (#7246)', () => {
35+
let knexInstance: any;
36+
37+
const makeDriver = () => {
38+
const d = new SqlDriver({
39+
client: 'better-sqlite3',
40+
connection: { filename: ':memory:' },
41+
useNullAsDefault: true,
42+
});
43+
knexInstance = (d as any).knex;
44+
(d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() };
45+
return d;
46+
};
47+
48+
/** The raw `CREATE TABLE` SQLite stored — the only unambiguous view of a DEFAULT. */
49+
const tableSql = async (table: string): Promise<string> => {
50+
const row = await knexInstance.raw(
51+
"SELECT sql FROM sqlite_master WHERE type = 'table' AND name = ?",
52+
[table],
53+
);
54+
return String(row?.[0]?.sql ?? row?.sql ?? '');
55+
};
56+
57+
afterEach(async () => {
58+
await knexInstance?.destroy();
59+
});
60+
61+
const optionZoo = [
62+
{
63+
name: 'option_zoo',
64+
fields: {
65+
title: { type: 'string' },
66+
// Option-default only — the shape 30 fields in the shipped corpus use.
67+
f_status: {
68+
type: 'select',
69+
options: [
70+
{ label: 'Draft', value: 'draft', default: true },
71+
{ label: 'Active', value: 'active' },
72+
],
73+
},
74+
// BOTH declared: the field-level literal is emitted (case 4), and it is
75+
// also the value the engine resolves — the two agree, which is the
76+
// property that matters when both sides can answer.
77+
f_stage: {
78+
type: 'select',
79+
defaultValue: 'approved',
80+
options: [
81+
{ label: 'Draft', value: 'draft', default: true },
82+
{ label: 'Approved', value: 'approved' },
83+
],
84+
},
85+
},
86+
},
87+
];
88+
89+
it('creates an option-defaulted column with NO database default', async () => {
90+
const driver = makeDriver();
91+
await driver.initObjects(optionZoo as any);
92+
93+
const info = await knexInstance('option_zoo').columnInfo();
94+
expect(info.f_status.defaultValue ?? null).toBeNull();
95+
96+
const sql = await tableSql('option_zoo');
97+
expect(sql).not.toContain("DEFAULT 'draft'");
98+
});
99+
100+
it('REGRESSION: a field-level `defaultValue` on the SAME field is still emitted', async () => {
101+
// The exclusion is scoped to the option flag. Losing case 4 here would be a
102+
// silent behaviour change for every literal default in the platform.
103+
const driver = makeDriver();
104+
await driver.initObjects(optionZoo as any);
105+
const info = await knexInstance('option_zoo').columnInfo();
106+
expect(String(info.f_stage.defaultValue)).toContain('approved');
107+
// ...and never the option the field-level key outranks.
108+
expect(String(info.f_stage.defaultValue)).not.toContain('draft');
109+
});
110+
111+
it('a driver-level insert that omits the field stores NULL — the engine, not the database, defaults it', async () => {
112+
// The consequence of the decision, stated rather than left implicit: this
113+
// is a RAW driver write, below the engine. Through `ObjectQL.insert` the
114+
// same omission stores 'draft' (pinned in
115+
// objectql/src/engine-select-option-default.test.ts), which is every
116+
// ObjectStack write path. Only a writer bypassing the engine sees this
117+
// NULL — and that writer is not reading `options` either.
118+
const driver = makeDriver();
119+
await driver.initObjects(optionZoo as any);
120+
await driver.create('option_zoo', { id: 'o1', title: 't' }, { bypassTenantAudit: true });
121+
const row = await knexInstance('option_zoo').where('id', 'o1').first();
122+
expect(row.f_status).toBeNull();
123+
});
124+
});

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8970,6 +8970,41 @@ export class SqlDriver implements IDataDriver {
89708970
* 3. **Objects** — Expression envelopes (`{ dialect, source }`), evaluated
89718971
* app-side; never a column DEFAULT.
89728972
* 4. **Everything else** — a real literal, emitted verbatim.
8973+
*
8974+
* # What this deliberately does NOT read: `options[].default` (#7246)
8975+
*
8976+
* A select field's option-level `default: true` is now a real initial value —
8977+
* `ObjectQL.applyFieldDefaults` falls back to the marked option when the field
8978+
* declares no `defaultValue`. It gets NO physical column DEFAULT here, and
8979+
* that asymmetry with case 4 is the decision, not an omission.
8980+
*
8981+
* The #4560 discipline does not forbid it: an option's `value` is a plain
8982+
* literal, not a runtime token, so it *could* be emitted. The reasons not to:
8983+
*
8984+
* - **One resolver owns the precedence.** `defaultValue` beats the option
8985+
* flag, and that ordering lives in the engine. A column DEFAULT is a
8986+
* second resolver that fires only where the engine did not, so keeping it
8987+
* out means there is exactly one place the question is answered.
8988+
* - **The shape has no scalar DDL form.** On `multiple: true` the default is
8989+
* an ARRAY of the marked options. Emitting would need a "…except for
8990+
* multi-selects" carve-out invisible to the author.
8991+
* - **It would divide deployments silently.** This method runs for a FRESH
8992+
* column ({@link createColumn}) and a re-materialized one
8993+
* ({@link rebuildSqliteTablePatched}) — never a retrofit of an existing
8994+
* table. Emitting would give new databases (and any SQLite rebuild) a
8995+
* DEFAULT that older databases on identical metadata lack, and
8996+
* `detectDrift`'s only `default_mismatch` producer is the #4560 token
8997+
* check — there is no general declared-literal-vs-physical comparison — so
8998+
* nothing would ever report the divergence.
8999+
* - **It would make SQL the odd driver out.** The engine fallback serves
9000+
* memory, mongodb, sql and turso identically from day one; a SQL-only
9001+
* second enforcement point is exactly the per-datasource split #4597 and
9002+
* #4560 were both about.
9003+
*
9004+
* The value the engine resolves is what every ObjectStack write path stores;
9005+
* only a writer bypassing the engine entirely sees NULL, and that writer is
9006+
* not reading `options` either. Pinned by
9007+
* `sql-driver-option-default-ddl.test.ts`.
89739008
*/
89749009
protected applyDeclaredColumnDefault(col: Knex.ColumnBuilder, field: any, type: string): void {
89759010
const dv = field?.defaultValue;

packages/lint/src/validate-expressions.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,18 @@ function fieldEntries(obj: AnyRec): Array<[string, AnyRec]> {
205205
* always-valued when it is `required`, carries a `defaultValue`, declares a
206206
* default option (`options: [{ …, default: true }]` — the select idiom), or is
207207
* an autonumber the platform populates.
208+
*
209+
* The select-idiom branch is grounded, not assumed (#7246). It was the only
210+
* consumer of `SelectOption.default` anywhere in the repo while the insert path
211+
* ignored the key entirely, so it concluded "always valued" about a column that
212+
* really did store `null` — a build-breaking verdict resting on a declaration
213+
* nothing honoured, and a predicate reading such a field could be silenced by
214+
* it. `ObjectQL.applyFieldDefaults` now falls back to the marked option when
215+
* the field declares no `defaultValue`, so the premise this branch always
216+
* stated is true on the write path. The two sides are kept honest BY
217+
* CONSTRUCTION: the engine reads the canonical `default` spelling, without a
218+
* `type` test, exactly as this does — so there is no field this calls
219+
* always-valued that the engine leaves empty.
208220
*/
209221
function isNullableField(def: AnyRec): boolean {
210222
if (def.required === true) return false;

0 commit comments

Comments
 (0)