Skip to content

Commit 810751c

Browse files
committed
fix(metadata-protocol): canonicalise listDrafts updatedAt at the adapter boundary
`SysMetadataRepository.listDrafts` declares `updatedAt: string | null` on an inline TypeScript return type and reached the field through `row.updated_at ?? row.created_at ?? null`. `??` fires only on nullish, so the JS `Date` that Postgres and MySQL materialise for the builtin audit columns walked straight past it into a field declared a string. Route the value through the file's existing `canonicalIsoInstant`, the same producer-side canonicalisation `rowToItem` applies, with the terminal chosen per call site: `null` here, because the chain being replaced already ended in `?? null` and that is what "absent" already means to this projection's consumers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
1 parent ac76425 commit 810751c

2 files changed

Lines changed: 363 additions & 3 deletions

File tree

Lines changed: 339 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,339 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#14938] `SysMetadataRepository.listDrafts` declares `updatedAt: string |
5+
* null` and used to emit the raw `updated_at` column, so on Postgres and MySQL
6+
* it handed a JS `Date` through a field its own signature calls a string.
7+
*
8+
* ## The defect
9+
*
10+
* `listDrafts`' return type is an INLINE TypeScript object type on the method
11+
* itself — not a Zod schema — and the projection reached the field through
12+
* `row.updated_at ?? row.created_at ?? null`. `??` fires only on nullish, so a
13+
* `Date` walks straight past it into the declared field.
14+
*
15+
* Two independent reasons nothing reported it, and both are why the site
16+
* survived the #13973 census twice: a schema search finds no schema (the
17+
* declaration is an inline return type), and `rows` is cast `as any[]` one line
18+
* above the map, so tsc sees a `string` assignment that never happened.
19+
*
20+
* ## Why the value is a `Date` on the live dialects
21+
*
22+
* `updated_at` / `created_at` are the BUILTIN audit columns on `sys_metadata`
23+
* (`Field.datetime`, `packages/metadata-core/src/objects/sys-metadata.object.ts`).
24+
* `SqlDriver#formatOutput` repairs the audit columns
25+
* (`repairNaiveUtcAuditTimestamp`) and folds the declared datetime columns
26+
* (`normalizeSqliteDatetimeOutput`) ONLY inside its `if (this.isSqlite)` arm,
27+
* and `withPostgresCalendarDayAsText` leaves `timestamptz` / `timestamp`
28+
* deliberately untouched because those are instants. That dialect fact is
29+
* pinned live in
30+
* `packages/drivers/driver-sql/src/sql-driver-13567-audit-stamp-materialisation.test.ts`;
31+
* this file does not re-derive it and takes on no driver dependency
32+
* (`@objectstack/metadata-protocol` has none, and the layering runs the other
33+
* way) — the `Date` is hand-made here for exactly that reason.
34+
*
35+
* ## What is asserted, and why it is not a hand-copied shape
36+
*
37+
* The conformance table below is keyed by `keyof DraftHeader`, where
38+
* `DraftHeader` is extracted from the method's own signature with
39+
* `Awaited<ReturnType<...>>[number]`. So the assertion reads the DECLARATION
40+
* under test rather than a second copy of it: a field added to or removed from
41+
* that inline type reddens this file at type-check time instead of silently
42+
* going unchecked.
43+
*
44+
* ⚠️ Every case drives a hand-made `Date` — the one shape the live dialects
45+
* produce and no existing fixture ever did — and each case guards
46+
* non-vacuity (`toBeInstanceOf(Date)`) on the seeded row BEFORE reading the
47+
* output. Without that guard a fixture that degraded to a string would keep
48+
* this file green while measuring nothing, because the input and the assertion
49+
* would share an identity.
50+
*/
51+
52+
import { describe, it, expect } from 'vitest';
53+
// The engine-double contract gate: a fake looser than ObjectQL's own verb
54+
// dispatch is how #4434 shipped a dead route with its suite green.
55+
import {
56+
assertEngineDeleteDispatch,
57+
assertEngineUpdateDispatch,
58+
assertEngineFindOnePredicate,
59+
} from '@objectstack/metadata-core';
60+
import { SysMetadataRepository } from './sys-metadata-repository.js';
61+
62+
interface Row {
63+
[k: string]: unknown;
64+
}
65+
66+
/**
67+
* The declared row shape, read off the method signature itself rather than
68+
* restated. `CONFORMS` below is a mapped type over its keys, so this file
69+
* cannot drift out of step with the declaration it pins.
70+
*/
71+
type DraftHeader = Awaited<ReturnType<SysMetadataRepository['listDrafts']>>[number];
72+
73+
/** Canonical instant text — exactly what `Date.prototype.toISOString` emits. */
74+
const ISO_Z = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/;
75+
76+
/**
77+
* The instant every case drives, as the live dialects hand it out: a JS
78+
* `Date`. Carries non-zero milliseconds on purpose — `String(date)` and
79+
* `date.toString()` both drop them, so a truncating regression stays
80+
* observable rather than coinciding with the canonical text.
81+
*/
82+
const PG_INSTANT = new Date('2026-03-04T05:06:07.089Z');
83+
const PG_CREATED = new Date('2026-01-02T03:04:05.006Z');
84+
85+
/**
86+
* Reachable on BOTH live dialects (#14409): mysql2 3.23.1 answers a module
87+
* constant literally named `INVALID_DATE` for a zero `DATETIME`, and
88+
* postgres-date 1.0.7 builds `new Date(NaN)` for every year in 275760..294276
89+
* — years Postgres itself stores.
90+
*/
91+
const INVALID_INSTANT = new Date(NaN);
92+
93+
/**
94+
* Runtime conformance for the declared projection, keyed by the declaration's
95+
* OWN keys. A `Date` reaching `updatedAt` satisfies neither arm of its union,
96+
* which is precisely the defect.
97+
*/
98+
const CONFORMS: { [K in keyof DraftHeader]: (value: DraftHeader[K]) => boolean } = {
99+
type: (v) => typeof v === 'string',
100+
name: (v) => typeof v === 'string',
101+
organizationId: (v) => v === null || typeof v === 'string',
102+
packageId: (v) => v === null || typeof v === 'string',
103+
updatedAt: (v) => v === null || typeof v === 'string',
104+
updatedBy: (v) => v === null || typeof v === 'string',
105+
};
106+
107+
/**
108+
* Assert every field of every row against the declared union, naming the field
109+
* in the assertion payload so a failure says WHICH one broke rather than
110+
* `false !== true`.
111+
*/
112+
function expectConformsToDeclaration(rows: DraftHeader[]): void {
113+
expect(rows.length).toBeGreaterThan(0);
114+
for (const row of rows) {
115+
for (const key of Object.keys(CONFORMS) as Array<keyof DraftHeader>) {
116+
const check = CONFORMS[key] as (value: unknown) => boolean;
117+
expect({ field: key, conforms: check(row[key]) }).toEqual({ field: key, conforms: true });
118+
}
119+
}
120+
}
121+
122+
/**
123+
* Minimal engine fake. Deliberately stores exactly what it is seeded with — no
124+
* key dropping, no coercion — so a `Date` planted in a row survives to the read
125+
* door the way a live driver's would. The `$or` arm is real because
126+
* `listDrafts` issues one for a non-null-org caller (the ADR-0005 overlay
127+
* reach), and an unimplemented combinator throws rather than silently reading
128+
* a `$`-prefixed key as a field name.
129+
*/
130+
function makeFakeEngine(seed: Row[] = []) {
131+
let nextId = 1;
132+
const rows: Row[] = seed.map((r) => ({ id: `seed_${nextId++}`, ...r }));
133+
const history: Row[] = [];
134+
135+
const matches = (row: Row, where: Record<string, unknown>): boolean => {
136+
for (const [k, v] of Object.entries(where)) {
137+
if (k === '$or') {
138+
const branches = v as Array<Record<string, unknown>>;
139+
if (!branches.some((b) => matches(row, b))) return false;
140+
continue;
141+
}
142+
if (k.startsWith('$')) {
143+
throw new Error(`fake matcher: unimplemented combinator ${k}`);
144+
}
145+
const rv = row[k] ?? null;
146+
if ((v ?? null) !== rv) return false;
147+
}
148+
return true;
149+
};
150+
151+
const tableOf = (name: string): Row[] => (name === 'sys_metadata' ? rows : history);
152+
153+
return {
154+
rows,
155+
history,
156+
async findOne(table: string, q: { where: Record<string, unknown> }) {
157+
assertEngineFindOnePredicate(table, q);
158+
return tableOf(table).find((r) => matches(r, q.where)) ?? null;
159+
},
160+
async find(table: string, q: { where: Record<string, unknown>; limit?: number }) {
161+
const matched = tableOf(table).filter((r) => matches(r, q.where));
162+
// Hold the caller's bound AFTER the filter and by PRESENCE — a double
163+
// that ignores `limit` answers more rows than the real engine would.
164+
return typeof q?.limit === 'number' ? matched.slice(0, q.limit) : matched;
165+
},
166+
async insert(table: string, data: Row) {
167+
const row = { id: `row_${nextId++}`, ...data };
168+
tableOf(table).push(row);
169+
return row;
170+
},
171+
async update(table: string, data: Row, opts: { where: Record<string, unknown> }) {
172+
assertEngineUpdateDispatch(data, opts);
173+
const row = tableOf(table).find((r) => matches(r, opts.where));
174+
if (row) Object.assign(row, data);
175+
return row;
176+
},
177+
async delete(_table: string, opts: { where?: Record<string, unknown> }) {
178+
assertEngineDeleteDispatch(opts);
179+
/* not exercised here */
180+
},
181+
};
182+
}
183+
184+
/** One env-wide draft row, seeded with whatever audit stamps a case drives. */
185+
const draftRow = (stamps: Row): Row => ({
186+
type: 'view',
187+
name: 'case_grid',
188+
organization_id: null,
189+
state: 'draft',
190+
package_id: null,
191+
metadata: '{"label":"Cases"}',
192+
checksum: 'sha-draft',
193+
version: 1,
194+
updated_by: 'usr_1',
195+
created_by: 'usr_0',
196+
...stamps,
197+
});
198+
199+
function makeRepo(engine: ReturnType<typeof makeFakeEngine>, organizationId: string | null = null) {
200+
return new SysMetadataRepository({
201+
engine: engine as never,
202+
organizationId,
203+
orgLabel: organizationId ?? 'env',
204+
} as never);
205+
}
206+
207+
describe('#14938 — listDrafts emits canonical ISO text for updatedAt, whatever the dialect materialised', () => {
208+
describe('§A updated_at as a JS Date — the Postgres/MySQL materialisation', () => {
209+
it('canonicalises it and the whole projection conforms to the declared type', async () => {
210+
const engine = makeFakeEngine([draftRow({ updated_at: PG_INSTANT, created_at: PG_CREATED })]);
211+
const repo = makeRepo(engine);
212+
213+
// Non-vacuity: if the fixture ever degrades to a string this file would
214+
// keep passing while testing the shape that was never broken.
215+
expect(engine.rows[0]!.updated_at).toBeInstanceOf(Date);
216+
217+
const drafts = await repo.listDrafts();
218+
expect(drafts).toHaveLength(1);
219+
expect(typeof drafts[0]!.updatedAt).toBe('string');
220+
expect(drafts[0]!.updatedAt).toMatch(ISO_Z);
221+
expect(drafts[0]!.updatedAt).toBe(PG_INSTANT.toISOString());
222+
expectConformsToDeclaration(drafts);
223+
});
224+
225+
it('reaches the same canonicalisation through the org-scoped $or read', async () => {
226+
// A non-null-org caller sees BOTH its own overlay drafts and the env-wide
227+
// ones (#3115); the canonicalisation must not depend on which arm matched.
228+
const engine = makeFakeEngine([
229+
draftRow({ organization_id: 'org_alpha', updated_at: PG_INSTANT }),
230+
draftRow({ name: 'lead_grid', updated_at: PG_INSTANT }),
231+
]);
232+
const repo = makeRepo(engine, 'org_alpha');
233+
234+
expect(engine.rows[0]!.updated_at).toBeInstanceOf(Date);
235+
236+
const drafts = await repo.listDrafts();
237+
expect(drafts).toHaveLength(2);
238+
for (const draft of drafts) expect(draft.updatedAt).toBe(PG_INSTANT.toISOString());
239+
expectConformsToDeclaration(drafts);
240+
});
241+
});
242+
243+
describe('§B the created_at fallback — the second link of the same chain', () => {
244+
it('canonicalises created_at when updated_at is absent', async () => {
245+
const engine = makeFakeEngine([draftRow({ created_at: PG_CREATED })]);
246+
const repo = makeRepo(engine);
247+
248+
expect(engine.rows[0]!.updated_at).toBeUndefined();
249+
expect(engine.rows[0]!.created_at).toBeInstanceOf(Date);
250+
251+
const drafts = await repo.listDrafts();
252+
expect(drafts[0]!.updatedAt).toBe(PG_CREATED.toISOString());
253+
expectConformsToDeclaration(drafts);
254+
});
255+
});
256+
257+
describe('§C SQLite — the dialect that was already correct is not reshaped', () => {
258+
it('passes an already-canonical string through byte-identically', async () => {
259+
const canonical = '2026-03-04T05:06:07.089Z';
260+
const engine = makeFakeEngine([draftRow({ updated_at: canonical })]);
261+
const repo = makeRepo(engine);
262+
263+
expect(typeof engine.rows[0]!.updated_at).toBe('string');
264+
265+
const drafts = await repo.listDrafts();
266+
expect(drafts[0]!.updatedAt).toBe(canonical);
267+
expectConformsToDeclaration(drafts);
268+
});
269+
});
270+
271+
describe('§D the terminal this call site keeps', () => {
272+
it('answers null — not a synthesised "now" — when both audit columns are absent', async () => {
273+
// This projection declares `updatedAt: string | null` and the chain being
274+
// replaced already ended in `?? null`, so `null` is what "absent" already
275+
// means to every consumer of this list. `rowToItem` terminates in
276+
// `?? new Date(...).toISOString()` instead; the terminal is chosen per
277+
// call site (#14078), and substituting one for the other here would
278+
// invent an edit instant for a row that never recorded one.
279+
const engine = makeFakeEngine([draftRow({})]);
280+
const repo = makeRepo(engine);
281+
282+
const drafts = await repo.listDrafts();
283+
expect(drafts[0]!.updatedAt).toBeNull();
284+
expectConformsToDeclaration(drafts);
285+
});
286+
287+
it('answers null for an Invalid Date rather than throwing or serving the text "Invalid Date"', async () => {
288+
// The total `Date` arm (#14078, ruled B): unguarded, `toISOString()`
289+
// raises `RangeError: Invalid time value`, and the spelling it replaced
290+
// served the visible text instead. Here the shape takes the same branch
291+
// an absent column takes.
292+
const engine = makeFakeEngine([draftRow({ updated_at: INVALID_INSTANT })]);
293+
const repo = makeRepo(engine);
294+
295+
expect(engine.rows[0]!.updated_at).toBeInstanceOf(Date);
296+
expect(Number.isNaN((engine.rows[0]!.updated_at as Date).getTime())).toBe(true);
297+
298+
const drafts = await repo.listDrafts();
299+
expect(drafts[0]!.updatedAt).toBeNull();
300+
expect(drafts[0]!.updatedAt).not.toBe('Invalid Date');
301+
expectConformsToDeclaration(drafts);
302+
});
303+
});
304+
305+
describe('§E updatedBy is deliberately NOT canonicalised — it is not a timestamp', () => {
306+
it('passes the lookup column straight through, and the dialect asymmetry never reaches it', async () => {
307+
// `updated_by` / `created_by` are `Field.lookup('sys_user')` on
308+
// `sys_metadata` — string ids, not `Field.datetime`. The identical `??`
309+
// shape on the next line is therefore correct as written; canonicalising
310+
// it would be `String(value)` applied to a value that is already a
311+
// string, and folding it into this fix would widen the card's scope to a
312+
// line with no defect.
313+
const engine = makeFakeEngine([draftRow({ updated_at: PG_INSTANT, updated_by: 'usr_7' })]);
314+
const repo = makeRepo(engine);
315+
316+
const drafts = await repo.listDrafts();
317+
expect(drafts[0]!.updatedBy).toBe('usr_7');
318+
expectConformsToDeclaration(drafts);
319+
});
320+
321+
it('falls back to created_by and terminates in null, unchanged by this card', async () => {
322+
const engine = makeFakeEngine([
323+
draftRow({ updated_at: PG_INSTANT, updated_by: undefined, created_by: 'usr_0' }),
324+
draftRow({
325+
name: 'lead_grid',
326+
updated_at: PG_INSTANT,
327+
updated_by: undefined,
328+
created_by: undefined,
329+
}),
330+
]);
331+
const repo = makeRepo(engine);
332+
333+
const drafts = await repo.listDrafts();
334+
expect(drafts.find((d) => d.name === 'case_grid')!.updatedBy).toBe('usr_0');
335+
expect(drafts.find((d) => d.name === 'lead_grid')!.updatedBy).toBeNull();
336+
expectConformsToDeclaration(drafts);
337+
});
338+
});
339+
});

packages/metadata-protocol/src/sys-metadata-repository.ts

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,9 @@ import { isWritablePackage } from './package-writability.js';
132132
* `"Invalid Date"` instead.
133133
*
134134
* The terminal value is chosen **per call site**, and this one's is
135-
* `undefined`: both callers (`getByHash` and `rowToItem`) already end in
136-
* `?? new Date(...).toISOString()`, the branch an absent column takes today.
135+
* `undefined`: every caller already carries such a chain — `getByHash` and
136+
* `rowToItem` end in `?? new Date(...).toISOString()`, `listDrafts` (#14938)
137+
* in `?? null` — the branch an absent column takes at each of them today.
137138
* The ruling assigns `undefined` exactly where "the field is optional and the
138139
* caller already carries a `?? default` chain". ⛔ NOT the visible text
139140
* `"Invalid Date"` — the fields fed from here are read by machines
@@ -1175,7 +1176,27 @@ export class SysMetadataRepository implements MetadataRepository {
11751176
name: row.name,
11761177
organizationId: row.organization_id ?? null,
11771178
packageId: row.package_id ?? null,
1178-
updatedAt: row.updated_at ?? row.created_at ?? null,
1179+
// [#14938] `updated_at` / `created_at` are the BUILTIN audit columns,
1180+
// so on Postgres and MySQL they arrive here as a JS `Date`: the audit
1181+
// repair and the declared-datetime fold both sit inside
1182+
// `SqlDriver#formatOutput`'s `if (this.isSqlite)` arm, and
1183+
// `withPostgresCalendarDayAsText` leaves `timestamptz` / `timestamp`
1184+
// alone because those are instants. `rows` is cast `as any[]` above,
1185+
// so tsc never saw the `Date` land in a field this signature declares
1186+
// `string | null`. Canonicalised at the producer — the same adapter
1187+
// boundary `rowToItem` uses, never a tolerant `??` in the console or a
1188+
// reshape at the driver's read door (#13973's two standing
1189+
// prohibitions).
1190+
//
1191+
// The terminal is chosen PER CALL SITE (#14078) and this one is
1192+
// `null`, not `rowToItem`'s `?? new Date(...).toISOString()`: this
1193+
// projection declares `updatedAt: string | null` and the chain being
1194+
// replaced already ended in `?? null`, so `null` is what "absent"
1195+
// already means to every consumer of this list. An Invalid `Date`
1196+
// takes that same branch (the total `Date` arm), so a row the driver
1197+
// could not materialise reads as absent rather than as the visible
1198+
// text `"Invalid Date"`.
1199+
updatedAt: canonicalIsoInstant(row.updated_at ?? row.created_at) ?? null,
11791200
updatedBy: row.updated_by ?? row.created_by ?? null,
11801201
}));
11811202
}

0 commit comments

Comments
 (0)