diff --git a/.changeset/builtin-column-collision-warning.md b/.changeset/builtin-column-collision-warning.md new file mode 100644 index 0000000000..2dcc688fdd --- /dev/null +++ b/.changeset/builtin-column-collision-warning.md @@ -0,0 +1,51 @@ +--- +"@objectstack/driver-sql": patch +--- + +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 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. 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, 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; +- **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. + +**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 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 new file mode 100644 index 0000000000..51d167d966 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-12015-builtin-column-collision-warning.test.ts @@ -0,0 +1,244 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #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 recorded that the declared type + * was discarded. + * + * 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 + * + * 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 + * + * 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 = /asks for storage the platform's own/; + +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, the attribute lost and what the column really is', 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 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("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', undelivered: ['type'], + }); + }); + + 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' }, // 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 () => { + // 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: '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', 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. + 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: { + 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); + // 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 'id'"); + expect(lines[0]).toContain('while syncing shard "collide_rot__r'); + 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' } } }, + ]); + 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, plus a length the driver never reads. + fields: { id: { type: 'text', maxLength: 12 }, region: { type: 'text' } }, + }, + ]); + + // 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); + + // 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 cf4f8ed509..aabf8bfc35 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'; @@ -8623,10 +8628,94 @@ export class SqlDriver implements IDataDriver { return { object: tableName, current, shards: retained, dropped }; } + /** + * [#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 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 + * 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 where = { + create: `while creating table "${tableName}"`, + alter: `while syncing existing table "${tableName}"`, + shard: `while syncing shard "${tableName}"`, + }[phase]; + + 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}' 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 warned; + } + /** 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 +9179,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 +9212,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