diff --git a/packages/realm-server/tests/card-endpoints-test.ts b/packages/realm-server/tests/card-endpoints-test.ts index 1ca746eb49c..1bb23114f7e 100644 --- a/packages/realm-server/tests/card-endpoints-test.ts +++ b/packages/realm-server/tests/card-endpoints-test.ts @@ -1815,6 +1815,80 @@ module(basename(import.meta.filename), function () { ); }); + test('the write response omits the transitive link closure the client discards', async function (assert) { + // A create's response is read only for the primary card's id and + // realm-info; the host discards its attributes, relationships and + // `included[]`. So the write path skips the `loadLinks` closure — + // a linked card is NOT inlined into the create response — while a + // subsequent GET, on the read path, still assembles it. + let target = await request + .post('/') + .send({ + data: { + type: 'card', + attributes: { firstName: 'Target' }, + meta: { + adoptsFrom: { module: rri('./friend.gts'), name: 'Friend' }, + }, + }, + } as LooseSingleCardDocument) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(target.status, 201, `HTTP 201: ${target.text}`); + let targetId = (target.body as SingleCardDocument).data.id!; + + let response = await request + .post('/') + .send({ + data: { + type: 'card', + attributes: { firstName: 'Consumer' }, + relationships: { + friend: { links: { self: targetId } }, + }, + meta: { + adoptsFrom: { module: rri('./friend.gts'), name: 'Friend' }, + }, + }, + } as LooseSingleCardDocument) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + response.status, + 201, + `HTTP 201: ${response.text}`, + ); + + let json = response.body as SingleCardDocument; + // Everything the client keeps from a write response is present... + assert.ok(json.data.id, 'the create response carries the new id'); + assert.ok( + json.data.meta.realmInfo, + 'the create response carries realm-info', + ); + assert.ok( + json.data.meta.lastModified, + 'the create response carries lastModified', + ); + // ...and the transitive closure it discards is absent. + assert.strictEqual( + json.included, + undefined, + 'the create response does not inline the linked card into included[]', + ); + + // The read path is unchanged: a GET of the same card still assembles + // the closure, so the linked card comes off the wire in included[]. + let read = await request + .get(`/${json.data.id!.slice(testRealmHref.length)}`) + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual(read.status, 200, `HTTP 200: ${read.text}`); + assert.ok( + (read.body.included ?? []).some( + (r: { id?: string }) => r.id === targetId, + ), + 'the GET response still inlines the linked card into included[]', + ); + }); + test('an echoed serve-time meta.screenshots never persists into the source file', async function (assert) { // The shape a card+json GET stamps — a client that GETs a doc and // POSTs it back to duplicate the card echoes this, and persisting @@ -3273,11 +3347,12 @@ module(basename(import.meta.filename), function () { ); }); - test('PATCH response carries an ETag and writes invalidate the previous one', async function (assert) { - // Capture the pre-patch ETag, mutate the card, and verify the PATCH - // response advertises a *different* ETag for the new state — that's - // the contract that lets the caller cache the post-patch body - // without an extra round-trip GET. + test('PATCH response omits the ETag; the write still advances the GET validator', async function (assert) { + // The write echo omits the link closure a GET assembles, so it must + // NOT carry the GET's validator — a client caching the echo body and + // revalidating with its ETag would be 304'd onto a closure-less + // representation. The write still rotates the validator a GET + // reports, so a follow-up conditional GET behaves correctly. let initialResponse = await request .get('/person-1') .set('Accept', 'application/vnd.card+json'); @@ -3301,16 +3376,9 @@ module(basename(import.meta.filename), function () { .set('Accept', 'application/vnd.card+json'); assert.strictEqual(patchResponse.status, 200, 'PATCH succeeds'); - let patchEtag = patchResponse.get('etag') ?? ''; - assert.ok(patchEtag, 'PATCH response carries an ETag'); - assert.true( - /^"\d+(?:-[0-9a-f]+)?:card-rri"$/.test(patchEtag), - `PATCH ETag matches "(-)?:card-rri" pattern (got ${patchEtag})`, - ); - assert.notStrictEqual( - patchEtag, - originalEtag, - 'PATCH advances the ETag because indexed_at bumps on the rewrite', + assert.notOk( + patchResponse.get('etag'), + 'the write echo carries no validator', ); // Sending the OLD etag against If-None-Match must NOT short-circuit @@ -3324,25 +3392,31 @@ module(basename(import.meta.filename), function () { 200, 'old ETag no longer matches → fresh 200', ); - assert.strictEqual( - staleResponse.get('etag'), - patchEtag, - 'GET reports the new ETag', + let newEtag = staleResponse.get('etag') ?? ''; + assert.ok(newEtag, 'GET reports a validator'); + assert.true( + /^"\d+(?:-[0-9a-f]+)?:card-rri"$/.test(newEtag), + `GET ETag matches "(-)?:card-rri" pattern (got ${newEtag})`, + ); + assert.notStrictEqual( + newEtag, + originalEtag, + 'the write advanced the GET validator because indexed_at bumps on the rewrite', ); - // And the new etag from the PATCH must short-circuit on next GET. + // And the advanced validator must short-circuit on the next GET. let cachedResponse = await request .get('/person-1') .set('Accept', 'application/vnd.card+json') - .set('If-None-Match', patchEtag); + .set('If-None-Match', newEtag); assert.strictEqual( cachedResponse.status, 304, - 'new ETag from PATCH lets a follow-up GET short-circuit', + 'the advanced validator lets a follow-up GET short-circuit', ); }); - test('no-op PATCH response carries an ETag matching the existing one', async function (assert) { + test('no-op PATCH response omits the ETag; the GET validator is unchanged', async function (assert) { // Prime once so the stored file is in canonical serialized form; // the no-op assertions below measure the steady state (see the // no-op lastModified test). @@ -3382,10 +3456,20 @@ module(basename(import.meta.filename), function () { .set('Accept', 'application/vnd.card+json'); assert.strictEqual(patchResponse.status, 200, 'no-op PATCH succeeds'); - assert.strictEqual( + assert.notOk( patchResponse.get('etag'), + 'the no-op write echo carries no validator either', + ); + + // The no-op didn't rewrite the file, so a GET still reports the same + // validator it did before the PATCH. + let afterResponse = await request + .get('/person-1') + .set('Accept', 'application/vnd.card+json'); + assert.strictEqual( + afterResponse.get('etag'), initialEtag, - 'no-op PATCH returns the same ETag (no rewrite, indexed_at unchanged)', + 'the GET validator is unchanged (no rewrite, indexed_at unchanged)', ); }); diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 93e9de51402..afed3b37866 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -7743,12 +7743,15 @@ export class Realm { lastModified, ); } else { + // The write response is read only for the primary card's assigned id + // and realm-info; the client discards its attributes, relationships and + // `included[]` (see `persistAndUpdate` in host store.ts). Skip the + // transitive `loadLinks` closure and query-backed expansion — assembling + // a link graph nothing reads is wasted work (the closure walk is the + // bulk of read-handler time), and side-loaded children round-trip their + // ids through their `lid`s, not this response. let entry = await this.#realmIndexQueryEngine.cardDocument( new URL(newURL), - { - loadLinks: true, - skipQueryBackedExpansion: false, - }, ); if (!entry || entry?.type === 'error') { let err = entry @@ -7803,8 +7806,7 @@ export class Realm { let duringPrerender = isDuringPrerenderRequest(request); // A skip-index-wait caller (see SKIP_INDEX_WAIT_HEADER) takes the same // write-side path as a prerender write — index deferred, answer from the - // serialized echo — without the prerender-only serialization tweaks - // (skipQueryBackedExpansion) that stay gated on `duringPrerender` below. + // serialized echo rather than a readback. let answerFromEcho = duringPrerender || isSkipIndexWaitRequest(request); let { data: patch, included: maybeIncluded } = await request.json(); @@ -7945,12 +7947,12 @@ export class Realm { // If the patch makes no semantic changes and doesn't include side-loaded // resources, short-circuit to avoid touching the file (and changing mtime). if (included.length === 0 && isEqual(primaryResource, original)) { + // No links closure: the PATCH response is read only for the primary + // card's id and realm-info, and this readback runs inside the + // realm-wide write lock every other writer queues on (see the + // non-short-circuit readback below for the full rationale). let entry = await this.#realmIndexQueryEngine.cardDocument( new URL(instanceURL), - { - loadLinks: true, - skipQueryBackedExpansion: duringPrerender, - }, ); if (entry && entry.type !== 'error') { let existingDoc = merge({}, entry.doc, { @@ -7962,11 +7964,10 @@ export class Realm { let createdAt = await this.getCreatedTime( this.paths.local(url) + '.json', ); - // The PATCH echo is the same served representation as a GET — - // including the joined `meta.screenshots` (the store replaces an - // instance's meta wholesale from a save response, so an echo - // without it would wipe the key client-side until the next GET) - // and the same validator components. + // The PATCH echo carries the joined `meta.screenshots` a GET would + // (the store replaces an instance's meta wholesale from a save + // response, so an echo without it would wipe the key client-side + // until the next GET). if (entry.screenshots) { existingDoc.data.meta = { ...existingDoc.data.meta, @@ -7976,18 +7977,11 @@ export class Realm { }), }; } - // entry.doc came from cardDocument(), which already called - // attachRealmInfo() and (re)populated the realm-info cache — - // so the cached hash is current as of this response. - await this.getRealmInfo(); - let foreignDeps = this.hasForeignRealmDeps(entry.deps); - let etag = foreignDeps - ? undefined - : buildCardJsonEtag( - entry.indexedAt, - this.getCachedRealmInfoHash(), - screenshotsEtagFingerprint(entry.screenshots), - ); + // No validator on the write echo: the echo omits the link closure a + // GET assembles, so it is a different representation than the GET + // whose ETag it would otherwise share — emitting one would let a + // conditional GET 304 onto this closure-less body. The client + // discards the echo body anyway, so it has no validator to gain. this.#serveInstanceIdsAsRRI(existingDoc); return createResponse({ body: JSON.stringify(existingDoc, null, 2), @@ -7995,8 +7989,6 @@ export class Realm { headers: { 'content-type': SupportedMimeType.CardJson, 'cache-control': this.cardJsonCacheControl(requestContext), - ...(etag ? { etag } : {}), - ...etagSuppressedHeader(foreignDeps), ...lastModifiedHeader(existingDoc), ...(createdAt != null ? { 'x-created': formatRFC7231(createdAt * 1000) } @@ -8101,12 +8093,15 @@ export class Realm { requestContext, }); } + // The write response is read only for the primary card's assigned id + // and realm-info; the client discards its attributes, relationships and + // `included[]` (see `persistAndUpdate` in host store.ts). Skip the + // transitive `loadLinks` closure and query-backed expansion — assembling + // a link graph nothing reads is wasted work, and here it is wasted + // inside the realm-wide write lock every other writer on this realm is + // serialized behind. let entry = await this.#realmIndexQueryEngine.cardDocument( new URL(instanceURL), - { - loadLinks: true, - skipQueryBackedExpansion: false, - }, ); if (!entry || entry?.type === 'error') { if ( @@ -8155,23 +8150,11 @@ export class Realm { }; } } - // Same rationale as the no-op short-circuit branch above: - // cardDocument() above primed the realm-info cache via - // attachRealmInfo(), but only when entry was a non-error doc. - // On the error fallback we may still need to populate it. - await this.getRealmInfo(); - let foreignDeps = - entry && entry.type !== 'error' - ? this.hasForeignRealmDeps(entry.deps) - : false; - let etag = - entry && entry.type !== 'error' && !foreignDeps - ? buildCardJsonEtag( - entry.indexedAt, - this.getCachedRealmInfoHash(), - screenshotsEtagFingerprint(entry.screenshots), - ) - : undefined; + // No validator on the write echo (same rationale as the short-circuit + // branch above): the echo omits the link closure a GET assembles, so it + // is a different representation than the GET whose ETag it would share — + // emitting one would let a conditional GET 304 onto this closure-less + // body. The client discards the echo body anyway. this.#serveInstanceIdsAsRRI(doc); return createResponse({ body: JSON.stringify(doc, null, 2), @@ -8179,8 +8162,6 @@ export class Realm { headers: { 'content-type': SupportedMimeType.CardJson, 'cache-control': this.cardJsonCacheControl(requestContext), - ...(etag ? { etag } : {}), - ...etagSuppressedHeader(foreignDeps), ...lastModifiedHeader(doc), ...(created ? { 'x-created': formatRFC7231(created * 1000) } : {}), },