Repository: urql-graphql/urql
Pull request #3493 (2024-02-01 19:01:11)

fix(graphcache): Restore `stale` being set to `true` on blocked (but in-flight) operations
> **Note:** I wasn't reasonably able to write tests for the looping protection yet that hit this particular case. Manual testing is needed!

## Summary

For partial cache results, `stale: false` was being set when the looping protection kicks in. In simple words, if an operation is triggered twice, once as part of an unrelated query updating, causing a partial result, then again, the second trigger will create a cache result with `stale: false`, even if the operation would be in flight.

This PR restores `stale: true` being set, if we reasonably believe the operation is still in flight.

## Set of changes

- Add separate `stale: true` override if operation is in flight


changeset-bot[bot]: ###  🦋  Changeset detected

Latest commit: bff560839a91a1052f78d426682e6590c2540142

**The changes in this PR will be included in the next version bump.**

<details><summary>This PR includes changesets to release 1 package</summary>

  | Name                      | Type  |
| ------------------------- | ----- |
| @urql/exchange-graphcache | Patch |

</details>

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/urql-graphql/urql/new/fix/restore-stale-true-on-blocked-ops?filename=.changeset/olive-nails-destroy.md&value=---%0A%22%40urql%2Fexchange-graphcache%22%3A%20patch%0A---%0A%0Afix(graphcache)%3A%20Restore%20%60stale%60%20being%20set%20to%20%60true%60%20on%20blocked%20(but%20in-flight)%20operations%0A)


AndrewIngram: Seems to address the issue I was having (mentioned here: https://twitter.com/andrewingram/status/1752787499053723667), but I also haven't been able to make a simplified reproduction :/
kitten: The test I added should sufficiently reproduce this scenario. It doesn't quite match the manner in which this case is triggered, but it should represent both the overall conditions that triggered this bug/issue and the fix itself

Patch:
diff --git a/.changeset/giant-beans-act.md b/.changeset/giant-beans-act.md
new file mode 100644
index 00000000..7981f017
--- /dev/null
+++ b/.changeset/giant-beans-act.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Set `stale: true` on cache results, even if a reexecution has been blocked by the loop protection, if the operation is already pending and in-flight.
diff --git a/.changeset/serious-tables-ring.md b/.changeset/serious-tables-ring.md
new file mode 100644
index 00000000..7dfc7a8b
--- /dev/null
+++ b/.changeset/serious-tables-ring.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix `@defer` state leaking into following operations.
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 19552865..260ca9db 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -32,6 +32,7 @@ import {
   noopDataState,
   hydrateData,
   reserveLayer,
+  hasLayer,
 } from './store/data';
 
 interface OperationResultWithMeta extends Partial<OperationResult> {
@@ -377,6 +378,14 @@ export const cacheExchange =
               (requestPolicy === 'cache-first' &&
                 res.outcome === 'partial' &&
                 !reexecutingOperations.has(res.operation.key)));
+          // Set stale to true anyway, even if the reexecute will be blocked, if the operation
+          // is in progress. We can be reasonably sure of that if a layer has been reserved for it.
+          const stale =
+            requestPolicy !== 'cache-only' &&
+            (shouldReexecute ||
+              (res.outcome === 'partial' &&
+                reexecutingOperations.has(res.operation.key) &&
+                hasLayer(store.data, res.operation.key)));
 
           const result: OperationResult = {
             operation: addMetadata(res.operation, {
@@ -385,7 +394,7 @@ export const cacheExchange =
             data: res.data,
             error: res.error,
             extensions: res.extensions,
-            stale: shouldReexecute && !res.hasNext,
+            stale: stale && !res.hasNext,
             hasNext: shouldReexecute && res.hasNext,
           };
 
diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts
index 3c3d1b6a..458e599f 100644
--- a/exchanges/graphcache/src/operations/shared.ts
+++ b/exchanges/graphcache/src/operations/shared.ts
@@ -64,6 +64,8 @@ export const makeContext = (
   entityKey: string,
   error: CombinedError | undefined
 ): Context => {
+  deferRef = false;
+
   const ctx: Context = {
     store,
     variables,
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index 7c3badef..174610a1 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -528,6 +528,11 @@ export const reserveLayer = (
   data.commutativeKeys.add(layerKey);
 };
 
+/** Checks whether a given layer exists */
+export const hasLayer = (data: InMemoryData, layerKey: number) =>
+  data.commutativeKeys.has(layerKey) ||
+  data.optimisticOrder.indexOf(layerKey) > -1;
+
 /** Creates an optimistic layer of links and records */
 const createLayer = (data: InMemoryData, layerKey: number) => {
   if (data.optimisticOrder.indexOf(layerKey) === -1) {


Test patch:
diff --git a/exchanges/graphcache/e2e-tests/query.spec.tsx b/exchanges/graphcache/e2e-tests/query.spec.tsx
index 747b98cf..c8823195 100644
--- a/exchanges/graphcache/e2e-tests/query.spec.tsx
+++ b/exchanges/graphcache/e2e-tests/query.spec.tsx
@@ -31,7 +31,8 @@ const schema = buildSchema(`
 `);
 
 const rootValue = {
-  movie: () => {
+  movie: async () => {
+    await new Promise(resolve => setTimeout(resolve, 50));
     return {
       id: 'foo',
       title: 'title',
diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts
index 170b2d5f..f4b89baf 100644
--- a/exchanges/graphcache/src/cacheExchange.test.ts
+++ b/exchanges/graphcache/src/cacheExchange.test.ts
@@ -6,8 +6,8 @@ import {
   OperationResult,
   CombinedError,
 } from '@urql/core';
-import { print, stripIgnoredCharacters } from 'graphql';
 
+import { print, stripIgnoredCharacters } from 'graphql';
 import { vi, expect, it, describe } from 'vitest';
 
 import {
@@ -2197,6 +2197,156 @@ describe('schema awareness', () => {
   });
 });
 
+describe('looping protection', () => {
+  it('applies stale to blocked looping queries', () => {
+    let normalData: OperationResult | undefined;
+    let extendedData: OperationResult | undefined;
+
+    const client = createClient({
+      url: 'http://0.0.0.0',
+      exchanges: [],
+    });
+
+    const { source: ops$, next: nextOp } = makeSubject<Operation>();
+    const { source: res$, next: nextRes } = makeSubject<OperationResult>();
+
+    vi.spyOn(client, 'reexecuteOperation').mockImplementation(nextOp);
+
+    const normalQuery = gql`
+      {
+        __typename
+        item {
+          __typename
+          id
+        }
+      }
+    `;
+
+    const extendedQuery = gql`
+      {
+        __typename
+        item {
+          __typename
+          extended: id
+          extra @_optional
+        }
+      }
+    `;
+
+    const forward = (ops$: Source<Operation>): Source<OperationResult> =>
+      share(
+        merge([
+          pipe(
+            ops$,
+            filter(() => false)
+          ) as any,
+          res$,
+        ])
+      );
+
+    pipe(
+      cacheExchange()({ forward, client, dispatchDebug })(ops$),
+      tap(result => {
+        if (result.operation.kind === 'query') {
+          if (result.operation.key === 1) {
+            normalData = result;
+          } else if (result.operation.key === 2) {
+            extendedData = result;
+          }
+        }
+      }),
+      publish
+    );
+
+    const normalOp = client.createRequestOperation(
+      'query',
+      {
+        key: 1,
+        query: normalQuery,
+        variables: undefined,
+      },
+      {
+        requestPolicy: 'cache-first',
+      }
+    );
+
+    const extendedOp = client.createRequestOperation(
+      'query',
+      {
+        key: 2,
+        query: extendedQuery,
+        variables: undefined,
+      },
+      {
+        requestPolicy: 'cache-first',
+      }
+    );
+
+    nextOp(normalOp);
+
+    nextRes({
+      operation: normalOp,
+      data: {
+        __typename: 'Query',
+        item: {
+          __typename: 'Node',
+          id: 'id',
+        },
+      },
+      stale: false,
+      hasNext: false,
+    });
+
+    expect(normalData).toMatchObject({ stale: false });
+    expect(client.reexecuteOperation).toHaveBeenCalledTimes(0);
+
+    nextOp(extendedOp);
+
+    expect(extendedData).toMatchObject({ stale: true });
+    expect(client.reexecuteOperation).toHaveBeenCalledTimes(1);
+
+    // Out of band re-execute first operation
+    nextOp(normalOp);
+    nextRes({
+      ...queryResponse,
+      operation: normalOp,
+      data: {
+        __typename: 'Query',
+        item: {
+          __typename: 'Node',
+          id: 'id',
+        },
+      },
+    });
+
+    expect(normalData).toMatchObject({ stale: false });
+    expect(extendedData).toMatchObject({ stale: true });
+    expect(client.reexecuteOperation).toHaveBeenCalledTimes(3);
+
+    nextOp(extendedOp);
+
+    expect(normalData).toMatchObject({ stale: false });
+    expect(extendedData).toMatchObject({ stale: true });
+    expect(client.reexecuteOperation).toHaveBeenCalledTimes(3);
+
+    nextRes({
+      ...queryResponse,
+      operation: extendedOp,
+      data: {
+        __typename: 'Query',
+        item: {
+          __typename: 'Node',
+          extended: 'id',
+          extra: 'extra',
+        },
+      },
+    });
+
+    expect(extendedData).toMatchObject({ stale: false });
+    expect(client.reexecuteOperation).toHaveBeenCalledTimes(4);
+  });
+});
+
 describe('commutativity', () => {
   it('applies results that come in out-of-order commutatively and consistently', () => {
     vi.useFakeTimers();
@@ -2873,8 +3023,7 @@ describe('commutativity', () => {
     });
     const { source: ops$, next: nextOp } = makeSubject<Operation>();
     const { source: res$, next: nextRes } = makeSubject<OperationResult>();
-
-    vi.spyOn(client, 'reexecuteOperation').mockImplementation(nextOp);
+    client.reexecuteOperation = nextOp;
 
     const normalQuery = gql`
       {
@@ -2911,11 +3060,11 @@ describe('commutativity', () => {
       }
     `;
 
-    const forward = (ops$: Source<Operation>): Source<OperationResult> =>
+    const forward = (operations$: Source<Operation>): Source<OperationResult> =>
       share(
         merge([
           pipe(
-            ops$,
+            operations$,
             filter(() => false)
           ) as any,
           res$,
@@ -3007,4 +3156,75 @@ describe('commutativity', () => {
     expect(combinedData).toHaveProperty('data.deferred.id', 1);
     expect(combinedData).toHaveProperty('data.node.id', 2);
   });
+
+  it('applies deferred logic only to deferred operations', () => {
+    let failingData: OperationResult | undefined;
+
+    const client = createClient({
+      url: 'http://0.0.0.0',
+      exchanges: [],
+    });
+
+    const { source: ops$, next: nextOp } = makeSubject<Operation>();
+    const { source: res$ } = makeSubject<OperationResult>();
+
+    const deferredQuery = gql`
+      {
+        ... @defer {
+          deferred {
+            id
+            name
+          }
+        }
+      }
+    `;
+
+    const failingQuery = gql`
+      {
+        deferred {
+          id
+          name
+        }
+      }
+    `;
+
+    const forward = (ops$: Source<Operation>): Source<OperationResult> =>
+      share(
+        merge([
+          pipe(
+            ops$,
+            filter(() => false)
+          ) as any,
+          res$,
+        ])
+      );
+
+    pipe(
+      cacheExchange()({ forward, client, dispatchDebug })(ops$),
+      tap(result => {
+        if (result.operation.kind === 'query') {
+          if (result.operation.key === 1) {
+            failingData = result;
+          }
+        }
+      }),
+      publish
+    );
+
+    const failingOp = client.createRequestOperation('query', {
+      key: 1,
+      query: failingQuery,
+      variables: undefined,
+    });
+    const deferredOp = client.createRequestOperation('query', {
+      key: 2,
+      query: deferredQuery,
+      variables: undefined,
+    });
+
+    nextOp(deferredOp);
+    nextOp(failingOp);
+
+    expect(failingData).not.toMatchObject({ hasNext: true });
+  });
 });