Skip to content

Commit 84de7e3

Browse files
os-warrenclaude
andauthored
fix(driver-sql): name the declared field a builtin column discards, on all three DDL paths (#12015) (#12109)
* fix(driver-sql): name the declared field a builtin column discards, on all three DDL paths `initObjects` emits `id`, `created_at` and `updated_at` itself and skipped any declared field colliding with one in silence — the declared type, length and constraints were dropped with no diagnostic anywhere. Measured on PostgreSQL 16.13 and again here on SQLite: a declared `id: { type: 'text', maxLength: 12 }` lands as `varchar(255)`. Every path that drops such a declaration now warns once per colliding field, naming the field, the object and the platform's ownership: the CREATE branch, the ADD COLUMN diff (a stock upgrade's path), and the rotation shard sync. Each path carries its own call and its own pin so a regression to a silent `continue` on one path fails by name. ⛔ Not a rejection door: the accept set is untouched and the DDL emitted is byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o * fix(driver-sql): report only the storage half a builtin-column collision discards The first cut warned on every declared field colliding with `id` / `created_at` / `updated_at`. Measured against the real corpus that was not merely noisy but FALSE: 116 warnings on a stock boot of platform-objects alone, against declarations like `id: Field.text({ label: 'Presence ID', required: true, readonly: true })` whose label IS applied (four generated locales, highlightFields, FLS, sortability) and whose `required` IS enforced (ADR-0113 write contract). "The declaration is NOT applied … remove the declaration" was untrue there, and following it would have deleted an author-facing label. The trigger is now "asks for storage the platform's own column does not deliver", decided by one classification table pinned against `FieldSchema.shape`, and the message names the lost attributes and what the column really is instead of denying the whole declaration. `id: { type: 'number' }` and `id: { type: 'text' }` still fire. ⛔ Not route C and not a rejection door: the platform still owns the column and the declaration still does not take effect. This changes what we say. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 39eda2f commit 84de7e3

6 files changed

Lines changed: 807 additions & 1 deletion
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): name the storage a declaration on a builtin column name loses, instead of discarding it in silence (#12015)
6+
7+
`initObjects` emits `id`, `created_at` and `updated_at` itself and then skips any
8+
declared field colliding with one — `if (builtinColumns.has(name)) continue;`, with
9+
no warning, no throw and no record anywhere that the author's declaration had been
10+
dropped. Measured on live PostgreSQL 16.13: an object declaring
11+
`id: { type: 'text' }` boots green and gets `id varchar(255)``table.string('id')`,
12+
not TEXT. Measured here on SQLite: the same substitution, and a declared
13+
`maxLength: 12` on that field binds nothing. The driver is right to own its primary
14+
key and audit stamps; the defect was that it disagreed with the author in silence —
15+
the declared-≠-enforced shape that bites hardest on AI-authored metadata, where the
16+
mismatch surfaces much later as data behaving oddly.
17+
18+
Every DDL path that drops such a declaration now says so, naming the field, the
19+
object, the attributes that were lost and what the platform's column actually is:
20+
21+
- **create**`while creating table "…"`, said before the CREATE runs, so the
22+
author hears it even when the CREATE goes on to fail for an unrelated reason;
23+
- **ADD COLUMN diff**`while syncing existing table "…"`; this path drops the
24+
declaration for a different reason (the builtin is already in the table, so the
25+
diff never proposes it), and it is the path a stock upgrade takes;
26+
- **rotation shard**`while syncing shard "…"`, covering both the shard-create and
27+
shard-column-sync branches.
28+
29+
A warning on one path with silence on the others just moves the trap, so each path
30+
carries its own call and its own pin: a regression to a silent `continue` on one path
31+
fails by name rather than being absorbed by a sibling.
32+
33+
**Only the STORAGE half is reported, because only the storage half is lost.** A
34+
declaration on a builtin column name still carries `label` (and the locales generated
35+
from it), `readonly`, `searchable` and the ADR-0113 write contract in `required` — all
36+
honoured on the platform's column exactly as on any other. So the diagnostic fires
37+
only when the declaration asks for storage the platform's own column does not deliver
38+
(a differing `type`, a `maxLength`, `unique`, `defaultValue`, `storage.notNull`, a
39+
`multiple` shape…) and stays silent when it does not: `created_at: { type: 'datetime',
40+
defaultValue: 'NOW()' }` describes precisely what lands, and says nothing.
41+
`id: { type: 'number' }` — an author expecting a numeric key — still fires, as does
42+
`id: { type: 'text' }`. The storage/presentation split is one table
43+
(`builtin-column-collision.ts`) pinned against `FieldSchema.shape`, so a field key
44+
added later is classified deliberately instead of defaulting into silence.
45+
46+
**Grade: `patch`, and deliberately.** Nothing about the accept set moves — every
47+
object that booted before still boots, the DDL emitted is byte-identical, no public
48+
type or metadata key changes, and the only observable difference is a line in the log
49+
for storage that was already being discarded. The platform still owns `id` /
50+
`created_at` / `updated_at`: this changes what the driver **says**, never what it
51+
**does**.
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #12015 — the storage/presentation split itself, pinned.
5+
*
6+
* The diagnostic in `SqlDriver` fires on what this module decides, so the
7+
* decision is worth more than the plumbing around it. Two claims live here:
8+
*
9+
* ① **The classification is exhaustive over `FieldSchema`.** A key added to
10+
* the spec later fails the first case until someone classifies it — the
11+
* whole point of keeping the table in one place. At runtime an unclassified
12+
* key is silent (a diagnostic must never invent a warning it cannot
13+
* justify), so without this pin an added key would default into silence
14+
* with nothing to notice it.
15+
*
16+
* ② **Delivered means delivered.** A declared storage attribute the platform's
17+
* own column already provides is NOT a disagreement and must not be
18+
* reported as one — that is the whole content of the 2026-08-25 narrowing,
19+
* and the case that makes the message true again.
20+
*/
21+
22+
import { describe, it, expect } from 'vitest';
23+
import { FieldSchema } from '@objectstack/spec/data';
24+
import {
25+
FIELD_KEY_STORAGE_CLASS,
26+
BUILTIN_COLUMN_DELIVERY,
27+
undeliveredStorageAttributes,
28+
} from './builtin-column-collision.js';
29+
30+
/** Just the keys, for readability in the assertions below. */
31+
const keysOf = (attrs: ReturnType<typeof undeliveredStorageAttributes>) => attrs.map((a) => a.key);
32+
33+
describe('the FieldSchema storage/presentation classification (#12015)', () => {
34+
it('classifies EVERY FieldSchema key, and invents none', () => {
35+
const declared = Object.keys(FieldSchema.shape).sort();
36+
const classified = Object.keys(FIELD_KEY_STORAGE_CLASS).sort();
37+
38+
// A spec key with no classification: it would be silent at runtime, which
39+
// is safe but undeliberate. Classify it in `FIELD_KEY_STORAGE_CLASS`.
40+
expect(declared.filter((k) => !classified.includes(k)), 'unclassified FieldSchema key(s)').toEqual([]);
41+
// A classification with no spec key: dead weight that reads as coverage.
42+
expect(classified.filter((k) => !declared.includes(k)), 'classified key(s) FieldSchema does not declare').toEqual([]);
43+
});
44+
45+
it('puts `required` on the PRESENTATION side — ADR-0113 makes it the WRITE contract, not a column constraint', () => {
46+
// The load-bearing classification: `required: true` appears on nearly every
47+
// platform object's `id`, the engine enforces it there exactly as anywhere
48+
// else, and calling it "discarded" is the false sentence this card removed.
49+
expect(FIELD_KEY_STORAGE_CLASS.required).toBe('presentation');
50+
// Its ADR-0113 sibling — the one that IS the column constraint.
51+
expect(FIELD_KEY_STORAGE_CLASS.storage).toBe('storage');
52+
});
53+
54+
it('puts the honoured half on the presentation side and the column shape on the storage side', () => {
55+
for (const key of ['label', 'readonly', 'searchable', 'description', 'inlineHelpText', 'group', 'name']) {
56+
expect(FIELD_KEY_STORAGE_CLASS[key], `${key} is honoured on a builtin column`).toBe('presentation');
57+
}
58+
for (const key of ['type', 'maxLength', 'unique', 'defaultValue', 'multiple', 'expression']) {
59+
expect(FIELD_KEY_STORAGE_CLASS[key], `${key} shapes the physical column`).toBe('storage');
60+
}
61+
});
62+
63+
it('records what each builtin column actually delivers, read off the emitting lines', () => {
64+
// `table.string('id').primary()` — varchar(255), NOT NULL, unique, no default.
65+
expect(BUILTIN_COLUMN_DELIVERY.id).toMatchObject({
66+
type: 'string', maxLength: 255, unique: true, notNull: true, defaultValue: null,
67+
});
68+
// `createAuditTimestampColumn` — a timestamp defaulted to the DB clock, left NULLABLE.
69+
for (const column of ['created_at', 'updated_at']) {
70+
expect(BUILTIN_COLUMN_DELIVERY[column]).toMatchObject({
71+
type: 'datetime', unique: false, notNull: false, defaultValue: 'NOW()',
72+
});
73+
}
74+
});
75+
});
76+
77+
describe('what a declaration on a builtin column name loses (#12015)', () => {
78+
it('FIRES on the author error the card was filed for', () => {
79+
// `id: { type: 'number' }` — an author expecting a numeric key.
80+
expect(keysOf(undeliveredStorageAttributes('id', { type: 'number' }))).toEqual(['type']);
81+
// The #11456 fixture's shape.
82+
expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', name: 'id' }))).toEqual(['type']);
83+
// …and names what the column really is, not just that something was lost.
84+
expect(undeliveredStorageAttributes('id', { type: 'text' })[0]).toMatchObject({
85+
key: 'type', declared: 'text', delivered: 'string',
86+
});
87+
});
88+
89+
it('is SILENT for a presentation-only declaration — the platform honours that half', () => {
90+
// `sys_presence.id`, verbatim in shape: the population the pre-narrowing
91+
// warning was false about.
92+
expect(
93+
undeliveredStorageAttributes('id', { type: 'string', label: 'Presence ID', required: true, readonly: true }),
94+
).toEqual([]);
95+
expect(
96+
undeliveredStorageAttributes('created_at', {
97+
type: 'datetime', label: 'Created At', defaultValue: 'NOW()', readonly: true,
98+
}),
99+
).toEqual([]);
100+
});
101+
102+
it('is SILENT for a storage attribute the column already delivers', () => {
103+
expect(undeliveredStorageAttributes('id', { type: 'string', maxLength: 255 })).toEqual([]);
104+
expect(undeliveredStorageAttributes('id', { type: 'string', unique: true })).toEqual([]); // the PK is unique
105+
expect(undeliveredStorageAttributes('id', { type: 'string', storage: { notNull: true } })).toEqual([]); // the PK is NOT NULL
106+
expect(undeliveredStorageAttributes('created_at', { type: 'datetime', defaultValue: 'now()' })).toEqual([]); // token, case-insensitive
107+
});
108+
109+
it('FIRES for a storage attribute the column does NOT deliver, one entry each', () => {
110+
expect(keysOf(undeliveredStorageAttributes('id', { type: 'string', maxLength: 12 }))).toEqual(['maxLength']);
111+
expect(keysOf(undeliveredStorageAttributes('id', { type: 'string', defaultValue: 'NOW()' }))).toEqual(['defaultValue']);
112+
// created_at IS nullable and NOT unique — asking for either is a real disagreement.
113+
expect(keysOf(undeliveredStorageAttributes('created_at', { type: 'datetime', unique: true }))).toEqual(['unique']);
114+
expect(keysOf(undeliveredStorageAttributes('created_at', { type: 'datetime', storage: { notNull: true } })))
115+
.toEqual(['storage.notNull']);
116+
// Several at once, in declaration order.
117+
expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', maxLength: 12, unique: false })))
118+
.toEqual(['type', 'maxLength']); // `unique: false` asks for nothing
119+
});
120+
121+
it('ignores a field that is not a builtin column name at all', () => {
122+
expect(undeliveredStorageAttributes('region', { type: 'text', maxLength: 12 })).toEqual([]);
123+
});
124+
125+
it('stays silent — never throws — on a key it does not know, and on a malformed declaration', () => {
126+
// Forward compatibility: an unclassified key cannot invent a warning. The
127+
// exhaustiveness case above is what makes its arrival visible.
128+
expect(undeliveredStorageAttributes('id', { type: 'string', someFutureKey: 'x' } as any)).toEqual([]);
129+
expect(undeliveredStorageAttributes('id', undefined)).toEqual([]);
130+
expect(undeliveredStorageAttributes('id', null as any)).toEqual([]);
131+
});
132+
});

0 commit comments

Comments
 (0)