Skip to content

Commit e161ad3

Browse files
fix(rest): a NOT NULL refusal's hint leads with the remedy for a declared column and names schema drift only as a condition (#21001)
Fixes #20963 Clause-②: no The `hint` on a driver's NOT NULL refusal no longer asserts that the schema has drifted. It first says the column requires a value (provide it, or declare the field `required`), then names drift in one sentence that is conditional on the object declaring neither `required` nor `storage: { notNull: true }`. Prose only, in the one branch of the driver-string classifier in `packages/rest/src/error-response.ts`; the branch's comment gets the same correction (ADR-0113 D1 / D2). The status, `code`, `error`, `fields` and `object` are unchanged, no key is added, no registry is read, and the import row is untouched (PR #20956 pins its key set). ## Repro (`hint`, same answer on both doors) Object `proj_notnull_hint_20963` declares `must: { type: 'text', storage: { notNull: true } }` with no `required`. `POST /api/v1/data/:object` with `must` omitted, and `PATCH /api/v1/data/:object/:id` writing `must: null`, over a real ObjectQL + SQLite `:memory:` (the pin's harness). Both answer `400 VALIDATION_FAILED`, `error: "must is required"`, `fields: [{ field: 'must', code: 'required', message: 'must is required' }]`, `object`. - **Before** (`origin/main` 05be352, the pin run red on it): "If 'must' is optional in your object metadata, the database column is still NOT NULL — the physical schema has drifted from metadata. Run 'os migrate' to reconcile (or reset the dev database)." - **After**: "The database column for 'must' requires a value: provide it, or declare the field `required` in the object metadata. If the object declares neither `required` nor `storage: { notNull: true }` for 'must', the physical schema has drifted from metadata instead: run 'os migrate' to reconcile (or reset the dev database)." ## What is pinned `packages/rest/src/data-not-null-hint-20963.test.ts`, three tests, by kind and not by wording: - the fixture is the ADR-0113 posture: the engine's own insert is refused by the database for the missing value, and the field declares no `required`; - the control, on both doors: status 400, `code`, `error`, the `required` finding and `object` as before, and the body's key set is exactly `code, error, fields, hint, object`; - the `hint`: its first sentence says the column requires a value, tells the caller to provide it or declare `required`, and names no drift; drift appears in exactly one later sentence, which begins with a condition naming `storage` and `notNull`; the update door answers the create door's hint byte for byte. Not pinned: telling the two cases apart. That needs a registry read, a wider change than this card asks. ## Verification (head 5920691) - **Red on the old text, green on the new.** The pin on `origin/main` before the edit: 2 failed, 1 passed (the fixture test). With the fix: 3 passed. - **Ablation.** With the fix committed (cf4bd61, before the rebase onto 7fa67da), `scripts/ablation-replace.mjs` put the old hint text back (anchor hit 1 time, blob 0b1a393 to 3197ecfe1; the mutated file resolves by relative source import from the test, so no rebuild applies) and the pin went red: 2 failed, 1 passed (the control and fixture held; the hint tests failed). The restore proved `blob == HEAD` and an empty `git diff HEAD`. Direction: turned red, the usual one. - **Package.** `pnpm --filter @objectstack/rest typecheck` exit 0 (`tsc --noEmit`, and `check:test-typecheck` over `tsconfig.test.json`: 0 files, 0 errors; the new test is in that program). `vitest run --project local` on `@objectstack/rest`: 250 files passed, 4964 tests passed, 114 skipped. The ten files that read this mapper, this file included, were also run alone: 10 passed, 567 tests. - **Gates.** `dispatch-gates --commands` derived 61 commands at this head; all 61 were run with exit codes recorded and `--ran` reconciled: 61 derived, 59 run, 2 NOT MEASURED, 0 unrun. The two NOT MEASURED exited 3, PREREQUISITE NOT MET: `check:dual-build-cjs-loads` and `check:type-check-debt` read every package's built output and need the full-repo build, which was not run on the shared box; `Build Core` and `Lint & Repo Gates` cover them in CI. The diff adds no package, no export and no ledger entry, and the package's own test-layer typecheck holds 0 debt. - **Lint.** `pnpm lint` (eslint over the whole repo, `--no-inline-config`) exit 0 at this head, no narrowing. ## Acceptance notes Noted, not filed (no defect, polish only): - `packages/rest/src/rest.test.ts` (the test "maps SQLite NOT NULL constraint → 400 VALIDATION_FAILED with required field", about `:2514`) keeps a comment that says reaching this branch means the metadata did not require the field, so it is physical-schema drift. Its assertions still hold (the hint keeps `os migrate` and "drifted from metadata" in the conditional sentence), so only the comment restates the corrected reading. It sits outside this claim's file surface, so it is untouched. - `packages/rest/src/error-response.ts` `:176` lists "not-null drift" as a label for the branch in a docblock about a different branch. A name, not a claim about the hint. - Branch note: rebased once onto `origin/main` 7fa67da (only `scripts/pm/fleet-write/` moved) with `--force-with-lease`, before any PR existed, so the dispatch-gates families were derived from a current tree. --- _Generated by [Claude Code](https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8055ff2 commit e161ad3

3 files changed

Lines changed: 201 additions & 7 deletions

File tree

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
fix(rest): the `hint` on a NOT NULL refusal leads with the remedy for a column that requires a value, and names schema drift only as a condition, so an author who declared `storage: { notNull: true }` is no longer sent to `os migrate` (#20963)
6+
7+
Clause-②: no
8+
9+
A field that declares `storage: { notNull: true }` without `required` is a column
10+
the database keeps NOT NULL on purpose (ADR-0113). A write with no value for it,
11+
through `POST /api/v1/data/:object` or `PATCH /api/v1/data/:object/:id`, is
12+
refused by the database and answers `400 VALIDATION_FAILED` with a `required`
13+
finding for the field. That answer was right. Its `hint` said the field is
14+
optional in the metadata and "the physical schema has drifted from metadata",
15+
and told the caller to run `os migrate`, which changes nothing for a column
16+
declared NOT NULL.
17+
18+
The `hint` now reads: "The database column for 'FIELD' requires a value: provide
19+
it, or declare the field `required` in the object metadata. If the object
20+
declares neither `required` nor `storage: { notNull: true }` for 'FIELD', the
21+
physical schema has drifted from metadata instead: run 'os migrate' to reconcile
22+
(or reset the dev database)." The first sentence holds for every NOT NULL
23+
refusal. The second names the drift case under the condition that makes it drift.
24+
25+
**What is not affected.** The status, `code`, `error`, `fields` and `object` of
26+
the answer are unchanged, and no key is added. The import row is untouched: it
27+
does not carry a `hint`. Nothing reads the field map to tell the two cases
28+
apart, so the `hint` stays advice for the author to read against the object's
29+
declaration.
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#20963] The `hint` on a driver's NOT NULL refusal, for a column the object
5+
* declares NOT NULL on purpose (ADR-0113 D1 / D2).
6+
*
7+
* `must` is `storage: { notNull: true }` and not `required`. The engine's record
8+
* validator judges `required` and reads `storage.notNull` nowhere, so a write
9+
* with no value for `must` reaches the driver and the DATABASE refuses it.
10+
* `mapDataError` classifies that refusal from the driver's text and the object's
11+
* name; it never sees the field map. The answer's `code`, `fields` and sentence
12+
* are right. What was wrong is the `hint`, which said `must` is optional in the
13+
* metadata and the physical schema has drifted, and sent the author to
14+
* `os migrate`, which changes nothing for a column declared NOT NULL.
15+
*
16+
* Read through the public doors, over a REAL {@link ObjectQL} +
17+
* {@link ObjectStackProtocolImplementation} + SQLite `:memory:`:
18+
* `POST /api/v1/data/:object` with `must` omitted, and
19+
* `PATCH /api/v1/data/:object/:id` writing `must: null`.
20+
*
21+
* Pinned by kind, not by wording: the answer is the one it was (the control), and
22+
* its `hint` leads with the remedy for a column that requires a value, then
23+
* names drift in one sentence that is conditional on the object declaring
24+
* neither `required` nor `storage.notNull`, never as a verdict on this column.
25+
* ⚠️ Not pinned: telling the two cases apart. That needs a registry read, which
26+
* the mapper does not make.
27+
*/
28+
29+
import { describe, it, expect, afterEach } from 'vitest';
30+
import { ObjectQL } from '@objectstack/objectql';
31+
import { SqlDriver } from '@objectstack/driver-sql';
32+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
33+
import { RestServer } from './rest-server';
34+
35+
const OBJECT = 'proj_notnull_hint_20963';
36+
37+
const PROJ = {
38+
name: OBJECT, label: 'Proj notnull hint 20963', systemFields: false,
39+
fields: {
40+
id: { name: 'id', type: 'text' as const, primaryKey: true },
41+
title: { name: 'title', type: 'text' as const },
42+
// Physically NOT NULL, not `required`: declared on purpose, so nothing has drifted.
43+
must: { name: 'must', type: 'text' as const, storage: { notNull: true } },
44+
},
45+
};
46+
47+
function createMockServer() {
48+
const noop = () => {};
49+
return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} };
50+
}
51+
52+
function makeRes() {
53+
const res: any = {
54+
write: () => true, end: () => {},
55+
header: () => res,
56+
status: (code: number) => { res._status = code; return res; },
57+
json: (body: any) => { res._json = body; return res; },
58+
};
59+
return res;
60+
}
61+
62+
const liveEngines: ObjectQL[] = [];
63+
afterEach(async () => {
64+
while (liveEngines.length) {
65+
try { await liveEngines.pop()?.destroy(); } catch { /* noop */ }
66+
}
67+
});
68+
69+
async function boot() {
70+
const engine = new ObjectQL();
71+
liveEngines.push(engine);
72+
engine.registerDriver(new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), true);
73+
await engine.init();
74+
engine.registry.registerObject(PROJ as any);
75+
await engine.syncSchemas();
76+
await engine.insert(OBJECT, { id: 'e1', title: 'existing', must: 'm' } as any);
77+
78+
const protocol = new ObjectStackProtocolImplementation(engine as any);
79+
const rest = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any);
80+
(rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
81+
rest.registerRoutes();
82+
const call = async (method: string, path: string, body: unknown, params: Record<string, string> = { object: OBJECT }): Promise<{ status: number; body: any }> => {
83+
const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path);
84+
expect(route, `${method} ${path}`).toBeDefined();
85+
const res = makeRes();
86+
await route!.handler({ params, body, query: {}, headers: {} } as any, res);
87+
return { status: res._status ?? 200, body: res._json };
88+
};
89+
return { engine, call };
90+
}
91+
92+
/** The control: the answer's code, status, `fields` and sentence are what they were. */
93+
function expectNotNullAnswer(door: { status: number; body: any }) {
94+
expect(door.status, JSON.stringify(door.body)).toBe(400);
95+
expect(door.body).toMatchObject({
96+
code: 'VALIDATION_FAILED',
97+
error: 'must is required',
98+
fields: [{ field: 'must', code: 'required', message: 'must is required' }],
99+
object: OBJECT,
100+
});
101+
// No key is added or dropped by the hint's correction.
102+
expect(Object.keys(door.body).sort()).toEqual(['code', 'error', 'fields', 'hint', 'object']);
103+
}
104+
105+
/** The hint's sentences: none of the prose here contains a full stop but the one ending a sentence. */
106+
function sentencesOf(hint: unknown): string[] {
107+
expect(typeof hint, 'a NOT NULL refusal answers a hint').toBe('string');
108+
return (hint as string).trim().split(/(?<=\.)\s+/);
109+
}
110+
111+
/** What the hint must be for a column that requires a value on purpose (ADR-0113). */
112+
function expectHintLeadsWithTheRemedy(hint: unknown) {
113+
const sentences = sentencesOf(hint);
114+
const text = String(hint);
115+
// First: the column requires a value, so provide it or declare the field `required`.
116+
expect(sentences[0], text).toMatch(/\brequires a value\b/i);
117+
expect(sentences[0], text).toMatch(/provide/i);
118+
expect(sentences[0], text).toContain('`required`');
119+
expect(sentences[0], text).not.toMatch(/drift|os migrate/i);
120+
// Then drift, in exactly one sentence, and only as a condition on the declaration.
121+
const drift = sentences.filter((s) => /drift|os migrate/i.test(s));
122+
expect(drift, text).toHaveLength(1);
123+
expect(drift[0], text).toMatch(/^If\b/);
124+
expect(drift[0], text).toMatch(/storage/);
125+
expect(drift[0], text).toMatch(/notNull/);
126+
expect(drift[0], text).toMatch(/os migrate/);
127+
expect(sentences.indexOf(drift[0]!), text).toBeGreaterThan(0);
128+
}
129+
130+
describe('[#20963] a declared `storage.notNull` column\'s refusal does not assert drift', () => {
131+
it('the fixture is the ADR-0113 posture: the database refuses what the validator lets through', async () => {
132+
const { engine } = await boot();
133+
expect(PROJ.fields.must).toMatchObject({ storage: { notNull: true } });
134+
expect('required' in PROJ.fields.must).toBe(false);
135+
await expect(engine.insert(OBJECT, { id: 'x', title: 't' } as any)).rejects.toThrow(/not null/i);
136+
});
137+
138+
it('POST /data/:object with no value for the column: the same answer, a hint that leads with the remedy', async () => {
139+
const { call } = await boot();
140+
const door = await call('POST', '/api/v1/data/:object', { title: 't' });
141+
expectNotNullAnswer(door);
142+
expectHintLeadsWithTheRemedy(door.body.hint);
143+
});
144+
145+
it('PATCH /data/:object/:id writing null onto the column: the same answer, the same hint', async () => {
146+
const { call } = await boot();
147+
const created = await call('POST', '/api/v1/data/:object', { title: 't' });
148+
const door = await call('PATCH', '/api/v1/data/:object/:id', { must: null }, { object: OBJECT, id: 'e1' });
149+
expectNotNullAnswer(door);
150+
expectHintLeadsWithTheRemedy(door.body.hint);
151+
// One branch answers both doors: the update says what the create said.
152+
expect(door.body.hint).toBe(created.body.hint);
153+
});
154+
});

‎packages/rest/src/error-response.ts‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1813,21 +1813,32 @@ function classifyDataError(error: any, object?: string): { status: number; body:
18131813
if (notNull) {
18141814
const field = notNull[1];
18151815
// The metadata required-check (`record-validator`) runs BEFORE the
1816-
// driver, so a NOT NULL violation that reaches this far means metadata
1817-
// did NOT consider the field required — i.e. the physical column has
1818-
// drifted from metadata (#2186), not a genuine missing-required-field.
1816+
// driver and judges `required` only; nothing before the driver reads
1817+
// `storage.notNull`. ADR-0113 D1 / D2 keep the write contract
1818+
// (`required`) and the column constraint (`storage.notNull`) apart on
1819+
// purpose, so a NOT NULL violation that reaches this far is NOT by itself
1820+
// evidence of drift: an object that declares `storage: { notNull: true }`
1821+
// without `required` lands here on a healthy schema, and `os migrate`
1822+
// changes nothing for it. Drift (#2186) is the other way in: the column
1823+
// is NOT NULL and the metadata declares neither. This branch sees the
1824+
// driver's text and the object's name, never the field map, and telling
1825+
// the two apart would take a registry read, so the `hint` leads with the
1826+
// remedy that holds for both (the column requires a value: provide it, or
1827+
// declare the field `required`) and names drift only as the case that is
1828+
// conditional on the declaration.
18191829
// We keep the `VALIDATION_FAILED` / `required` envelope for back-compat
1820-
// (form UIs key off it) but add an actionable `hint` so the message
1821-
// stops being misleading.
1830+
// (form UIs key off it); `code`, `status`, `fields` and the sentence
1831+
// are the same for both.
18221832
return {
18231833
status: 400,
18241834
body: {
18251835
error: `${field} is required`,
18261836
code: 'VALIDATION_FAILED',
18271837
fields: [{ field, code: 'required', message: `${field} is required` }],
18281838
hint:
1829-
`If '${field}' is optional in your object metadata, the database column is still NOT NULL — ` +
1830-
`the physical schema has drifted from metadata. Run 'os migrate' to reconcile ` +
1839+
`The database column for '${field}' requires a value: provide it, or declare the field \`required\` in the object metadata. ` +
1840+
`If the object declares neither \`required\` nor \`storage: { notNull: true }\` for '${field}', ` +
1841+
`the physical schema has drifted from metadata instead: run 'os migrate' to reconcile ` +
18311842
`(or reset the dev database).`,
18321843
...(object ? { object } : {}),
18331844
},

0 commit comments

Comments
 (0)