Preserve compound field data backed by unexported field defs in server-side writes#5539
Conversation
The definition-driven file serializer walks a card's stored Definition
to normalize each field on every server-side instance write (POST,
PATCH, /_atomic). The definition store only carries exported classes,
so a compound field typed with an unexported FieldDef has a
fieldOf/ancestorOf code ref whose child definition can't be looked up.
The serializer treated that as "no fields": containsMany entries
serialized to {}, contains values were dropped, and relationships
pathing through such a field were dropped — any edit or sync of such a
card silently wiped its data on disk. Unresolvable-child values now
pass through verbatim.
The PATCH handler also merged patches over the index's pristine_doc.
The index is downstream of the file and can lag it, so a merge over
stale index state silently reverts fields the index hasn't caught up
on. The merge base is now the stored source file, read inside the same
per-realm write lock as the write.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-compound-field-json-object-data-gets-wiped-out
With the stored file as the merge base, a PATCH writes only the fields the file and the patch specify — the index's schema-defaulted nulls are no longer materialized into the file. File-shape expectations drop the materialized cardInfo, and the no-op tests prime once so their assertions measure the canonical-form steady state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1b89704b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates server-side write flows to prevent data loss when serializing compound fields whose child Definition cannot be resolved (notably when the declared type is an unexported FieldDef), and makes PATCH merging use the stored source file as the merge base rather than potentially-stale index state.
Changes:
file-serializerpreserves compound attribute values verbatim when the child definition can’t be resolved, and preserves relationships whose dotted path crosses such a field.- PATCH merge base is switched from index state to the on-disk JSON source (read under the realm write lock), with clearer errors for non-card stored files.
- Adds realm-server tests covering PATCH and
/_atomic?waitForIndex=truebehavior for unresolvable compound field definitions and index lag scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/runtime-common/realm.ts | PATCH reads/parses the stored instance file under the write lock and merges against it instead of index-derived state. |
| packages/runtime-common/file-serializer.ts | Pass-through behavior for unresolvable compound subtrees; relationship preservation for dotted paths crossing such fields. |
| packages/realm-server/tests/card-endpoints-test.ts | Adds PATCH tests asserting compound-field preservation and merge-base correctness when index lags. |
| packages/realm-server/tests/atomic-endpoints-test.ts | Adds /_atomic?waitForIndex=true test asserting compound-field preservation when writing raw source instances. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
normalizeRelationship only derives links from a single-resource data object, so a JSON:API to-many `data: [...]` relationship under an unresolvable compound field was preserved as an empty object and its targets lost. Expand it into the indexed .N keys to-many relationships are stored under, one entry per resource identifier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review focused on the two places a wrong branch silently destroys user data: the serializer's new preserve-vs-drop boundary, and the PATCH merge-base move — both sit directly on the write path.
Bottom line: no blocking issues. Every mechanism claim in the description checks out against the code, and the affected suites pass locally against this branch: card-endpoints-test.ts + atomic-endpoints-test.ts 79/79 green, plus a fresh 60/60 card-endpoints-test.ts run at head 78f80435 covering the to-many expansion and its new assertion.
What lands right:
- The tri-state lives at the correct altitude. Distinguishing "path names nothing the card declares" (drop) from "path crosses a type the definition store cannot hold" (preserve) in
resolveDottedFieldDefis the minimal precise fix; the tempting wrong version — blanket preservation of unknown relationship keys — would have turned typo'd keys into permanent file residue. I verified the boundary is exact (inline confirmation): unknown keys still drop, and resolved-but-missing types still fail the write loudly becauselookupDefinitionthrows rather than returning undefined. - The merge-base move keeps the critical-section shape and simplifies its guarantee: instead of "wait for indexing inside the lock so the next waiter's index read is fresh", the next waiter now reads the same bytes the lock ordered. Write and index flush both complete inside the lock, so the concurrent-PATCH lost-update guarantee holds (inline confirmation has the full trace).
- Sparse files converge PATCH with POST and
/_atomicon stored shape, and GET responses are untouched since they serialize from the index. The tests' priming pattern honestly documents the one-time canonicalization rewrite a hand-authored file gets on first PATCH.
Recommendations (detail in the inline threads):
- Mirror the to-many
data: [...]expansion into the declaredlinksToManyfan-out — the resolvable branch still empties the shape the unresolvable branch now preserves (pre-existing, non-blocking; file-serializer thread). - Pin the corrupt-file
systemErrorpath with a test — it's the only new behavior a regression could silently undo (card-endpoints-test thread). - Delete the now-dead
original.metafallbacks inpatchCardInstance(nit; realm.ts thread).
Adjacent, out of scope: containsMany per-item polymorphic overrides cannot influence relationship-path resolution — parseRelationshipKey strips the index (its own comment calls the transform lossy) and isMeta rejects the array-shaped meta.fields entry — so a relationship declared only on an item's polymorphic subtype under a resolvable declared type still drops silently. This PR narrows that family of loss (unexported declared types now preserve), but the lossy key-cleaning root remains; noted for whoever picks up the schema-driven key parsing the existing comment already asks for.
| // Returns `undefined` when a segment does not exist on its definition | ||
| // (or descends past a primitive) — the path names nothing the card | ||
| // declares. Returns `'unresolvable'` when a segment exists but its | ||
| // child definition can't be resolved (an unexported class), so the | ||
| // caller can preserve the value rather than treat it as unknown. |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation — the tri-state boundary is exactly right, plus one contract it silently depends on.
I traced every way resolveChildDef can hand this walk an undefined child (which this function then reports as 'unresolvable'):
- A genuinely unknown key never reaches it. A missing segment fails
getImmediateFieldDefand returnsundefined— the caller still drops it, same as before. - An unexported class is the only producer. Its
fieldOrCardis afieldOf/ancestorOfref, and a polymorphicmeta.fieldsoverride whoseadoptsFromis itself such a ref behaves identically (codeRefWithAbsoluteIdentifierpasses type-refs through, soisResolvedCodeReffails) — both take the preserve path. - A resolved ref naming a missing type does NOT take this path.
CachingDefinitionLookup.lookupDefinitionthrowsFilterRefersToNonexistentTypeErrorrather than returning undefined (packages/runtime-common/definition-lookup.ts,lookupDefinitionWithContext), so a deleted/renamed module still fails the whole write loudly instead of quietly preserving stale data.
So 'unresolvable' means precisely "the card declares this holder field, but its type is not an export the definition store can hold" — the one case where verbatim preservation is the only non-lossy option.
Two properties worth knowing, neither blocking:
- Segments past the unresolvable hop are unverifiable, so a bogus deeper key (
entries.0.notARealField) is preserved rather than dropped. Right bias — a false preserve costs a stray key; a false drop costs data. - The precision leans on
DefinitionLookup.lookupDefinition's throwing contract (Promise<Definition>, never undefined-for-missing). If an implementation ever returned undefined for a resolved-but-missing type, "type was deleted" would silently reroute into "preserve verbatim". If you want that pinned, a sentence on theDefinitionLookupinterface is the cheap spot.
Class: confirmation; non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] The dependence on the throwing contract is now stated where it lives: resolveChildDef's doc comment spells out that undefined means "ref can't name an export" and that a resolved-but-missing definition throws in lookupDefinition before reaching the preserve path.
| } else if (Array.isArray(value.data)) { | ||
| // JSON:API to-many `data: [...]` form. `normalizeRelationship` | ||
| // only derives links from a single-resource `data`, so expand to | ||
| // the indexed keys to-many relationships are stored under, one | ||
| // entry per resource identifier, before normalizing each. | ||
| value.data.forEach((item, index) => { | ||
| result[`${relationshipKey}.${index}`] = normalizeRelationship({ | ||
| ...(value.meta ? { meta: value.meta } : {}), | ||
| data: item, | ||
| }); | ||
| }); | ||
| } else { | ||
| result[relationshipKey] = normalizeRelationship(value); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] The declared-linksToMany fan-out later in this function still flattens the exact input shape this branch now expands — pre-existing, not introduced here, but the two branches now disagree.
This expansion itself is correct: each data entry goes through normalizeRelationship({ data: item }), whose single-resource branch converts id → links.self, and the results land under the indexed .N keys the file format stores to-many links under. The entries.0.coauthors.0 assertion pins it.
The resolvable twin. Walk the same { data: [...] } value through the declared path below (fieldDefinition.type === 'linksToMany' && Array.isArray(value.data)): normalizeRelationship(value) runs on the whole value first — an array-valued data falls through every branch, delete processedValue.data erases the identifiers, and the fan-out then stamps { links: processedValue.links, meta: … } (i.e. links: undefined) onto every .N key. All targets are lost; and when links is present alongside the array, every entry gets the same single self. Net effect today: a to-many data: [...] under an unexported compound field survives, while the same relationship on a field the definition can resolve is silently emptied.
Reachability. isRelationship accepts array-valued data (its 'type' in data probes don't fire on arrays), and promoteLocalIdsToRemoteIds only rewrites lid entries — so this shape reaches the serializer via /_atomic raw source and hand-built POST/PATCH bodies.
The way out is the block right here: per-item normalizeRelationship({ ...(value.meta ? { meta: value.meta } : {}), data: item }) in the declared fan-out, replacing the shared processedValue.links.
Class: pre-existing bug, made visible by this branch; non-blocking — fine as a follow-up, though it's a ~6-line mirror if you'd rather not ship the two branches disagreeing.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed — the declared-linksToMany fan-out now derives each indexed entry from its own resource identifier via normalizeRelationship, the same expansion the unresolvable branch uses, so a to-many data: [...] with no relationship-level links keeps its targets. Behavior with relationship-level links present is unchanged (each entry still carries them).
| // The merge base is the stored source file, not the index. The | ||
| // index is downstream of the file and can lag it — a backlogged or | ||
| // in-flight index job would hand us a stale `pristine_doc`, and | ||
| // merging the patch over stale state silently reverts every field | ||
| // the index hasn't caught up on. The file is written inside this | ||
| // same write lock, so it is always current. | ||
| let existingFile = await this.readFileAsText(`${localPath}.json`); | ||
| if (!existingFile) { | ||
| return notFound(request, requestContext); | ||
| } | ||
| let original = cloneDeep( | ||
| indexEntry.instance ?? { | ||
| type: 'card', | ||
| meta: { adoptsFrom: patch.meta.adoptsFrom }, | ||
| }, | ||
| ) as CardResource; | ||
| let original: CardResource; | ||
| try { | ||
| let existingDoc = JSON.parse(existingFile.content); | ||
| if (!isCardResource(existingDoc?.data)) { | ||
| throw new Error('stored file is not a card document'); | ||
| } | ||
| original = cloneDeep(existingDoc.data) as CardResource; | ||
| } catch (err: any) { | ||
| return systemError({ | ||
| requestContext, | ||
| message: `cannot apply patch, the existing file for ${instanceURL} is not a valid card document: ${err.message}`, | ||
| additionalError: err, | ||
| id: instanceURL, | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation — the stored-file merge base preserves the concurrent-PATCH guarantee, and the failure triage is complete. What I verified, for the record:
Lost-update safety. withWriteLock (a Postgres advisory lock keyed on the realm URL) serializes read→merge→write across replicas. Inside it, _batchWriteUnlocked awaits both the adapter write and — because this call site omits waitForIndex, taking the default-true branch — the post-write performIndex() before the lock releases. The next waiter's readFileAsText therefore opens the file only after the previous writer finished it; with close-to-open consistency on the shared filesystem, that read sees the committed bytes. File reads were already load-bearing cross-replica (source GETs, _batchWriteUnlocked's own no-op content check), so no new consistency assumption class is introduced.
Path correctness. The PATCH route pattern is '/.+(?<!.json)', so localPath never carries the extension and ${localPath}.json names the right file.
Error triage. isCardResource requires meta with a valid adoptsFrom CodeRef, and its 'fields' in meta probe throws inside this try for meta: null — so unparseable JSON, non-card JSON, and mangled meta all reach the systemError branch. That branch also changes the corrupt-file-with-healthy-index case from "silently clobber the corrupt file with patch-over-index-state" to "500 and leave the bytes for recovery" — the right trade, since whatever the corrupt file held is exactly what a clobber would destroy. A file that exists but was never indexed now merges fine, and the response still comes from the index because the in-lock performIndex() has run by then.
Type guard on disk refs. internalKeyFor resolves the file's relative adoptsFrom against the extensionless instance URL; URL-join drops the last path segment either way, so refs written relative to <instance>.json resolve identically, and trimExecutableExtension makes a fixture's ./person.gts compare equal to a patch's ./person. One tightening worth noting: the old index-error fallback could make the type-change guard vacuous (original's adoptsFrom defaulted to the patch's own); the guard now always compares against the file's real type.
The conditions under which this stops being safe: the adapter write becoming deferred (returning before durability) or the index flush moving outside the lock — either would let the next waiter read stale state.
Class: confirmation; non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Thanks for the trace — nothing further to change here.
| original.meta ??= { adoptsFrom: patch.meta.adoptsFrom }; | ||
| original.meta.adoptsFrom = | ||
| original.meta.adoptsFrom ?? patch.meta.adoptsFrom; |
There was a problem hiding this comment.
[Claude Code 🤖] These two fallbacks are dead now that the merge base is guarded by isCardResource: it returns false unless meta is an object whose adoptsFrom is a valid CodeRef, so by this point original.meta.adoptsFrom always exists — the ??= never fires and the ?? patch.meta.adoptsFrom arm is unreachable. Likewise the originalClone.meta?.adoptsFrom && condition on the type-change guard below is always truthy. Harmless, but they read as though meta-less stored files are supported on this path, which the guard forecloses (those land in the systemError branch). Worth deleting for the next reader.
Class: dead code left behind by the merge-base change; nit, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Removed — isCardResource requires meta with a valid adoptsFrom code ref, so the guard makes both fallbacks unreachable. Only the delete original.meta.lastModified normalization remains.
| test('PATCH merges against the stored file even when the index lags it', async function (assert) { | ||
| let cardFile = join( | ||
| dir.name, | ||
| 'realm_server_1', | ||
| 'test', | ||
| 'log-1.json', | ||
| ); | ||
| // Edit the stored file directly — no realm write, no file | ||
| // watcher in this fixture — so the index still reflects the | ||
| // original content. A PATCH of an unrelated field must keep | ||
| // this edit: the merge base is the stored file, and using the | ||
| // (lagging) index instead would silently revert it. | ||
| let onDisk = readJSONSync(cardFile); | ||
| onDisk.data.attributes.cardInfo = { notes: 'edited on disk' }; | ||
| writeFileSync(cardFile, JSON.stringify(onDisk, null, 2)); |
There was a problem hiding this comment.
[Claude Code 🤖] The one behavioral consequence without a test pin: PATCH against a corrupt stored file must fail rather than overwrite. The systemError branch in patchCardInstance is what prevents an unreadable merge base from degrading into "write just the patch" — but nothing exercises it: the two index-error tests corrupt boxel_index, not the file, and this test writes valid JSON. A regression back to overwrite-on-parse-failure would pass the whole suite.
This module already has the exact pattern needed — one more test that writeFileSyncs garbage (or valid JSON that isn't a card document) into log-1.json, PATCHes, and asserts 500 plus byte-identical file content afterward would pin the contract. The sibling behavior — 404 when the .json file is missing even if the index still has a row — is worth pinning the same way.
Class: test gap for behavior this PR introduces; non-blocking, but recommended while the machinery is fresh.
There was a problem hiding this comment.
[Claude Code 🤖] Added — a garbage-file PATCH test now pins the behavior: 500 response and the stored bytes left untouched.
Preview deploymentsHost Test Results 1 files 1 suites 3h 11m 21s ⏱️ Results for commit 78f8043. For more details on these errors, see this check. Realm Server Test Results 1 files ±0 1 suites ±0 17m 33s ⏱️ -20s Results for commit dce7f34. ± Comparison against earlier commit 78f8043. |
…tests The declared-linksToMany fan-out had the same flattening the unresolvable branch fixed: normalizeRelationship deletes array-valued data without deriving links, so entries with no relationship-level links lost their targets. Derive each indexed entry from its own resource identifier instead. The merge-base fallbacks for a missing meta/adoptsFrom are dead code behind the isCardResource guard, which requires both. A PATCH against a corrupt stored file now has a test pinning the fail-without- overwrite behavior. Host test expectations updated for the stored-file merge base: files keep their author's key order (the index's jsonb round-trip alphabetized them) and stay sparse. The linksToMany-removal test previously patched the same single pet its fixture started with — a semantic no-op that only wrote because the index form differed cosmetically; it now starts with two pets and removes one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Server-side instance writes (POST, PATCH,
/_atomic) re-serialize the card document through the definition-driven file serializer before it lands on disk. The serializer walks the card's storedDefinitionto normalize each field, and for compound (FieldDef-backed) fields it descends into a child definition looked up from the field'sfieldOrCardcode ref.The definition store only holds entries for exported classes. A compound field whose declared type is an unexported FieldDef carries a
fieldOf/ancestorOfcode ref, which can't be resolved to a definition. The serializer treated that unresolvable lookup as "this field has no fields": everycontainsManyentry serialized to{}(array length preserved, attribute data gone), acontainsvalue was dropped outright, and any relationship whose dotted path crosses such a field was dropped. Editing such a card in the UI or pushing it through/_atomicsilently wiped its compound field data on disk.The serializer now passes values through verbatim whenever a compound field's child definition can't be resolved, and preserves relationships whose dotted path crosses such a field. Schema-aware normalization (relativizing inner code refs, stripping computed fields) is skipped for exactly that subtree — an unnormalized code ref is recoverable, dropped field data is not. Fields the definition doesn't declare at all are still dropped, as before.
The PATCH handler had a second, independent path to the same symptom: it built its merge base from the index's
pristine_docrather than the stored file. The index is downstream of the file and can lag it — an in-flight or backlogged index job hands back stale state, and merging a patch over a stale base silently reverts every field the index hasn't caught up on. The merge base is now the stored source file, read inside the same per-realm write lock as the write itself.Behavioral consequences of the merge-base change: a PATCH against an instance whose file exists but isn't indexed yet now succeeds instead of returning 404; a PATCH against a file that isn't parseable as a card document fails with a clear error instead of overwriting the file with just the patch; and the stored file keeps only the fields the file and the patch actually specify — PATCH no longer materializes the index's schema-defaulted nulls (e.g. an untouched
cardInfo) into the file, matching how POST and/_atomicwrites already store documents. GET responses are unaffected since they serialize from the index, which materializes the full field shape.New realm-server tests cover the three surfaces: PATCH preservation of
containsMany/containsvalues and nested relationships backed by an unexported FieldDef, PATCH merging against a file the index hasn't caught up with, and/_atomic?waitForIndex=truepreserving compound entries pushed as raw source.🤖 Generated with Claude Code
https://claude.ai/code/session_01BzRJdC6yxAU8oXY42qn4pW