Repository: FormidableLabs/urql
Pull request #1367 (2021-02-05 13:48:49)

(graphcache) - Fix uncached GraphQLError fields making it impossible to get first API result
This is a follow-up for #1356

## Summary

The #1356 change introduced behaviour that prevents `null` values from fields that have errored and are associated with `GraphQLError` from being cached. This caused a regression where when we "read-back" results from the cache that have just arrived from the API these fields would cause a cache miss:

https://github.com/FormidableLabs/urql/blob/6a687091b67dc3d0ee78353ca79b0aa401ba983c/exchanges/graphcache/src/cacheExchange.ts#L237-L243

We can prevent this by also tracking errored fields in the `query` and ignoring these uncached fields there too by replacing them with `null` while we still have access to the `GraphQLError`s.
This is preferable to other solutions as we've gotten rid of comparing the cached result to the API result in #1196 as we had trouble differentiating between `null` selection sets from previous selections and `null` values from the API result.

No changeset is needed since the PR is unreleased.

## Set of changes

- Add (previously failing) test for retrieving a result with `null`'d fields due to `GraphQLError`s from an API
- Track `ctx.__internal.path` in all environments in `query` too
- Check for errors before aborting a selection in `query` and set the field to `null` if it does have an error


changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 806055ac0f5755eaee93f7a8bf52d956b9a461a2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/FormidableLabs/urql/new/fix/graphcache-readback-results?filename=.changeset/hip-geckos-scream.md&value=---%0A%22%40urql%2Fexchange-graphcache%22%3A%20patch%0A---%0A%0A(graphcache)%20-%20Fix%20uncached%20GraphQLError%20fields%20making%20it%20impossible%20to%20get%20first%20API%20result%0A)



Patch:
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 9adf8949..3773e933 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -234,7 +234,13 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
       ).dependencies;
       collectPendingOperations(pendingOperations, writeDependencies);
 
-      const queryResult = query(store, operation, result.data, key);
+      const queryResult = query(
+        store,
+        operation,
+        result.data,
+        result.error,
+        key
+      );
       result.data = queryResult.data;
       if (operation.kind === 'query') {
         // Collect the query's dependencies for future pending operation updates
diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts
index 06f9ab01..cf0085ed 100644
--- a/exchanges/graphcache/src/operations/query.ts
+++ b/exchanges/graphcache/src/operations/query.ts
@@ -1,4 +1,5 @@
 import { FieldNode, DocumentNode, FragmentDefinitionNode } from 'graphql';
+import { CombinedError } from '@urql/core';
 
 import {
   getSelectionSet,
@@ -6,9 +7,6 @@ import {
   SelectionSet,
   getFragmentTypeName,
   getFieldAlias,
-} from '../ast';
-
-import {
   getFragments,
   getMainOperation,
   normalizeVariables,
@@ -44,6 +42,7 @@ import {
   ensureData,
   makeContext,
   updateContext,
+  getFieldError,
 } from './shared';
 
 import {
@@ -62,10 +61,11 @@ export const query = (
   store: Store,
   request: OperationRequest,
   data?: Data,
+  error?: CombinedError | undefined,
   key?: number
 ): QueryResult => {
   initDataState('read', store.data, (data && key) || null);
-  const result = read(store, request, data);
+  const result = read(store, request, data, error);
   clearDataState();
   return result;
 };
@@ -73,7 +73,8 @@ export const query = (
 export const read = (
   store: Store,
   request: OperationRequest,
-  input?: Data
+  input?: Data,
+  error?: CombinedError | undefined
 ): QueryResult => {
   const operation = getMainOperation(request.query);
   const rootKey = store.rootFields[operation.operation];
@@ -84,7 +85,9 @@ export const read = (
     normalizeVariables(operation, request.variables),
     getFragments(request.query),
     rootKey,
-    rootKey
+    rootKey,
+    false,
+    error
   );
 
   if (process.env.NODE_ENV !== 'production') {
@@ -117,21 +120,22 @@ const readRoot = (
   select: SelectionSet,
   originalData: Data
 ): Data => {
-  if (typeof originalData.__typename !== 'string') {
+  const typename = ctx.store.rootNames[entityKey]
+    ? entityKey
+    : originalData.__typename;
+  if (typeof typename !== 'string') {
     return originalData;
   }
 
   const iterate = makeSelectionIterator(entityKey, entityKey, select, ctx);
-  const data = {} as Data;
-  data.__typename = originalData.__typename;
+  const data = { __typename: typename };
 
   let node: FieldNode | void;
   while ((node = iterate())) {
     const fieldAlias = getFieldAlias(node);
     const fieldValue = originalData[fieldAlias];
     // Add the current alias to the walked path before processing the field's value
-    if (process.env.NODE_ENV !== 'production')
-      ctx.__internal.path.push(fieldAlias);
+    ctx.__internal.path.push(fieldAlias);
     // Process the root field's value
     if (node.selectionSet && fieldValue !== null) {
       const fieldData = ensureData(fieldValue);
@@ -140,7 +144,7 @@ const readRoot = (
       data[fieldAlias] = fieldValue;
     }
     // After processing the field, remove the current alias from the path again
-    if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop();
+    ctx.__internal.path.pop();
   }
 
   return data;
@@ -155,11 +159,11 @@ const readRootField = (
     const newData = new Array(originalData.length);
     for (let i = 0, l = originalData.length; i < l; i++) {
       // Add the current index to the walked path before reading the field's value
-      if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i);
+      ctx.__internal.path.push(i);
       // Recursively read the root field's value
       newData[i] = readRootField(ctx, select, originalData[i]);
       // After processing the field, remove the current index from the path
-      if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop();
+      ctx.__internal.path.pop();
     }
 
     return newData;
@@ -312,8 +316,7 @@ const readSelection = (
     // means that the value is missing from the cache
     let dataFieldValue: void | DataField;
     // Add the current alias to the walked path before processing the field's value
-    if (process.env.NODE_ENV !== 'production')
-      ctx.__internal.path.push(fieldAlias);
+    ctx.__internal.path.push(fieldAlias);
 
     if (resultValue !== undefined && node.selectionSet === undefined) {
       // The field is a scalar and can be retrieved directly from the result
@@ -396,8 +399,15 @@ const readSelection = (
       }
     }
 
+    // If we have an error registered for the current field change undefined values to null
+    if (dataFieldValue === undefined && !!getFieldError(ctx)) {
+      hasPartials = true;
+      dataFieldValue = null;
+    }
+
     // After processing the field, remove the current alias from the path again
-    if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop();
+    ctx.__internal.path.pop();
+
     // Now that dataFieldValue has been retrieved it'll be set on data
     // If it's uncached (undefined) but nullable we can continue assembling
     // a partial query result
@@ -442,7 +452,7 @@ const resolveResolverResult = (
     const data = new Array(result.length);
     for (let i = 0, l = result.length; i < l; i++) {
       // Add the current index to the walked path before reading the field's value
-      if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i);
+      ctx.__internal.path.push(i);
       // Recursively read resolver result
       const childResult = resolveResolverResult(
         ctx,
@@ -455,7 +465,7 @@ const resolveResolverResult = (
         result[i]
       );
       // After processing the field, remove the current index from the path
-      if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop();
+      ctx.__internal.path.pop();
       // Check the result for cache-missed values
       if (childResult === undefined && !_isListNullable) {
         return undefined;
@@ -504,7 +514,7 @@ const resolveLink = (
     const newLink = new Array(link.length);
     for (let i = 0, l = link.length; i < l; i++) {
       // Add the current index to the walked path before reading the field's value
-      if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i);
+      ctx.__internal.path.push(i);
       // Recursively read the link
       const childLink = resolveLink(
         ctx,
@@ -515,7 +525,7 @@ const resolveLink = (
         prevData != null ? prevData[i] : undefined
       );
       // After processing the field, remove the current index from the path
-      if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop();
+      ctx.__internal.path.pop();
       // Check the result for cache-missed values
       if (childLink === undefined && !_isListNullable) {
         return undefined;


Test patch:
diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts
index b36bc959..fed11aed 100644
--- a/exchanges/graphcache/src/cacheExchange.test.ts
+++ b/exchanges/graphcache/src/cacheExchange.test.ts
@@ -4,6 +4,7 @@ import {
   ExchangeIO,
   Operation,
   OperationResult,
+  CombinedError,
 } from '@urql/core';
 
 import {
@@ -610,6 +611,69 @@ describe('data dependencies', () => {
     jest.runAllTimers();
     expect(updates.Mutation.concealAuthor).toHaveBeenCalledTimes(2);
   });
+
+  it('marks errored null fields as uncached but delivers them as expected', () => {
+    const client = createClient({ url: 'http://0.0.0.0' });
+    const { source: ops$, next } = makeSubject<Operation>();
+
+    const query = gql`
+      {
+        field
+        author {
+          id
+        }
+      }
+    `;
+
+    const operation = client.createRequestOperation('query', {
+      key: 1,
+      query,
+    });
+
+    const queryResult: OperationResult = {
+      operation,
+      data: {
+        __typename: 'Query',
+        field: 'test',
+        author: null,
+      },
+      error: new CombinedError({
+        graphQLErrors: [
+          {
+            message: 'Test',
+            path: ['author'],
+          },
+        ],
+      }),
+    };
+
+    const reexecuteOperation = jest
+      .spyOn(client, 'reexecuteOperation')
+      .mockImplementation(next);
+
+    const response = jest.fn(
+      (forwardOp: Operation): OperationResult => {
+        if (forwardOp.key === 1) return queryResult;
+        return undefined as any;
+      }
+    );
+
+    const result = jest.fn();
+    const forward: ExchangeIO = ops$ => pipe(ops$, map(response));
+
+    pipe(
+      cacheExchange({})({ forward, client, dispatchDebug })(ops$),
+      tap(result),
+      publish
+    );
+
+    next(operation);
+
+    expect(response).toHaveBeenCalledTimes(1);
+    expect(result).toHaveBeenCalledTimes(1);
+    expect(reexecuteOperation).toHaveBeenCalledTimes(0);
+    expect(result.mock.calls[0][0]).toHaveProperty('data.author', null);
+  });
 });
 
 describe('optimistic updates', () => {