Repository: FormidableLabs/urql
Pull request #1196 (2020-12-04 19:59:53)

(graphcache) - Fix API results from being reused incorrectly for queries 
This was originally reported by @sarmeyer.

Tracking down what the root culprit and intended fix is was difficult since the effects of this bug were subtle, the cause was hard to find, and this can poorly by fixed by several small issues. Ultimately, tl;dr, the bug can be seen here: https://github.com/FormidableLabs/urql/blob/eee828914fb6083b2aaa3203484ed6b36b7beff5/exchanges/graphcache/src/operations/query.ts#L93-L97

## Summary

Fix reusing original query data from APIs accidentally, which can lead to subtle mismatches in results when the API's incoming `query` results are being updated by the `cacheExchange`, to apply resolvers. Specifically this may lead to relations from being set back to `null` when the resolver returns a different list of links than the result, since some `null` relations may unintentionally exist but aren't related. If you're using `relayPagination` then this fix is critical.

## Investigation

When a result comes back from the API it is both written to the cached, but the `cacheExchange` also queries it again from the API, to update its data using `resolvers`. This is a great technique to get any API result from looking the same as if it was directly queried from the cache.

For mutations and subscriptions this is also nice, because we have a special `readRoot` case that copies values over from the `originalData`, since not all fields on "root results" are cached, since they're not normalised. This is easily confused with `readSelection`'s concept of `data` which is the **target** where we write results too. At some point we must've gotten either confused or assumed that it'd be great for `readSelection` (which is for normalised data) to _also_ use the original API data as its `data` input.

This becomes a problem because it's not necessary and can cause bugs. It's not necessary because we're dealing with normalised data, so the data should be completely queryable from the cache and reusing the original data will just cause confusion. It also causes bugs — which is how @sarmeyer discovered this — because if a resolver returns a list of items that in turn have links (i.e. relations) to other entities, then the result's original data may have items in a different order or length. This data will still be used, but if it's set to `null` then another special case causes the field to be considered `null` again, whether that's actually correct **or not**, see: https://github.com/FormidableLabs/urql/blob/eee828914fb6083b2aaa3203484ed6b36b7beff5/exchanges/graphcache/src/operations/query.ts#L499

In this line we check whether `prevData === null`. This is important because a past selection set may have contained _uncached_ fields but a future one for the same path in the query may not, which would mean that the data would be returned even though it contained _uncached fields_. This test illustrates why this check exists: https://github.com/FormidableLabs/urql/blob/eee828914fb6083b2aaa3203484ed6b36b7beff5/exchanges/graphcache/src/operations/query.test.ts#L225-L233

But when we use the API result as an input then `prevData` isn't just the data that Graphcache built up from its cache, it's also the data from the API result that's being reused. And when a list contains unrelated items with `null` fields then this check will instead cause this relation to become `null` although the cached data exists.

Another side-effect of this bug is that because API results are reused, some references/objects/instances are _overwritten_ by Graphcache, which is bad. It should never mutate data it gets from the API, but just write it to its cache then create a _new_ query result.

## Set of changes

- Add a reproduction test case
- Fix some tests that should've warned us about this being a problem 🤦 
- Ensure that `readSelection` in `read` always gets a _new, empty_ object to write to.

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

Latest commit: 1438102192f98a9a43a663307ea9a7cbe3f83188

**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/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/FormidableLabs/urql/new/fix/graphcache-reused-original-data?filename=.changeset/rude-tigers-vanish.md&value=---%0A%22%40urql%2Fexchange-graphcache%22%3A%20patch%0A---%0A%0AFix%2Fgraphcache%20reused%20original%20data%0A)



Patch:
diff --git a/.changeset/wise-rats-whisper.md b/.changeset/wise-rats-whisper.md
new file mode 100644
index 00000000..29b9e418
--- /dev/null
+++ b/.changeset/wise-rats-whisper.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix reusing original query data from APIs accidentally, which can lead to subtle mismatches in results when the API's incoming `query` results are being updated by the `cacheExchange`, to apply resolvers. Specifically this may lead to relations from being set back to `null` when the resolver returns a different list of links than the result, since some `null` relations may unintentionally exist but aren't related. If you're using `relayPagination` then this fix is critical.
diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts
index d40ed7cc..b8bd9425 100644
--- a/exchanges/graphcache/src/operations/query.ts
+++ b/exchanges/graphcache/src/operations/query.ts
@@ -90,11 +90,14 @@ export const read = (
     pushDebugNode(rootKey, operation);
   }
 
-  let data: Data | undefined = input || ({} as Data);
-  data =
+  // NOTE: This may reuse "previous result data" as indicated by the
+  // `originalData` argument in readRoot(). This behaviour isn't used
+  // for readSelection() however, which always produces results from
+  // scratch
+  const data =
     rootKey !== ctx.store.rootFields['query']
-      ? readRoot(ctx, rootKey, rootSelect, data)
-      : readSelection(ctx, rootKey, rootSelect, data);
+      ? readRoot(ctx, rootKey, rootSelect, input || ({} as Data))
+      : readSelection(ctx, rootKey, rootSelect, {} as Data);
 
   if (process.env.NODE_ENV !== 'production') {
     popDebugNode();
@@ -102,8 +105,8 @@ export const read = (
 
   return {
     dependencies: getCurrentDependencies(),
-    partial: data === undefined ? false : ctx.partial,
-    data: data === undefined ? null : data,
+    partial: ctx.partial || !data,
+    data: data || null,
   };
 };
 
@@ -122,10 +125,10 @@ const readRoot = (
   data.__typename = originalData.__typename;
 
   let node: FieldNode | void;
-  while ((node = iterate()) !== undefined) {
+  while ((node = iterate())) {
     const fieldAlias = getFieldAlias(node);
     const fieldValue = originalData[fieldAlias];
-    if (node.selectionSet !== undefined && fieldValue !== null) {
+    if (node.selectionSet && fieldValue !== null) {
       const fieldData = ensureData(fieldValue);
       data[fieldAlias] = readRootField(ctx, getSelectionSet(node), fieldData);
     } else {
@@ -171,7 +174,7 @@ export const readFragment = (
   const fragments = getFragments(query);
   const names = Object.keys(fragments);
   const fragment = fragments[names[0]] as FragmentDefinitionNode;
-  if (fragment === undefined) {
+  if (!fragment) {
     warn(
       'readFragment(...) was called with an empty fragment.\n' +
         'You have to call it with at least one fragment in your GraphQL document.',
@@ -323,7 +326,7 @@ const readSelection = (
         ctx
       );
 
-      if (node.selectionSet !== undefined) {
+      if (node.selectionSet) {
         // When it has a selection set we are resolving an entity with a
         // subselection. This can either be a list or an object.
         dataFieldValue = resolveResolverResult(
@@ -332,7 +335,7 @@ const readSelection = (
           fieldName,
           key,
           getSelectionSet(node),
-          (data[fieldAlias] || {}) as Data,
+          data[fieldAlias] as Data,
           dataFieldValue
         );
       }
@@ -346,7 +349,7 @@ const readSelection = (
         // current field
         return undefined;
       }
-    } else if (node.selectionSet === undefined) {
+    } else if (!node.selectionSet) {
       // The field is a scalar but isn't on the result, so it's retrieved from the cache
       dataFieldValue = fieldValue;
     } else if (resultValue !== undefined) {
diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts
index 3a3a42d4..950da5e6 100644
--- a/exchanges/graphcache/src/operations/write.ts
+++ b/exchanges/graphcache/src/operations/write.ts
@@ -144,7 +144,7 @@ export const writeFragment = (
   const fragments = getFragments(query);
   const names = Object.keys(fragments);
   const fragment = fragments[names[0]] as FragmentDefinitionNode;
-  if (fragment === undefined) {
+  if (!fragment) {
     return warn(
       'writeFragment(...) was called with an empty fragment.\n' +
         'You have to call it with at least one fragment in your GraphQL document.',
@@ -336,7 +336,7 @@ const writeField = (
 
   if (
     parentFieldKey &&
-    ctx.store.keys[data.__typename] === undefined &&
+    !ctx.store.keys[data.__typename] &&
     entityKey === null &&
     typeof typename === 'string' &&
     !KEYLESS_TYPE_RE.test(typename)


Test patch:
diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts
index f776d1e6..b36bc959 100644
--- a/exchanges/graphcache/src/cacheExchange.test.ts
+++ b/exchanges/graphcache/src/cacheExchange.test.ts
@@ -1062,9 +1062,7 @@ describe('custom resolvers', () => {
     expect(fakeResolver).toHaveBeenCalledTimes(1);
     expect(result).toHaveBeenCalledTimes(1);
     expect(result.mock.calls[0][0].data).toEqual({
-      __typename: 'Query',
       author: {
-        __typename: 'Author',
         id: '123',
         name: 'newName',
       },
@@ -1430,7 +1428,6 @@ describe('schema awareness', () => {
     expect(response).toHaveBeenCalledTimes(1);
     expect(reexec).toHaveBeenCalledTimes(0);
     expect(result.mock.calls[0][0].data).toEqual({
-      __typename: 'Query',
       todos: [
         {
           __typename: 'Todo',
diff --git a/exchanges/graphcache/src/offlineExchange.test.ts b/exchanges/graphcache/src/offlineExchange.test.ts
index f10c0557..6c170a03 100644
--- a/exchanges/graphcache/src/offlineExchange.test.ts
+++ b/exchanges/graphcache/src/offlineExchange.test.ts
@@ -168,7 +168,10 @@ describe('offline', () => {
 
     next(queryOp);
     expect(result).toBeCalledTimes(1);
-    expect(result.mock.calls[0][0].data).toEqual(queryOneData);
+    expect(result.mock.calls[0][0].data).toEqual({
+      ...queryOneData,
+      __typename: undefined,
+    });
 
     next(mutationOp);
     expect(result).toBeCalledTimes(1);
diff --git a/exchanges/graphcache/src/operations/query.test.ts b/exchanges/graphcache/src/operations/query.test.ts
index 94cdbb29..f6524ca3 100644
--- a/exchanges/graphcache/src/operations/query.test.ts
+++ b/exchanges/graphcache/src/operations/query.test.ts
@@ -3,6 +3,7 @@
 import { gql } from '@urql/core';
 import { minifyIntrospectionQuery } from '@urql/introspection';
 
+import { Data } from '../types';
 import { Store } from '../store';
 import { write } from './write';
 import { query } from './query';
@@ -130,6 +131,85 @@ describe('Query', () => {
     expect((console.warn as any).mock.calls[0][0]).toMatch(/writer/);
   });
 
+  it('should not overwrite different arrays from results and queries', () => {
+    const TODO_QUERY = gql`
+      query Todos {
+        todos {
+          __typename
+          node {
+            __typename
+            id
+            text
+            author {
+              __typename
+              id
+            }
+          }
+        }
+      }
+    `;
+
+    store = new Store({
+      resolvers: {
+        Query: {
+          todos: (_parent, _args, cache) => cache.resolve('Query', 'todos'),
+        },
+      },
+    });
+
+    const expected = ({
+      todos: [
+        {
+          __typename: 'TodoEdge',
+          node: {
+            __typename: 'Todo',
+            id: '0',
+            text: 'Teach',
+            author: {
+              __typename: 'Author',
+              id: 'writy-mcwriteface',
+            },
+          },
+        },
+        {
+          __typename: 'TodoEdge',
+          node: {
+            __typename: 'Todo',
+            id: '1',
+            text: 'Learn',
+            author: null,
+          },
+        },
+      ],
+    } as any) as Data;
+
+    write(store, { query: TODO_QUERY }, expected);
+
+    const result = query(
+      store,
+      { query: TODO_QUERY },
+      {
+        __typename: 'Query',
+        todos: [
+          // NOTE: This is a partial list of later results
+          {
+            __typename: 'TodoEdge',
+            node: {
+              id: '1',
+              text: 'Learn',
+              __typename: 'Todo',
+              author: null,
+            },
+          },
+        ],
+      }
+    );
+
+    expect(result.data).toEqual(expected);
+    expect(result.data).not.toBe(expected);
+    expect(result.data!.todos![0]).not.toBe(expected!.todos![0]);
+  });
+
   // Issue#64
   it('should not crash for valid queries', () => {
     const VALID_QUERY = gql`