From a9ddd14a93e749c267c9a801b24372d5ffee2267 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sat, 4 Jul 2026 16:11:36 -0600 Subject: [PATCH 1/5] fix(serialization): surface @computed scalars on default reads + guard cyclic @enumerable serialization (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REST GET silently dropped @computed attributes on the default (no-select) read because their getters are non-enumerable and JSON.stringify skips inherited getters. Naively making them enumerable would (a) install the whole-prototype- chain `for..in` toJSON on every such table (3.4–32x slower, scaling with chain depth) and (b) risk unbounded recursion, since non-enumerability is the only thing preventing @enumerable relationship cycles from overflowing. Unify both concerns in the struct toJSON (installed only when a table has an @enumerable or non-table @computed attribute): - Bounded enumeration: own keys + a known name list (every @enumerable attribute plus @computed attributes whose type is not a table type), replacing the O(inherited-enumerables) `for..in`. Proven equivalent — msgpackr struct prototypes carry no enumerable inherited props. - Cycle guard: @enumerable table-typed edges form a graph; detectCyclicEnumerable (lazy, self-loop aware) flags tables that can cycle. Those get a path-scoped, id-keyed guard (Map>) that collapses a repeated ancestor to a { [primaryKey]: id } reference stub; resolveStructForJSON fully resolves nested structs so none escape to native stringify after unwind. Acyclic tables keep a guard-free fast path. Table-typedness keys off the attribute `definition` (set early) rather than `.tableClass` (assigned later — unset for self/forward refs at collection time), resolving tableClass lazily at first serialization. Composite/array primary keys are normalized for the visited set. Tables with an unconstrained (`Any`/untyped) @computed take the guarded path, since such a resolver could return a live cyclic entity the static graph can't see. Name lists rebuild on schema reload. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../computed-and-cyclic-serialization.test.ts | 101 ++++++++++++ .../resources.js | 9 ++ .../schema.graphql | 39 +++++ resources/Table.ts | 147 ++++++++++++++++-- 4 files changed, 283 insertions(+), 13 deletions(-) create mode 100644 integrationTests/resources/computed-and-cyclic-serialization.test.ts create mode 100644 integrationTests/resources/computed-and-cyclic-serialization/resources.js create mode 100644 integrationTests/resources/computed-and-cyclic-serialization/schema.graphql diff --git a/integrationTests/resources/computed-and-cyclic-serialization.test.ts b/integrationTests/resources/computed-and-cyclic-serialization.test.ts new file mode 100644 index 0000000000..87cd24d221 --- /dev/null +++ b/integrationTests/resources/computed-and-cyclic-serialization.test.ts @@ -0,0 +1,101 @@ +/** + * harper#1484 — computed-scalar default surfacing (Fix A) + cyclic-enumerable serialization guard (Fix B). + * + * Fix A: REST GET with no explicit select must include @computed *scalar* attributes (their getters are + * non-enumerable, so JSON.stringify used to silently drop them). Table-typed computeds/relationships + * stay lazy by default. + * Fix B: @enumerable relationships that form a cycle (Author<->Book, and the Category self-loop) must + * serialize as { } reference stubs rather than overflowing the stack / 500ing. + * + * Reproduction: + * npm run test:integration -- "integrationTests/resources/computed-and-cyclic-serialization.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { ok, strictEqual, deepStrictEqual } from 'node:assert/strict'; +import { resolve } from 'node:path'; +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +// @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine +import { createApiClient } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'computed-and-cyclic-serialization'); +const skipSuite = process.platform === 'win32'; +const ENGINE = process.env.HARPER_STORAGE_ENGINE || 'rocksdb(default)'; + +suite( + `harper#1484 computed + cyclic serialization [engine=${ENGINE}]`, + { skip: skipSuite }, + (ctx: ContextWithHarper) => { + let httpURL: string; + let auth: string; + let client: ReturnType; + + async function rest(method: string, path: string, body?: unknown): Promise<{ status: number; body: any }> { + const r = await fetch(`${httpURL}${path}`, { + method, + headers: { 'Content-Type': 'application/json', 'Authorization': auth }, + body: body == null ? undefined : JSON.stringify(body), + }); + let parsed: any = null; + try { + parsed = await r.json(); + } catch { + /* ignore */ + } + return { status: r.status, body: parsed }; + } + + before(async () => { + await setupHarperWithFixture(ctx, FIXTURE_PATH, { config: {}, env: {} }); + client = createApiClient(ctx.harper); + httpURL = ctx.harper.httpURL; + auth = client.headers.Authorization; + + await rest('PUT', '/Author/1', { id: '1', name: 'Ada' }); + await rest('PUT', '/Book/10', { id: '10', authorId: '1', title: 'Analytical Engine' }); + // Category self-loop: 1 -> 2 -> 1 + await rest('PUT', '/Category/1', { id: '1', name: 'root', parentId: '2' }); + await rest('PUT', '/Category/2', { id: '2', name: 'child', parentId: '1' }); + // Node link cycle via an unconstrained (`Any`) @computed resolver: A -> B -> A + await rest('PUT', '/Node/A', { id: 'A', linkId: 'B' }); + await rest('PUT', '/Node/B', { id: 'B', linkId: 'A' }); + }); + + after(async () => { + await teardownHarper(ctx); + }); + + // ---- Fix A: computed scalar on the default read path ---- + + test('A1: default GET includes @computed scalar (displayName)', async () => { + const { status, body } = await rest('GET', '/Author/1'); + strictEqual(status, 200); + strictEqual(body.name, 'Ada'); + strictEqual(body.displayName, 'Ada', 'computed scalar must be surfaced without an explicit select'); + }); + + // ---- Fix B: cyclic @enumerable serialization does not overflow ---- + + test('B1: mutual enumerable cycle (Author<->Book) serializes with a reference stub', async () => { + const { status, body } = await rest('GET', '/Author/1'); + strictEqual(status, 200, 'must not 500/overflow on a cyclic-enumerable read'); + // Author.books is enumerable -> each Book.author is enumerable -> back to this Author = cycle. + const book = Array.isArray(body.books) ? body.books[0] : undefined; + ok(book, 'enumerable books should be present in serialization'); + deepStrictEqual(book.author, { id: '1' }, 'cyclic back-edge collapses to a { id } reference stub'); + }); + + test('B2: self-referential tree (Category.parent) does not overflow', async () => { + const { status, body } = await rest('GET', '/Category/1'); + strictEqual(status, 200, 'self-loop must not overflow'); + strictEqual(body.parent?.id, '2'); + deepStrictEqual(body.parent.parent, { id: '1' }, 'tree cycle collapses to a reference stub'); + }); + + test('B3: unconstrained @computed returning a live entity cycle does not overflow', async () => { + const { status, body } = await rest('GET', '/Node/A'); + strictEqual(status, 200, 'untyped-computed runtime cycle must not overflow (guarded, not fast path)'); + strictEqual(body.linked?.id, 'B', 'the computed live entity is surfaced'); + deepStrictEqual(body.linked.linked, { id: 'A' }, 'runtime cycle collapses to a reference stub'); + }); + } +); diff --git a/integrationTests/resources/computed-and-cyclic-serialization/resources.js b/integrationTests/resources/computed-and-cyclic-serialization/resources.js new file mode 100644 index 0000000000..561b8a7f2a --- /dev/null +++ b/integrationTests/resources/computed-and-cyclic-serialization/resources.js @@ -0,0 +1,9 @@ +// harper#1484 — resolver for the unconstrained (`Any`) @computed `Node.linked`, which returns a LIVE +// entity (another Node). The static cycle detector can't see this edge, so the table must fall to the +// guarded serialization path; two Nodes that link each other must serialize as reference stubs, not +// overflow. Exercises the "new exposure" the cross-model review flagged for computed-scalar-only tables. +// getSync returns the live decoded struct synchronously (a Promise/async get would serialize as {} and +// never cycle) — this is what makes the runtime cycle reachable during synchronous JSON serialization. +tables.Node.setComputedAttribute('linked', (record) => + record.linkId != null ? tables.Node.primaryStore.getSync(record.linkId) : undefined +); diff --git a/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql b/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql new file mode 100644 index 0000000000..03e67eff09 --- /dev/null +++ b/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql @@ -0,0 +1,39 @@ +# harper#1484 — default-read computed-scalar surfacing + cyclic-enumerable serialization guard. +# +# Fix A: a @computed *scalar* must appear on the default (no-select) REST read, even though its +# getter is non-enumerable. A @computed whose type is a table must stay lazy (edge guard). +# Fix B: @enumerable relationships that form a cycle must serialize as { primaryKey } reference +# stubs instead of overflowing the stack. + +type Author @table @export { + id: ID @primaryKey + name: String + # Fix A: computed SCALAR — surfaced by default. + displayName: String @computed(from: "name") + # Fix B: enumerable side of a mutual cycle (Author.books -> Book.author -> Author ...). + books: [Book] @relationship(to: authorId) @enumerable +} + +type Book @table @export { + id: ID @primaryKey + authorId: ID @indexed + title: String + author: Author @relationship(from: authorId) @enumerable +} + +# Self-referential tree: enumerable parent pointing back at the same table (a self-loop cycle). +type Category @table @export { + id: ID @primaryKey + name: String + parentId: ID @indexed + parent: Category @relationship(from: parentId) @enumerable +} + +# Unconstrained (`Any`) @computed whose resolver (resources.js) returns a live entity. The static edge +# graph can't see this, so `linked` forces the guarded serialization path; two Nodes linking each other +# must not overflow. This is the "new exposure" the cross-model review flagged for computed-scalar tables. +type Node @table @export { + id: ID @primaryKey + linkId: ID + linked: Any @computed +} diff --git a/resources/Table.ts b/resources/Table.ts index e0f4a78465..807d19cc91 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -197,6 +197,46 @@ type ResidencyDefinition = number | string[] | void; * Instances of the returned class are Resource instances, intended to provide a consistent view or transaction of the table * @param options */ +// Ambient, path-scoped cycle guard for the enumerable-struct `toJSON` serialization path. A record on +// a cyclically-enumerable table can (transitively) reference itself, which would recurse forever through +// JSON.stringify. The struct getters resolve related records by id — and without a shared cache each +// resolution decodes a fresh instance — so object-identity tracking is defeated; we key on (tableId, id). +// Only cyclic tables allocate/consult this map; acyclic-enumerable tables keep a guard-free fast path. +// Keyed by table class → set of primary keys currently on the serialization path (avoids any tableId +// stringification/collision concern). +let structSerializationVisited: Map> | null = null; +// Fully resolve a value to plain JSON within a guarded serialization, so no live struct escapes back to +// native JSON.stringify (which would call its toJSON *after* our path-scoped unwind and miss the cycle). +function resolveStructForJSON(value: any): any { + if (value == null || typeof value !== 'object') return value; + if (Array.isArray(value)) return value.map(resolveStructForJSON); + // A struct/Date resolves through its own toJSON (a struct's re-enters the guard under the shared path set). + if (typeof value.toJSON === 'function') return value.toJSON(); + // Plain object: recurse its own values so a struct embedded in a computed's object return can't escape + // the guard and re-trigger unbounded recursion in native JSON.stringify after we unwind. + const out = {}; + for (const key of Object.keys(value)) out[key] = resolveStructForJSON(value[key]); + return out; +} +// Is `start` reachable from itself through @enumerable table-typed edges? (Includes self-loops, e.g. a +// tree table with an enumerable parent/children relationship.) Walks the per-table `enumerableRelationDefs` +// graph, resolving each definition's `.tableClass` here — this runs lazily on first serialization, by which +// point every table class is assigned (so self/forward refs whose tableClass was unset at collection time +// resolve correctly). +function detectCyclicEnumerable(start: any): boolean { + const queue = start.enumerableRelationDefs ? [...start.enumerableRelationDefs] : []; + const seen = new Set(); + while (queue.length) { + const target = queue.pop()?.tableClass; + if (!target) continue; + if (target === start) return true; + if (seen.has(target)) continue; + seen.add(target); + if (target.enumerableRelationDefs) for (const def of target.enumerableRelationDefs) queue.push(def); + } + return false; +} + // #section: setup-and-factory export function makeTable(options) { const { @@ -254,6 +294,19 @@ export function makeTable(options) { let expirationWarningChecked = false; let propertyResolvers: any; let hasRelationships = false; + // Attribute names surfaced by the struct `toJSON` on the default (no-select) read: everything that is + // @enumerable, PLUS @computed attributes whose declared type is NOT a table type (scalars/objects/arrays + // that don't resolve to another entity — harper#1484). Table-typed computed attributes and non-enumerable + // relationships stay lazy, preserving the edge/cycle guard. `enumerableRelationDefs` holds the type + // definitions reached by an @enumerable *table-typed* attribute — the edges that can form a cycle. We store + // the definition (set early, during connectPropertyType) rather than its `.tableClass` (assigned later, so + // unset for self/forward refs at collection time); `.tableClass` is resolved lazily at detection. + let enumerableAttributeNames: string[] = []; + const enumerableRelationDefs = new Set(); + // True when a surfaced @computed attribute has an unconstrained type (`Any`/none) — its runtime value + // could be a live entity that cycles, which the static edge graph can't detect, so such a table takes + // the guarded serialization path rather than the raw fast path. + let hasUnconstrainedComputed = false; let runningRecordExpiration: boolean; const isRocksDB = primaryStore instanceof RocksDatabase; type BigInt64ArrayAndMaxSafeId = BigInt64Array & { maxSafeId: number }; @@ -295,6 +348,64 @@ export function makeTable(options) { return this.addTo(property, -value); } } + // Install the struct `toJSON` for a table that has @enumerable getters. JSON.stringify only walks own + // enumerable props (it skips inherited getters), so without this the enumerable getters never appear. + // The bounded enumeration (own keys + the known enumerable names) replaces the old whole-prototype-chain + // `for..in`, which cost O(inherited enumerables) per record. On a cyclically-enumerable table it also + // applies the path-scoped cycle guard; acyclic tables keep the cheap raw-value fast path. + function installEnumerableToJSON(structPrototype: any, tableClass: any, hasUnconstrainedComputed: boolean) { + const enumNames = enumerableAttributeNames; + let isCyclic: boolean | undefined; // lazily resolved on first serialization (once all tables loaded) + Object.defineProperty(structPrototype, 'toJSON', { + configurable: true, + value() { + if (isCyclic === undefined) { + isCyclic = detectCyclicEnumerable(tableClass); + if (isCyclic && getWorkerIndex() === 0) + harperLogger.warn?.( + `Table "${tableName}" has cyclically-enumerable relationships; cyclic references will be serialized as { ${primaryKey} } reference stubs. Consider removing @enumerable from one side of the cycle.` + ); + } + // The fast path leaves surfaced values raw for native JSON.stringify to recurse — safe only when + // nothing surfaced can be a live entity that cycles. Statically-cyclic tables are excluded, and so + // are tables with an unconstrained (`Any`/untyped) @computed, whose runtime return could be a + // cyclic struct the static edge graph can't see; those take the guarded path so the id-keyed + // guard + resolveStructForJSON catch any runtime cycle. + if (structSerializationVisited == null && !isCyclic && !hasUnconstrainedComputed) { + // fast path: bounded copy, values left raw (matches the previous for..in output). `name in json` + // treats an own stored key (already copied above) as taking precedence over its getter. + const json = {}; + for (const key of Object.keys(this)) json[key] = this[key]; + for (const name of enumNames) if (!(name in json)) json[name] = this[name]; + return json; + } + // guarded path: track (tableClass, id) on the current serialization path and fully resolve + // nested structs so none escape back to native stringify after we unwind. + const isTop = structSerializationVisited == null; + if (isTop) structSerializationVisited = new Map(); + const id = this[primaryKey]; + // Composite/array primary keys decode to a fresh array each read, so identity-based membership + // would miss the same logical record; normalize to a stable string key. + const idKey = Array.isArray(id) ? JSON.stringify(id) : id; + let ids: Set | undefined; + if (id != null) { + ids = structSerializationVisited.get(tableClass); + if (!ids) structSerializationVisited.set(tableClass, (ids = new Set())); + if (ids.has(idKey)) return { [primaryKey]: id }; // already on the path -> reference stub + ids.add(idKey); + } + try { + const json = {}; + for (const key of Object.keys(this)) json[key] = resolveStructForJSON(this[key]); + for (const name of enumNames) if (!(name in json)) json[name] = resolveStructForJSON(this[name]); + return json; + } finally { + if (ids && id != null) ids.delete(idKey); + if (isTop) structSerializationVisited = null; + } + }, + }); + } class TableResource extends Resource { #record: any; // the stored/frozen record from the database and stored in the cache (should not be modified directly) #changes: any; // the changes to the record that have been made (should not be modified directly) @@ -326,6 +437,7 @@ export function makeTable(options) { static createdTimeProperty = createdTimeProperty; static updatedTimeProperty = updatedTimeProperty; static propertyResolvers; + static enumerableRelationDefs; static userResolvers = {}; // `@embed` hook registry. `userSetEmbedders` records names set explicitly via // `setEmbedAttribute` so a schema reload refreshes defaults without clobbering them. @@ -4303,6 +4415,11 @@ export function makeTable(options) { } assignTrackedAccessors(this, this); assignTrackedAccessors(Updatable, this, true); + // updatedAttributes() re-runs on every schema reload, so rebuild these from scratch rather than + // accumulating stale/duplicate entries across reloads. + enumerableAttributeNames = []; + enumerableRelationDefs.clear(); + hasUnconstrainedComputed = false; for (const attribute of attributes) { const name = attribute.name; if (attribute.resolve) { @@ -4316,21 +4433,25 @@ export function makeTable(options) { configurable: true, enumerable: attribute.enumerable, }); - if (attribute.enumerable && !primaryStore.encoder.structPrototype.toJSON) { - Object.defineProperty(primaryStore.encoder.structPrototype, 'toJSON', { - configurable: true, - value() { - const json = {}; - for (const key in this) { - // copy all enumerable properties, including from prototype - json[key] = this[key]; - } - return json; - }, - }); - } + // The type definition (set early) marks a table-typed attribute; its `.tableClass` may not be + // assigned yet for self/forward refs, so key table-typedness off the definition, not tableClass. + const relationDef = attribute.definition || attribute.elements?.definition; + // Surface on the default read when @enumerable, or when a @computed attribute is NOT + // table-typed (harper#1484 — computed scalars). Table-typed computeds stay lazy. + if (attribute.enumerable || (attribute.computed && !relationDef)) enumerableAttributeNames.push(name); + // Only @enumerable *table-typed* attributes create a serialization edge that can cycle. + if (attribute.enumerable && relationDef) enumerableRelationDefs.add(relationDef); + // A surfaced computed with an unconstrained type could return a live entity at runtime. + if (attribute.computed && !relationDef && (attribute.type == null || attribute.type === 'Any')) + hasUnconstrainedComputed = true; } } + this.enumerableRelationDefs = enumerableRelationDefs; + // Re-install each reload so the toJSON closure captures the rebuilt name list; if a reload + // removed all enumerable/computed-scalar attributes, drop the now-unneeded toJSON. + if (enumerableAttributeNames.length > 0) + installEnumerableToJSON(primaryStore.encoder.structPrototype, this, hasUnconstrainedComputed); + else if (primaryStore.encoder.structPrototype.toJSON) delete primaryStore.encoder.structPrototype.toJSON; } // #section: computed-history static setComputedAttribute(attribute_name, resolver) { From f96b650597a7d29c6a823ae2ab72c5102b3f7162 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Sun, 5 Jul 2026 17:51:04 -0600 Subject: [PATCH 2/5] fix(serialization): harden cyclic-guard state setup + BigInt-safe id key (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review on #1601: - Move the guarded-path state setup (Map allocation, primaryKey getter, id-key normalization) inside the try so a throw can never leak structSerializationVisited as a non-null Map for the rest of the thread's serializations (would falsely treat later top-level reads as nested). Track membership with an `added` flag. - Composite/array primary keys containing a BigInt no longer crash JSON.stringify — normalize via a bigint->string replacer. - Tests: optional chaining on response-body assertions (A1/B1/B2/B3) so a non-JSON/empty response can't mask the real server error. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../computed-and-cyclic-serialization.test.ts | 16 ++++----- resources/Table.ts | 34 ++++++++++++------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/integrationTests/resources/computed-and-cyclic-serialization.test.ts b/integrationTests/resources/computed-and-cyclic-serialization.test.ts index 87cd24d221..cc4d7f0313 100644 --- a/integrationTests/resources/computed-and-cyclic-serialization.test.ts +++ b/integrationTests/resources/computed-and-cyclic-serialization.test.ts @@ -69,8 +69,8 @@ suite( test('A1: default GET includes @computed scalar (displayName)', async () => { const { status, body } = await rest('GET', '/Author/1'); strictEqual(status, 200); - strictEqual(body.name, 'Ada'); - strictEqual(body.displayName, 'Ada', 'computed scalar must be surfaced without an explicit select'); + strictEqual(body?.name, 'Ada'); + strictEqual(body?.displayName, 'Ada', 'computed scalar must be surfaced without an explicit select'); }); // ---- Fix B: cyclic @enumerable serialization does not overflow ---- @@ -79,23 +79,23 @@ suite( const { status, body } = await rest('GET', '/Author/1'); strictEqual(status, 200, 'must not 500/overflow on a cyclic-enumerable read'); // Author.books is enumerable -> each Book.author is enumerable -> back to this Author = cycle. - const book = Array.isArray(body.books) ? body.books[0] : undefined; + const book = Array.isArray(body?.books) ? body.books[0] : undefined; ok(book, 'enumerable books should be present in serialization'); - deepStrictEqual(book.author, { id: '1' }, 'cyclic back-edge collapses to a { id } reference stub'); + deepStrictEqual(book?.author, { id: '1' }, 'cyclic back-edge collapses to a { id } reference stub'); }); test('B2: self-referential tree (Category.parent) does not overflow', async () => { const { status, body } = await rest('GET', '/Category/1'); strictEqual(status, 200, 'self-loop must not overflow'); - strictEqual(body.parent?.id, '2'); - deepStrictEqual(body.parent.parent, { id: '1' }, 'tree cycle collapses to a reference stub'); + strictEqual(body?.parent?.id, '2'); + deepStrictEqual(body?.parent?.parent, { id: '1' }, 'tree cycle collapses to a reference stub'); }); test('B3: unconstrained @computed returning a live entity cycle does not overflow', async () => { const { status, body } = await rest('GET', '/Node/A'); strictEqual(status, 200, 'untyped-computed runtime cycle must not overflow (guarded, not fast path)'); - strictEqual(body.linked?.id, 'B', 'the computed live entity is surfaced'); - deepStrictEqual(body.linked.linked, { id: 'A' }, 'runtime cycle collapses to a reference stub'); + strictEqual(body?.linked?.id, 'B', 'the computed live entity is surfaced'); + deepStrictEqual(body?.linked?.linked, { id: 'A' }, 'runtime cycle collapses to a reference stub'); }); } ); diff --git a/resources/Table.ts b/resources/Table.ts index 807d19cc91..7ba4654b43 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -380,27 +380,35 @@ export function makeTable(options) { return json; } // guarded path: track (tableClass, id) on the current serialization path and fully resolve - // nested structs so none escape back to native stringify after we unwind. + // nested structs so none escape back to native stringify after we unwind. All state setup lives + // inside the try so a throw from the primaryKey getter or the id-key normalization can never leak + // structSerializationVisited to a non-null Map for the rest of the thread's serializations. const isTop = structSerializationVisited == null; - if (isTop) structSerializationVisited = new Map(); - const id = this[primaryKey]; - // Composite/array primary keys decode to a fresh array each read, so identity-based membership - // would miss the same logical record; normalize to a stable string key. - const idKey = Array.isArray(id) ? JSON.stringify(id) : id; let ids: Set | undefined; - if (id != null) { - ids = structSerializationVisited.get(tableClass); - if (!ids) structSerializationVisited.set(tableClass, (ids = new Set())); - if (ids.has(idKey)) return { [primaryKey]: id }; // already on the path -> reference stub - ids.add(idKey); - } + let idKey: any; + let added = false; try { + if (isTop) structSerializationVisited = new Map(); + const id = this[primaryKey]; + // Composite/array primary keys decode to a fresh array each read, so identity-based membership + // would miss the same logical record; normalize to a stable string key. The replacer keeps a + // BigInt component from throwing (JSON.stringify can't serialize BigInt natively). + idKey = Array.isArray(id) + ? JSON.stringify(id, (_, v) => (typeof v === 'bigint' ? v.toString() : v)) + : id; + if (id != null) { + ids = structSerializationVisited!.get(tableClass); + if (!ids) structSerializationVisited!.set(tableClass, (ids = new Set())); + if (ids.has(idKey)) return { [primaryKey]: id }; // already on the path -> reference stub + ids.add(idKey); + added = true; + } const json = {}; for (const key of Object.keys(this)) json[key] = resolveStructForJSON(this[key]); for (const name of enumNames) if (!(name in json)) json[name] = resolveStructForJSON(this[name]); return json; } finally { - if (ids && id != null) ids.delete(idKey); + if (added) ids!.delete(idKey); if (isTop) structSerializationVisited = null; } }, From dd741331d31062da01fede710c60184fd1226528 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 8 Jul 2026 18:07:42 -0600 Subject: [PATCH 3/5] fix(serialization): guard object-typed PKs + all surfaced computeds in cyclic serialization (#1484) Address PR #1601 review (cb1kenobi): - Normalize any non-primitive primary key (object/Date/Bytes, not just arrays) to a stable string key so the id-keyed cycle guard collapses object-PK cycles instead of overflowing. - Route ANY surfaced non-table @computed through the guarded serialization path, not only Any/untyped ones: a resolver can return a live cyclic entity regardless of its declared scalar type. Renamed hasUnconstrainedComputed -> hasSurfacedComputed. - Add integration test B4 (typed String @computed returning a live entity cycle) which overflowed on the fast path before this change. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../computed-and-cyclic-serialization.test.ts | 13 ++++++ .../resources.js | 7 +++ .../schema.graphql | 10 +++++ resources/Table.ts | 44 ++++++++++--------- 4 files changed, 54 insertions(+), 20 deletions(-) diff --git a/integrationTests/resources/computed-and-cyclic-serialization.test.ts b/integrationTests/resources/computed-and-cyclic-serialization.test.ts index cc4d7f0313..53363fe917 100644 --- a/integrationTests/resources/computed-and-cyclic-serialization.test.ts +++ b/integrationTests/resources/computed-and-cyclic-serialization.test.ts @@ -58,6 +58,9 @@ suite( // Node link cycle via an unconstrained (`Any`) @computed resolver: A -> B -> A await rest('PUT', '/Node/A', { id: 'A', linkId: 'B' }); await rest('PUT', '/Node/B', { id: 'B', linkId: 'A' }); + // Ref link cycle via a *typed-scalar* (`String`) @computed resolver that returns a live entity: X -> Y -> X + await rest('PUT', '/Ref/X', { id: 'X', linkId: 'Y' }); + await rest('PUT', '/Ref/Y', { id: 'Y', linkId: 'X' }); }); after(async () => { @@ -97,5 +100,15 @@ suite( strictEqual(body?.linked?.id, 'B', 'the computed live entity is surfaced'); deepStrictEqual(body?.linked?.linked, { id: 'A' }, 'runtime cycle collapses to a reference stub'); }); + + test('B4: typed-scalar @computed returning a live entity cycle does not overflow (guarded path)', async () => { + // Regression for the review follow-up: a `String`-typed @computed whose resolver returns a live + // struct took the raw fast path before the fix (only `Any`/untyped computeds were guarded) and + // overflowed. It must now route through the guarded path regardless of the declared type. + const { status, body } = await rest('GET', '/Ref/X'); + strictEqual(status, 200, 'a typed-scalar computed returning a live struct must be guarded, not fast-path'); + strictEqual(body?.linked?.id, 'Y', 'the computed live entity is surfaced'); + deepStrictEqual(body?.linked?.linked, { id: 'X' }, 'runtime cycle collapses to a reference stub'); + }); } ); diff --git a/integrationTests/resources/computed-and-cyclic-serialization/resources.js b/integrationTests/resources/computed-and-cyclic-serialization/resources.js index 561b8a7f2a..22192031d6 100644 --- a/integrationTests/resources/computed-and-cyclic-serialization/resources.js +++ b/integrationTests/resources/computed-and-cyclic-serialization/resources.js @@ -7,3 +7,10 @@ tables.Node.setComputedAttribute('linked', (record) => record.linkId != null ? tables.Node.primaryStore.getSync(record.linkId) : undefined ); + +// Review follow-up: `Ref.linked` is declared `String` but the resolver returns a live entity (another +// Ref). The declared scalar type made this take the raw fast path before the fix; it must now be guarded +// so two Refs linking each other serialize as reference stubs rather than overflowing. +tables.Ref.setComputedAttribute('linked', (record) => + record.linkId != null ? tables.Ref.primaryStore.getSync(record.linkId) : undefined +); diff --git a/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql b/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql index 03e67eff09..0a80685188 100644 --- a/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql +++ b/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql @@ -37,3 +37,13 @@ type Node @table @export { linkId: ID linked: Any @computed } + +# Review follow-up (cb1kenobi): a @computed declared with a concrete SCALAR type whose resolver +# nonetheless returns a live entity. JS can't enforce the declared return type, so before the fix this +# took the raw fast path (only `Any`/untyped computeds were guarded) and overflowed on a runtime cycle. +# Any surfaced non-table @computed now routes through the guarded path. +type Ref @table @export { + id: ID @primaryKey + linkId: ID + linked: String @computed +} diff --git a/resources/Table.ts b/resources/Table.ts index 7ba4654b43..80a88ed3f2 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -303,10 +303,10 @@ export function makeTable(options) { // unset for self/forward refs at collection time); `.tableClass` is resolved lazily at detection. let enumerableAttributeNames: string[] = []; const enumerableRelationDefs = new Set(); - // True when a surfaced @computed attribute has an unconstrained type (`Any`/none) — its runtime value - // could be a live entity that cycles, which the static edge graph can't detect, so such a table takes - // the guarded serialization path rather than the raw fast path. - let hasUnconstrainedComputed = false; + // True when the table surfaces any non-table @computed attribute. A resolver can return a live (possibly + // cyclic) entity at runtime regardless of its declared scalar type, and the static edge graph can't see + // it, so such a table takes the guarded serialization path rather than the raw fast path. + let hasSurfacedComputed = false; let runningRecordExpiration: boolean; const isRocksDB = primaryStore instanceof RocksDatabase; type BigInt64ArrayAndMaxSafeId = BigInt64Array & { maxSafeId: number }; @@ -353,7 +353,7 @@ export function makeTable(options) { // The bounded enumeration (own keys + the known enumerable names) replaces the old whole-prototype-chain // `for..in`, which cost O(inherited enumerables) per record. On a cyclically-enumerable table it also // applies the path-scoped cycle guard; acyclic tables keep the cheap raw-value fast path. - function installEnumerableToJSON(structPrototype: any, tableClass: any, hasUnconstrainedComputed: boolean) { + function installEnumerableToJSON(structPrototype: any, tableClass: any, hasSurfacedComputed: boolean) { const enumNames = enumerableAttributeNames; let isCyclic: boolean | undefined; // lazily resolved on first serialization (once all tables loaded) Object.defineProperty(structPrototype, 'toJSON', { @@ -368,10 +368,10 @@ export function makeTable(options) { } // The fast path leaves surfaced values raw for native JSON.stringify to recurse — safe only when // nothing surfaced can be a live entity that cycles. Statically-cyclic tables are excluded, and so - // are tables with an unconstrained (`Any`/untyped) @computed, whose runtime return could be a - // cyclic struct the static edge graph can't see; those take the guarded path so the id-keyed - // guard + resolveStructForJSON catch any runtime cycle. - if (structSerializationVisited == null && !isCyclic && !hasUnconstrainedComputed) { + // are tables surfacing any non-table @computed: a resolver's runtime return could be a cyclic + // struct the static edge graph can't see, whatever its declared type. Those take the guarded path + // so the id-keyed guard + resolveStructForJSON catch any runtime cycle. + if (structSerializationVisited == null && !isCyclic && !hasSurfacedComputed) { // fast path: bounded copy, values left raw (matches the previous for..in output). `name in json` // treats an own stored key (already copied above) as taking precedence over its getter. const json = {}; @@ -390,12 +390,15 @@ export function makeTable(options) { try { if (isTop) structSerializationVisited = new Map(); const id = this[primaryKey]; - // Composite/array primary keys decode to a fresh array each read, so identity-based membership - // would miss the same logical record; normalize to a stable string key. The replacer keeps a - // BigInt component from throwing (JSON.stringify can't serialize BigInt natively). - idKey = Array.isArray(id) - ? JSON.stringify(id, (_, v) => (typeof v === 'bigint' ? v.toString() : v)) - : id; + // Composite/array PKs and object-typed single PKs (Bytes/Uint8Array/Date/object) decode to a + // fresh instance each read, so identity-based membership would miss the same logical record; + // normalize any non-primitive id to a stable string key. The replacer keeps a BigInt component + // from throwing (JSON.stringify can't serialize BigInt natively); a primitive BigInt PK stays + // raw (Set membership is by value). + idKey = + id !== null && typeof id === 'object' + ? JSON.stringify(id, (_, v) => (typeof v === 'bigint' ? v.toString() : v)) + : id; if (id != null) { ids = structSerializationVisited!.get(tableClass); if (!ids) structSerializationVisited!.set(tableClass, (ids = new Set())); @@ -4427,7 +4430,7 @@ export function makeTable(options) { // accumulating stale/duplicate entries across reloads. enumerableAttributeNames = []; enumerableRelationDefs.clear(); - hasUnconstrainedComputed = false; + hasSurfacedComputed = false; for (const attribute of attributes) { const name = attribute.name; if (attribute.resolve) { @@ -4449,16 +4452,17 @@ export function makeTable(options) { if (attribute.enumerable || (attribute.computed && !relationDef)) enumerableAttributeNames.push(name); // Only @enumerable *table-typed* attributes create a serialization edge that can cycle. if (attribute.enumerable && relationDef) enumerableRelationDefs.add(relationDef); - // A surfaced computed with an unconstrained type could return a live entity at runtime. - if (attribute.computed && !relationDef && (attribute.type == null || attribute.type === 'Any')) - hasUnconstrainedComputed = true; + // Any surfaced non-table @computed can return a live (possibly cyclic) entity at runtime, + // regardless of its declared scalar type — the static edge graph can't see it — so route it + // through the guarded serialization path. + if (attribute.computed && !relationDef) hasSurfacedComputed = true; } } this.enumerableRelationDefs = enumerableRelationDefs; // Re-install each reload so the toJSON closure captures the rebuilt name list; if a reload // removed all enumerable/computed-scalar attributes, drop the now-unneeded toJSON. if (enumerableAttributeNames.length > 0) - installEnumerableToJSON(primaryStore.encoder.structPrototype, this, hasUnconstrainedComputed); + installEnumerableToJSON(primaryStore.encoder.structPrototype, this, hasSurfacedComputed); else if (primaryStore.encoder.structPrototype.toJSON) delete primaryStore.encoder.structPrototype.toJSON; } // #section: computed-history From 29e7f60bcd9fe877ee4adc034b71968d95f042f2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 10 Jul 2026 15:13:34 -0600 Subject: [PATCH 4/5] fix(lint): use node:assert not node:assert/strict in computed-and-cyclic-serialization test --- .../resources/computed-and-cyclic-serialization.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrationTests/resources/computed-and-cyclic-serialization.test.ts b/integrationTests/resources/computed-and-cyclic-serialization.test.ts index 53363fe917..4da4c6c717 100644 --- a/integrationTests/resources/computed-and-cyclic-serialization.test.ts +++ b/integrationTests/resources/computed-and-cyclic-serialization.test.ts @@ -11,7 +11,7 @@ * npm run test:integration -- "integrationTests/resources/computed-and-cyclic-serialization.test.ts" */ import { suite, test, before, after } from 'node:test'; -import { ok, strictEqual, deepStrictEqual } from 'node:assert/strict'; +import { ok, strictEqual, deepStrictEqual } from 'node:assert'; import { resolve } from 'node:path'; import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; // @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine From 620926948d18d34b9388ebe4aa514d387ec1f6ce Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 10 Jul 2026 15:41:28 -0600 Subject: [PATCH 5/5] fix(test): update pre-existing tests for harper#1484 computed-scalar default surfacing basicREST-test.mjs asserted ageInMonths (a non-enumerable @computed scalar) stays undefined on a default read -- that was exactly the pre-fix behavior this PR's Fix A intentionally changes (computed scalars, enumerable or not, now surface by default; only table-typed computeds/relationships stay lazy). cache-test.mjs round-tripped a GET response straight into a PUT, stripping only nameTitle. Now that ageInMonths also surfaces by default, it rode along into the PUT body and got rejected (400) as a write to a computed attribute. --- unitTests/apiTests/basicREST-test.mjs | 2 +- unitTests/apiTests/cache-test.mjs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/unitTests/apiTests/basicREST-test.mjs b/unitTests/apiTests/basicREST-test.mjs index 99017b4b06..1f3e35cecd 100644 --- a/unitTests/apiTests/basicREST-test.mjs +++ b/unitTests/apiTests/basicREST-test.mjs @@ -240,7 +240,7 @@ describe('test REST calls', () => { assert.equal(response.status, 200); assert.equal(response.data.length, 5); assert.equal(response.data[4].age, 24); - assert.equal(response.data[4].ageInMonths, undefined); // computed property shouldn't be returned by default + assert.equal(response.data[4].ageInMonths, 288); // harper#1484: computed scalars now surface on default reads assert.equal(response.data[4].nameTitle, 'name4 title4'); // enumerable computed property should be returned by // default }); diff --git a/unitTests/apiTests/cache-test.mjs b/unitTests/apiTests/cache-test.mjs index 2c8bd24763..8eff9dc5c6 100644 --- a/unitTests/apiTests/cache-test.mjs +++ b/unitTests/apiTests/cache-test.mjs @@ -32,6 +32,7 @@ describe('test REST calls with cache table', () => { let data = response.data; data.name = 'name change'; delete data.nameTitle; // don't send a computed property + delete data.ageInMonths; // harper#1484: computed scalars now also surface on default reads response = await axios.put(`${baseUrl}/FourProp/3`, data); assert.equal(response.status, 204); await delay(20);