From adcdb1afc5795b601f86f50876a76f9e6182a381 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 14:23:51 +0000 Subject: [PATCH 1/2] fix(driver-sql,objectql): JSON values must not depend on DDL having run (#10995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a Postgres deployment that manages DDL out-of-band (`skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1`), writing an array to a JSON field returned 500 `DATABASE_ERROR`, a bare string returned 500, and an empty array was accepted and silently stored as an empty object. `formatInput` does stringify JSON-field values on every non-SQLite dialect, but only for fields in the per-object `jsonFields` registry — and that registry was built exclusively as the first step of a DDL call, so a boot that skips schema sync served writes with every coercion registry empty and let node-postgres' per-type defaults encode the value: object -> JSON text (accidentally correct), array -> Postgres array literal -> `22P02`, `[]` -> `{}` (valid JSON, hence accepted and corrupted), bare string -> raw -> `22P02`. SQLite hid it behind a dialect-local bind-safety net, which is why the Turso/SQLite suites are blind. Registration is now separable from DDL, per the #7737/#10629 ruling for federated objects: `SqlDriver.registerObjectMetadata()` (declared optional on `IDataDriver`) installs the coercion registries with no DDL and no round-trip, a `skipSchemaSync` boot and every metadata reload take that route, and `initObjects` registers before the ADR-0015 DDL gate refuses so guest datasources are covered too. The refusal itself is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- .../pg-json-binding-ddl-free-registration.md | 45 ++++ ...ql-driver-json-binding-without-ddl.test.ts | 223 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 222 +++++++++++------ packages/objectql/src/plugin.ts | 137 ++++++++++- ...ema-sync-registers-object-metadata.test.ts | 180 ++++++++++++++ packages/spec/src/contracts/data-driver.ts | 24 ++ 6 files changed, 755 insertions(+), 76 deletions(-) create mode 100644 .changeset/pg-json-binding-ddl-free-registration.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts create mode 100644 packages/objectql/src/skip-schema-sync-registers-object-metadata.test.ts diff --git a/.changeset/pg-json-binding-ddl-free-registration.md b/.changeset/pg-json-binding-ddl-free-registration.md new file mode 100644 index 0000000000..65964f6655 --- /dev/null +++ b/.changeset/pg-json-binding-ddl-free-registration.md @@ -0,0 +1,45 @@ +--- +"@objectstack/driver-sql": patch +"@objectstack/objectql": patch +"@objectstack/spec": patch +--- + +Fix JSON-field writes on Postgres deployments that manage DDL out-of-band +(`skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1`): a non-empty array and a bare +string were rejected with a 500, and an empty array was **silently stored as an +empty object** (#10995). + +The SQL driver does `JSON.stringify` a JSON field's value on every non-SQLite +dialect — but only for fields listed in its per-object `jsonFields` registry, +and that registry (like the boolean / numeric / date / datetime / time / +auto_number registries and the tenant-isolation column) was filled **only** as +the first step of a DDL call. A deployment that skips boot schema sync therefore +served every write knowing nothing about its objects, and values reached +node-postgres to be encoded by its per-type defaults: + +- an **object** became JSON text — accidentally correct; +- an **array** became a Postgres ARRAY LITERAL (`{…}`) — `22P02 invalid input + syntax for type json`, a 500 on every write; +- **except `[]`**, whose array literal `{}` is valid JSON, so an empty array was + accepted and stored as an empty **object** — corruption, not an error; +- a **bare string** was passed raw (`x` is not JSON text, `"x"` is) — a 500, + while a number survived because `42` already is valid JSON. + +SQLite never showed any of it: `formatInput` ends with a bind-safety net gated +on that dialect, so the same empty registry is invisible there — which is why +tenant environments on Turso/SQLite and the suites that run on them were blind +to a defect live on every Postgres deployment. + +The registration is now separable from the DDL, on the ruling #7737/#10629 +already made for federated objects — that flag is about DDL, and a binding that +is DDL-free must not ride on it: + +- `SqlDriver.registerObjectMetadata(objects)` installs a managed object's + coercion metadata with no `CREATE TABLE`, no `ALTER TABLE`, no existence probe + and no round-trip — the managed sibling of `registerExternalObject`, declared + optional on `IDataDriver` so drivers that don't need it omit it; +- a `skipSchemaSync` boot (and metadata reload) now takes that route instead of + doing nothing, keeping the cold-start budget the flag exists to protect; +- `initObjects` registers before the ADR-0015 DDL gate refuses, so objects on a + datasource ObjectStack is only a guest in are encoded from their declared + field types too. The refusal itself is unchanged. diff --git a/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts b/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts new file mode 100644 index 0000000000..56bae6a0c7 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts @@ -0,0 +1,223 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10995] A JSON field must round-trip on Postgres when the driver was told + * about its object WITHOUT running DDL. + * + * ## The defect, measured rather than inferred + * + * `formatInput` DOES `JSON.stringify` a JSON field's value on every non-SQLite + * dialect — but only for fields listed in `jsonFields[object]`, and that + * registry is filled exclusively by the DDL entry points (`initObjects` / + * `syncSchema`, plus `registerExternalObject` for federated objects). A + * deployment that manages DDL out-of-band — `skipSchemaSync` / + * `OS_SKIP_SCHEMA_SYNC=1`, the documented posture after running migrations + * manually, and the one cold-start-sensitive runtimes are told to use — never + * calls them, so it serves writes with EVERY coercion registry empty. What + * reaches Postgres is then whatever node-postgres does with a bare JS value: + * + * | value written to a `json` field | with an empty registry (measured on PG 16) | + * | :--- | :--- | + * | `{a:1}` object | JSON text — accidentally correct | + * | `42` number | `42` — already valid JSON | + * | `[{type:'app'}]` array | `{"(type,app)"}`-style ARRAY LITERAL → `22P02 invalid input syntax for type json` → 500 | + * | `'x'` bare string | raw `x` → not JSON text (`"x"` would be) → 500 | + * | `[]` empty array | array literal `{}` — **valid JSON**, so it is ACCEPTED and silently stored as an empty OBJECT | + * + * That last row is the one that outlives a fix aimed at the crashes: it does + * not error, it corrupts. Every row above was reproduced against a live + * Postgres before the fix, on INSERT and on UPDATE alike. + * + * ## Why no existing suite caught it + * + * `formatInput` ends with a bind-safety net that stringifies any leftover + * object/array — gated on `isSqlite`, because better-sqlite3 cannot bind them + * at all. So on SQLite an empty registry is invisible, and tenant environments + * run Turso/SQLite: the seed and data suites exercise a different dialect + * branch of the same function. Postgres has no such net, and both control + * planes are Postgres. + * + * ## What this file pins, and on which driver path + * + * Every test in §1–§3 runs against a **live Postgres** (`OS_TEST_POSTGRES_URL`, + * provisioned by CI's `temporal-conformance` job) through the real + * `SqlDriver.create()` / `update()` paths — the dialect the defect is on. + * §4 runs the same matrix on SQLite as an expected NON-effect. The tables here + * are created with raw SQL, exactly as an out-of-band migration would, and the + * driver under test never runs DDL against them. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { PG_CELL, dialectCell } from './live-dialect-matrix.testkit.js'; +import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; + +const PG_URL = PG_CELL.url; + +/** The object as an out-of-band migration would have created it. */ +const PREF_FIELDS = { + id: { type: 'text' }, + key: { type: 'text' }, + value: { type: 'json' }, +} as const; + +function prefObject(name: string) { + return { name, fields: { ...PREF_FIELDS } } as any; +} + +const OPTS = { bypassTenantAudit: true } as any; + +/** `create table … (id text primary key, key text, value jsonb)` — no driver DDL. */ +async function migrateOutOfBand(driver: SqlDriver, table: string): Promise { + await (driver as any).knex.raw( + `create table if not exists "${table}" (id text primary key, key text, value jsonb, ` + + `created_at timestamptz, updated_at timestamptz)`, + ); +} + +/** Write `value` on a fresh row and read back what storage actually holds. */ +async function insertAndRead(driver: SqlDriver, table: string, value: unknown): Promise { + const id = `i_${Math.random().toString(36).slice(2, 10)}`; + await driver.create(table, { id, key: 'ui.recent', value }, OPTS); + const row: any = await driver.findOne(table, { where: { id } } as any, OPTS); + return row?.value; +} + +/** Seed a row, PATCH only `value` onto it, and read back what storage holds. */ +async function updateAndRead(driver: SqlDriver, table: string, value: unknown): Promise { + const id = `u_${Math.random().toString(36).slice(2, 10)}`; + await driver.create(table, { id, key: 'ui.recent', value: { seeded: true } }, OPTS); + await driver.update(table, id, { value }, OPTS); + const row: any = await driver.findOne(table, { where: { id } } as any, OPTS); + return row?.value; +} + +describe.skipIf(!PG_URL)('#10995 — live Postgres, driver told about the object without DDL', () => { + // §1–§3 share one driver: the registration under test is per-object, and a + // shared connection keeps the live cell to one pool. + let driver: SqlDriver; + const TABLE = 'os10995_pref'; + + beforeAll(async () => { + driver = new SqlDriver(PG_CELL.config()); + await migrateOutOfBand(driver, TABLE); + // THE LINE UNDER TEST: the object's field types reach the driver with no + // CREATE TABLE, no ALTER TABLE and no round-trip — what a `skipSchemaSync` + // boot now does in place of doing nothing. + driver.registerObjectMetadata([prefObject(TABLE)]); + }); + + afterAll(async () => { + await driver?.disconnect(); + }); + + // ── §1 The three rows the card is about ────────────────────────────────── + + it('§1a a NON-EMPTY ARRAY round-trips (insert and update)', async () => { + const recents = [ + { type: 'app', id: 'crm' }, + { type: 'record', id: 'acc_1' }, + ]; + const inserted = await insertAndRead(driver, TABLE, recents); + expect(Array.isArray(inserted)).toBe(true); + expect(inserted).toEqual(recents); + + const updated = await updateAndRead(driver, TABLE, recents); + expect(Array.isArray(updated)).toBe(true); + expect(updated).toEqual(recents); + }); + + it('§1b a BARE STRING and the other scalar JSON documents round-trip (insert and update)', async () => { + // `"x"` is a legal JSON document; a fix that special-cases arrays re-fails + // this row, which is why it is pinned apart from §1a. + expect(await insertAndRead(driver, TABLE, 'x')).toBe('x'); + expect(await updateAndRead(driver, TABLE, 'x')).toBe('x'); + + expect(await insertAndRead(driver, TABLE, true)).toBe(true); + expect(await updateAndRead(driver, TABLE, false)).toBe(false); + }); + + it('§1c an EMPTY ARRAY round-trips as [] — not as {}', async () => { + // The row that does not crash. Postgres' array literal for `[]` is `{}`, + // which is valid JSON, so the write was accepted and the value silently + // became an empty OBJECT. Both halves are asserted: the shape that must be + // there, and the shape that must NOT. + const inserted = await insertAndRead(driver, TABLE, []); + expect(Array.isArray(inserted)).toBe(true); + expect(inserted).toEqual([]); + expect(inserted).not.toEqual({}); + + const updated = await updateAndRead(driver, TABLE, []); + expect(Array.isArray(updated)).toBe(true); + expect(updated).toEqual([]); + expect(updated).not.toEqual({}); + }); + + // ── §2 Expected NON-effects on the same path ───────────────────────────── + + it('§2 objects, nested arrays and numbers are unchanged (they already worked)', async () => { + expect(await insertAndRead(driver, TABLE, { a: 1 })).toEqual({ a: 1 }); + expect(await updateAndRead(driver, TABLE, { items: [1, 2] })).toEqual({ items: [1, 2] }); + expect(await insertAndRead(driver, TABLE, 42)).toBe(42); + // A non-JSON column keeps its own binding: `key` is text, and text is what + // comes back — the registration must not turn every column into JSON. + const id = `k_${Math.random().toString(36).slice(2, 10)}`; + await driver.create(TABLE, { id, key: 'ui.recent', value: null }, OPTS); + const row: any = await driver.findOne(TABLE, { where: { id } } as any, OPTS); + expect(row.key).toBe('ui.recent'); + expect(row.value).toBeNull(); + }); + + // ── §3 The other posture with the same empty registry: DDL REFUSED ─────── + + it('§3 a datasource we are a guest in registers its objects even though DDL is refused', async () => { + // `schemaMode !== 'managed'` (ADR-0015): `initObjects` must still refuse the + // DDL — and must no longer leave the driver ignorant of the objects it was + // just handed, which is what made every JSON write on a federated Postgres + // datasource take the node-postgres defaults above. + const guestTable = 'os10995_guest'; + const guest = new SqlDriver({ ...PG_CELL.config(), schemaMode: 'validate-only' } as any); + try { + await migrateOutOfBand(guest, guestTable); + await expect(guest.initObjects([prefObject(guestTable)])).rejects.toBeInstanceOf( + ExternalSchemaModeViolationError, + ); + expect(await insertAndRead(guest, guestTable, [{ type: 'app' }])).toEqual([{ type: 'app' }]); + expect(await updateAndRead(guest, guestTable, [])).toEqual([]); + expect(await updateAndRead(guest, guestTable, 'x')).toBe('x'); + } finally { + await guest.disconnect(); + } + }); +}); + +// ── §4 The SQLite path is unchanged ──────────────────────────────────────── + +describe('#10995 — the SQLite path is unaffected', () => { + it('§4 round-trips the same matrix, with and without the DDL-free registration', async () => { + const driver = new SqlDriver(dialectCell('sqlite').config()); + try { + const TABLE = 'os10995_sqlite'; + await (driver as any).knex.raw( + `create table if not exists "${TABLE}" (id text primary key, key text, value text)`, + ); + // Un-registered, SQLite neither crashes nor corrupts: `formatInput`'s + // dialect-local bind-safety net stringifies the array, so what lands on + // disk is the right JSON text — it just comes back as TEXT, because the + // read-side parse is keyed by the same empty registry. Storing the right + // bytes is what makes the empty registry invisible here, and it is why + // the SQLite/Turso suites never showed the Postgres defect. + expect(await insertAndRead(driver, TABLE, [{ type: 'app' }])).toBe('[{"type":"app"}]'); + expect(await updateAndRead(driver, TABLE, [])).toBe('[]'); + + // Registered: unchanged. + driver.registerObjectMetadata([prefObject(TABLE)]); + expect(await insertAndRead(driver, TABLE, [{ type: 'app' }])).toEqual([{ type: 'app' }]); + expect(await updateAndRead(driver, TABLE, [])).toEqual([]); + expect(await updateAndRead(driver, TABLE, 'x')).toBe('x'); + expect(await insertAndRead(driver, TABLE, { a: 1 })).toEqual({ a: 1 }); + } finally { + await driver.disconnect(); + } + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 4d4eb2b837..035fea7ef0 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -7750,6 +7750,144 @@ export class SqlDriver implements IDataDriver { if (timeCols.length) this.timeFields[key] = new Set(timeCols); } + /** + * Register one MANAGED object's in-memory metadata — and run no DDL. + * + * Everything the write and read paths need to encode a value correctly is + * installed here: the JSON / boolean / numeric / date / datetime / time + * coercion registries, the auto_number descriptors, the tenant-isolation + * column, and the `managedObjectFields` / `managedObjectIndexes` entries + * drift detection diffs against. {@link SqlDriver.initObjects} calls it as + * its first step and then issues DDL; {@link registerObjectMetadata} calls + * it and stops. + * + * @returns the physical table name registered, and the tenant column + * {@link computeAndRecordTenantField} resolved for it — both of + * which `initObjects` goes on to use for its DDL. + */ + protected registerManagedObjectMetadata( + obj: { name: string; fields?: Record; tenancy?: any }, + ): { tableName: string; tenantField: string | null } { + const tableName = StorageNameMapping.resolveTableName(obj); + // #2186: remember the authoritative metadata field set for this table so + // drift detection / `os migrate` can diff the physical schema against it. + this.managedObjectFields.set(tableName, obj.fields ?? {}); + // Always overwrite — a metadata change that REMOVES `indexes` must clear + // the previous entry, or drift detection keeps expecting an index nobody + // declares any more (and never reports it as orphaned). + if (Array.isArray((obj as any).indexes)) { + this.managedObjectIndexes.set(tableName, (obj as any).indexes); + } else { + this.managedObjectIndexes.delete(tableName); + } + // [#8621] This call may create the table, or add a unique index to one + // that already exists, so whatever the upsert pre-flight introspected + // before it is stale. Dropping the entry is enough — the entry is + // re-read lazily, and a refusal re-reads unconditionally. + this.physicalKeyIndexes.delete(tableName); + + const jsonCols: string[] = []; + const booleanCols: string[] = []; + const numericCols: string[] = []; + const autoNumberCols: Array<{ name: string; format: string; tokens: AutonumberToken[]; tenantField: string | null }> = []; + // Tenant-isolation column: explicit tenancy opt-out → declared field → + // implicit `organization_id`. See {@link computeAndRecordTenantField} + // (shared with registerExternalObject so the two paths can't drift). + const tenantField = this.computeAndRecordTenantField(tableName, obj); + if (obj.fields) { + for (const [name, field] of Object.entries(obj.fields)) { + const type = field.type || 'string'; + if (this.isJsonField(type, field)) { + jsonCols.push(name); + } + // `toggle` shares boolean storage/affinity, so it needs the same + // read coercion (stored 1/0 → JS true/false) or it leaks back as a + // number/string instead of a boolean (#field-zoo). + if (type === 'boolean' || type === 'toggle') { + booleanCols.push(name); + } + // Numeric scalars are coerced back to JS numbers on read so legacy + // TEXT-affinity columns (created before they were mapped to a numeric + // column) still return numbers, not strings — see NUMERIC_SCALAR_TYPES. + if (NUMERIC_SCALAR_TYPES.has(type) && !field.multiple) { + numericCols.push(name); + } + if (type === 'date') { + (this.dateFields[tableName] ??= new Set()).add(name); + } + if (type === 'datetime') { + (this.datetimeFields[tableName] ??= new Set()).add(name); + } + if (type === 'time') { + (this.timeFields[tableName] ??= new Set()).add(name); + } + if (type === 'auto_number' || type === 'autonumber') { + const fmt = resolveAutonumberFormat(field); + // Tokenize once: the renderer resolves date tokens (`{YYYYMMDD}`), + // field interpolation (`{island_zone}`) and the sequence slot at + // fill time. The counter scopes to whatever renders before the slot. + const tokens = parseAutonumberFormat(fmt); + autoNumberCols.push({ name, format: fmt, tokens, tenantField }); + } + } + } + this.jsonFields[tableName] = jsonCols; + this.booleanFields[tableName] = booleanCols; + this.numericFields[tableName] = numericCols; + this.autoNumberFields[tableName] = autoNumberCols; + this.tenantFieldByTable[tableName] = tenantField; + return { tableName, tenantField }; + } + + /** + * Install the read/write metadata for MANAGED objects WITHOUT running DDL — + * the managed sibling of {@link SqlDriver.registerExternalObject}. + * + * ## Why this exists as its own entry point + * + * Until this method, the ONLY way to tell this driver a managed object's + * field types was {@link initObjects} / `syncSchema()` — which also runs + * CREATE TABLE / ALTER TABLE. A deployment that manages DDL out-of-band + * (`skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1`, the documented posture after + * running migrations manually) therefore booted with EVERY coercion + * registry empty, and value encoding silently degraded to whatever the + * underlying client does with a bare JS value. + * + * On Postgres that is not a cosmetic difference. `formatInput` stringifies a + * JSON field ONLY when the field is in `jsonFields`; with the registry empty + * the value reaches node-postgres, whose per-type defaults are: + * + * - object -> JSON text (accidentally correct); + * - array -> a Postgres ARRAY LITERAL (`{…}`) -> `22P02 invalid input + * syntax for type json` -> 500, **except `[]`, whose literal `{}` is + * valid JSON**, so an empty array is accepted and silently stored as an + * empty OBJECT — data corruption, not an error; + * - bare string -> passed raw -> not valid JSON text -> 500 (a number + * survives because `42` already is valid JSON). + * + * SQLite never showed it: `formatInput` ends with a bind-safety net that + * stringifies any leftover object/array on that dialect, so the same empty + * registry is invisible there — which is why the SQLite/Turso suites are + * blind to this and every Postgres deployment inherited it. + * + * Registration is the DDL-free half, so it is safe on any datasource and on + * any boot: no `assertSchemaMutable` gate (nothing here mutates a schema), + * no `ensureDatabaseExists` probe, no round-trip at all. That is what makes + * it affordable on exactly the cold-start-sensitive boots `skipSchemaSync` + * exists to protect: it costs one pass over the object list, in memory. + * + * It is the same ruling #7737/#10629 already made for FEDERATED objects — + * `OS_SKIP_SCHEMA_SYNC` is about DDL, and a binding that is DDL-free must + * not ride on it — extended to the managed ones. + * + * Idempotent: pure metadata assignment, safe to re-drive on every reload. + */ + registerObjectMetadata( + objects: Array<{ name: string; fields?: Record; tenancy?: any }>, + ): void { + for (const obj of objects) this.registerManagedObjectMetadata(obj); + } + // `tenancy` is part of what this method READS — each object flows into // `computeAndRecordTenantField`, which consumes `obj.tenancy` to pick the // tenant column and to set or clear the sticky explicit-opt-out. It went @@ -7757,82 +7895,30 @@ export class SqlDriver implements IDataDriver { // `computeAndRecordTenantField` both had it), so a caller spelling the key // correctly was rejected by the type while the driver read it regardless. async initObjects(objects: Array<{ name: string; fields?: Record; tenancy?: any }>): Promise { + // In-memory registration FIRST, and deliberately ahead of the DDL gate + // below: being refused permission to alter a schema is not a reason to stay + // ignorant of the objects we were just told about. On a datasource we are a + // guest in (`schemaMode !== 'managed'`) the gate throws — and before this + // line ran here, it threw with every coercion registry still empty, so the + // very next write to a JSON field on that datasource was bound by the + // client's per-type defaults (see {@link registerObjectMetadata} for what + // node-postgres does with an array and with `[]`). Nothing in this call + // touches the database; the refusal it precedes is unchanged. + this.registerObjectMetadata(objects); + // DDL gate (ADR-0015 §5.1): createTable/alterTable below mutate schema. // Also covers `syncSchema`, which delegates here. this.assertSchemaMutable('initObjects'); await this.ensureDatabaseExists(); for (const obj of objects) { + // Re-read what the registration above recorded, rather than recomputing: + // `computeAndRecordTenantField` carries a sticky explicit-opt-out, so the + // recorded answer IS the answer. const tableName = StorageNameMapping.resolveTableName(obj); - // #2186: remember the authoritative metadata field set for this table so - // drift detection / `os migrate` can diff the physical schema against it. - this.managedObjectFields.set(tableName, obj.fields ?? {}); - // Always overwrite — a metadata change that REMOVES `indexes` must clear - // the previous entry, or drift detection keeps expecting an index nobody - // declares any more (and never reports it as orphaned). - if (Array.isArray((obj as any).indexes)) { - this.managedObjectIndexes.set(tableName, (obj as any).indexes); - } else { - this.managedObjectIndexes.delete(tableName); - } - // [#8621] This call may create the table, or add a unique index to one - // that already exists, so whatever the upsert pre-flight introspected - // before it is stale. Dropping the entry is enough — the entry is - // re-read lazily, and a refusal re-reads unconditionally. - this.physicalKeyIndexes.delete(tableName); - - const jsonCols: string[] = []; - const booleanCols: string[] = []; - const numericCols: string[] = []; - const autoNumberCols: Array<{ name: string; format: string; tokens: AutonumberToken[]; tenantField: string | null }> = []; - // Tenant-isolation column: explicit tenancy opt-out → declared field → - // implicit `organization_id`. See {@link computeAndRecordTenantField} - // (shared with registerExternalObject so the two paths can't drift). - const tenantField = this.computeAndRecordTenantField(tableName, obj); - if (obj.fields) { - for (const [name, field] of Object.entries(obj.fields)) { - const type = field.type || 'string'; - if (this.isJsonField(type, field)) { - jsonCols.push(name); - } - // `toggle` shares boolean storage/affinity, so it needs the same - // read coercion (stored 1/0 → JS true/false) or it leaks back as a - // number/string instead of a boolean (#field-zoo). - if (type === 'boolean' || type === 'toggle') { - booleanCols.push(name); - } - // Numeric scalars are coerced back to JS numbers on read so legacy - // TEXT-affinity columns (created before they were mapped to a numeric - // column) still return numbers, not strings — see NUMERIC_SCALAR_TYPES. - if (NUMERIC_SCALAR_TYPES.has(type) && !field.multiple) { - numericCols.push(name); - } - if (type === 'date') { - (this.dateFields[tableName] ??= new Set()).add(name); - } - if (type === 'datetime') { - (this.datetimeFields[tableName] ??= new Set()).add(name); - } - if (type === 'time') { - (this.timeFields[tableName] ??= new Set()).add(name); - } - if (type === 'auto_number' || type === 'autonumber') { - const fmt = resolveAutonumberFormat(field); - // Tokenize once: the renderer resolves date tokens (`{YYYYMMDD}`), - // field interpolation (`{island_zone}`) and the sequence slot at - // fill time. The counter scopes to whatever renders before the slot. - const tokens = parseAutonumberFormat(fmt); - autoNumberCols.push({ name, format: fmt, tokens, tenantField }); - } - } - } - this.jsonFields[tableName] = jsonCols; - this.booleanFields[tableName] = booleanCols; - this.numericFields[tableName] = numericCols; - this.autoNumberFields[tableName] = autoNumberCols; - this.tenantFieldByTable[tableName] = tenantField; + const tenantField = this.tenantFieldByTable[tableName] ?? null; - // Deferred-DDL mode (#3917): everything above is in-memory metadata + // Deferred-DDL mode (#3917): the call above is in-memory metadata // registration — coercion maps, tenancy, and the `managedObjectFields` // entry `detectManagedDrift()` diffs against. Everything below issues // DDL. `os migrate plan` / `apply` boot with the deferral armed so the diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index d82bf85c0a..2e792b8a95 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -543,6 +543,14 @@ export class ObjectQLPlugin implements Plugin { // manage DDL out-of-band, and serializes through // `reloadSchemaSync` so overlapping reload events can't race DDL. this.ingestReloadedObjects(ctx, payload); + if (this.skipSchemaSync) { + // DDL is managed out-of-band here, but the objects this reload just + // ingested still have to be TOLD to the driver, or every JSON / + // datetime / boolean value written to them is encoded by the + // client's per-type defaults. No DDL, no round-trip, nothing to + // serialize against — see `registerSchemasWithoutDdl`. + await this.registerSchemasWithoutDdl(ctx); + } if (!this.skipSchemaSync) { this.reloadSchemaSync = this.reloadSchemaSync.then(async () => { try { @@ -647,11 +655,7 @@ export class ObjectQLPlugin implements Plugin { // table; we only assume the DDL is in place and skip straight to // hydration. This avoids one round-trip per table × N objects on // every cold boot. - if (this.skipSchemaSync) { - ctx.logger.info('Skipping schema sync (OS_SKIP_SCHEMA_SYNC=1) — assuming DDL is managed out-of-band'); - } else { - await this.syncRegisteredSchemas(ctx); - } + await this.installRegisteredSchemas(ctx); // Phase 2: Hydrate SchemaRegistry from sys_metadata (loads custom/template objects). // Project kernels (environmentId set) USUALLY source metadata from the @@ -674,9 +678,7 @@ export class ObjectQLPlugin implements Plugin { // Phase 3: Sync any new schemas that were just hydrated from the DB // (e.g. CRM objects seeded via template — they must have tables before use). - if (!this.skipSchemaSync) { - await this.syncRegisteredSchemas(ctx); - } + await this.installRegisteredSchemas(ctx); // Bridge all SchemaRegistry objects to metadata service. // @@ -1341,6 +1343,125 @@ export class ObjectQLPlugin implements Plugin { ); } + /** + * Install every registered object's schema with its driver — the ONE seam + * `start()` uses for that, on both the syncing and the DDL-free path. + * + * `skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1` opts a deployment out of **DDL**, + * not out of telling the driver what its objects look like. Those were the + * same thing until now: {@link syncRegisteredSchemas} is the only caller of + * `syncSchema()`, and a SQL driver builds its per-object coercion registries + * (JSON, boolean, numeric, date/datetime/time, auto_number, and the + * tenant-isolation column) as the first step of that DDL call. So a boot that + * skipped it came up with those registries EMPTY and every value written + * afterwards encoded by whatever the underlying client does with a bare JS + * value. + * + * On Postgres that is a live data-correctness defect, not a slow path: an + * array value on a `json` field reaches node-postgres, which renders it as a + * Postgres ARRAY LITERAL — `22P02 invalid input syntax for type json`, a 500 + * on every write — **except `[]`, whose literal `{}` is valid JSON**, so an + * empty array is accepted and silently stored as an empty OBJECT. A bare + * string 500s for the same reason (`x` is not JSON text; `"x"` is). SQLite + * hides all of it behind a dialect-local bind-safety net, which is why the + * SQLite/Turso suites never saw it. + * + * The split is the same ruling #7737/#10629 made for federated objects — + * that flag is about DDL, and a binding that is DDL-free must not ride on it + * — applied to the managed ones. + */ + private async installRegisteredSchemas(ctx: PluginContext): Promise { + if (!this.skipSchemaSync) { + await this.syncRegisteredSchemas(ctx); + return; + } + await this.registerSchemasWithoutDdl(ctx); + } + + /** + * Tell every driver about its objects **without running any DDL** — what a + * `skipSchemaSync` boot does instead of {@link syncRegisteredSchemas}. + * + * Pure in-memory metadata assignment on the driver side + * (`registerObjectMetadata`, the managed sibling of + * `registerExternalObject`): no `CREATE TABLE`, no `ALTER TABLE`, no + * existence probe, no round-trip. That matters, because the reason + * `skipSchemaSync` exists is a cold-start budget shorter than "one round-trip + * per table × N objects" — this pass costs one pass over the object list and + * keeps the promise the flag actually makes. + * + * Federated (external) objects are skipped here on purpose: their DDL-free + * binding is `registerExternalObject`, driven by {@link syncRegisteredSchemas} + * on a syncing boot and unconditionally by {@link reconcileFederatedBindings} + * at `kernel:ready` — i.e. they were already correct under this flag, and + * managed objects were the half with nothing. + * + * Idempotent, so it is safe to re-drive after every metadata reload. + */ + private async registerSchemasWithoutDdl(ctx: PluginContext): Promise { + if (!this.ql) return; + + // [#9285] Same propagation contract as the sync pass: "the registry holds + // nothing" and "the registry could not be read" have opposite consequences + // and only the first is a truthful reason to register nothing. + const allObjects = this.readRegisteredObjects('registerSchemasWithoutDdl'); + if (allObjects.length === 0) return; + + const groups = new Map(); + let federated = 0; + let unsupported = 0; + let unbound = 0; + + for (const obj of allObjects) { + if ((obj as any).external != null) { + federated++; + continue; + } + const driver: any = this.ql.getDriverForObject(obj.name); + if (!driver) { + unbound++; + continue; + } + if (typeof driver.registerObjectMetadata !== 'function') { + unsupported++; + continue; + } + let group = groups.get(driver); + if (!group) { + group = []; + groups.set(driver, group); + } + group.push(obj); + } + + let registered = 0; + for (const [driver, objects] of groups) { + try { + await driver.registerObjectMetadata(objects); + registered += objects.length; + } catch (e: unknown) { + // Reported at `error` per the AGENTS.md degradation-log-level rule: the + // deployment looks healthy — the tables exist, the objects are served — + // while the values written to them are encoded by the client's + // per-type defaults rather than by their declared field types. + ctx.logger.error( + `[ObjectQLPlugin] DDL-free schema registration FAILED for driver '${driver?.name}' — its objects stay registered and ` + + `served, but the driver was never told their field types, so values written to them are encoded by the database ` + + `client's per-type defaults instead: on Postgres an array on a JSON field is rejected as a malformed array literal ` + + `and an empty array is silently stored as an empty object. Fix the driver error below and restart.`, + e instanceof Error ? e : new Error(String(e)), + { driver: driver?.name, objects: objects.length }, + ); + } + } + + ctx.logger.info( + 'Skipping schema sync (OS_SKIP_SCHEMA_SYNC=1) — assuming DDL is managed out-of-band; ' + + `registered ${registered} object schema(s) with their drivers WITHOUT DDL`, + { registered, federated, unsupported, unbound, total: allObjects.length }, + ); + } + /** * Synchronize all registered object schemas to the database. * diff --git a/packages/objectql/src/skip-schema-sync-registers-object-metadata.test.ts b/packages/objectql/src/skip-schema-sync-registers-object-metadata.test.ts new file mode 100644 index 0000000000..3c58361f7f --- /dev/null +++ b/packages/objectql/src/skip-schema-sync-registers-object-metadata.test.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10995] `OS_SKIP_SCHEMA_SYNC` is about DDL — it must not also stop the + * drivers from being TOLD what their objects look like. + * + * ## The accident this pins + * + * A SQL driver builds its per-object coercion registries (JSON, boolean, + * numeric, date/datetime/time, auto_number, tenant column) as the first step of + * `syncSchema()` — a DDL call. `skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1` is the + * documented posture for deployments whose migrations run out-of-band, and it + * skipped that call entirely, so those deployments served every write with the + * registries EMPTY. On Postgres that is not a slow path but a data-correctness + * defect: an array on a `json` field is rendered by node-postgres as a Postgres + * ARRAY LITERAL and rejected (`22P02`) — **except `[]`, whose literal `{}` is + * valid JSON, so an empty array was accepted and silently stored as an empty + * OBJECT**. `packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts` + * pins the encoding half against a live Postgres; this file pins the boot half: + * that the flag routes to the DDL-FREE registration instead of to nothing. + * + * It is the same ruling #7737/#10629 already made for FEDERATED objects — that + * flag is about DDL, and a binding that is DDL-free must not ride on it — + * extended to the managed ones. + */ + +import { describe, it, expect } from 'vitest'; +import type { ServiceObject } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; +import { ObjectQLPlugin } from './plugin.js'; + +interface Recorded { + level: 'debug' | 'info' | 'warn' | 'error'; + message: string; + args: unknown[]; +} + +function recordingLogger() { + const records: Recorded[] = []; + const push = (level: Recorded['level']) => (message: string, ...args: unknown[]) => + void records.push({ level, message: String(message), args }); + return { + records, + logger: { debug: push('debug'), info: push('info'), warn: push('warn'), error: push('error') }, + at(level: Recorded['level']) { + return records.filter((r) => r.level === level); + }, + }; +} + +/** A driver that records which of the two registration routes it was given. */ +function recordingDriver(opts: { metadataRoute?: boolean; throws?: boolean } = {}) { + const withMetadataRoute = opts.metadataRoute !== false; + const calls = { + syncSchema: [] as string[], + registerObjectMetadata: [] as string[][], + registerExternalObject: [] as string[], + }; + const driver: Record = { + name: 'default', + supports: {}, + async find() { + return []; + }, + async syncSchema(table: string) { + calls.syncSchema.push(table); + }, + async registerExternalObject(obj: any) { + calls.registerExternalObject.push(obj?.name); + }, + }; + if (withMetadataRoute) { + driver.registerObjectMetadata = (objects: any[]) => { + if (opts.throws) throw new Error('registry write failed'); + calls.registerObjectMetadata.push(objects.map((o) => o?.name)); + }; + } + return { driver, calls }; +} + +/** Drive the boot seam directly: which route the flag takes IS the unit. */ +async function install( + skipSchemaSync: boolean, + objects: ServiceObject[], + driver: unknown, +) { + const rec = recordingLogger(); + const plugin = new ObjectQLPlugin(); + const engine = new ObjectQL({ logger: rec.logger } as any); + engine.registerDriver(driver as any); + for (const obj of objects) engine.registerObject(obj); + (plugin as any).ql = engine; + (plugin as any).skipSchemaSync = skipSchemaSync; + await (plugin as any).installRegisteredSchemas({ logger: rec.logger }); + return rec; +} + +const PREF: ServiceObject = { + name: 'sys_user_preference', + label: 'User Preference', + fields: { id: { type: 'text' }, key: { type: 'text' }, value: { type: 'json' } }, +} as ServiceObject; + +const NOTE: ServiceObject = { + name: 'note', + label: 'Note', + fields: { id: { type: 'text' }, body: { type: 'text' } }, +} as ServiceObject; + +describe('OS_SKIP_SCHEMA_SYNC boot registers object metadata without DDL (#10995)', () => { + it('registers every managed object with the driver, and runs no DDL', async () => { + const { driver, calls } = recordingDriver(); + await install(true, [PREF, NOTE], driver); + + // The whole point: the objects reached the driver … + expect(calls.registerObjectMetadata).toHaveLength(1); + expect(calls.registerObjectMetadata[0]).toEqual(['sys_user_preference', 'note']); + // … and no DDL was issued, which is what the flag actually opts out of. + expect(calls.syncSchema).toEqual([]); + }); + + it('still syncs (and does NOT take the metadata-only route) when the flag is off', async () => { + const { driver, calls } = recordingDriver(); + await install(false, [PREF, NOTE], driver); + + expect(calls.syncSchema).toEqual(['sys_user_preference', 'note']); + expect(calls.registerObjectMetadata).toEqual([]); + }); + + it('reports the registration in the skip line, so a boot can be audited', async () => { + const { driver } = recordingDriver(); + const rec = await install(true, [PREF, NOTE], driver); + + const line = rec.at('info').find((r) => /OS_SKIP_SCHEMA_SYNC/.test(r.message)); + expect(line).toBeDefined(); + // Before this change the same line said only that sync was skipped — a boot + // that had told its drivers nothing read exactly like one that had. + expect(line!.message).toMatch(/WITHOUT DDL/); + expect(line!.message).toContain('registered 2 object schema(s)'); + expect(line!.args[0]).toMatchObject({ registered: 2, total: 2 }); + }); + + it('leaves federated objects to registerExternalObject, which is already DDL-free', async () => { + const external = { + name: 'legacy_customer', + label: 'Legacy Customer', + fields: { id: { type: 'text' } }, + external: { remoteName: 'customers' }, + } as unknown as ServiceObject; + const { driver, calls } = recordingDriver(); + await install(true, [PREF, external], driver); + + // The managed one only — #7737/#10629 already bind the federated one at + // `kernel:ready`, and handing it to the managed route would register it + // under its OBJECT name instead of its remote table. + expect(calls.registerObjectMetadata[0]).toEqual(['sys_user_preference']); + }); + + it('degrades without throwing when a driver has no metadata route', async () => { + const { driver, calls } = recordingDriver({ metadataRoute: false }); + const rec = await install(true, [PREF], driver); + + expect(calls.syncSchema).toEqual([]); + const line = rec.at('info').find((r) => /OS_SKIP_SCHEMA_SYNC/.test(r.message)); + expect(line!.args[0]).toMatchObject({ registered: 0, unsupported: 1 }); + }); + + it('reports a failed registration at error, naming the consequence', async () => { + const { driver } = recordingDriver({ throws: true }); + const rec = await install(true, [PREF], driver); + + const err = rec.at('error')[0]; + expect(err).toBeDefined(); + // From the outside the deployment looks healthy, so the log has to say what + // is actually wrong with it. + expect(err.message).toMatch(/never told their field types/); + expect(err.message).toMatch(/empty array is silently stored as an empty object/); + expect(err.args[0]).toBeInstanceOf(Error); + }); +}); diff --git a/packages/spec/src/contracts/data-driver.ts b/packages/spec/src/contracts/data-driver.ts index 67d80a7beb..2789172d7f 100644 --- a/packages/spec/src/contracts/data-driver.ts +++ b/packages/spec/src/contracts/data-driver.ts @@ -317,6 +317,30 @@ export interface IDataDriver { */ registerExternalObject?(schema: unknown): void | Promise; + /** + * Register MANAGED objects' read/write metadata WITHOUT running DDL — the + * managed sibling of {@link registerExternalObject}. + * + * A driver that encodes values from an object's DECLARED field types (the SQL + * driver's JSON / boolean / numeric / date / datetime / time coercion + * registries, its auto_number descriptors and its tenant-isolation column) + * normally learns them as a side effect of `syncSchema()`. A deployment that + * manages DDL out-of-band (`skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1`) never + * calls that — so without this entry point the driver serves writes while + * knowing nothing about the objects, and every value is encoded by whatever + * the underlying database client does with a bare JS value. On Postgres that + * rejects an array on a JSON field outright and silently stores `[]` as `{}`. + * + * Implementations MUST NOT issue DDL, probe for existence, or otherwise touch + * the database here: the flag this serves exists to protect a cold-start + * budget, so registration has to be pure in-memory bookkeeping. Idempotent — + * the engine re-drives it after every metadata reload. + * + * Optional: drivers whose encoding does not depend on declared field types + * (memory, mongodb) simply omit it, and the engine skips them. + */ + registerObjectMetadata?(schemas: unknown[]): void | Promise; + /** * What this driver's schema synchronisation has DONE since `connect()`: * how many tables it created, and how many it found already present. From a68f1a21035d5a805721e24087c3c68acecca88c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 14:59:05 +0000 Subject: [PATCH 2/2] test(driver-sql): type the query options in the #10995 pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:query-options-erasure` counts `as any` on a driver query bag in test code too — the new pins pushed the test surface 240 -> 243. The options here are on-contract, so they are typed rather than grandfathered. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- .../src/sql-driver-json-binding-without-ddl.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts b/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts index 56bae6a0c7..defaf4a5a4 100644 --- a/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-json-binding-without-ddl.test.ts @@ -65,7 +65,7 @@ function prefObject(name: string) { return { name, fields: { ...PREF_FIELDS } } as any; } -const OPTS = { bypassTenantAudit: true } as any; +const OPTS = { bypassTenantAudit: true }; /** `create table … (id text primary key, key text, value jsonb)` — no driver DDL. */ async function migrateOutOfBand(driver: SqlDriver, table: string): Promise { @@ -79,7 +79,7 @@ async function migrateOutOfBand(driver: SqlDriver, table: string): Promise async function insertAndRead(driver: SqlDriver, table: string, value: unknown): Promise { const id = `i_${Math.random().toString(36).slice(2, 10)}`; await driver.create(table, { id, key: 'ui.recent', value }, OPTS); - const row: any = await driver.findOne(table, { where: { id } } as any, OPTS); + const row: any = await driver.findOne(table, { where: { id } }, OPTS); return row?.value; } @@ -88,7 +88,7 @@ async function updateAndRead(driver: SqlDriver, table: string, value: unknown): const id = `u_${Math.random().toString(36).slice(2, 10)}`; await driver.create(table, { id, key: 'ui.recent', value: { seeded: true } }, OPTS); await driver.update(table, id, { value }, OPTS); - const row: any = await driver.findOne(table, { where: { id } } as any, OPTS); + const row: any = await driver.findOne(table, { where: { id } }, OPTS); return row?.value; } @@ -163,7 +163,7 @@ describe.skipIf(!PG_URL)('#10995 — live Postgres, driver told about the object // comes back — the registration must not turn every column into JSON. const id = `k_${Math.random().toString(36).slice(2, 10)}`; await driver.create(TABLE, { id, key: 'ui.recent', value: null }, OPTS); - const row: any = await driver.findOne(TABLE, { where: { id } } as any, OPTS); + const row: any = await driver.findOne(TABLE, { where: { id } }, OPTS); expect(row.key).toBe('ui.recent'); expect(row.value).toBeNull(); });