Skip to content

fix(serialization): surface @computed scalars on default reads + guard cyclic @enumerable serialization (#1484)#1601

Draft
kriszyp wants to merge 5 commits into
mainfrom
kris/computed-default-select-1484
Draft

fix(serialization): surface @computed scalars on default reads + guard cyclic @enumerable serialization (#1484)#1601
kriszyp wants to merge 5 commits into
mainfrom
kris/computed-default-select-1484

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 4, 2026

Copy link
Copy Markdown
Member

Closes #1484.

Problem

REST GET silently omits @computed attributes on the default (no-select) read: their getters are installed non-enumerable, and JSON.stringify only serializes own-enumerable properties (it skips inherited getters). Naively flipping computed getters to enumerable is the wrong lever:

  • It installs a toJSON that does for..in over the entire struct prototype chain, eagerly invoking every enumerable getter per record — 3.4×–32× slower, scaling worse with chain depth (benchmarked).
  • Non-enumerability is currently the only thing preventing @enumerable relationship cycles from overflowing the stack — there is no visited-set anywhere. @relationship @enumerable on both sides of a cycle already stack-overflows today.

Fix

Both concerns are unified in the struct toJSON, installed only when a table has an @enumerable attribute or a non-table @computed attribute (plain tables keep the raw-struct fast path and never pay a cost):

  • Bounded enumeration — own keys + a precomputed name list (@enumerable attributes ∪ @computed attributes whose type is not a table type), replacing the for..in. Verified equivalent: msgpackr struct prototypes carry zero enumerable inherited props, so for..in yielded exactly this set.
  • Cycle guard@enumerable table-typed attributes form an edge graph; detectCyclicEnumerable (lazy, memoized, self-loop aware) flags tables that can cycle. Those take a path-scoped, id-keyed guard (Map<tableClass, Set<idKey>>): a record already on the current serialization path collapses to a { [primaryKey]: id } reference stub. resolveStructForJSON fully resolves nested structs so none escape to native JSON.stringify after unwind. Acyclic-enumerable tables keep a guard-free fast path. A one-time warn fires when a cyclic-enumerable schema is detected.

Table-typedness keys off the attribute definition (set early) rather than .tableClass (assigned later — unset for self/forward refs at collection time); tableClass is resolved lazily at first serialization. Composite/array primary keys are normalized (JSON.stringify) for the visited set. Name lists rebuild on schema reload (updatedAttributes() re-runs). Tables with an unconstrained (Any/untyped) @computed take the guarded path, since such a resolver can return a live cyclic entity the static graph can't see.

Note: select(computedField) already returned computed fields correctly — the bug was purely the no-select / * path.

Tests

integrationTests/resources/computed-and-cyclic-serialization.test.ts (real Harper + RocksDB + REST), 4/4:

  • A1 — computed scalar surfaced on default GET.
  • B1 — mutual Author↔Book @enumerable cycle → {id} stub, no overflow.
  • B2 — self-referential Category.parent tree → stub, no overflow.
  • B3 — unconstrained Any @computed returning a live entity (A→B→A) → stub, no overflow (exercises the guarded-path routing).

Cross-model review (Codex + Gemini + Harper domain)

Adjudicated; all findings addressed in-tree before this PR:

  • Blocker (Codex+Gemini): self/forward-ref detection read .tableClass before assignment → fixed (definition-based, lazy resolution).
  • Significant (Codex): composite/array PK broke the id-keyed Set → fixed (normalized key).
  • Significant (Codex+Gemini): resolveStructForJSON didn't recurse plain objects → fixed.
  • Significant (Gemini): schema-reload accumulation of the name lists → fixed (reset in updatedAttributes()).
  • Significant (Codex+Gemini): untyped @computed returning a runtime-cyclic struct was a new exposure on the fast path (a computed-scalar-only table installed no toJSON on main, so it couldn't overflow before) → fixed (routed through the guarded path; covered by test B3).
  • Concurrency of the module-level guard confirmed safe (synchronous toJSON, per-worker, torn down in finally within one frame).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses issue #1484 by surfacing computed scalar attributes on default reads and introducing a cycle guard for cyclic-enumerable serialization to prevent stack overflows. The review feedback points out critical issues in the serialization guard implementation, specifically a potential global state leak on error and a crash when serializing composite primary keys containing BigInt values. It also suggests using optional chaining in test assertions to avoid masking server errors.

Comment thread resources/Table.ts
Comment thread integrationTests/resources/computed-and-cyclic-serialization.test.ts Outdated
@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Both prior findings are resolved: basicREST-test.mjs:243 now expects 288 and cache-test.mjs strips ageInMonths before the PUT (commit 620926948). The new hasSurfacedComputed guard (widened from Any-only to all non-table @computed regardless of declared scalar type) is correct, and B4 covers the typed-scalar-returning-live-entity case.

kriszyp pushed a commit that referenced this pull request Jul 5, 2026
…key (#1484)

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) <noreply@anthropic.com>
Comment thread resources/Table.ts Outdated
Comment thread resources/Table.ts Outdated
Comment thread resources/Table.ts
kriszyp pushed a commit that referenced this pull request Jul 9, 2026
…n 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) <noreply@anthropic.com>
@Ethan-Arrowood

Copy link
Copy Markdown
Member

Code looks right, but the unit failures are this PR's: the basicREST computed-index assertion asserts the pre-fix behavior and needs updating, cache-test's 400 needs a look, and the new test imports node:assert/strict which lint now rejects (#1753). Happy to approve once CI is green.

sent with Claude Fable 5

Kris Zyp and others added 4 commits July 10, 2026 15:13
…d cyclic @enumerable serialization (#1484)

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<tableClass, Set<idKey>>) 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) <noreply@anthropic.com>
…key (#1484)

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) <noreply@anthropic.com>
…n 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) <noreply@anthropic.com>
@kriszyp kriszyp force-pushed the kris/computed-default-select-1484 branch from 367d850 to 29e7f60 Compare July 10, 2026 21:22
…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.
@kriszyp

kriszyp commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

CI was red on 3 things after the rebase — pushed fixes for the two that were real, left the third alone since it's yours to weigh in on:

  1. Its own node:assert/strict lint violation in the new computed-and-cyclic-serialization.test.ts — fixed (house rule is node:assert).
  2. Two pre-existing tests broke against Fix A's own stated intent, not a code bug:
    • basicREST-test.mjs asserted ageInMonths (a non-enumerable @computed @indexed scalar) stays undefined on a default read — that's exactly the pre-fix behavior this PR's Fix A deliberately changes ("computed scalars... their getters are non-enumerable, so JSON.stringify used to silently drop them... must include"). Updated the assertion to 288 to match.
    • cache-test.mjs's "change source and get" test round-trips a GET response straight into a PUT, stripping only nameTitle before sending it back. Now that ageInMonths also surfaces by default, it rode along and got rejected as a write to a computed attribute (400). Added delete data.ageInMonths alongside the existing strip.

Didn't touch anything else — just wanted CI green so you can review the actual diff without noise. Let me know if you want the ageInMonths default-surfacing behavior itself reconsidered instead of the test updated.

— KrAIs (Claude Sonnet 5)

@kriszyp kriszyp marked this pull request as draft July 10, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REST GET silently omits @computed fields from responses (computed getters are non-enumerable)

3 participants