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..4da4c6c717 --- /dev/null +++ b/integrationTests/resources/computed-and-cyclic-serialization.test.ts @@ -0,0 +1,114 @@ +/** + * 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'; +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' }); + // 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 () => { + 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'); + }); + + 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 new file mode 100644 index 0000000000..22192031d6 --- /dev/null +++ b/integrationTests/resources/computed-and-cyclic-serialization/resources.js @@ -0,0 +1,16 @@ +// 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 +); + +// 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 new file mode 100644 index 0000000000..0a80685188 --- /dev/null +++ b/integrationTests/resources/computed-and-cyclic-serialization/schema.graphql @@ -0,0 +1,49 @@ +# 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 +} + +# 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 e0f4a78465..80a88ed3f2 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 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 }; @@ -295,6 +348,75 @@ 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, hasSurfacedComputed: 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 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 = {}; + 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. 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; + let ids: Set | undefined; + let idKey: any; + let added = false; + try { + if (isTop) structSerializationVisited = new Map(); + const id = this[primaryKey]; + // 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())); + 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 (added) 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 +448,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 +4426,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(); + hasSurfacedComputed = false; for (const attribute of attributes) { const name = attribute.name; if (attribute.resolve) { @@ -4316,21 +4444,26 @@ 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); + // 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, hasSurfacedComputed); + else if (primaryStore.encoder.structPrototype.toJSON) delete primaryStore.encoder.structPrototype.toJSON; } // #section: computed-history static setComputedAttribute(attribute_name, resolver) { 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);