diff --git a/.changeset/generate-field-type-vocabulary-totality.md b/.changeset/generate-field-type-vocabulary-totality.md new file mode 100644 index 0000000000..ad47812fa5 --- /dev/null +++ b/.changeset/generate-field-type-vocabulary-totality.md @@ -0,0 +1,40 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os generate` now has an answer for every field type, instead of silently guessing + +Three hand-kept vocabularies in `generate.ts` decide what `os generate types` +and `os generate migration` emit for a field: the TypeScript type, the SQL +column type, and the knex builder call. None of them was ever checked against +the `FieldType` enum they describe, and measured against the 49 members on +`main`, **21 real members had no entry in either lookup table and 24 had no arm +in the migration switch**. + +An unmapped member did not fail — it fell to the default. So a `secret` field +scaffolded as TypeScript `unknown` and a `TEXT` column, a `location` as +`unknown` and `TEXT`, and `address` / `composite` / `repeater` / `record` — all +four stored as JSON on the parent row — as scalar `TEXT` columns. The output +looked plausible and nothing said otherwise, which is what made this worth +fixing rather than tidying. + +All 49 members now have an entry in all three, and the values are read off the +platform rather than invented: the spec's ADR-0104 D1 value classes +(`STRING_VALUE_TYPES`, `NUMERIC_VALUE_TYPES`, `STRUCTURED_JSON_TYPES`, …) decide +the class, and `driver-sql`'s own DDL emitter — which creates the real columns — +decides the shape. `location` becomes a JSON column, not a `POINT`: that is what +the driver does, `POINT` is not portable to SQLite, and the spec's own value +contract for it is `{lat, lng, altitude?, accuracy?}`. `location` and `address` +now emit the spec's exported `Data.LocationValue` / `Data.AddressValue` types, +so the generated interface cannot drift from the value contract. + +The gap can no longer reopen quietly. Both lookup tables are +`satisfies Record`, so a field type added to the spec is a +named compile error here; the switch — whose scrutinee is a plain string off an +unvalidated config and so cannot carry one — is held by +`generate-field-type-vocabulary.pin.test.ts`, which walks the real enum and +names any member left unmapped. + +The runtime fallbacks (`|| 'unknown'`, `|| 'TEXT'`, `default:`) are unchanged +and still reachable: they answer a `type` string that is not a field type at +all, which the unvalidated authoring door can still deliver. diff --git a/packages/cli/src/commands/generate-field-type-vocabulary.pin.test.ts b/packages/cli/src/commands/generate-field-type-vocabulary.pin.test.ts index ea39dc87bc..590d1d4c65 100644 --- a/packages/cli/src/commands/generate-field-type-vocabulary.pin.test.ts +++ b/packages/cli/src/commands/generate-field-type-vocabulary.pin.test.ts @@ -43,15 +43,42 @@ * accepts, and an AI or a human reading this switch to learn the field types * would learn four that do not exist. * - * ## What this pin asserts, and what it deliberately does NOT + * ## What this pin asserts * - * FORWARD ONLY: every token the three vocabularies key on is a `FieldType` - * member. The converse is NOT asserted — plenty of real members (`secret`, - * `address`, `location`, `code`, `tags`, …) have no entry and fall to the - * `default` arm / the `|| fallback`, and that fallback is deliberate. Demanding - * total coverage would be a different card with a different decision behind it - * (what column type each unmapped member deserves), and this pin is written so - * it does not prejudge that. + * BOTH DIRECTIONS, since #14657. + * + * FORWARD (#13871): every token the three vocabularies key on is a `FieldType` + * member. + * + * BACKWARD (#14657): every `FieldType` member is keyed on by all three. #13871 + * deliberately did not assert this, because "what column type does each + * unmapped member deserve" was an open question; #14657 answered it member by + * member and this half became assertable. It matters because the gap was + * SILENT: 21 real members had no entry in either map (24 in the switch), and + * every one of them generated a plausible-looking wrong schema — TS `unknown`, + * a `TEXT` / `table.text` column — with nothing to tell the author. `secret` + * and `location` were among them. + * + * The two lookup tables carry the same rule a second time as + * `satisfies Record`, which makes a missing member a named + * `tsc` error (`packages/cli` type-checks `src/**`) as well as a red test. That + * annotation is itself pinned below: the extractor here REQUIRES it as each + * table's terminator, so deleting it cannot quietly demote the type-level half + * to nothing. The `switch` cannot carry a `satisfies` — its scrutinee is a + * plain `string` off an unvalidated config — so for that vocabulary this file + * is the only mechanism, which is why the totality assertion lives here rather + * than being left to the compiler. + * + * ⚠️ What is NOT asserted, and why the difference is the point: that a mapping + * is CORRECT. This pin measures presence, not the value — a wrong-but-present + * entry is a different defect (`autonumber: 'SERIAL'` against a runtime that + * writes a rendered string, `formula` given a column the runtime never + * creates), filed separately rather than pinned here on a guess. + * + * The runtime fallbacks (`|| 'unknown'`, `|| 'TEXT'`, `default:`) stay and are + * NOT dead: they answer a `type` string that is not a `FieldType` at all, which + * the UNVALIDATED authoring door still delivers. Totality is over the enum, not + * over every string that can reach the generator. * * The `FieldType` side is imported, never transcribed: a list written out here * would just relocate the drift into this file. And the vocabularies are read @@ -88,13 +115,36 @@ function lookupTableNames(): string[] { return [...SOURCE.matchAll(LOOKUP_TABLE_DECL)].map((m) => m[1]); } +/** + * The terminator every lookup table must carry — the type-level half of the + * #14657 totality rule. Required rather than tolerated: if someone deletes the + * annotation, extraction fails loudly here instead of the compiler silently + * stopping to check. + */ +const TABLE_TERMINATOR = '} satisfies Record;'; + /** The keys of one top-level `Record` table, in source order. */ function lookupTableKeys(name: string): string[] { const declaration = `const ${name}: Record = {`; const start = SOURCE.indexOf(declaration); if (start < 0) throw new Error(`lookup table not found in generate.ts: ${name}`); - const end = SOURCE.indexOf('\n};', start); - if (end < 0) throw new Error(`unterminated lookup table in generate.ts: ${name}`); + // Bound the table at ITS OWN closing line — the first line starting with `}` + // after the declaration — and then require that line to be the terminator. + // Searching for the terminator directly would silently run past a table + // whose annotation was deleted and swallow the NEXT table's body, turning a + // removed guard into a wrong measurement instead of a named failure. + const closing = SOURCE.slice(start).search(/\n\}/); + if (closing < 0) throw new Error(`unterminated lookup table in generate.ts: ${name}`); + const end = start + closing; + const closingLine = SOURCE.slice(end + 1, SOURCE.indexOf('\n', end + 1)); + if (closingLine !== TABLE_TERMINATOR) { + throw new Error( + `${name} in generate.ts must be closed by \`${TABLE_TERMINATOR}\`, but it is closed by ` + + `\`${closingLine}\`. That annotation is the type-level half of the #14657 rule that every ` + + 'FieldType member has an entry: without it, adding a field type to the spec stops being a ' + + 'compile error here and goes back to silently generating `unknown` / a TEXT column.', + ); + } const body = SOURCE.slice(start + declaration.length, end); return [...body.matchAll(/^ {2}([A-Za-z_][\w]*):/gm)].map((m) => m[1]); } @@ -147,4 +197,56 @@ describe('generate.ts field-type vocabularies (#13871)', () => { const ghosts = labels.filter((l) => !REAL_FIELD_TYPES.has(l)); expect(ghosts, 'the field-type switch cases on types that are not FieldType members').toEqual([]); }); + + // ── The #14657 half: no real member may go unmapped ────────────────────── + // + // Read this as one rule stated three times, not three rules: the authority is + // `FieldType`, and each vocabulary is measured against it. A member added to + // the spec with no answer here used to produce TS `unknown` and a `TEXT` + // column in silence; it now names itself in a failing assertion. + + const VOCABULARIES: ReadonlyArray string[]]> = [ + ['FIELD_TYPE_MAP (os generate types)', () => lookupTableKeys('FIELD_TYPE_MAP')], + ['FIELD_TYPE_SQL_MAP (os generate migration --format sql)', () => lookupTableKeys('FIELD_TYPE_SQL_MAP')], + ['the migration switch (os generate migration, typescript)', migrationSwitchLabels], + ]; + + for (const [label, read] of VOCABULARIES) { + it(`${label} covers every FieldType member`, () => { + const covered = new Set(read()); + // Non-vacuity: the same control the forward assertions buy. An extractor + // that returned nothing would make "everything is missing" the finding, + // not a silent pass — but state it anyway so the failure is legible. + expect(covered.size).toBeGreaterThan(20); + + const unmapped = [...REAL_FIELD_TYPES].filter((t) => !covered.has(t)); + expect( + unmapped, + `${label} has no entry for these real FieldType members, so each one silently ` + + 'takes the generator default (TS `unknown` / a TEXT column). Add an entry — or, ' + + 'if the default is genuinely the right answer for it, say so with an explicit ' + + 'entry that spells the default out, so the decision is written down rather than ' + + 'left as an absence.', + ).toEqual([]); + }); + } + + it('the two lookup tables carry the type-level totality annotation', () => { + // The runtime half above and the compile-time half must both be present: + // `tsc` names a missing member at build time, this file names it in CI even + // if the annotation is loosened. `lookupTableKeys` throws without it, so + // this assertion is the readable statement of a rule already enforced. + for (const table of ['FIELD_TYPE_MAP', 'FIELD_TYPE_SQL_MAP'] as const) { + expect( + SOURCE.includes(`const ${table}: Record = {`), + `${table} declaration moved`, + ).toBe(true); + expect(() => lookupTableKeys(table)).not.toThrow(); + } + expect( + SOURCE.match(/^\} satisfies Record;$/gm), + 'both FIELD_TYPE_MAP and FIELD_TYPE_SQL_MAP must close with the satisfies annotation ' + + 'that makes an unmapped FieldType member a compile error', + ).toHaveLength(2); + }); }); diff --git a/packages/cli/src/commands/generate.ts b/packages/cli/src/commands/generate.ts index 3eec38566e..4afa4d5d0f 100644 --- a/packages/cli/src/commands/generate.ts +++ b/packages/cli/src/commands/generate.ts @@ -4,6 +4,11 @@ import { Args, Command, Flags } from '@oclif/core'; import chalk from 'chalk'; import fs from 'fs'; import path from 'path'; + +// Type-only (erased at runtime): the three field-type vocabularies below are +// `satisfies Record`, which is what makes a field type added to +// the spec a named compile error here instead of a silent fallback (#14657). +import type { FieldType } from '@objectstack/spec/data'; import { printHeader, printSuccess, printError, printInfo, printStep, createTimer, CLI_ALIAS } from '../utils/format.js'; import { metadataFileName } from '../utils/metadata-file-name.js'; @@ -464,9 +469,23 @@ function toSnakeCase(str: string): string { * honour. `generate-field-type-vocabulary.pin.test.ts` now fails on any such * key, in this table and in the two vocabularies below it. * - * The set is deliberately NOT total: a real member with no entry falls to the - * `|| 'unknown'` below, which is the intended behaviour for a type this - * generator has nothing specific to say about. + * TOTAL since #14657, and total BY CONSTRUCTION: the `satisfies + * Record` below makes a missing member a named `tsc` error + * (`Property 'x' is missing …`), so the next field type the spec adds cannot + * arrive here in silence. Before it, 21 real members had no entry and every one + * of them silently generated `unknown` — a plausible-looking wrong type with + * nothing to tell the author. The `|| 'unknown'` below stays, and now means + * only what it always should have: this generator's answer for a `type` string + * that is not a `FieldType` at all, which the UNVALIDATED authoring door (a + * plain-object config export, `defineStack(x, { strict: false })`) can still + * deliver. + * + * Values are MEASURED, not invented — each one is the shape the platform + * actually implements, read from the spec's ADR-0104 D1 value classes + * (`@objectstack/spec/data` `field-value.zod.ts`) and cross-checked against the + * `driver-sql` DDL emitter that creates the real columns. The two structured + * types point AT the spec's own exported types rather than transcribing them, + * so the generated interface cannot drift from the value contract. */ const FIELD_TYPE_MAP: Record = { text: 'string', @@ -497,7 +516,48 @@ const FIELD_TYPE_MAP: Record = { color: 'string', rating: 'number', vector: 'number[]', -}; + // #14657 — the members that used to fall to `|| 'unknown'`. Grouped by the + // spec's ADR-0104 D1 value class, which is what decides each answer. + // STRING_VALUE_TYPES. `secret` is a string because the ROW holds an opaque + // ref, not the credential: the engine encrypts via the ICryptoProvider, + // stores the ciphertext handle in `sys_secret`, and masks on read (ADR-0100). + secret: 'string', + code: 'string', + signature: 'string', + qrcode: 'string', + // BOOLEAN_VALUE_TYPES. + toggle: 'boolean', + // SINGLE_OPTION_TYPES / MULTI_OPTION_TYPES — an option code, or an array of + // them. `tags` is the free-form member of the multi class. + radio: 'string', + checkboxes: 'string[]', + tags: 'string[]', + // NUMERIC_VALUE_TYPES — `valueSchemaFor` gives all three `z.number()`. + slider: 'number', + progress: 'number', + summary: 'number', + // REFERENCE_VALUE_TYPES — the STORED form of a reference is the related + // record's id string; the expanded record is the read shape and is never + // stored. `user` stores identically to `lookup` (field.zod says so). + user: 'string', + tree: 'string', + // FILE_REFERENCE_TYPES — the stored form is an opaque `sys_file` id string + // (`FileReferenceIdValueSchema`), which is why `file`/`image` above are + // already `string`; these three are the same class and take the same answer. + avatar: 'string', + video: 'string', + audio: 'string', + // STRUCTURED_JSON_TYPES — embedded structured values stored as JSON on the + // parent row. ONE decision for the whole family, not four independent ones. + // `location` and `address` name the spec's own exported value types (the + // generated file already imports `* as Data`), so the emitted interface is + // derived from the value contract instead of transcribing `{lat, lng}` here. + composite: 'Record', + repeater: 'Record[]', + record: 'Record>', + location: 'Data.LocationValue', + address: 'Data.AddressValue', +} satisfies Record; function fieldTypeToTs(fieldType: string, multiple?: boolean): string { const base = FIELD_TYPE_MAP[fieldType] || 'unknown'; @@ -873,9 +933,24 @@ async function runClientGeneration(configPath: string | undefined, flags: { outp /** * The SQL column type each authored field type generates (#13871). * - * Same invariant as `FIELD_TYPE_MAP`: every key is a `FieldType` member, an - * unmapped member falls to the `|| 'TEXT'` default on purpose, and the pin test - * enforces the first half. + * Same invariant as `FIELD_TYPE_MAP`, and since #14657 the same totality: every + * key is a `FieldType` member AND every `FieldType` member has a key, enforced + * by the `satisfies` below. The `|| 'TEXT'` default now covers only a `type` + * string that is not a field type at all (the unvalidated authoring door). + * + * ⚠️ Five entries that PREDATE #14657 disagree with the platform and are + * deliberately left exactly as they are — correcting an existing entry changes + * emitted DDL for apps that already generated against it, which is a different + * card from filling the gaps. They are filed rather than silently mirrored, and + * the new entries below do NOT copy them: `multiselect: 'TEXT'` (the driver + * stores the multi-option class in a JSON column, and `json: 'JSONB'` in this + * same table already says so), `autonumber: 'SERIAL'` (the runtime issues a + * RENDERED STRING — prefix, counter, suffix — and `driver-sql` gives it + * `table.string`; a SERIAL cannot hold `INV-0001`), `formula: 'TEXT'` (a + * formula is virtual: `driver-sql` creates NO column for it), `vector: + * 'VECTOR'` (the driver stores vectors in a JSON column), and `lookup` / + * `master_detail` at `VARCHAR(36)` (a platform id is a 26-character ULID, so + * 36 fits, but the driver's own width is 255). */ const FIELD_TYPE_SQL_MAP: Record = { text: 'VARCHAR(255)', @@ -906,7 +981,51 @@ const FIELD_TYPE_SQL_MAP: Record = { color: 'VARCHAR(7)', rating: 'INTEGER', vector: 'VECTOR', -}; + // #14657 — the members that used to fall to `|| 'TEXT'`. Same ADR-0104 D1 + // classes as `FIELD_TYPE_MAP`, resolved to this table's own SQL vocabulary. + // STRING_VALUE_TYPES. `secret` holds the opaque `sys_secret` ref, not the + // credential, so it is an ordinary short string column (ADR-0100). + secret: 'VARCHAR(255)', + // `code` / `signature` / `qrcode` are the text family in `driver-sql`'s own + // DDL switch (#11794, #11875): their values are unbounded unless the field + // declares a `maxLength`, which the write seam — not the column — enforces. + code: 'TEXT', + signature: 'TEXT', + qrcode: 'TEXT', + // BOOLEAN_VALUE_TYPES. + toggle: 'BOOLEAN', + // SINGLE_OPTION_TYPES: one option code, exactly like `select`. + radio: 'VARCHAR(255)', + // MULTI_OPTION_TYPES: arrays, so a JSON column — matching `json` above and + // `driver-sql`'s `JSON_COLUMN_TYPES`, which is seeded from this same class. + checkboxes: 'JSONB', + tags: 'JSONB', + // NUMERIC_VALUE_TYPES. `progress` takes `percent`'s narrower shape because it + // is the same 0-100 quantity; `slider` and `summary` are open-range. + slider: 'DECIMAL(18,2)', + progress: 'DECIMAL(5,2)', + summary: 'DECIMAL(18,2)', + // REFERENCE_VALUE_TYPES: the stored value is the related record's id, so the + // width belongs to the TARGET's id column, never to this field. + user: 'VARCHAR(36)', + tree: 'VARCHAR(36)', + // FILE_REFERENCE_TYPES: the ADR-0104 D3 stored form is an opaque `sys_file` + // id string, which is why `file` / `image` above are already a varchar; these + // three are the same class and take the same answer. + avatar: 'VARCHAR(2048)', + video: 'VARCHAR(2048)', + audio: 'VARCHAR(2048)', + // STRUCTURED_JSON_TYPES — the embedded-structured family answered ONCE. + // `location` is JSON, NOT `POINT`: the spec's own value contract is + // `{lat, lng, altitude?, accuracy?}` and `driver-sql` gives every member of + // this class a JSON column. (`POINT` was the invented `geo_point` ghost this + // table used to carry, and it is not portable to SQLite.) + composite: 'JSONB', + repeater: 'JSONB', + record: 'JSONB', + location: 'JSONB', + address: 'JSONB', +} satisfies Record; function fieldTypeToSql(fieldType: string): string { return FIELD_TYPE_SQL_MAP[fieldType] || 'TEXT'; @@ -1003,19 +1122,31 @@ function generateMigrationTs(config: Record): string { switch (fType) { case 'text': case 'email': case 'phone': case 'url': case 'select': case 'password': case 'color': + // #14657 — `secret` holds the opaque `sys_secret` ref, not the + // credential (ADR-0100); `radio` is a single option code like `select`. + case 'secret': case 'radio': colMethod = `table.string('${fieldName}')`; break; case 'textarea': case 'richtext': case 'html': case 'markdown': case 'formula': + // #14657 — `driver-sql`'s own DDL switch puts these three in the text + // family (#11794, #11875): the declared `maxLength`, when there is one, + // is enforced at the write seam rather than by the column. + case 'code': case 'signature': case 'qrcode': colMethod = `table.text('${fieldName}')`; break; case 'number': case 'currency': case 'percent': + // #14657 — NUMERIC_VALUE_TYPES: `valueSchemaFor` gives all of these + // `z.number()`, and `driver-sql` gives them a float column. + case 'slider': case 'progress': case 'summary': colMethod = `table.decimal('${fieldName}')`; break; case 'rating': colMethod = `table.integer('${fieldName}')`; break; case 'boolean': + // #14657 — BOOLEAN_VALUE_TYPES; `driver-sql` shares one arm for the pair. + case 'toggle': colMethod = `table.boolean('${fieldName}')`; break; case 'date': @@ -1028,6 +1159,16 @@ function generateMigrationTs(config: Record): string { colMethod = `table.time('${fieldName}')`; break; case 'json': case 'multiselect': + // #14657 — the rest of MULTI_OPTION_TYPES, the whole + // STRUCTURED_JSON_TYPES family answered ONCE, and `vector`. Every one + // of these is a member of `driver-sql`'s `JSON_COLUMN_TYPES`, which is + // seeded from these very spec classes, so a JSON column here is what + // the runtime already creates. `location` is JSON, not `POINT` — the + // spec's value contract is `{lat, lng, altitude?, accuracy?}`, and + // `POINT` is not portable to SQLite. + case 'checkboxes': case 'tags': + case 'composite': case 'repeater': case 'record': + case 'location': case 'address': case 'vector': colMethod = `table.jsonb('${fieldName}')`; break; case 'lookup': case 'master_detail': @@ -1035,10 +1176,21 @@ function generateMigrationTs(config: Record): string { break; // `user` references sys_user, whose id is a text identifier (not a uuid), // so store it as a string column — consistent with the runtime sql-driver. - case 'user': + // #14657 — `tree` is the same REFERENCE_VALUE_TYPES class pointing at the + // object's own id, and the FILE_REFERENCE_TYPES class stores an opaque + // `sys_file` id string (ADR-0104 D3). `autonumber` is a RENDERED string + // (prefix + counter + suffix), which is both what `FIELD_TYPE_MAP` says + // and what `driver-sql` emits — a SERIAL could not hold `INV-0001`. + case 'user': case 'tree': + case 'image': case 'file': case 'avatar': case 'video': case 'audio': + case 'autonumber': colMethod = `table.string('${fieldName}')`; break; default: + // Reachable only through the UNVALIDATED authoring door — a `type` + // that is not a `FieldType` at all. Every real member is cased above, + // and `generate-field-type-vocabulary.pin.test.ts` fails if one stops + // being. colMethod = `table.text('${fieldName}')`; } diff --git a/packages/runtime/src/dispatcher-error-vocabulary.ts b/packages/runtime/src/dispatcher-error-vocabulary.ts index 3690a65db6..ba7389e376 100644 --- a/packages/runtime/src/dispatcher-error-vocabulary.ts +++ b/packages/runtime/src/dispatcher-error-vocabulary.ts @@ -309,6 +309,23 @@ export const UNREGISTERED_CODE_SITES: readonly UnregisteredCodeSite[] = [ // (no source site — the producer is tenant code; see SANDBOX_AUTHORED_LIMB) // ── foreign vocabularies: spelled `code`, not an ADR-0112 error.code ──── + { + code: 'TEXT', + file: 'packages/cli/src/commands/generate.ts', + shape: 'objlit', + door: 'none', + verdict: 'foreign-vocabulary', + why: + 'Not a vocabulary of codes at all — it is the SQL column type for the `code` FIELD TYPE ' + + "(a code editor), one entry of `FIELD_TYPE_SQL_MAP`, whose keys are `FieldType` members and " + + 'whose values are DDL types. The scan reaches it because the key is spelled `code` and the ' + + 'value happens to be upper-case; its siblings (`signature: \'TEXT\'`, `qrcode: \'TEXT\'`) ' + + 'are the same string and are invisible only because their keys are not `code`. Nothing here ' + + 'is thrown, returned, or stamped on an envelope: the value is interpolated into generated ' + + "`CREATE TABLE` text by `os generate migration --format sql`. The twin entry in " + + '`FIELD_TYPE_MAP` reads `code: \'string\'` and is below the grammar for the same reason ' + + 'this one is above it — case, not meaning.', + }, { code: 'MODULE_NOT_FOUND', file: 'packages/types/src/node.ts',