Repository: urql-graphql/urql (ts)

Issue:
(graphcache): Getting non-stale flags for partial results
### Describe the bug

I'm using `urql`, `@urql/core`, `@urql/exchange-graphcache` and `@urql/exchange-execute` and I think I'm getting at least a stale state wrong. There is a small reproduction of the issue available that shows the setup, but in short:

When loading two queries (covering in part the same sections) in parallel and a partial result can be available for the slower of them the both the loading and the stale state of the slower query is set to false. This is the output I get from the reproduction (altered for clarity):

```
T1 {fetching: true, stale: false, data: undefined }
T2 {fetching: true, stale: false, data: undefined }

// T2 query is done loading (subFeature2 resolver returning)

T1 {fetching: false, stale: false, data: PARTIAL DATA }
T2 {fetching: false, stale: false, data: FULL DATA }

// T2 query is done loading (subFeature1 resolver returning)

T1 {fetching: false, stale: false, data: FULL DATA }
T2 {fetching: false, stale: false, data: FULL DATA }
```

This seems to be in contrast with the information available at <https://formidable.com/open-source/urql/docs/graphcache/schema-awareness/#partial-results> which suggest the stale flag should be set until the full data is available.

There has also been some discussions on this topic earlier in <https://github.com/urql-graphql/urql/issues/2282> which also seems to align with the documentation.

I would perhaps also expect the fetching flag to be set until the first real non-partial data is present but I don't know if that is expected or not.

### Reproduction

https://codesandbox.io/s/js-playground-forked-i715qc?file=/src/index.tsx

### Urql version

@urql/core v3.1.1
@urql/exchange-execute v2.1.1
@urql/exchange-graphcache v5.0.8
urql v3.0.3
graphql v16.6.0


### Validations

- [X] I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which [GitHub Discussions](https://github.com/urql-graphql/urql/discussions) should be used
- [X] Read the [docs](https://formidable.com/open-source/urql/docs/).
- [X] Follow our [Code of Conduct](https://github.com/urql-graphql/urql/blob/main/CODE_OF_CONDUCT.md)

Pull request:
fix(graphcache): Fix regressed stale flag for looping protection
Resolves #2937

## Summary

This fixes `stale: true` not being set on partial responses that had been blocked by the looping protection. Even if we detect that a partial response may lead to a loop, the `stale` flag should still be set to indicate that the response is incomplete/partial, as per the documentation.

This is a regression from #2831

## Set of changes

- Add `result.stale` for partial results, even if the reexecution is blockd

Patch:
diff --git a/.changeset/afraid-gorillas-hope.md b/.changeset/afraid-gorillas-hope.md
new file mode 100644
index 00000000..36f8cf8e
--- /dev/null
+++ b/.changeset/afraid-gorillas-hope.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix regression which caused partial results, whose refetches were blocked by the looping protection, to not have a `stale: true` flag added to them. This is a regression from https://github.com/urql-graphql/urql/pull/2831 and only applies to `cacheExchange`s that had the `schema` option set.
diff --git a/.changeset/wet-buses-hide.md b/.changeset/wet-buses-hide.md
new file mode 100644
index 00000000..a33ed301
--- /dev/null
+++ b/.changeset/wet-buses-hide.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Add `invariant` to data layer that prevents cache writes during cache query operations. This prevents `cache.writeFragment`, `cache.updateQuery`, and `cache.link` from being called in `resolvers` for instance.
diff --git a/docs/graphcache/errors.md b/docs/graphcache/errors.md
index 25c7d16e..4c9d800e 100644
--- a/docs/graphcache/errors.md
+++ b/docs/graphcache/errors.md
@@ -395,3 +395,17 @@ This error occurs when you are using an `interface` or `union` rather than an
 implemented type for these.
 
 Check the type mentioned and change it to one of the specific types.
+
+## (27) Invalid Cache write
+
+> Invalid Cache write: You may not write to the cache during cache reads.
+> Accesses to `cache.writeFragment`, `cache.updateQuery`, and `cache.link` may
+> not be made inside `resolvers` for instance.
+
+If you're using the `Cache` inside your `cacheExchange` config you receive it
+either inside callbacks that are called when the cache is queried (e.g.
+`resolvers`) or when data is written to the cache (e.g. `updates`). You may not
+write to the cache when it's being queried.
+
+Please make sure that you're not calling `cache.updateQuery`,
+`cache.writeFragment`, or `cache.link` inside `resolvers`.
diff --git a/docs/graphcache/local-resolvers.md b/docs/graphcache/local-resolvers.md
index abc814ac..75fc7df3 100644
--- a/docs/graphcache/local-resolvers.md
+++ b/docs/graphcache/local-resolvers.md
@@ -65,6 +65,11 @@ record field into a link, i.e. replace a scalar with an entity. Instead, local r
 to transform records, like dates in our previous example, or to imitate server-side logic to allow
 Graphcache to retrieve more data from its cache without sending a query to our API.
 
+Furthermore, while we see on this page that we get access to methods like `cache.resolve` and other
+methods to read from our cache, only ["Cache Updates"](./cache-updates.md) get to write and change
+the cache. If you call `cache.updateQuery`, `cache.writeFragment`, or `cache.link` in resolvers,
+you‘ll get an error, since it‘s not possible to update the cache while reading from it.
+
 ## Transforming Records
 
 As we've explored in the ["Normalized Caching" page's section on
@@ -101,8 +106,7 @@ current query traversal, and the part of the query document the cache is process
 operating on. Hence, we can create a reusable resolver like so:
 
 ```js
-const transformToDate = (parent, _args, _cache, info) =>
-  new Date(parent[info.fieldName]);
+const transformToDate = (parent, _args, _cache, info) => new Date(parent[info.fieldName]);
 
 cacheExchange({
   resolvers: {
@@ -124,9 +128,7 @@ cacheExchange({
   resolvers: {
     Todo: {
       text: (parent, args) => {
-        return args.capitalize && parent.text
-          ? parent.text.toUpperCase()
-          : parent.text;
+        return args.capitalize && parent.text ? parent.text.toUpperCase() : parent.text;
       },
     },
   },
@@ -178,8 +180,7 @@ cacheExchange({
 The `__typename` field is required. Graphcache will [use its keying
 logic](./normalized-caching.md#custom-keys-and-non-keyable-entities), and your custom `keys`
 configuration to generate a key for this entity and will then be able to look this entity up in its
-local cache. As with regular queries, the resolver is known to return a link since the `todo(id:
-$id) { id }` will be used with a selection set, querying fields on the entity.
+local cache. As with regular queries, the resolver is known to return a link since the `todo(id: $id) { id }` will be used with a selection set, querying fields on the entity.
 
 ### Resolving by keys
 
@@ -198,8 +199,7 @@ While it doesn't make much sense in this case, our example can be rewritten as:
 cacheExchange({
   resolvers: {
     Query: {
-      todo: (_, args, cache) =>
-        cache.keyOfEntity({ __typename: 'Todo', id: args.id }),
+      todo: (_, args, cache) => cache.keyOfEntity({ __typename: 'Todo', id: args.id }),
     },
   },
 });
@@ -253,8 +253,7 @@ avoid using the `parent[fieldName]` shortcut:
 cacheExchange({
   resolvers: {
     Todo: {
-      updatedAt: (parent, _args, cache) =>
-        new Date(cache.resolve(parent, "updatedAt")),
+      updatedAt: (parent, _args, cache) => new Date(cache.resolve(parent, 'updatedAt')),
     },
   },
 });
@@ -279,8 +278,7 @@ case we could write a resolver like so:
 cacheExchange({
   resolvers: {
     Todo: {
-      updatedAt: (parent, _args, cache) =>
-        parent.updatedAt || cache.resolve(parent, "createdAt")
+      updatedAt: (parent, _args, cache) => parent.updatedAt || cache.resolve(parent, 'createdAt'),
     },
   },
 });
@@ -299,10 +297,7 @@ cacheExchange({
   resolvers: {
     Todo: {
       createdAt: (parent, _args, cache) =>
-        cache.resolve(
-          cache.resolve(parent, "author"), /* "Author:1" */
-          "createdAt"
-        )
+        cache.resolve(cache.resolve(parent, 'author') /* "Author:1" */, 'createdAt'),
     },
   },
 });
@@ -387,17 +382,17 @@ variables we can execute an entirely new GraphQL query against our cached data:
 
 ```js
 import { gql } from '@urql/core';
-import { cacheExchange } from '@urql/exchange-graphcache';
+import { cacheExchange } from '@urql/exchange-graphcache';
 
 const cache = cacheExchange({
   updates: {
     Mutation: {
       addTodo: (result, args, cache) => {
         const data = cache.readQuery({ query: Todos, variables: { from: 0, limit: 10 } });
-      }
-    }
-  }
-})
+      },
+    },
+  },
+});
 ```
 
 This way we'll get the stored data for the `TodosQuery` for the given `variables`.
@@ -411,7 +406,7 @@ accepts a `fragment` and an `id`. This looks like the following.
 
 ```js
 import { gql } from '@urql/core';
-import { cacheExchange } from '@urql/exchange-graphcache';
+import { cacheExchange } from '@urql/exchange-graphcache';
 
 const cache = cacheExchange({
   resolvers: {
@@ -426,10 +421,10 @@ const cache = cacheExchange({
           `,
           { id: 1 }
         );
-      }
-    }
-  }
-})
+      },
+    },
+  },
+});
 ```
 
 > **Note:** In the above example, we've used
@@ -528,7 +523,7 @@ const cache = cacheExchange({
     // Or if the pagination happens in a nested field:
     User: {
       todos: relayPagination(),
-    }
+    },
   },
 });
 ```
diff --git a/docs/graphcache/normalized-caching.md b/docs/graphcache/normalized-caching.md
index 9d5c6fd8..8cf7deea 100644
--- a/docs/graphcache/normalized-caching.md
+++ b/docs/graphcache/normalized-caching.md
@@ -445,6 +445,10 @@ As we can see, we may perform manual changes inside of `updates` functions, whic
 affect other parts of the cache (like `Query.todos` here) beyond the automatic updates that a
 normalized cache is expected to perform.
 
+We get methods like `cache.updateQuery`, `cache.writeFragment`, and `cache.link` in our updater
+functions, which aren't available to us in local resolvers, and can only be used in these `updates`
+entries to change the data that the cache holds.
+
 [Read more about writing cache updates on the "Cache Updates" page.](./cache-updates.md)
 
 ## Deterministic Cache Updates
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 46f22b04..f387003e 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -315,30 +315,38 @@ export const cacheExchange = <C extends Partial<CacheExchangeOpts>>(
       ),
       map(
         (res: OperationResultWithMeta): OperationResult => {
-          const { operation, outcome, dependencies } = res;
+          const { requestPolicy } = res.operation.context;
+
+          // We don't mark cache-only responses as partial, as this would indicate
+          // that we expect a new result to come from the network, which cannot
+          // happen
+          const isPartial =
+            res.outcome === 'partial' && requestPolicy !== 'cache-only';
+
+          // We reexecute requests marked as `cache-and-network`, and partial responses,
+          // if we wouldn't cause a request loop
+          const shouldReexecute =
+            requestPolicy === 'cache-and-network' ||
+            (requestPolicy === 'cache-first' &&
+              isPartial &&
+              !reexecutingOperations.has(res.operation.key));
+
           const result: OperationResult = {
-            operation: addCacheOutcome(operation, outcome),
+            operation: addCacheOutcome(res.operation, res.outcome),
             data: res.data,
             error: res.error,
             extensions: res.extensions,
+            stale: shouldReexecute || isPartial,
           };
 
-          if (
-            operation.context.requestPolicy === 'cache-and-network' ||
-            (operation.context.requestPolicy === 'cache-first' &&
-              outcome === 'partial' &&
-              !reexecutingOperations.has(res.operation.key))
-          ) {
-            result.stale = true;
-            if (!isBlockedByOptimisticUpdate(dependencies)) {
-              client.reexecuteOperation(
-                toRequestPolicy(operation, 'network-only')
-              );
-            } else if (
-              operation.context.requestPolicy === 'cache-and-network'
-            ) {
-              requestedRefetch.add(operation.key);
-            }
+          if (!shouldReexecute) {
+            /*noop*/
+          } else if (!isBlockedByOptimisticUpdate(res.dependencies)) {
+            client.reexecuteOperation(
+              toRequestPolicy(res.operation, 'network-only')
+            );
+          } else if (requestPolicy === 'cache-and-network') {
+            requestedRefetch.add(res.operation.key);
           }
 
           dispatchDebug({
diff --git a/exchanges/graphcache/src/helpers/help.ts b/exchanges/graphcache/src/helpers/help.ts
index 1775a445..9c09b6be 100644
--- a/exchanges/graphcache/src/helpers/help.ts
+++ b/exchanges/graphcache/src/helpers/help.ts
@@ -31,7 +31,8 @@ export type ErrorCode =
   | 23
   | 24
   | 25
-  | 26;
+  | 26
+  | 27;
 
 type DebugNode = ExecutableDefinitionNode | InlineFragmentNode;
 
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index 9ab321e2..5b9008fb 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -255,6 +255,16 @@ const setNode = <T>(
   fieldKey: string,
   value: T
 ) => {
+  if (process.env.NODE_ENV !== 'production') {
+    invariant(
+      currentOperation !== 'read',
+      'Invalid Cache write: You may not write to the cache during cache reads. ' +
+        ' Accesses to `cache.writeFragment`, `cache.updateQuery`, and `cache.link` may ' +
+        ' not be made inside `resolvers` for instance.',
+      27
+    );
+  }
+
   // Optimistic values are written to a map in the optimistic dict
   // All other values are written to the base map
   const keymap: KeyMap<Dict<T | undefined>> = currentOptimisticKey
@@ -550,6 +560,7 @@ const squashLayer = (layerKey: number) => {
   // Hide current dependencies from squashing operations
   const previousDependencies = currentDependencies;
   currentDependencies = new Set();
+  currentOperation = 'write';
 
   const links = currentData!.links.optimistic.get(layerKey);
   if (links) {


Test patch:
diff --git a/exchanges/graphcache/e2e-tests/query.spec.tsx b/exchanges/graphcache/e2e-tests/query.spec.tsx
index 3736b941..8609a442 100644
--- a/exchanges/graphcache/e2e-tests/query.spec.tsx
+++ b/exchanges/graphcache/e2e-tests/query.spec.tsx
@@ -128,7 +128,7 @@ describe('Graphcache Queries', () => {
     });
 
     const FirstComponent = () => {
-      const [{ fetching, data, error }] = useQuery({
+      const [{ fetching, data, error, stale }] = useQuery({
         query: `{
           movie {
             id
@@ -149,6 +149,7 @@ describe('Graphcache Queries', () => {
               <div>First Component</div>
               <div id="first-data">{`Data: ${data.movie?.title}`}</div>
               <div id="first-error">{`Error: ${error?.message}`}</div>
+              <div id="first-stale">{`Stale: ${!!stale}`}</div>
             </div>
           )}
         </div>
@@ -156,7 +157,7 @@ describe('Graphcache Queries', () => {
     };
 
     const SecondComponent = () => {
-      const [{ error, data, fetching }] = useQuery({
+      const [{ error, data, fetching, stale }] = useQuery({
         query: `{
           movie {
             id
@@ -176,6 +177,7 @@ describe('Graphcache Queries', () => {
           <div>Second Component</div>
           <div id="second-data">{`Data: ${data.movie.id}`}</div>
           <div id="second-error">{`Error: ${error?.message}`}</div>
+          <div id="second-stale">{`Stale: ${!!stale}`}</div>
         </div>
       );
     };
@@ -189,6 +191,7 @@ describe('Graphcache Queries', () => {
 
     cy.get('#first-data').should('have.text', 'Data: title');
     cy.get('#second-data').should('have.text', 'Data: foo');
+    cy.get('#second-stale').should('have.text', 'Stale: true');
     // TODO: ideally we would be able to keep the error here but...
     // cy.get('#first-error').should('have.text', 'Error: [GraphQL] Test');
     // cy.get('#second-error').should('have.text', 'Error: [GraphQL] Test');
diff --git a/exchanges/graphcache/src/offlineExchange.test.ts b/exchanges/graphcache/src/offlineExchange.test.ts
index f78d083c..bc0d9b74 100644
--- a/exchanges/graphcache/src/offlineExchange.test.ts
+++ b/exchanges/graphcache/src/offlineExchange.test.ts
@@ -218,6 +218,7 @@ describe('offline', () => {
       error: undefined,
       extensions: undefined,
       operation: expect.any(Object),
+      stale: false,
     });
 
     expect(result.mock.calls[0][0]).toHaveProperty(
diff --git a/exchanges/graphcache/src/store/__snapshots__/store.test.ts.snap b/exchanges/graphcache/src/store/__snapshots__/store.test.ts.snap
index 1c80885c..8e6e3a90 100644
--- a/exchanges/graphcache/src/store/__snapshots__/store.test.ts.snap
+++ b/exchanges/graphcache/src/store/__snapshots__/store.test.ts.snap
@@ -1,4 +1,4 @@
-// Vitest Snapshot v1
+// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
 
 exports[`Store with storage > should be able to persist embedded data 1`] = `
 {
diff --git a/exchanges/graphcache/src/store/store.test.ts b/exchanges/graphcache/src/store/store.test.ts
index aee9d14c..47331558 100644
--- a/exchanges/graphcache/src/store/store.test.ts
+++ b/exchanges/graphcache/src/store/store.test.ts
@@ -468,7 +468,7 @@ describe('Store with OptimisticMutationConfig', () => {
     );
     let { data } = query(store, { query: connection });
 
-    InMemoryData.initDataState('read', store.data, null);
+    InMemoryData.initDataState('write', store.data, null);
     expect((data as any).exercisesConnection).toEqual(null);
     const fields = store.inspectFields({ __typename: 'Query' });
     fields.forEach(({ fieldName, arguments: args }) => {
@@ -483,7 +483,7 @@ describe('Store with OptimisticMutationConfig', () => {
   });
 
   it('should be able to write a fragment', () => {
-    InMemoryData.initDataState('read', store.data, null);
+    InMemoryData.initDataState('write', store.data, null);
 
     store.writeFragment(
       gql`
@@ -520,7 +520,7 @@ describe('Store with OptimisticMutationConfig', () => {
   });
 
   it('should be able to write a fragment by name', () => {
-    InMemoryData.initDataState('read', store.data, null);
+    InMemoryData.initDataState('write', store.data, null);
 
     store.writeFragment(
       gql`
@@ -626,7 +626,7 @@ describe('Store with OptimisticMutationConfig', () => {
   });
 
   it('should be able to update a query', () => {
-    InMemoryData.initDataState('read', store.data, null);
+    InMemoryData.initDataState('write', store.data, null);
     store.updateQuery({ query: Todos }, data => ({
       ...data,
       todos: [
@@ -686,7 +686,7 @@ describe('Store with OptimisticMutationConfig', () => {
       }
     );
 
-    InMemoryData.initDataState('read', store.data, null);
+    InMemoryData.initDataState('write', store.data, null);
     store.updateQuery({ query: Appointment, variables: { id: '1' } }, data => ({
       ...data,
       appointment: {
@@ -827,6 +827,7 @@ describe('Store with OptimisticMutationConfig', () => {
 
   describe('Invalidating an entity', () => {
     it('removes an entity from a list.', () => {
+      InMemoryData.initDataState('write', store.data, null);
       store.invalidate(todosData.todos[1]);
       const { data } = query(store, { query: Todos });
       expect(data).toBe(null);