Skip to content

Commit a037f7c

Browse files
Elon Muskclaude
andauthored
fix(driver-sql,objectql): JSON-field values must not depend on DDL having run (#10995) (#11070)
* fix(driver-sql,objectql): JSON values must not depend on DDL having run (#10995) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM * test(driver-sql): type the query options in the #10995 pin `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 93304c2 commit a037f7c

6 files changed

Lines changed: 755 additions & 76 deletions

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
"@objectstack/objectql": patch
4+
"@objectstack/spec": patch
5+
---
6+
7+
Fix JSON-field writes on Postgres deployments that manage DDL out-of-band
8+
(`skipSchemaSync` / `OS_SKIP_SCHEMA_SYNC=1`): a non-empty array and a bare
9+
string were rejected with a 500, and an empty array was **silently stored as an
10+
empty object** (#10995).
11+
12+
The SQL driver does `JSON.stringify` a JSON field's value on every non-SQLite
13+
dialect — but only for fields listed in its per-object `jsonFields` registry,
14+
and that registry (like the boolean / numeric / date / datetime / time /
15+
auto_number registries and the tenant-isolation column) was filled **only** as
16+
the first step of a DDL call. A deployment that skips boot schema sync therefore
17+
served every write knowing nothing about its objects, and values reached
18+
node-postgres to be encoded by its per-type defaults:
19+
20+
- an **object** became JSON text — accidentally correct;
21+
- an **array** became a Postgres ARRAY LITERAL (`{…}`) — `22P02 invalid input
22+
syntax for type json`, a 500 on every write;
23+
- **except `[]`**, whose array literal `{}` is valid JSON, so an empty array was
24+
accepted and stored as an empty **object** — corruption, not an error;
25+
- a **bare string** was passed raw (`x` is not JSON text, `"x"` is) — a 500,
26+
while a number survived because `42` already is valid JSON.
27+
28+
SQLite never showed any of it: `formatInput` ends with a bind-safety net gated
29+
on that dialect, so the same empty registry is invisible there — which is why
30+
tenant environments on Turso/SQLite and the suites that run on them were blind
31+
to a defect live on every Postgres deployment.
32+
33+
The registration is now separable from the DDL, on the ruling #7737/#10629
34+
already made for federated objects — that flag is about DDL, and a binding that
35+
is DDL-free must not ride on it:
36+
37+
- `SqlDriver.registerObjectMetadata(objects)` installs a managed object's
38+
coercion metadata with no `CREATE TABLE`, no `ALTER TABLE`, no existence probe
39+
and no round-trip — the managed sibling of `registerExternalObject`, declared
40+
optional on `IDataDriver` so drivers that don't need it omit it;
41+
- a `skipSchemaSync` boot (and metadata reload) now takes that route instead of
42+
doing nothing, keeping the cold-start budget the flag exists to protect;
43+
- `initObjects` registers before the ADR-0015 DDL gate refuses, so objects on a
44+
datasource ObjectStack is only a guest in are encoded from their declared
45+
field types too. The refusal itself is unchanged.
Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#10995] A JSON field must round-trip on Postgres when the driver was told
5+
* about its object WITHOUT running DDL.
6+
*
7+
* ## The defect, measured rather than inferred
8+
*
9+
* `formatInput` DOES `JSON.stringify` a JSON field's value on every non-SQLite
10+
* dialect — but only for fields listed in `jsonFields[object]`, and that
11+
* registry is filled exclusively by the DDL entry points (`initObjects` /
12+
* `syncSchema`, plus `registerExternalObject` for federated objects). A
13+
* deployment that manages DDL out-of-band — `skipSchemaSync` /
14+
* `OS_SKIP_SCHEMA_SYNC=1`, the documented posture after running migrations
15+
* manually, and the one cold-start-sensitive runtimes are told to use — never
16+
* calls them, so it serves writes with EVERY coercion registry empty. What
17+
* reaches Postgres is then whatever node-postgres does with a bare JS value:
18+
*
19+
* | value written to a `json` field | with an empty registry (measured on PG 16) |
20+
* | :--- | :--- |
21+
* | `{a:1}` object | JSON text — accidentally correct |
22+
* | `42` number | `42` — already valid JSON |
23+
* | `[{type:'app'}]` array | `{"(type,app)"}`-style ARRAY LITERAL → `22P02 invalid input syntax for type json` → 500 |
24+
* | `'x'` bare string | raw `x` → not JSON text (`"x"` would be) → 500 |
25+
* | `[]` empty array | array literal `{}` — **valid JSON**, so it is ACCEPTED and silently stored as an empty OBJECT |
26+
*
27+
* That last row is the one that outlives a fix aimed at the crashes: it does
28+
* not error, it corrupts. Every row above was reproduced against a live
29+
* Postgres before the fix, on INSERT and on UPDATE alike.
30+
*
31+
* ## Why no existing suite caught it
32+
*
33+
* `formatInput` ends with a bind-safety net that stringifies any leftover
34+
* object/array — gated on `isSqlite`, because better-sqlite3 cannot bind them
35+
* at all. So on SQLite an empty registry is invisible, and tenant environments
36+
* run Turso/SQLite: the seed and data suites exercise a different dialect
37+
* branch of the same function. Postgres has no such net, and both control
38+
* planes are Postgres.
39+
*
40+
* ## What this file pins, and on which driver path
41+
*
42+
* Every test in §1–§3 runs against a **live Postgres** (`OS_TEST_POSTGRES_URL`,
43+
* provisioned by CI's `temporal-conformance` job) through the real
44+
* `SqlDriver.create()` / `update()` paths — the dialect the defect is on.
45+
* §4 runs the same matrix on SQLite as an expected NON-effect. The tables here
46+
* are created with raw SQL, exactly as an out-of-band migration would, and the
47+
* driver under test never runs DDL against them.
48+
*/
49+
50+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
51+
import { SqlDriver } from '../src/index.js';
52+
import { PG_CELL, dialectCell } from './live-dialect-matrix.testkit.js';
53+
import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared';
54+
55+
const PG_URL = PG_CELL.url;
56+
57+
/** The object as an out-of-band migration would have created it. */
58+
const PREF_FIELDS = {
59+
id: { type: 'text' },
60+
key: { type: 'text' },
61+
value: { type: 'json' },
62+
} as const;
63+
64+
function prefObject(name: string) {
65+
return { name, fields: { ...PREF_FIELDS } } as any;
66+
}
67+
68+
const OPTS = { bypassTenantAudit: true };
69+
70+
/** `create table … (id text primary key, key text, value jsonb)` — no driver DDL. */
71+
async function migrateOutOfBand(driver: SqlDriver, table: string): Promise<void> {
72+
await (driver as any).knex.raw(
73+
`create table if not exists "${table}" (id text primary key, key text, value jsonb, ` +
74+
`created_at timestamptz, updated_at timestamptz)`,
75+
);
76+
}
77+
78+
/** Write `value` on a fresh row and read back what storage actually holds. */
79+
async function insertAndRead(driver: SqlDriver, table: string, value: unknown): Promise<any> {
80+
const id = `i_${Math.random().toString(36).slice(2, 10)}`;
81+
await driver.create(table, { id, key: 'ui.recent', value }, OPTS);
82+
const row: any = await driver.findOne(table, { where: { id } }, OPTS);
83+
return row?.value;
84+
}
85+
86+
/** Seed a row, PATCH only `value` onto it, and read back what storage holds. */
87+
async function updateAndRead(driver: SqlDriver, table: string, value: unknown): Promise<any> {
88+
const id = `u_${Math.random().toString(36).slice(2, 10)}`;
89+
await driver.create(table, { id, key: 'ui.recent', value: { seeded: true } }, OPTS);
90+
await driver.update(table, id, { value }, OPTS);
91+
const row: any = await driver.findOne(table, { where: { id } }, OPTS);
92+
return row?.value;
93+
}
94+
95+
describe.skipIf(!PG_URL)('#10995 — live Postgres, driver told about the object without DDL', () => {
96+
// §1–§3 share one driver: the registration under test is per-object, and a
97+
// shared connection keeps the live cell to one pool.
98+
let driver: SqlDriver;
99+
const TABLE = 'os10995_pref';
100+
101+
beforeAll(async () => {
102+
driver = new SqlDriver(PG_CELL.config());
103+
await migrateOutOfBand(driver, TABLE);
104+
// THE LINE UNDER TEST: the object's field types reach the driver with no
105+
// CREATE TABLE, no ALTER TABLE and no round-trip — what a `skipSchemaSync`
106+
// boot now does in place of doing nothing.
107+
driver.registerObjectMetadata([prefObject(TABLE)]);
108+
});
109+
110+
afterAll(async () => {
111+
await driver?.disconnect();
112+
});
113+
114+
// ── §1 The three rows the card is about ──────────────────────────────────
115+
116+
it('§1a a NON-EMPTY ARRAY round-trips (insert and update)', async () => {
117+
const recents = [
118+
{ type: 'app', id: 'crm' },
119+
{ type: 'record', id: 'acc_1' },
120+
];
121+
const inserted = await insertAndRead(driver, TABLE, recents);
122+
expect(Array.isArray(inserted)).toBe(true);
123+
expect(inserted).toEqual(recents);
124+
125+
const updated = await updateAndRead(driver, TABLE, recents);
126+
expect(Array.isArray(updated)).toBe(true);
127+
expect(updated).toEqual(recents);
128+
});
129+
130+
it('§1b a BARE STRING and the other scalar JSON documents round-trip (insert and update)', async () => {
131+
// `"x"` is a legal JSON document; a fix that special-cases arrays re-fails
132+
// this row, which is why it is pinned apart from §1a.
133+
expect(await insertAndRead(driver, TABLE, 'x')).toBe('x');
134+
expect(await updateAndRead(driver, TABLE, 'x')).toBe('x');
135+
136+
expect(await insertAndRead(driver, TABLE, true)).toBe(true);
137+
expect(await updateAndRead(driver, TABLE, false)).toBe(false);
138+
});
139+
140+
it('§1c an EMPTY ARRAY round-trips as [] — not as {}', async () => {
141+
// The row that does not crash. Postgres' array literal for `[]` is `{}`,
142+
// which is valid JSON, so the write was accepted and the value silently
143+
// became an empty OBJECT. Both halves are asserted: the shape that must be
144+
// there, and the shape that must NOT.
145+
const inserted = await insertAndRead(driver, TABLE, []);
146+
expect(Array.isArray(inserted)).toBe(true);
147+
expect(inserted).toEqual([]);
148+
expect(inserted).not.toEqual({});
149+
150+
const updated = await updateAndRead(driver, TABLE, []);
151+
expect(Array.isArray(updated)).toBe(true);
152+
expect(updated).toEqual([]);
153+
expect(updated).not.toEqual({});
154+
});
155+
156+
// ── §2 Expected NON-effects on the same path ─────────────────────────────
157+
158+
it('§2 objects, nested arrays and numbers are unchanged (they already worked)', async () => {
159+
expect(await insertAndRead(driver, TABLE, { a: 1 })).toEqual({ a: 1 });
160+
expect(await updateAndRead(driver, TABLE, { items: [1, 2] })).toEqual({ items: [1, 2] });
161+
expect(await insertAndRead(driver, TABLE, 42)).toBe(42);
162+
// A non-JSON column keeps its own binding: `key` is text, and text is what
163+
// comes back — the registration must not turn every column into JSON.
164+
const id = `k_${Math.random().toString(36).slice(2, 10)}`;
165+
await driver.create(TABLE, { id, key: 'ui.recent', value: null }, OPTS);
166+
const row: any = await driver.findOne(TABLE, { where: { id } }, OPTS);
167+
expect(row.key).toBe('ui.recent');
168+
expect(row.value).toBeNull();
169+
});
170+
171+
// ── §3 The other posture with the same empty registry: DDL REFUSED ───────
172+
173+
it('§3 a datasource we are a guest in registers its objects even though DDL is refused', async () => {
174+
// `schemaMode !== 'managed'` (ADR-0015): `initObjects` must still refuse the
175+
// DDL — and must no longer leave the driver ignorant of the objects it was
176+
// just handed, which is what made every JSON write on a federated Postgres
177+
// datasource take the node-postgres defaults above.
178+
const guestTable = 'os10995_guest';
179+
const guest = new SqlDriver({ ...PG_CELL.config(), schemaMode: 'validate-only' } as any);
180+
try {
181+
await migrateOutOfBand(guest, guestTable);
182+
await expect(guest.initObjects([prefObject(guestTable)])).rejects.toBeInstanceOf(
183+
ExternalSchemaModeViolationError,
184+
);
185+
expect(await insertAndRead(guest, guestTable, [{ type: 'app' }])).toEqual([{ type: 'app' }]);
186+
expect(await updateAndRead(guest, guestTable, [])).toEqual([]);
187+
expect(await updateAndRead(guest, guestTable, 'x')).toBe('x');
188+
} finally {
189+
await guest.disconnect();
190+
}
191+
});
192+
});
193+
194+
// ── §4 The SQLite path is unchanged ────────────────────────────────────────
195+
196+
describe('#10995 — the SQLite path is unaffected', () => {
197+
it('§4 round-trips the same matrix, with and without the DDL-free registration', async () => {
198+
const driver = new SqlDriver(dialectCell('sqlite').config());
199+
try {
200+
const TABLE = 'os10995_sqlite';
201+
await (driver as any).knex.raw(
202+
`create table if not exists "${TABLE}" (id text primary key, key text, value text)`,
203+
);
204+
// Un-registered, SQLite neither crashes nor corrupts: `formatInput`'s
205+
// dialect-local bind-safety net stringifies the array, so what lands on
206+
// disk is the right JSON text — it just comes back as TEXT, because the
207+
// read-side parse is keyed by the same empty registry. Storing the right
208+
// bytes is what makes the empty registry invisible here, and it is why
209+
// the SQLite/Turso suites never showed the Postgres defect.
210+
expect(await insertAndRead(driver, TABLE, [{ type: 'app' }])).toBe('[{"type":"app"}]');
211+
expect(await updateAndRead(driver, TABLE, [])).toBe('[]');
212+
213+
// Registered: unchanged.
214+
driver.registerObjectMetadata([prefObject(TABLE)]);
215+
expect(await insertAndRead(driver, TABLE, [{ type: 'app' }])).toEqual([{ type: 'app' }]);
216+
expect(await updateAndRead(driver, TABLE, [])).toEqual([]);
217+
expect(await updateAndRead(driver, TABLE, 'x')).toBe('x');
218+
expect(await insertAndRead(driver, TABLE, { a: 1 })).toEqual({ a: 1 });
219+
} finally {
220+
await driver.disconnect();
221+
}
222+
});
223+
});

0 commit comments

Comments
 (0)