From 8a45ee2e4ecb8f724b0d84b6d58ba9b5410f9dc7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 08:45:30 +0000 Subject: [PATCH 1/2] fix(driver-sql): name the declared field a builtin column discards, on all three DDL paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --- .../builtin-column-collision-warning.md | 40 ++++ ...5-builtin-column-collision-warning.test.ts | 194 ++++++++++++++++++ ...sql-driver-aggregation-conformance.test.ts | 6 +- packages/drivers/driver-sql/src/sql-driver.ts | 71 +++++++ 4 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 .changeset/builtin-column-collision-warning.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts diff --git a/.changeset/builtin-column-collision-warning.md b/.changeset/builtin-column-collision-warning.md new file mode 100644 index 0000000000..a8059fbb6d --- /dev/null +++ b/.changeset/builtin-column-collision-warning.md @@ -0,0 +1,40 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): a declared field colliding with a builtin column is named at load time instead of silently discarded (#12015) + +`initObjects` emits `id`, `created_at` and `updated_at` itself and then skips any +declared field colliding with one — `if (builtinColumns.has(name)) continue;`, with +no warning, no throw and no record anywhere that the author's declaration was +dropped. Measured on live PostgreSQL 16.13: an object declaring +`id: { type: 'text' }` boots green and gets `id varchar(255)` — `table.string('id')`, +not TEXT. Measured here on SQLite: the same substitution, and a declared +`maxLength: 12` on that field binds nothing (`varchar(255)`). The driver is right to +own its primary key and audit stamps; the defect was that it disagreed with the +author in silence — the declared-≠-enforced shape that bites hardest on AI-authored +metadata, where the mismatch surfaces much later as data behaving oddly. + +Every DDL path that drops such a declaration now says so, once per colliding field, +naming the field, the object and the platform's ownership: + +- **create** — `while creating table "…"`, said before the CREATE runs, so the + author hears it even when the CREATE goes on to fail for an unrelated reason; +- **ADD COLUMN diff** — `while syncing existing table "…"`; this path drops the + declaration for a different reason (the builtin is already in the table, so the + diff never proposes it), and it is the path a stock upgrade takes; +- **rotation shard** — `while syncing shard "…"`, covering both the shard-create and + shard-column-sync branches. + +A warning on one path with silence on the others just moves the trap, so each path +carries its own call and its own pin: a regression to a silent `continue` on one path +fails by name rather than being absorbed by a sibling. + +**Grade: `patch`, and deliberately.** Nothing about the accept set moves — every +object that booted before still boots, the DDL emitted is byte-identical, no public +type or metadata key changes, and the only observable difference is a line in the +log for a declaration that was already being discarded. Maintainer ruling 2026-08-25 +took exactly this and explicitly did **not** take "refuse boot" (a new rejection door +on an existing accept path, which needs an inventory of existing objects first) or +"make the declaration meaningful" (capability expansion with no pull — the platform +owning its primary key is correct design; the defect was only the silence). diff --git a/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts b/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts new file mode 100644 index 0000000000..c05fc41f20 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts @@ -0,0 +1,194 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12015 — a declared field named after a builtin column is DROPPED, and until + * now it was dropped in silence. + * + * `initObjects` emits `id`, `created_at` and `updated_at` itself, then skips + * any declared field colliding with one. The driver is right to own its + * primary key and audit stamps; the defect was that it disagreed with the + * author without saying so — an object declaring `id: { type: 'text' }` boots + * green and gets `varchar(255)`, and nothing anywhere records that the + * declaration was discarded. + * + * Maintainer ruling 2026-08-25: a **named, loud load-time warning** on all + * three paths that drop such a declaration. Explicitly NOT a rejection door + * (that needs an inventory of existing objects first) and explicitly NOT + * "make the declaration meaningful" (capability expansion with no pull). + * + * ## What each case is worth + * + * The warning exists to be present, so these are presence pins — one **per + * path**, because a warning on one path with silence on the others just moves + * the trap. Each asserts the phase-specific phrasing, so a regression to a + * silent `continue` on ONE path fails by name rather than being absorbed by a + * sibling case: + * + * - `while creating table "…"` — the CREATE TABLE branch + * - `while syncing existing table "…"` — the ADD COLUMN diff branch + * - `while syncing shard "…"` — the rotation shard path + * + * Two non-presence cases carry the rest of the claim: the warning does NOT + * fire for an object that declares no builtin name (a pin that fires on + * everything is not a diagnostic), and the accept set is untouched — the + * object still boots, and the physical column is still the platform's, which + * is also this file's SQLite measurement of the defect itself (the card + * measured PostgreSQL 16.13 only). + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { SqlDriver } from './index.js'; + +/** Every line this file cares about; the `[sql-driver]` prefix alone is shared with much else. */ +const COLLISION_WARNINGS = /collides with a builtin column the platform owns/; + +function warnings(driver: SqlDriver): string[] { + return ((driver as any).logger.warn as ReturnType).mock.calls + .map((call: unknown[]) => String(call[0])) + .filter((message: string) => COLLISION_WARNINGS.test(message)); +} + +/** The structured meta of every collision warning, in emission order. */ +function warningMeta(driver: SqlDriver): Array> { + return ((driver as any).logger.warn as ReturnType).mock.calls + .filter((call: unknown[]) => COLLISION_WARNINGS.test(String(call[0]))) + .map((call: unknown[]) => call[1] as Record); +} + +describe('a declared field colliding with a builtin column is named at load time (#12015)', () => { + let driver: SqlDriver; + + beforeEach(() => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + (driver as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn() }; + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('CREATE path: names the field, the object, and that the platform owns the column', async () => { + await driver.initObjects([ + { + name: 'collide_create', + fields: { + // The #11456 fixture's exact shape — the declaration that started this card. + id: { type: 'text', name: 'id' }, + region: { type: 'text' }, + }, + }, + ]); + + const lines = warnings(driver); + expect(lines).toHaveLength(1); + // The line owes the author three things: WHICH field, on WHICH object, and + // that the platform — not the author — owns that column. + expect(lines[0]).toContain("declared field 'id'"); + expect(lines[0]).toContain('collide_create'); + expect(lines[0]).toContain('collides with a builtin column the platform owns'); + expect(lines[0]).toContain('the declaration is NOT applied'); + // Path identity, so a silent regression on THIS path cannot be masked by + // the other two still warning. + expect(lines[0]).toContain('while creating table "collide_create"'); + expect(warningMeta(driver)[0]).toMatchObject({ table: 'collide_create', field: 'id', phase: 'create' }); + }); + + it('CREATE path: one line per colliding field, and only for the colliding ones', async () => { + await driver.initObjects([ + { + name: 'collide_three', + fields: { + id: { type: 'text' }, + created_at: { type: 'datetime' }, + updated_at: { type: 'datetime' }, + payload: { type: 'text' }, + }, + }, + ]); + + expect(warningMeta(driver).map((m) => m.field)).toEqual(['id', 'created_at', 'updated_at']); + }); + + it('ADD COLUMN diff path: an EXISTING table warns too — the diff never proposes the builtin', async () => { + // Boot once with no collision so the table exists… + await driver.initObjects([{ name: 'collide_alter', fields: { payload: { type: 'text' } } }]); + expect(warnings(driver), 'the no-collision boot must be silent').toHaveLength(0); + + // …then re-register the same object WITH a colliding declaration. This is + // the shape an upgrade takes: the table is already there, so the column + // diff below is the only thing that runs. + await driver.initObjects([ + { + name: 'collide_alter', + fields: { payload: { type: 'text' }, created_at: { type: 'datetime' }, note: { type: 'text' } }, + }, + ]); + + const lines = warnings(driver); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain("declared field 'created_at'"); + expect(lines[0]).toContain('while syncing existing table "collide_alter"'); + expect(warningMeta(driver)[0]).toMatchObject({ table: 'collide_alter', field: 'created_at', phase: 'alter' }); + + // Non-vacuity: this really was the ADD COLUMN branch — the ordinary new + // column landed, so the diff ran rather than the create branch. + const info = await (driver as any).knex('collide_alter').columnInfo(); + expect(Object.keys(info)).toContain('note'); + }); + + it('SHARD path: a rotation-declared object warns while its shard is column-synced', async () => { + await driver.initObjects([ + { + name: 'collide_rot', + fields: { payload: { type: 'text' }, created_at: { type: 'datetime' } }, + lifecycle: { class: 'telemetry', storage: { strategy: 'rotation', shards: 3, unit: 'day' } }, + } as any, + ]); + + const lines = warnings(driver); + // The rotation path is the ONLY one that ran — the base name is a view, + // so the managed create/alter branches never saw this object. + expect(lines).toHaveLength(1); + expect(lines[0]).toContain("declared field 'created_at'"); + expect(lines[0]).toContain('while syncing shard "collide_rot__r'); + expect(warningMeta(driver)[0]).toMatchObject({ field: 'created_at', phase: 'shard' }); + expect(String(warningMeta(driver)[0].table)).toMatch(/^collide_rot__r\d{6,8}$/); + }); + + it('does not fire for an object that declares no builtin column name', async () => { + await driver.initObjects([ + { name: 'no_collision', fields: { region: { type: 'text' }, score: { type: 'number' } } }, + ]); + expect(warnings(driver)).toHaveLength(0); + }); + + it('⛔ ACCEPT SET UNCHANGED — the object boots, and the platform column is what lands (SQLite)', async () => { + await driver.initObjects([ + { + name: 'collide_accept', + // A declaration that asks for something quite different from what the + // platform emits: TEXT, unbounded, plus a length the driver never reads. + fields: { id: { type: 'text', maxLength: 12 }, region: { type: 'text' } }, + }, + ]); + + // Booted, warned, and still fully usable — a warning moves no door. + expect(warnings(driver)).toHaveLength(1); + await driver.create('collide_accept', { id: 'r1', region: 'emea' }, { bypassTenantAudit: true }); + expect(await driver.count('collide_accept', {})).toBe(1); + + // The measurement the warning exists to announce, on SQLite: `id` is the + // platform's `table.string('id')` — varchar(255) — NOT the declared TEXT, + // and not the declared 12-char bound. (The card measured the same + // substitution on PostgreSQL 16.13.) + const info = await (driver as any).knex('collide_accept').columnInfo(); + expect(String(info.id.type).toLowerCase()).toBe('varchar'); + expect(Number(info.id.maxLength)).toBe(255); + // …while the field that did NOT collide got its declared type. + expect(String(info.region.type).toLowerCase()).toBe('text'); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts b/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts index e488edad7a..3671175c5a 100644 --- a/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts @@ -170,7 +170,11 @@ import { const CONFORMANCE_OBJECT = { name: 'conformance_agg', fields: { - id: { type: 'text', name: 'id' }, + // [#12015] `id` is NOT declared here on purpose: the platform owns the + // column (`table.string('id').primary()`), so a declaration under that + // name is discarded by every DDL path — and now says so out loud at + // load time. The rows still carry `id`, and the ordering below still + // reads it; what is gone is a declaration that never bound anything. region: { type: 'text', name: 'region' }, // Nullable, and it must stay that way — see `AggregationRow.stage`. stage: { type: 'text', name: 'stage' }, diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 6c12539f68..bfdbb89212 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -8623,10 +8623,70 @@ export class SqlDriver implements IDataDriver { return { object: tableName, current, shards: retained, dropped }; } + /** + * [#12015] Say out loud that a declared field was dropped for colliding with + * a builtin column. + * + * `id`, `created_at` and `updated_at` are emitted by the driver itself, so a + * field an author DECLARES under one of those names never reaches + * {@link createColumn} — its declared type, length and constraints are + * discarded. The driver is right to own its primary key and audit stamps; + * the defect was that it disagreed with the author in **silence**. Measured + * on live PostgreSQL 16.13: an object declaring `id: { type: 'text' }` boots + * green and gets `id varchar(255)` — `table.string('id')`, not TEXT — and + * nothing anywhere said the declaration had been thrown away. That is the + * declared-≠-enforced hazard this repo treats as first-class, and it bites + * hardest on AI-authored metadata, where the mismatch surfaces much later as + * data behaving oddly. + * + * ⛔ This is a diagnostic, NOT a rejection door. Maintainer ruling + * 2026-08-25 took the warning and explicitly did **not** take "refuse boot" + * (a new rejection door on an existing accept path, whose stock inventory is + * unmeasured) nor "make the declaration meaningful" (capability expansion + * with no pull — the platform owning its primary key is correct design; the + * defect was only the silence). Every object that boots today still boots, + * with one more line in the log. + * + * `phase` is part of the message because all THREE paths that drop such a + * declaration carry this warning — create, the ADD COLUMN diff, and the + * shard path — and a warning on one path with silence on the others just + * moves the trap. Each phase is pinned separately, so a regression to a + * silent `continue` on ONE path fails by name. + */ + protected warnBuiltinColumnCollisions( + tableName: string, + fields: Record | undefined, + builtinColumns: ReadonlySet, + phase: 'create' | 'alter' | 'shard', + ): string[] { + const collisions = Object.keys(fields ?? {}).filter((name) => builtinColumns.has(name)); + if (collisions.length === 0) return collisions; + const where = { + create: `while creating table "${tableName}"`, + alter: `while syncing existing table "${tableName}"`, + shard: `while syncing shard "${tableName}"`, + }[phase]; + for (const field of collisions) { + this.logger.warn( + `[sql-driver] ${where}: declared field '${field}' collides with a builtin column the platform ` + + `owns (id, created_at, updated_at) — the declaration is NOT applied. Its declared type, ` + + `length and constraints are ignored; the platform's own '${field}' column stands. Remove the ` + + `declaration, or rename the field if you meant a column of your own.`, + { table: tableName, field, phase }, + ); + } + return collisions; + } + /** Create/column-sync one physical shard table (mirrors the managed-table * branch of {@link initObjects}, scoped to a shard). */ protected async ensureShardTable(shardName: string, obj: { fields?: Record; tenancy?: any }): Promise { const builtinColumns = new Set(['id', 'created_at', 'updated_at']); + // [#12015] Both branches below drop a declared field named after a builtin + // column — the create branch skips it explicitly, the column-sync branch + // finds the column already present — so the shard path warns once here, + // ahead of either. + this.warnBuiltinColumnCollisions(shardName, obj.fields, builtinColumns, 'shard'); const exists = await this.knex.schema.hasTable(shardName); // #11374: a shard carries the base table's declared indexes (below), so its // columns need the same keyable-text decision the managed path makes. @@ -9090,6 +9150,10 @@ export class SqlDriver implements IDataDriver { }); if (!exists) { + // [#12015] The `continue` below drops a declared field that collides + // with a builtin column. Said BEFORE the DDL runs, so the author hears + // it even when the CREATE goes on to fail for an unrelated reason. + this.warnBuiltinColumnCollisions(tableName, obj.fields, builtinColumns, 'create'); try { await this.knex.schema.createTable(tableName, (table) => { table.string('id').primary(); @@ -9119,6 +9183,13 @@ export class SqlDriver implements IDataDriver { this.tablesWithTimestamps.add(tableName); } + // [#12015] The ADD COLUMN diff is keyed on "column not already there", + // so a declared `id`/`created_at`/`updated_at` is dropped on this path + // too — the builtin is already in `existingColumns`, so the diff below + // never proposes it and `addedColumns` excludes it by name. Silent on + // every boot of an existing table until now. + this.warnBuiltinColumnCollisions(tableName, obj.fields, builtinColumns, 'alter'); + // #11565: the row budget is a property of the WHOLE row, so adding one // ordinary column to a wide table is refused by the width of columns // nobody is touching — with the same column-less server error. Named From be08012875b79e162645607a027c3a662a356932 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 09:56:37 +0000 Subject: [PATCH 2/2] fix(driver-sql): report only the storage half a builtin-column collision discards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --- .../builtin-column-collision-warning.md | 39 ++- .../src/builtin-column-collision.test.ts | 132 +++++++++ .../src/builtin-column-collision.ts | 275 ++++++++++++++++++ ...5-builtin-column-collision-warning.test.ts | 120 +++++--- packages/drivers/driver-sql/src/sql-driver.ts | 85 ++++-- 5 files changed, 574 insertions(+), 77 deletions(-) create mode 100644 packages/drivers/driver-sql/src/builtin-column-collision.test.ts create mode 100644 packages/drivers/driver-sql/src/builtin-column-collision.ts diff --git a/.changeset/builtin-column-collision-warning.md b/.changeset/builtin-column-collision-warning.md index a8059fbb6d..2dcc688fdd 100644 --- a/.changeset/builtin-column-collision-warning.md +++ b/.changeset/builtin-column-collision-warning.md @@ -2,21 +2,21 @@ "@objectstack/driver-sql": patch --- -fix(driver-sql): a declared field colliding with a builtin column is named at load time instead of silently discarded (#12015) +fix(driver-sql): name the storage a declaration on a builtin column name loses, instead of discarding it in silence (#12015) `initObjects` emits `id`, `created_at` and `updated_at` itself and then skips any declared field colliding with one — `if (builtinColumns.has(name)) continue;`, with -no warning, no throw and no record anywhere that the author's declaration was +no warning, no throw and no record anywhere that the author's declaration had been dropped. Measured on live PostgreSQL 16.13: an object declaring `id: { type: 'text' }` boots green and gets `id varchar(255)` — `table.string('id')`, not TEXT. Measured here on SQLite: the same substitution, and a declared -`maxLength: 12` on that field binds nothing (`varchar(255)`). The driver is right to -own its primary key and audit stamps; the defect was that it disagreed with the -author in silence — the declared-≠-enforced shape that bites hardest on AI-authored -metadata, where the mismatch surfaces much later as data behaving oddly. +`maxLength: 12` on that field binds nothing. The driver is right to own its primary +key and audit stamps; the defect was that it disagreed with the author in silence — +the declared-≠-enforced shape that bites hardest on AI-authored metadata, where the +mismatch surfaces much later as data behaving oddly. -Every DDL path that drops such a declaration now says so, once per colliding field, -naming the field, the object and the platform's ownership: +Every DDL path that drops such a declaration now says so, naming the field, the +object, the attributes that were lost and what the platform's column actually is: - **create** — `while creating table "…"`, said before the CREATE runs, so the author hears it even when the CREATE goes on to fail for an unrelated reason; @@ -30,11 +30,22 @@ A warning on one path with silence on the others just moves the trap, so each pa carries its own call and its own pin: a regression to a silent `continue` on one path fails by name rather than being absorbed by a sibling. +**Only the STORAGE half is reported, because only the storage half is lost.** A +declaration on a builtin column name still carries `label` (and the locales generated +from it), `readonly`, `searchable` and the ADR-0113 write contract in `required` — all +honoured on the platform's column exactly as on any other. So the diagnostic fires +only when the declaration asks for storage the platform's own column does not deliver +(a differing `type`, a `maxLength`, `unique`, `defaultValue`, `storage.notNull`, a +`multiple` shape…) and stays silent when it does not: `created_at: { type: 'datetime', +defaultValue: 'NOW()' }` describes precisely what lands, and says nothing. +`id: { type: 'number' }` — an author expecting a numeric key — still fires, as does +`id: { type: 'text' }`. The storage/presentation split is one table +(`builtin-column-collision.ts`) pinned against `FieldSchema.shape`, so a field key +added later is classified deliberately instead of defaulting into silence. + **Grade: `patch`, and deliberately.** Nothing about the accept set moves — every object that booted before still boots, the DDL emitted is byte-identical, no public -type or metadata key changes, and the only observable difference is a line in the -log for a declaration that was already being discarded. Maintainer ruling 2026-08-25 -took exactly this and explicitly did **not** take "refuse boot" (a new rejection door -on an existing accept path, which needs an inventory of existing objects first) or -"make the declaration meaningful" (capability expansion with no pull — the platform -owning its primary key is correct design; the defect was only the silence). +type or metadata key changes, and the only observable difference is a line in the log +for storage that was already being discarded. The platform still owns `id` / +`created_at` / `updated_at`: this changes what the driver **says**, never what it +**does**. diff --git a/packages/drivers/driver-sql/src/builtin-column-collision.test.ts b/packages/drivers/driver-sql/src/builtin-column-collision.test.ts new file mode 100644 index 0000000000..41adb24ae9 --- /dev/null +++ b/packages/drivers/driver-sql/src/builtin-column-collision.test.ts @@ -0,0 +1,132 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12015 — the storage/presentation split itself, pinned. + * + * The diagnostic in `SqlDriver` fires on what this module decides, so the + * decision is worth more than the plumbing around it. Two claims live here: + * + * ① **The classification is exhaustive over `FieldSchema`.** A key added to + * the spec later fails the first case until someone classifies it — the + * whole point of keeping the table in one place. At runtime an unclassified + * key is silent (a diagnostic must never invent a warning it cannot + * justify), so without this pin an added key would default into silence + * with nothing to notice it. + * + * ② **Delivered means delivered.** A declared storage attribute the platform's + * own column already provides is NOT a disagreement and must not be + * reported as one — that is the whole content of the 2026-08-25 narrowing, + * and the case that makes the message true again. + */ + +import { describe, it, expect } from 'vitest'; +import { FieldSchema } from '@objectstack/spec/data'; +import { + FIELD_KEY_STORAGE_CLASS, + BUILTIN_COLUMN_DELIVERY, + undeliveredStorageAttributes, +} from './builtin-column-collision.js'; + +/** Just the keys, for readability in the assertions below. */ +const keysOf = (attrs: ReturnType) => attrs.map((a) => a.key); + +describe('the FieldSchema storage/presentation classification (#12015)', () => { + it('classifies EVERY FieldSchema key, and invents none', () => { + const declared = Object.keys(FieldSchema.shape).sort(); + const classified = Object.keys(FIELD_KEY_STORAGE_CLASS).sort(); + + // A spec key with no classification: it would be silent at runtime, which + // is safe but undeliberate. Classify it in `FIELD_KEY_STORAGE_CLASS`. + expect(declared.filter((k) => !classified.includes(k)), 'unclassified FieldSchema key(s)').toEqual([]); + // A classification with no spec key: dead weight that reads as coverage. + expect(classified.filter((k) => !declared.includes(k)), 'classified key(s) FieldSchema does not declare').toEqual([]); + }); + + it('puts `required` on the PRESENTATION side — ADR-0113 makes it the WRITE contract, not a column constraint', () => { + // The load-bearing classification: `required: true` appears on nearly every + // platform object's `id`, the engine enforces it there exactly as anywhere + // else, and calling it "discarded" is the false sentence this card removed. + expect(FIELD_KEY_STORAGE_CLASS.required).toBe('presentation'); + // Its ADR-0113 sibling — the one that IS the column constraint. + expect(FIELD_KEY_STORAGE_CLASS.storage).toBe('storage'); + }); + + it('puts the honoured half on the presentation side and the column shape on the storage side', () => { + for (const key of ['label', 'readonly', 'searchable', 'description', 'inlineHelpText', 'group', 'name']) { + expect(FIELD_KEY_STORAGE_CLASS[key], `${key} is honoured on a builtin column`).toBe('presentation'); + } + for (const key of ['type', 'maxLength', 'unique', 'defaultValue', 'multiple', 'expression']) { + expect(FIELD_KEY_STORAGE_CLASS[key], `${key} shapes the physical column`).toBe('storage'); + } + }); + + it('records what each builtin column actually delivers, read off the emitting lines', () => { + // `table.string('id').primary()` — varchar(255), NOT NULL, unique, no default. + expect(BUILTIN_COLUMN_DELIVERY.id).toMatchObject({ + type: 'string', maxLength: 255, unique: true, notNull: true, defaultValue: null, + }); + // `createAuditTimestampColumn` — a timestamp defaulted to the DB clock, left NULLABLE. + for (const column of ['created_at', 'updated_at']) { + expect(BUILTIN_COLUMN_DELIVERY[column]).toMatchObject({ + type: 'datetime', unique: false, notNull: false, defaultValue: 'NOW()', + }); + } + }); +}); + +describe('what a declaration on a builtin column name loses (#12015)', () => { + it('FIRES on the author error the card was filed for', () => { + // `id: { type: 'number' }` — an author expecting a numeric key. + expect(keysOf(undeliveredStorageAttributes('id', { type: 'number' }))).toEqual(['type']); + // The #11456 fixture's shape. + expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', name: 'id' }))).toEqual(['type']); + // …and names what the column really is, not just that something was lost. + expect(undeliveredStorageAttributes('id', { type: 'text' })[0]).toMatchObject({ + key: 'type', declared: 'text', delivered: 'string', + }); + }); + + it('is SILENT for a presentation-only declaration — the platform honours that half', () => { + // `sys_presence.id`, verbatim in shape: the population the pre-narrowing + // warning was false about. + expect( + undeliveredStorageAttributes('id', { type: 'string', label: 'Presence ID', required: true, readonly: true }), + ).toEqual([]); + expect( + undeliveredStorageAttributes('created_at', { + type: 'datetime', label: 'Created At', defaultValue: 'NOW()', readonly: true, + }), + ).toEqual([]); + }); + + it('is SILENT for a storage attribute the column already delivers', () => { + expect(undeliveredStorageAttributes('id', { type: 'string', maxLength: 255 })).toEqual([]); + expect(undeliveredStorageAttributes('id', { type: 'string', unique: true })).toEqual([]); // the PK is unique + expect(undeliveredStorageAttributes('id', { type: 'string', storage: { notNull: true } })).toEqual([]); // the PK is NOT NULL + expect(undeliveredStorageAttributes('created_at', { type: 'datetime', defaultValue: 'now()' })).toEqual([]); // token, case-insensitive + }); + + it('FIRES for a storage attribute the column does NOT deliver, one entry each', () => { + expect(keysOf(undeliveredStorageAttributes('id', { type: 'string', maxLength: 12 }))).toEqual(['maxLength']); + expect(keysOf(undeliveredStorageAttributes('id', { type: 'string', defaultValue: 'NOW()' }))).toEqual(['defaultValue']); + // created_at IS nullable and NOT unique — asking for either is a real disagreement. + expect(keysOf(undeliveredStorageAttributes('created_at', { type: 'datetime', unique: true }))).toEqual(['unique']); + expect(keysOf(undeliveredStorageAttributes('created_at', { type: 'datetime', storage: { notNull: true } }))) + .toEqual(['storage.notNull']); + // Several at once, in declaration order. + expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', maxLength: 12, unique: false }))) + .toEqual(['type', 'maxLength']); // `unique: false` asks for nothing + }); + + it('ignores a field that is not a builtin column name at all', () => { + expect(undeliveredStorageAttributes('region', { type: 'text', maxLength: 12 })).toEqual([]); + }); + + it('stays silent — never throws — on a key it does not know, and on a malformed declaration', () => { + // Forward compatibility: an unclassified key cannot invent a warning. The + // exhaustiveness case above is what makes its arrival visible. + expect(undeliveredStorageAttributes('id', { type: 'string', someFutureKey: 'x' } as any)).toEqual([]); + expect(undeliveredStorageAttributes('id', undefined)).toEqual([]); + expect(undeliveredStorageAttributes('id', null as any)).toEqual([]); + }); +}); diff --git a/packages/drivers/driver-sql/src/builtin-column-collision.ts b/packages/drivers/driver-sql/src/builtin-column-collision.ts new file mode 100644 index 0000000000..3e78a6cd31 --- /dev/null +++ b/packages/drivers/driver-sql/src/builtin-column-collision.ts @@ -0,0 +1,275 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12015 — WHICH HALF of a declaration on a builtin column name is discarded. + * + * `initObjects` emits `id`, `created_at` and `updated_at` itself and skips any + * declared field colliding with one, so the column that lands is the + * platform's rather than the author's. The first cut of the diagnostic said so + * for *any* such declaration — and on the dominant population that sentence + * was not merely noisy, it was **false**. Measured before this narrowing: 116 + * warnings on a stock boot of `@objectstack/platform-objects` alone, against + * declarations like + * + * ```ts + * id: Field.text({ label: 'Presence ID', required: true, readonly: true }) + * ``` + * + * whose `label` **is** applied — it feeds the generated + * `*.objects.generated.ts` translation files in four locales, `highlightFields`, + * FLS and sortability — and whose `required` **is** enforced, by the engine's + * write contract. Only the *storage* half is thrown away. "The declaration is + * NOT applied … remove the declaration" was therefore untrue there, and acting + * on it would have deleted an author-facing label for a column every list view + * shows. + * + * Maintainer ruling 2026-08-25: narrow the trigger to declarations asking for + * **storage behaviour the platform's own column does not deliver**, and stay + * silent when only the honoured half is declared. The narrowing changes what we + * SAY, not what we DO: the platform still owns these three columns, the + * declaration still does not take effect, no accept/reject door moves. + * ⛔ Not route C (make the declaration meaningful) and ⛔ not route B (refuse). + * + * # Why the classification lives here, in one table + * + * Scattering "is this key storage?" across the three call sites is how the two + * halves drift apart. One table, one answer, and {@link FIELD_KEY_STORAGE_CLASS} + * is pinned against `FieldSchema.shape` by + * `builtin-column-collision.test.ts` — so a key added to the spec later fails + * that pin until someone classifies it deliberately, rather than defaulting + * into silence unnoticed. + * + * At RUNTIME an unclassified key is treated as presentation (silent). That is + * the safe direction for a diagnostic on a boot path: an unknown key can only + * ever fail to produce a warning, never invent a false one, and never refuse a + * boot — the pin, not a throw, is what makes the omission visible. + */ + +import { isNowDefaultToken } from '@objectstack/spec/data'; + +/** + * Storage-affecting = the SQL driver's DDL layer would have read this key to + * shape the physical column or its indexes, so on a builtin column name it is + * discarded. Presentation = honoured by some other layer (metadata, i18n, the + * engine's write contract, the UI), so on a builtin column name it still takes + * effect and must NOT be reported as discarded. + * + * ⚠️ The line is drawn at what the DDL reads, not at what the key "feels" like. + * The load-bearing example is `required`: ADR-0113 makes it the **write + * contract**, deliberately NOT a column constraint (`storage.notNull` is), and + * the engine enforces it on the platform's own column exactly as on any other. + * Classifying it as storage would re-create the false sentence this narrowing + * exists to remove. + * + * Where a key is storage-class in intent but this driver's DDL happens not to + * read it yet (`precision`, `scale`, `dimensions`, `deleteBehavior`), it is + * still classified `storage`. A diagnostic may say "this was discarded" about + * something no emitter honours; it may not stay silent about something an + * emitter would have honoured. + */ +export type FieldKeyClass = 'storage' | 'presentation'; + +export const FIELD_KEY_STORAGE_CLASS: Readonly> = Object.freeze({ + // ---- storage: the column's own shape ------------------------------------- + type: 'storage', // `createColumn`: the column type itself + maxLength: 'storage', // `createColumn`: varchar(n) vs TEXT, and the #11374 keyable decision + multiple: 'storage', // `createColumn`: a multi-value field is a JSON column + precision: 'storage', // numeric column shape (this driver does not read it yet) + scale: 'storage', // numeric column shape (this driver does not read it yet) + dimensions: 'storage', // vector column width + defaultValue: 'storage', // `createColumn`: the physical column DEFAULT + storage: 'storage', // ADR-0113: `storage.notNull` IS the column constraint + unique: 'storage', // materialized as a UNIQUE index by `syncTableIndexes` + reference: 'storage', // `createColumn` shapes a lookup column after its target key + referenceVia: 'storage', // junction storage for a multi-value reference + deleteBehavior: 'storage', // referential behaviour of the stored key + expression: 'storage', // a formula field materializes NO column (`fieldHasColumn`) + returnType: 'storage', // the formula's stored/returned type + summaryOperations: 'storage', // rollup storage + currencyConfig: 'storage', // currency mode can change what is physically stored + + // ---- presentation: honoured by layers other than the DDL ----------------- + name: 'presentation', // identity; the physical column is the field KEY (`columnName` was retired, #2377) + label: 'presentation', // metadata + i18n; the half the false sentence used to deny + description: 'presentation', + inlineHelpText: 'presentation', + placeholder: 'presentation', + format: 'presentation', // display/validation hint + required: 'presentation', // ADR-0113: the WRITE contract, enforced by the engine, not the column + minLength: 'presentation', // write-time validation + min: 'presentation', // write-time validation + max: 'presentation', // write-time validation + step: 'presentation', // input granularity + useGrouping: 'presentation', + options: 'presentation', // select values: validation + UI, no DDL + accept: 'presentation', // upload validation + maxSize: 'presentation', // upload validation + language: 'presentation', // editor language hint + searchable: 'presentation', + sortable: 'presentation', + trackHistory: 'presentation', // history rows live in their own object + group: 'presentation', + widget: 'presentation', + hidden: 'presentation', + internal: 'presentation', + readonly: 'presentation', + visibleWhen: 'presentation', + readonlyWhen: 'presentation', + requiredWhen: 'presentation', + conditionalRequired: 'presentation', + requiredPermissions: 'presentation', + maskingRule: 'presentation', + ackPlaintextMasking: 'presentation', + system: 'presentation', + inlineEdit: 'presentation', + inlineTitle: 'presentation', + inlineColumns: 'presentation', + inlineAmountField: 'presentation', + relatedList: 'presentation', + relatedListTitle: 'presentation', + relatedListColumns: 'presentation', + relatedListFilter: 'presentation', + displayField: 'presentation', + descriptionField: 'presentation', + lookupColumns: 'presentation', + lookupPageSize: 'presentation', + lookupFilters: 'presentation', + dependsOn: 'presentation', + allowCreate: 'presentation', + // Read by the WRITE path, never by the DDL: `initObjects` registers an + // autonumber generator off `type: 'auto_number'` without skipping builtin + // names, so the format an author declares is honoured rather than discarded. + autonumberFormat: 'presentation', + externalId: 'presentation', // upsert matching semantics; no column of its own + // ADR-0010 governance/provenance markers stamped by the metadata loader. + // Metadata about the declaration, never about the column. + _lock: 'presentation', + _lockReason: 'presentation', + _lockSource: 'presentation', + _lockDocsUrl: 'presentation', + _packageId: 'presentation', + _packageVersion: 'presentation', + _provenance: 'presentation', +}); + +/** + * What the platform's own column ACTUALLY delivers — read off the emitting + * lines in `SqlDriver`, not assumed: + * + * - `id` — `table.string('id').primary()`: varchar(255), NOT NULL, + * PRIMARY KEY (so: unique), no column default (the engine + * generates the key). + * - `created_at` / `updated_at` — `createAuditTimestampColumn`: a timestamp + * (MySQL `datetime(3)`) defaulted to the database clock, + * left NULLABLE and stamped by the driver on every write. + * + * A declared storage attribute equal to what the column already delivers is + * NOT a disagreement and must stay silent — `created_at: { type: 'datetime', + * defaultValue: 'NOW()' }` describes precisely what lands. + */ +export interface BuiltinColumnDelivery { + /** The field type whose column this builtin actually is. */ + type: string; + /** Fixed varchar width, when the column is bounded. */ + maxLength?: number; + /** Does the column already carry a uniqueness guarantee? */ + unique: boolean; + /** Does the column already carry a physical NOT NULL? */ + notNull: boolean; + /** `'NOW()'` when the column defaults to the database clock; `null` for no default. */ + defaultValue: 'NOW()' | null; +} + +export const BUILTIN_COLUMN_DELIVERY: Readonly> = Object.freeze({ + id: Object.freeze({ type: 'string', maxLength: 255, unique: true, notNull: true, defaultValue: null }), + created_at: Object.freeze({ type: 'datetime', unique: false, notNull: false, defaultValue: 'NOW()' as const }), + updated_at: Object.freeze({ type: 'datetime', unique: false, notNull: false, defaultValue: 'NOW()' as const }), +}); + +/** One storage attribute the declaration asked for and the builtin column does not provide. */ +export interface UndeliveredAttribute { + /** The declared key, spelled as the author wrote it. */ + key: string; + /** What the author asked for. */ + declared: unknown; + /** What the platform's column provides instead, or `undefined` when it provides nothing of the kind. */ + delivered: unknown; +} + +/** A declared value that asks for nothing at all — absent, or an explicit opt-out. */ +function asksForNothing(value: unknown): boolean { + if (value === undefined || value === null || value === false) return true; + if (typeof value === 'string' && value.trim() === '') return true; + if (Array.isArray(value) && value.length === 0) return true; + if (typeof value === 'object' && Object.keys(value as object).length === 0) return true; + return false; +} + +/** + * The storage attributes `field` declares that the builtin `column` does not + * deliver — empty when the declaration and the platform's column agree, or + * when only the honoured (presentation) half was declared. + * + * Pure and side-effect free: the caller decides whether to log. `field` is + * whatever the author wrote, so every read is defensive. + */ +export function undeliveredStorageAttributes( + column: string, + field: Record | undefined, +): UndeliveredAttribute[] { + const delivery = BUILTIN_COLUMN_DELIVERY[column]; + if (!delivery || !field || typeof field !== 'object') return []; + + const out: UndeliveredAttribute[] = []; + for (const [key, declared] of Object.entries(field)) { + if (FIELD_KEY_STORAGE_CLASS[key] !== 'storage') continue; + + // `storage: { notNull }` is the one nested storage key (ADR-0113): compare + // the constraint it asks for, not the wrapper object. + if (key === 'storage') { + const notNull = (declared as { notNull?: unknown } | undefined)?.notNull; + if (asksForNothing(notNull)) continue; + if (delivery.notNull === true) continue; + out.push({ key: 'storage.notNull', declared: notNull, delivered: false }); + continue; + } + + if (asksForNothing(declared)) continue; + + switch (key) { + case 'type': + if (declared === delivery.type) continue; + out.push({ key, declared, delivered: delivery.type }); + continue; + case 'maxLength': + if (delivery.maxLength !== undefined && declared === delivery.maxLength) continue; + out.push({ key, declared, delivered: delivery.maxLength }); + continue; + case 'unique': + if (delivery.unique) continue; + out.push({ key, declared, delivered: false }); + continue; + case 'defaultValue': + // The framework's clock token is a vocabulary, not a literal — a + // column already defaulted to the database clock DELIVERS it. + if (delivery.defaultValue === 'NOW()' && isNowDefaultToken(declared as string)) continue; + out.push({ key, declared, delivered: delivery.defaultValue ?? undefined }); + continue; + default: + // Every other storage key: the builtin column provides nothing of the + // kind, so asking for it is always a disagreement. + out.push({ key, declared, delivered: undefined }); + continue; + } + } + return out; +} + +/** `type: 'text'` → `type: 'text'`; objects are summarized rather than dumped. */ +export function formatAttribute(attr: UndeliveredAttribute): string { + const show = (v: unknown): string => + typeof v === 'string' ? `'${v}'` : typeof v === 'object' && v !== null ? JSON.stringify(v) : String(v); + return attr.delivered === undefined + ? `${attr.key}: ${show(attr.declared)}` + : `${attr.key}: ${show(attr.declared)} (the column is ${show(attr.delivered)})`; +} diff --git a/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts b/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts index c05fc41f20..51d167d966 100644 --- a/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts @@ -1,20 +1,25 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * #12015 — a declared field named after a builtin column is DROPPED, and until - * now it was dropped in silence. + * #12015 — a declared field named after a builtin column loses its STORAGE + * half, and until now it lost it in silence. * * `initObjects` emits `id`, `created_at` and `updated_at` itself, then skips * any declared field colliding with one. The driver is right to own its * primary key and audit stamps; the defect was that it disagreed with the * author without saying so — an object declaring `id: { type: 'text' }` boots - * green and gets `varchar(255)`, and nothing anywhere records that the - * declaration was discarded. + * green and gets `varchar(255)`, and nothing recorded that the declared type + * was discarded. * - * Maintainer ruling 2026-08-25: a **named, loud load-time warning** on all - * three paths that drop such a declaration. Explicitly NOT a rejection door - * (that needs an inventory of existing objects first) and explicitly NOT - * "make the declaration meaningful" (capability expansion with no pull). + * Maintainer ruling 2026-08-25, twice: a **named, loud load-time warning** on + * all three paths that drop such a declaration, then **narrowed** to the + * declarations that actually lose something — those asking for storage the + * platform's own column does not deliver. A presentation-only declaration + * (`label`, `readonly`, the ADR-0113 write contract in `required`) is honoured + * on the platform's column exactly as anywhere else, so warning about it was + * not merely noisy but false, and its advice ("remove the declaration") would + * have deleted a label four locales are generated from. Explicitly NOT a + * rejection door and NOT "make the declaration meaningful". * * ## What each case is worth * @@ -28,19 +33,21 @@ * - `while syncing existing table "…"` — the ADD COLUMN diff branch * - `while syncing shard "…"` — the rotation shard path * - * Two non-presence cases carry the rest of the claim: the warning does NOT - * fire for an object that declares no builtin name (a pin that fires on - * everything is not a diagnostic), and the accept set is untouched — the - * object still boots, and the physical column is still the platform's, which - * is also this file's SQLite measurement of the defect itself (the card - * measured PostgreSQL 16.13 only). + * Three non-presence cases carry the rest of the claim: a presentation-only + * declaration boots **silently** end-to-end (the narrowing, measured through + * the real driver rather than through the classifier alone — that module's own + * split is pinned in `builtin-column-collision.test.ts`); an object declaring + * no builtin name is silent; and the accept set is untouched — the object + * still boots and the physical column is still the platform's, which is also + * this file's SQLite measurement of the defect itself (the card measured + * PostgreSQL 16.13 only). */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { SqlDriver } from './index.js'; /** Every line this file cares about; the `[sql-driver]` prefix alone is shared with much else. */ -const COLLISION_WARNINGS = /collides with a builtin column the platform owns/; +const COLLISION_WARNINGS = /asks for storage the platform's own/; function warnings(driver: SqlDriver): string[] { return ((driver as any).logger.warn as ReturnType).mock.calls @@ -71,7 +78,7 @@ describe('a declared field colliding with a builtin column is named at load time await driver.disconnect(); }); - it('CREATE path: names the field, the object, and that the platform owns the column', async () => { + it('CREATE path: names the field, the object, the attribute lost and what the column really is', async () => { await driver.initObjects([ { name: 'collide_create', @@ -85,32 +92,40 @@ describe('a declared field colliding with a builtin column is named at load time const lines = warnings(driver); expect(lines).toHaveLength(1); - // The line owes the author three things: WHICH field, on WHICH object, and - // that the platform — not the author — owns that column. + // The line owes the author four things: WHICH field, on WHICH object, WHAT + // was lost, and what the platform's column actually is. expect(lines[0]).toContain("declared field 'id'"); expect(lines[0]).toContain('collide_create'); - expect(lines[0]).toContain('collides with a builtin column the platform owns'); - expect(lines[0]).toContain('the declaration is NOT applied'); + expect(lines[0]).toContain("asks for storage the platform's own 'id' column does not provide"); + expect(lines[0]).toContain("type: 'text' (the column is 'string')"); + // ⛔ And it must NOT deny the half that IS applied, nor advise deleting it. + expect(lines[0]).toContain('is honoured as written'); + expect(lines[0]).not.toContain('Remove the declaration'); // Path identity, so a silent regression on THIS path cannot be masked by // the other two still warning. expect(lines[0]).toContain('while creating table "collide_create"'); - expect(warningMeta(driver)[0]).toMatchObject({ table: 'collide_create', field: 'id', phase: 'create' }); + expect(warningMeta(driver)[0]).toMatchObject({ + table: 'collide_create', field: 'id', phase: 'create', undelivered: ['type'], + }); }); - it('CREATE path: one line per colliding field, and only for the colliding ones', async () => { + it('CREATE path: one line per colliding field that loses something, and only those', async () => { await driver.initObjects([ { name: 'collide_three', fields: { - id: { type: 'text' }, - created_at: { type: 'datetime' }, - updated_at: { type: 'datetime' }, + id: { type: 'text' }, // text ≠ the varchar(255) key + created_at: { type: 'date' }, // date ≠ the timestamp column + updated_at: { type: 'datetime', unique: true }, // the audit column carries no uniqueness + // Declared, colliding, and losing NOTHING — the platform's column is + // exactly this, and the label/readonly half is honoured. payload: { type: 'text' }, }, }, ]); expect(warningMeta(driver).map((m) => m.field)).toEqual(['id', 'created_at', 'updated_at']); + expect(warningMeta(driver).map((m) => m.undelivered)).toEqual([['type'], ['type'], ['unique']]); }); it('ADD COLUMN diff path: an EXISTING table warns too — the diff never proposes the builtin', async () => { @@ -124,15 +139,18 @@ describe('a declared field colliding with a builtin column is named at load time await driver.initObjects([ { name: 'collide_alter', - fields: { payload: { type: 'text' }, created_at: { type: 'datetime' }, note: { type: 'text' } }, + fields: { payload: { type: 'text' }, created_at: { type: 'date' }, note: { type: 'text' } }, }, ]); const lines = warnings(driver); expect(lines).toHaveLength(1); expect(lines[0]).toContain("declared field 'created_at'"); + expect(lines[0]).toContain("type: 'date' (the column is 'datetime')"); expect(lines[0]).toContain('while syncing existing table "collide_alter"'); - expect(warningMeta(driver)[0]).toMatchObject({ table: 'collide_alter', field: 'created_at', phase: 'alter' }); + expect(warningMeta(driver)[0]).toMatchObject({ + table: 'collide_alter', field: 'created_at', phase: 'alter', undelivered: ['type'], + }); // Non-vacuity: this really was the ADD COLUMN branch — the ordinary new // column landed, so the diff ran rather than the create branch. @@ -144,21 +162,50 @@ describe('a declared field colliding with a builtin column is named at load time await driver.initObjects([ { name: 'collide_rot', - fields: { payload: { type: 'text' }, created_at: { type: 'datetime' } }, + fields: { + id: { type: 'text' }, + payload: { type: 'text' }, + // Declared AND colliding, but it describes the column the platform + // emits — so it must not appear below. + created_at: { type: 'datetime' }, + }, lifecycle: { class: 'telemetry', storage: { strategy: 'rotation', shards: 3, unit: 'day' } }, } as any, ]); const lines = warnings(driver); - // The rotation path is the ONLY one that ran — the base name is a view, - // so the managed create/alter branches never saw this object. + // One line: `id` only. The rotation path is also the ONLY path that ran — + // the base name is a view, so the managed create/alter branches never saw + // this object. expect(lines).toHaveLength(1); - expect(lines[0]).toContain("declared field 'created_at'"); + expect(lines[0]).toContain("declared field 'id'"); expect(lines[0]).toContain('while syncing shard "collide_rot__r'); - expect(warningMeta(driver)[0]).toMatchObject({ field: 'created_at', phase: 'shard' }); + expect(warningMeta(driver)[0]).toMatchObject({ field: 'id', phase: 'shard', undelivered: ['type'] }); expect(String(warningMeta(driver)[0].table)).toMatch(/^collide_rot__r\d{6,8}$/); }); + it('THE NARROWING, end-to-end: a presentation-only declaration boots in silence', async () => { + // `sys_presence` in shape — the population the pre-narrowing warning fired + // on 116 times per stock boot of platform-objects while telling the author + // something untrue about it. + await driver.initObjects([ + { + name: 'collide_presentation', + fields: { + id: { type: 'string', label: 'Presence ID', required: true, readonly: true }, + created_at: { type: 'datetime', label: 'Created At', defaultValue: 'NOW()', readonly: true }, + updated_at: { type: 'datetime', label: 'Updated At', defaultValue: 'NOW()', readonly: true }, + status: { type: 'text' }, + }, + }, + ]); + + expect(warnings(driver)).toHaveLength(0); + // Non-vacuity: the object really did boot through the collision path. + const info = await (driver as any).knex('collide_presentation').columnInfo(); + expect(Object.keys(info).sort()).toEqual(['created_at', 'id', 'status', 'updated_at']); + }); + it('does not fire for an object that declares no builtin column name', async () => { await driver.initObjects([ { name: 'no_collision', fields: { region: { type: 'text' }, score: { type: 'number' } } }, @@ -171,13 +218,16 @@ describe('a declared field colliding with a builtin column is named at load time { name: 'collide_accept', // A declaration that asks for something quite different from what the - // platform emits: TEXT, unbounded, plus a length the driver never reads. + // platform emits: TEXT, plus a length the driver never reads. fields: { id: { type: 'text', maxLength: 12 }, region: { type: 'text' } }, }, ]); - // Booted, warned, and still fully usable — a warning moves no door. - expect(warnings(driver)).toHaveLength(1); + // Booted, warned once naming BOTH lost attributes, and still fully usable — + // a warning moves no door. + const lines = warnings(driver); + expect(lines).toHaveLength(1); + expect(warningMeta(driver)[0].undelivered).toEqual(['type', 'maxLength']); await driver.create('collide_accept', { id: 'r1', region: 'emea' }, { bypassTenantAudit: true }); expect(await driver.count('collide_accept', {})).toBe(1); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index bfdbb89212..db4846beda 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -97,6 +97,11 @@ import { type PhysicalColumn, type PendingSchemaWork, } from './schema-drift.js'; +import { + undeliveredStorageAttributes, + formatAttribute, + type UndeliveredAttribute, +} from './builtin-column-collision.js'; import knex, { Knex } from 'knex'; import { nanoid } from 'nanoid'; import { createHash } from 'node:crypto'; @@ -8624,28 +8629,41 @@ export class SqlDriver implements IDataDriver { } /** - * [#12015] Say out loud that a declared field was dropped for colliding with - * a builtin column. + * [#12015] Say out loud which half of a declaration on a builtin column name + * was discarded — and say nothing when nothing was. * * `id`, `created_at` and `updated_at` are emitted by the driver itself, so a * field an author DECLARES under one of those names never reaches - * {@link createColumn} — its declared type, length and constraints are - * discarded. The driver is right to own its primary key and audit stamps; - * the defect was that it disagreed with the author in **silence**. Measured - * on live PostgreSQL 16.13: an object declaring `id: { type: 'text' }` boots - * green and gets `id varchar(255)` — `table.string('id')`, not TEXT — and - * nothing anywhere said the declaration had been thrown away. That is the - * declared-≠-enforced hazard this repo treats as first-class, and it bites - * hardest on AI-authored metadata, where the mismatch surfaces much later as - * data behaving oddly. - * - * ⛔ This is a diagnostic, NOT a rejection door. Maintainer ruling - * 2026-08-25 took the warning and explicitly did **not** take "refuse boot" - * (a new rejection door on an existing accept path, whose stock inventory is - * unmeasured) nor "make the declaration meaningful" (capability expansion - * with no pull — the platform owning its primary key is correct design; the - * defect was only the silence). Every object that boots today still boots, - * with one more line in the log. + * {@link createColumn}: its declared column shape is dropped. The driver is + * right to own its primary key and audit stamps; the defect was that it + * disagreed with the author in **silence**. Measured on live PostgreSQL + * 16.13: an object declaring `id: { type: 'text' }` boots green and gets + * `id varchar(255)` — `table.string('id')`, not TEXT — with nothing anywhere + * recording that the declaration had been thrown away. Same substitution + * measured here on SQLite, where a declared `maxLength` binds nothing either. + * + * # Why this fires on a SUBSET of collisions, and why that is the point + * + * Only the STORAGE half is discarded. The rest of the declaration — `label` + * and its four generated locales, `readonly`, `searchable`, the ADR-0113 + * write contract in `required` — is honoured, on the platform's column + * exactly as on any other. The first cut of this warning fired on every + * collision and told the author "the declaration is NOT applied … remove the + * declaration": measured at 116 lines on a stock boot of + * `@objectstack/platform-objects` alone, where the dominant shape is + * `id: Field.text({ label: 'Presence ID', required: true, readonly: true })`. + * That sentence was **false** there, and following it would have deleted an + * author-facing label for a column every list view shows. + * + * So the trigger is "asks for storage the platform's own column does not + * deliver" ({@link undeliveredStorageAttributes}), the message names the + * attributes rather than the declaration, and a presentation-only + * declaration is silent. Maintainer ruling 2026-08-25, narrowing its own + * earlier ruling on this card. + * + * ⛔ Still a diagnostic, NOT a rejection door, and NOT route C: the platform + * still owns the column and the declaration still does not take effect. What + * changed is what we SAY. Every object that booted before still boots. * * `phase` is part of the message because all THREE paths that drop such a * declaration carry this warning — create, the ADD COLUMN diff, and the @@ -8659,23 +8677,34 @@ export class SqlDriver implements IDataDriver { builtinColumns: ReadonlySet, phase: 'create' | 'alter' | 'shard', ): string[] { - const collisions = Object.keys(fields ?? {}).filter((name) => builtinColumns.has(name)); - if (collisions.length === 0) return collisions; const where = { create: `while creating table "${tableName}"`, alter: `while syncing existing table "${tableName}"`, shard: `while syncing shard "${tableName}"`, }[phase]; - for (const field of collisions) { + + const warned: string[] = []; + for (const [field, declaration] of Object.entries(fields ?? {})) { + if (!builtinColumns.has(field)) continue; + const undelivered: UndeliveredAttribute[] = undeliveredStorageAttributes( + field, + declaration as Record, + ); + // Declared, colliding, and yet nothing was lost — the honoured half only. + if (undelivered.length === 0) continue; + this.logger.warn( - `[sql-driver] ${where}: declared field '${field}' collides with a builtin column the platform ` + - `owns (id, created_at, updated_at) — the declaration is NOT applied. Its declared type, ` + - `length and constraints are ignored; the platform's own '${field}' column stands. Remove the ` + - `declaration, or rename the field if you meant a column of your own.`, - { table: tableName, field, phase }, + `[sql-driver] ${where}: declared field '${field}' asks for storage the platform's own ` + + `'${field}' column does not provide — ${undelivered.map(formatAttribute).join('; ')}. ` + + `The platform emits id/created_at/updated_at itself, so THOSE attributes are not applied; ` + + `the rest of the declaration (label, help text, the ADR-0113 write contract, and everything ` + + `other layers read) is honoured as written. Drop the storage attribute(s) named above, or ` + + `rename the field if you meant a column of your own.`, + { table: tableName, field, phase, undelivered: undelivered.map((a) => a.key) }, ); + warned.push(field); } - return collisions; + return warned; } /** Create/column-sync one physical shard table (mirrors the managed-table