Increment queryInfo.lastRequestId only when using links to get results (

#7956) Co-authored-by: Daniel <dcochran@netflix.com> Co-authored-by: Ben Newman <ben@apollographql.com>
apollographql · Apr 7, 2021 · 35158e3 · 35158e3
1 parent deaa2d9
commit 35158e3
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 30 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,9 @@
 ## Apollo Client 3.4.0 (not yet released)
 
 ### Bug fixes
-TBD
+
+- Increment `queryInfo.lastRequestId` only when making a network request through the `ApolloLink` chain, rather than every time `fetchQueryByPolicy` is called. <br/>
+  [@dannycochran](https://github.com/dannycochran) in [#7956](https://github.com/apollographql/apollo-client/pull/7956)
 
 ### Potentially breaking changes
 

diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts
@@ -855,7 +855,7 @@ export class QueryManager<TStore> {
       | "fetchPolicy"
       | "errorPolicy">,
   ): Observable<ApolloQueryResult<TData>> {
-    const { lastRequestId } = queryInfo;
+    const requestId = queryInfo.lastRequestId = this.generateRequestId();
 
     return asyncMap(
       this.getObservableFromLink(
@@ -867,7 +867,9 @@ export class QueryManager<TStore> {
       result => {
         const hasErrors = isNonEmptyArray(result.errors);
 
-        if (lastRequestId >= queryInfo.lastRequestId) {
+        // If we interrupted this request by calling getResultsFromLink again
+        // with the same QueryInfo object, we ignore the old results.
+        if (requestId >= queryInfo.lastRequestId) {
           if (hasErrors && options.errorPolicy === "none") {
             // Throwing here effectively calls observer.error.
             throw queryInfo.markError(new ApolloError({
@@ -896,7 +898,8 @@ export class QueryManager<TStore> {
           ? networkError
           : new ApolloError({ networkError });
 
-        if (lastRequestId >= queryInfo.lastRequestId) {
+        // Avoid storing errors from older interrupted queries.
+        if (requestId >= queryInfo.lastRequestId) {
           queryInfo.markError(error);
         }
 
@@ -1059,7 +1062,6 @@ export class QueryManager<TStore> {
     queryInfo.init({
       document: query,
       variables,
-      lastRequestId: this.generateRequestId(),
       networkStatus,
     });
 

diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts
@@ -15,6 +15,28 @@ import mockQueryManager from '../../utilities/testing/mocking/mockQueryManager';
 import mockWatchQuery from '../../utilities/testing/mocking/mockWatchQuery';
 import wrap from '../../utilities/testing/wrap';
 
+export const mockFetchQuery = (queryManager: QueryManager<any>) => {
+  const fetchQueryObservable = queryManager.fetchQueryObservable;
+  const fetchQueryByPolicy: QueryManager<any>["fetchQueryByPolicy"] =
+    (queryManager as any).fetchQueryByPolicy;
+
+  const mock = <T extends
+    | typeof fetchQueryObservable
+    | typeof fetchQueryByPolicy
+  >(original: T) => jest.fn<ReturnType<T>, Parameters<T>>(function () {
+    return original.apply(queryManager, arguments);
+  });
+
+  const mocks = {
+    fetchQueryObservable: mock(fetchQueryObservable),
+    fetchQueryByPolicy: mock(fetchQueryByPolicy),
+  };
+
+  Object.assign(queryManager, mocks);
+
+  return mocks;
+};
+
 describe('ObservableQuery', () => {
   // Standard data for all these tests
   const query = gql`
@@ -929,31 +951,6 @@ describe('ObservableQuery', () => {
   });
 
   describe('refetch', () => {
-    function mockFetchQuery(queryManager: QueryManager<any>) {
-      const fetchQueryObservable = queryManager.fetchQueryObservable;
-      const fetchQueryByPolicy: QueryManager<any>["fetchQueryByPolicy"] =
-        (queryManager as any).fetchQueryByPolicy;
-
-      const mock = <T extends
-        | typeof fetchQueryObservable
-        | typeof fetchQueryByPolicy
-      >(original: T) => jest.fn<
-        ReturnType<T>,
-        Parameters<T>
-      >(function () {
-        return original.apply(queryManager, arguments);
-      });
-
-      const mocks = {
-        fetchQueryObservable: mock(fetchQueryObservable),
-        fetchQueryByPolicy: mock(fetchQueryByPolicy),
-      };
-
-      Object.assign(queryManager, mocks);
-
-      return mocks;
-    }
-
     itAsync('calls fetchRequest with fetchPolicy `network-only` when using a non-networked fetch policy', (resolve, reject) => {
       const mockedResponses = [
         {

diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts
@@ -37,6 +37,7 @@ import subscribeAndCount from '../../../utilities/testing/subscribeAndCount';
 import { stripSymbols } from '../../../utilities/testing/stripSymbols';
 import { itAsync } from '../../../utilities/testing/itAsync';
 import { ApolloClient } from '../../../core'
+import { mockFetchQuery } from '../ObservableQuery';
 
 interface MockedMutation {
   reject: (reason: any) => any;
@@ -2722,6 +2723,64 @@ describe('QueryManager', () => {
     ]).then(resolve, reject);
   });
 
+
+  itAsync('only increments "queryInfo.lastRequestId" when fetching data from network', (resolve, reject) => {
+    const query = gql`
+      query query($id: ID!) {
+        people_one(id: $id) {
+          name
+        }
+      }
+    `;
+    const variables = { id: 1 };
+    const dataOne = {
+      people_one: {
+        name: 'Luke Skywalker',
+      },
+    };
+    const mockedResponses = [
+      {
+        request: { query, variables },
+        result: { data: dataOne },
+      },
+    ];
+
+    const queryManager = mockQueryManager(reject, ...mockedResponses);
+    const queryOptions: WatchQueryOptions<any> = {
+      query,
+      variables,
+      fetchPolicy: 'cache-and-network',
+    };
+    const observable = queryManager.watchQuery(queryOptions);
+
+    const mocks = mockFetchQuery(queryManager);
+    const queryId = '1';
+    const getQuery: QueryManager<any>["getQuery"] =
+      (queryManager as any).getQuery.bind(queryManager);
+
+    subscribeAndCount(reject, observable, async (handleCount) => {
+      const query = getQuery(queryId);
+      const fqbpCalls = mocks.fetchQueryByPolicy.mock.calls;
+      expect(query.lastRequestId).toEqual(1);
+      expect(fqbpCalls.length).toBe(1);
+
+      // Simulate updating the options of the query, which will trigger
+      // fetchQueryByPolicy, but it should just read from cache and not
+      // update "queryInfo.lastRequestId". For more information, see
+      // https://github.com/apollographql/apollo-client/pull/7956#issue-610298427
+      await observable.setOptions({
+        ...queryOptions,
+        fetchPolicy: 'cache-first',
+      });
+
+      // "fetchQueryByPolicy" was called, but "lastRequestId" does not update
+      // since it was able to read from cache.
+      expect(query.lastRequestId).toEqual(1);
+      expect(fqbpCalls.length).toBe(2);
+      resolve();
+    });
+  })
+
   describe('polling queries', () => {
     itAsync('allows you to poll queries', (resolve, reject) => {
       const query = gql`