Merge pull request #3367 from NewSpring/fetch-more-query-undefined-fix

Add `QueryStore` safeguards to prevent invalid `fetchMoreForQueryId` network status setting attempts
apollographql · May 25, 2018 · b4ff8a4 · b4ff8a4
2 parents e7fb8b1 + 7a6c30f
commit b4ff8a4
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 7 deletions.
diff --git a/package.json b/package.json
@@ -11,16 +11,14 @@
     "benchmark": "cd packages/apollo-client && npm run benchmark",
     "prelint": "npm run lint-fix",
     "lint": "lerna run -- lint",
-    "lint-fix":
-      "prettier --trailing-comma all --single-quote --write \"packages/*/{src,tests,test,benchmark}/**/*.{j,t}s*\"",
+    "lint-fix": "prettier --trailing-comma all --single-quote --write \"packages/*/{src,tests,test,benchmark}/**/*.{j,t}s*\"",
     "lint-staged": "lint-staged",
     "filesize": "lerna run -- filesize && bundlesize",
     "type-check": "lerna run -- type-check",
     "coverage": "lerna run -- coverage",
     "coverage:upload": "codecov",
     "danger": "danger run --verbose",
-    "deploy":
-      "lerna publish -m \"chore: Publish\" --independent && cd packages/apollo-client && npm run deploy"
+    "deploy": "lerna publish -m \"chore: Publish\" --independent && cd packages/apollo-client && npm run deploy"
   },
   "bundlesize": [
     {
@@ -64,7 +62,10 @@
       "prettier --trailing-comma all --single-quote --write",
       "git add"
     ],
-    "*.js*": ["prettier --trailing-comma all --single-quote --write", "git add"]
+    "*.js*": [
+      "prettier --trailing-comma all --single-quote --write",
+      "git add"
+    ]
   },
   "pre-commit": "lint-staged",
   "devDependencies": {

diff --git a/packages/apollo-client/CHANGELOG.md b/packages/apollo-client/CHANGELOG.md
@@ -6,6 +6,11 @@
   (Android), by instead setting the `prototype` of `this` manually.
   [Issue #3236](https://github.com/apollographql/apollo-client/issues/3236)
   [PR #3306](https://github.com/apollographql/apollo-client/pull/3306)
+- Added safeguards to make sure `QueryStore.initQuery` and
+  `QueryStore.markQueryResult` don't try to set the network status of a
+  `fetchMoreForQueryId` query, if it does not exist in the store.
+  [Issue #3345](https://github.com/apollographql/apollo-client/issues/3345)
+  [PR #3367](https://github.com/apollographql/apollo-client/pull/3367)
 
 ### 2.3.0
 - fixed edge case bug of changing fetchPolicies right after resetStore with no variables present

diff --git a/packages/apollo-client/src/data/__tests__/queries.ts b/packages/apollo-client/src/data/__tests__/queries.ts
@@ -0,0 +1,75 @@
+import { QueryStore } from '../queries';
+import { NetworkStatus } from '../../core/networkStatus';
+import { DocumentNode } from 'graphql';
+
+describe('QueryStore', () => {
+  const queryId = 'abc123';
+  let queryStore;
+
+  beforeEach(() => {
+    queryStore = new QueryStore();
+    queryStore.initQuery({
+      queryId,
+      document: {} as DocumentNode,
+      storePreviousVariables: false,
+      variables: {},
+      isPoll: false,
+      isRefetch: false,
+      metadata: {},
+      fetchMoreForQueryId: queryId,
+    });
+  });
+
+  describe('initQuery', () => {
+    it(
+      'should set the network status of a query to `fetchMore` if the ' +
+        'query has a `fetchMoreForQueryId` property',
+      () => {
+        expect(queryStore.get(queryId).networkStatus).toBe(
+          NetworkStatus.fetchMore,
+        );
+      },
+    );
+
+    it(
+      'should not attempt to set the network status of a ' +
+        '`fetchMoreForQueryId` query, if it does not exist in the store',
+      () => {
+        queryStore.stopQuery(queryId);
+        expect(() => {
+          queryStore.initQuery({
+            queryId: 'new-query-id',
+            document: {} as DocumentNode,
+            storePreviousVariables: false,
+            variables: {},
+            isPoll: false,
+            isRefetch: false,
+            metadata: {},
+            fetchMoreForQueryId: queryId,
+          });
+        }).not.toThrow("Cannot set property 'networkStatus' of undefined");
+      },
+    );
+  });
+
+  describe('markQueryResult', () => {
+    it(
+      'should set the network status of a `fetchMoreForQueryId` query to ' +
+        '`ready` in the store, if it exists',
+      () => {
+        queryStore.markQueryResult(queryId, {}, queryId);
+        expect(queryStore.get(queryId).networkStatus).toBe(NetworkStatus.ready);
+      },
+    );
+
+    it(
+      'should not attempt to set the network status of a ' +
+        '`fetchMoreForQueryId` query, if it does not exist in the store',
+      () => {
+        expect(() => {
+          queryStore.markQueryResult(queryId, {}, 'id-does-not-exist');
+        }).not.toThrow("Cannot set property 'networkStatus' of undefined");
+      },
+    );
+  });
+});
diff --git a/packages/apollo-client/src/data/queries.ts b/packages/apollo-client/src/data/queries.ts
@@ -103,7 +103,10 @@ export class QueryStore {
     // error action branch, but importantly *not* in the client result branch.
     // This is because the implementation of `fetchMore` *always* sets
     // `fetchPolicy` to `network-only` so we would never have a client result.
-    if (typeof query.fetchMoreForQueryId === 'string') {
+    if (
+      typeof query.fetchMoreForQueryId === 'string' &&
+      this.store[query.fetchMoreForQueryId]
+    ) {
       this.store[query.fetchMoreForQueryId].networkStatus =
         NetworkStatus.fetchMore;
     }
@@ -125,7 +128,10 @@ export class QueryStore {
     // If we have a `fetchMoreForQueryId` then we need to update the network
     // status for that query. See the branch for query initialization for more
     // explanation about this process.
-    if (typeof fetchMoreForQueryId === 'string') {
+    if (
+      typeof fetchMoreForQueryId === 'string' &&
+      this.store[fetchMoreForQueryId]
+    ) {
       this.store[fetchMoreForQueryId].networkStatus = NetworkStatus.ready;
     }
   }