From fc4eda3e36c0c3d193165ee50d0f7144d10d3962 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 08:26:31 +0000 Subject: [PATCH 1/2] =?UTF-8?q?wip(driver-sql,platform-objects):=20#12131?= =?UTF-8?q?=20option=20D=20=E2=80=94=20delivery=20table=20id.type=20+=20sy?= =?UTF-8?q?s=5Fmigration=20rider?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/builtin-column-collision.ts | 27 ++++++++++++++++--- .../src/system/sys-migration.object.ts | 9 ++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/packages/drivers/driver-sql/src/builtin-column-collision.ts b/packages/drivers/driver-sql/src/builtin-column-collision.ts index 3e78a6cd31..dfb00e6cb3 100644 --- a/packages/drivers/driver-sql/src/builtin-column-collision.ts +++ b/packages/drivers/driver-sql/src/builtin-column-collision.ts @@ -158,7 +158,8 @@ export const FIELD_KEY_STORAGE_CLASS: Readonly> = * * - `id` — `table.string('id').primary()`: varchar(255), NOT NULL, * PRIMARY KEY (so: unique), no column default (the engine - * generates the key). + * generates the key). varchar canonicalizes to the field + * type `text`, which is what the table below records. * - `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. @@ -168,7 +169,24 @@ export const FIELD_KEY_STORAGE_CLASS: Readonly> = * defaultValue: 'NOW()' }` describes precisely what lands. */ export interface BuiltinColumnDelivery { - /** The field type whose column this builtin actually is. */ + /** + * The field type whose column this builtin actually is, spelled in the + * SPEC's `FieldType` vocabulary — because that is the vocabulary a + * declaration's `type` is written in, and this value is compared against it + * with `===`. + * + * ⛔ NEVER the knex builder name. `id` is emitted by `table.string('id')`, + * but knex's `string` IS varchar(255), and `canonicalizeSqlType('varchar(255)')` + * is `'text'` — so the field type this column delivers is `'text'`, and + * `isCompatible('varchar(255)', 'text')` is EXACT (both pinned in + * `type-compat.test.ts`). Spelling the builder name here compares two + * vocabularies and reports every correct `id: Field.text(...)` declaration + * as a disagreement: measured at 45 false warnings on a stock boot of + * `@objectstack/platform-objects`, on declarations that were right all + * along. `'string'` is not even authorable — it is absent from `FieldType`'s + * members and `FieldSchema` refuses it — so no declaration could have + * silenced it. + */ type: string; /** Fixed varchar width, when the column is bounded. */ maxLength?: number; @@ -181,7 +199,10 @@ export interface BuiltinColumnDelivery { } export const BUILTIN_COLUMN_DELIVERY: Readonly> = Object.freeze({ - id: Object.freeze({ type: 'string', maxLength: 255, unique: true, notNull: true, defaultValue: null }), + // `table.string('id')` is knex's varchar(255); the FIELD TYPE it delivers is + // `text` (see the vocabulary note on `BuiltinColumnDelivery.type` — do not + // put the builder name back here). + id: Object.freeze({ type: 'text', 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 }), }); diff --git a/packages/platform-objects/src/system/sys-migration.object.ts b/packages/platform-objects/src/system/sys-migration.object.ts index d3957d252c..dc21a49df3 100644 --- a/packages/platform-objects/src/system/sys-migration.object.ts +++ b/packages/platform-objects/src/system/sys-migration.object.ts @@ -60,11 +60,18 @@ export const SysMigration = ObjectSchema.create({ highlightFields: ['id', 'blocking', 'verified_at', 'last_run_at'], fields: { + // ⛔ No `maxLength` here. `id` is a builtin column the platform emits + // itself (`table.string('id').primary()` — varchar(255)), so the DDL + // discards a declared width; and `validateRecord` skips `id` by name on + // both the insert and the update path, so it never binds at write time + // either. The `maxLength: 128` this field used to carry was inert in + // every seam and disagreed with the column it described — the one honest + // line in the #12015 corpus (#12131). The 44 sibling system objects + // declare no width on `id`; this one now matches them. id: Field.text({ label: 'Migration ID', required: true, readonly: true, - maxLength: 128, description: 'Well-known migration id (e.g. adr-0104-file-references). One row per migration.', }), From e385a1212dee8655171b6b982fdfa580d815884c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 08:30:38 +0000 Subject: [PATCH 2/2] fix(driver-sql,platform-objects): builtin-column delivery table speaks the spec field-type vocabulary (#12131) --- .../builtin-column-collision-warning.md | 7 +- .changeset/builtin-column-delivery-id-type.md | 46 ++++++++++++ .../src/builtin-column-collision.test.ts | 71 ++++++++++++++----- ...5-builtin-column-collision-warning.test.ts | 41 +++++++---- 4 files changed, 133 insertions(+), 32 deletions(-) create mode 100644 .changeset/builtin-column-delivery-id-type.md diff --git a/.changeset/builtin-column-collision-warning.md b/.changeset/builtin-column-collision-warning.md index 2dcc688fdd..6cd185c95d 100644 --- a/.changeset/builtin-column-collision-warning.md +++ b/.changeset/builtin-column-collision-warning.md @@ -38,8 +38,11 @@ only when the declaration asks for storage the platform's own column does not de (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 +`id: { type: 'number' }` — an author expecting a numeric key — still fires. +`id: { type: 'text' }` does **not**: varchar(255) canonicalizes to the field type +`text`, so that declaration asks for precisely what the column delivers (#12131 — +the delivery table recorded the knex builder name `'string'` there at first, and +reported all 45 of the platform's own correct `id` declarations as disagreements). 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. diff --git a/.changeset/builtin-column-delivery-id-type.md b/.changeset/builtin-column-delivery-id-type.md new file mode 100644 index 0000000000..21d27f1618 --- /dev/null +++ b/.changeset/builtin-column-delivery-id-type.md @@ -0,0 +1,46 @@ +--- +"@objectstack/driver-sql": patch +"@objectstack/platform-objects": patch +--- + +fix(driver-sql): the builtin-column delivery table speaks the spec's field-type vocabulary, not knex's builder names (#12131) + +`BUILTIN_COLUMN_DELIVERY.id.type` recorded `'string'` — the **knex builder name** from +`table.string('id').primary()` — and `undeliveredStorageAttributes` compares that value +with `===` against a declaration's `type`, which is a spec `FieldType`. The two are +different vocabularies, and `'string'` is not a member of the one being compared: it is +absent from `FieldType`'s 49 options, `Field.string` is absent from the builder's keys, +and `FieldSchema` refuses `type: 'string'` outright. So **no declaration could ever +match it**, and the #12015 diagnostic reported every correct declaration on the +platform's own key as a disagreement. + +Measured on a stock boot of `@objectstack/platform-objects`: **45 warnings, one per +system object**, each saying `type: 'text' (the column is 'string')` about a +declaration that was right all along. `varchar` canonicalizes to the field type `text` +(`canonicalizeSqlType('varchar(255)') === 'text'`, `suggestFieldTypeForSqlType('varchar(255)') === 'text'`, +`isCompatible('varchar(255)', 'text') === true` — all pinned in `type-compat.test.ts`), +so `id: Field.text(...)` asks for exactly what the platform's column delivers. The +delivery table now records `text`, and the 45 lines go silent because they were false, +not because they were suppressed. + +`sys_migration.id`'s `maxLength: 128` was the one **honest** disagreement in that corpus +— the column is varchar(255) — and it is removed rather than widened to 255. It bound +nothing in any seam: the DDL discards a declared width on a builtin column name, and +`validateRecord` skips `id` by name on both the insert and the update path (it is also +`readonly`). Declaring a width that nothing enforces is the shape enforce-or-remove +exists to prevent, and the 44 sibling system objects declare none. + +The classification pin now holds **every** entry in the delivery table to +`FieldType.options`, so a builder name written there fails by name instead of surfacing +as a corpus of false warnings. The fixtures in both #12015 pin files were written +against the delivery table rather than against the source — `sys_presence.id` was spelled +`type: 'string'` in the "silent" cases, which is why they passed while the same +declaration as actually written warned. They now use the shapes as declared, and the +firing cases declare a type that genuinely disagrees. + +**Grade: `patch` for both, and deliberately.** No door moves and no DDL changes: the +platform still owns `id` / `created_at` / `updated_at`, the emitted column is +byte-identical, every object that booted before still boots, and `BUILTIN_COLUMN_DELIVERY` +is internal to the package (it is not re-exported from the package entry). The +`platform-objects` half removes one metadata key that was measured inert in every seam +that could read it. What changes is what the driver **says**. diff --git a/packages/drivers/driver-sql/src/builtin-column-collision.test.ts b/packages/drivers/driver-sql/src/builtin-column-collision.test.ts index 41adb24ae9..921a85ed8a 100644 --- a/packages/drivers/driver-sql/src/builtin-column-collision.test.ts +++ b/packages/drivers/driver-sql/src/builtin-column-collision.test.ts @@ -17,10 +17,19 @@ * 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. + * + * ③ **The delivery table speaks the SPEC's vocabulary** (#12131). Its `type` + * is compared with `===` against a declaration's `type`, so a knex builder + * name there is a cross-vocabulary comparison that no declaration can + * satisfy. `id` used to record `'string'` — the `table.string('id')` + * builder name — and reported all 45 correct `id: Field.text(...)` + * declarations in `@objectstack/platform-objects` as disagreements while + * `'string'` was not even authorable. The first case below now holds every + * entry to `FieldType`, so the class fails by name rather than by corpus. */ import { describe, it, expect } from 'vitest'; -import { FieldSchema } from '@objectstack/spec/data'; +import { FieldSchema, FieldType } from '@objectstack/spec/data'; import { FIELD_KEY_STORAGE_CLASS, BUILTIN_COLUMN_DELIVERY, @@ -60,10 +69,30 @@ describe('the FieldSchema storage/presentation classification (#12015)', () => { } }); + it('⛔ spells every delivered `type` in the SPEC vocabulary, never a knex builder name (#12131)', () => { + // The one that got away: `id` recorded `'string'`, the knex builder name, + // and `undeliveredStorageAttributes` compares it with `===` against a + // declaration's `type` — a spec `FieldType`. No declaration could match it + // (`'string'` is absent from FieldType's 49 members and `FieldSchema` + // refuses it), so every correct `id: Field.text(...)` was reported as a + // disagreement: 45 of them on a stock boot of platform-objects. + for (const [column, delivery] of Object.entries(BUILTIN_COLUMN_DELIVERY)) { + expect( + FieldType.options as readonly string[], + `BUILTIN_COLUMN_DELIVERY.${column}.type must be a spec FieldType, not a builder name`, + ).toContain(delivery.type); + } + }); + it('records what each builtin column actually delivers, read off the emitting lines', () => { - // `table.string('id').primary()` — varchar(255), NOT NULL, unique, no default. + // `table.string('id').primary()` — varchar(255), NOT NULL, unique, no + // default. varchar canonicalizes to the field type `text` + // (`canonicalizeSqlType('varchar(255)') === 'text'`, pinned in + // `type-compat.test.ts`), so `text` is what this column DELIVERS — which + // is why the platform's own `id: Field.text(...)` declarations agree with + // it exactly (#12131). expect(BUILTIN_COLUMN_DELIVERY.id).toMatchObject({ - type: 'string', maxLength: 255, unique: true, notNull: true, defaultValue: null, + type: 'text', 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']) { @@ -76,22 +105,30 @@ describe('the FieldSchema storage/presentation classification (#12015)', () => { 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. + // `id: { type: 'number' }` — an author expecting a numeric key. This is + // the real author error; ⛔ NOT `{ type: 'text' }`, which is what the + // column delivers (see the silent case below). expect(keysOf(undeliveredStorageAttributes('id', { type: 'number' }))).toEqual(['type']); - // The #11456 fixture's shape. - expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', name: 'id' }))).toEqual(['type']); + expect(keysOf(undeliveredStorageAttributes('id', { type: 'number', 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', + expect(undeliveredStorageAttributes('id', { type: 'number' })[0]).toMatchObject({ + key: 'type', declared: 'number', delivered: 'text', }); }); 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. + // warning was false about. ⚠️ It is `Field.text`, and this fixture used to + // spell it `type: 'string'` — matching the delivery table's builder name + // rather than the source. That made this case pass while the same + // declaration as actually written warned (#12131). Verbatim now. expect( - undeliveredStorageAttributes('id', { type: 'string', label: 'Presence ID', required: true, readonly: true }), + undeliveredStorageAttributes('id', { type: 'text', label: 'Presence ID', required: true, readonly: true }), ).toEqual([]); + // The #11456 fixture's exact shape — the declaration that started #12015. + // It asks for precisely what the column delivers, so it is SILENT; it was + // reported as a disagreement until the delivery table was corrected. + expect(undeliveredStorageAttributes('id', { type: 'text', name: 'id' })).toEqual([]); expect( undeliveredStorageAttributes('created_at', { type: 'datetime', label: 'Created At', defaultValue: 'NOW()', readonly: true, @@ -100,21 +137,21 @@ describe('what a declaration on a builtin column name loses (#12015)', () => { }); 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('id', { type: 'text', maxLength: 255 })).toEqual([]); + expect(undeliveredStorageAttributes('id', { type: 'text', unique: true })).toEqual([]); // the PK is unique + expect(undeliveredStorageAttributes('id', { type: 'text', 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']); + expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', maxLength: 12 }))).toEqual(['maxLength']); + expect(keysOf(undeliveredStorageAttributes('id', { type: 'text', 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 }))) + expect(keysOf(undeliveredStorageAttributes('id', { type: 'number', maxLength: 12, unique: false }))) .toEqual(['type', 'maxLength']); // `unique: false` asks for nothing }); @@ -125,7 +162,7 @@ describe('what a declaration on a builtin column name loses (#12015)', () => { 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', { type: 'text', 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/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 51d167d966..57e167b8bc 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 @@ -7,9 +7,18 @@ * `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. + * author without saying so — an object declaring `id: { type: 'number' }` + * boots green and gets `varchar(255)`, and nothing recorded that the declared + * type was discarded. + * + * ⚠️ #12131: that example used to read `id: { type: 'text' }`, and so did most + * of the firing fixtures below. It was the wrong example — varchar(255) + * canonicalizes to the field type `text`, so `{ type: 'text' }` asks for + * exactly what lands. The delivery table recorded the knex builder name + * `'string'` instead, which no declaration could match, and the fixtures were + * written to that table rather than to the source. The firing cases now + * declare a type that genuinely disagrees; the agreeing shapes are pinned + * SILENT. * * 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 @@ -83,8 +92,10 @@ describe('a declared field colliding with a builtin column is named at load time { name: 'collide_create', fields: { - // The #11456 fixture's exact shape — the declaration that started this card. - id: { type: 'text', name: 'id' }, + // An author expecting a numeric key. (⛔ Not the #11456 shape + // `{ type: 'text', name: 'id' }` — that asks for what the column + // delivers and is pinned SILENT in the narrowing case below.) + id: { type: 'number', name: 'id' }, region: { type: 'text' }, }, }, @@ -97,7 +108,7 @@ describe('a declared field colliding with a builtin column is named at load time 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')"); + expect(lines[0]).toContain("type: 'number' (the column is 'text')"); // ⛔ 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'); @@ -114,7 +125,7 @@ describe('a declared field colliding with a builtin column is named at load time { name: 'collide_three', fields: { - id: { type: 'text' }, // text ≠ the varchar(255) key + id: { type: 'number' }, // a numeric key ≠ 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 @@ -163,7 +174,7 @@ describe('a declared field colliding with a builtin column is named at load time { name: 'collide_rot', fields: { - id: { type: 'text' }, + id: { type: 'number' }, payload: { type: 'text' }, // Declared AND colliding, but it describes the column the platform // emits — so it must not appear below. @@ -187,12 +198,16 @@ describe('a declared field colliding with a builtin column is named at load time 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. + // something untrue about it. ⚠️ #12131: `sys_presence.id` is `Field.text`, + // and this fixture used to spell it `type: 'string'` to match the delivery + // table's builder name. So it passed while the declaration as actually + // written warned — the 45 lines this file's own narrowing was supposed to + // have silenced. Verbatim now, and `id` below is the load-bearing half. await driver.initObjects([ { name: 'collide_presentation', fields: { - id: { type: 'string', label: 'Presence ID', required: true, readonly: true }, + id: { type: 'text', 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' }, @@ -218,8 +233,8 @@ 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, plus a length the driver never reads. - fields: { id: { type: 'text', maxLength: 12 }, region: { type: 'text' } }, + // platform emits: a numeric key, plus a length the driver never reads. + fields: { id: { type: 'number', maxLength: 12 }, region: { type: 'text' } }, }, ]); @@ -232,7 +247,7 @@ describe('a declared field colliding with a builtin column is named at load time 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, + // platform's `table.string('id')` — varchar(255) — NOT a numeric column, // 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();