PersistedQueryLink: do not permanently skip persisted queries after…

… a 400 or 500 status code (#10806) * PersistedQueryLink: do not permanently skip persisted queries after a 400 or 500 status code recreates commits on current `main`: ee35ea3 152e77d a84a14e 698e943 * Update src/link/persisted-queries/__tests__/persisted-queries.test.ts Co-authored-by: Jerel Miller <[email protected]> * Update src/link/persisted-queries/__tests__/persisted-queries.test.ts Co-authored-by: Jerel Miller <[email protected]> * rename `retryQuery` option to `retry` and `retry` to `handlePossibleRetry` * remove `zenObservableToPromise` helpers * revert exports test changes * fixup half-rename of `retry`, ensure types * do not expose `byMessage` and `byCode` * changes from PR discussion --------- Co-authored-by: Jerel Miller <[email protected]>
apollographql · Jun 20, 2023 · cb15405 · cb15405
1 parent f26ff2e
commit cb15405
Show file tree

Hide file tree

Showing 3 changed files with 126 additions and 174 deletions.
diff --git a/.changeset/angry-ducks-explain.md b/.changeset/angry-ducks-explain.md
@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Fix a bug in `PersistedQueryLink` that would cause it to permanently skip persisted queries after a 400 or 500 status code.
diff --git a/src/link/persisted-queries/__tests__/persisted-queries.test.ts b/src/link/persisted-queries/__tests__/persisted-queries.test.ts
@@ -10,6 +10,7 @@ import { createHttpLink } from '../../http/createHttpLink';
 
 import { createPersistedQueryLink as createPersistedQuery, VERSION } from '..';
 import { itAsync } from '../../../testing';
+import { toPromise } from '../../utils';
 
 // Necessary configuration in order to mock multiple requests
 // to a single (/graphql) endpoint
@@ -436,129 +437,103 @@ describe('failure path', () => {
     }, reject);
   });
 
-  itAsync('handles a 500 network error and still retries', (resolve, reject) => {
-    let failed = false;
-    fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response})), { repeat: 1 });
-
-    // mock it again so we can verify it doesn't try anymore
+  describe.each([[400],[500]])('status %s', (status) => {
+    itAsync(`handles a ${status} network with a "PERSISTED_QUERY_NOT_FOUND" error and still retries`, (resolve, reject) => {
+      let requestCount = 0;
       fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response})), { repeat: 1 });
 
-    const fetcher = (...args: any[]) => {
-      if (!failed) {
-        failed = true;
-        return Promise.resolve({
-          json: () => Promise.resolve('This will blow up'),
-          text: () => Promise.resolve('THIS WILL BLOW UP'),
-          status: 500,
-        });
-      }
-      return global.fetch.apply(null, args);
-    };
-    const link = createPersistedQuery({ sha256 }).concat(
-      createHttpLink({ fetch: fetcher } as any),
-    );
+      // mock it again so we can verify it doesn't try anymore
+      fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response })), { repeat: 5 });
 
-    execute(link, { query, variables }).subscribe(result => {
-      expect(result.data).toEqual(data);
-      const [[,success]] = fetchMock.calls();
-      expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
-      expect(JSON.parse(success!.body!.toString()).extensions).toBeUndefined();
-      execute(link, { query, variables }).subscribe(secondResult => {
-        expect(secondResult.data).toEqual(data);
-        const [,[,success]] = fetchMock.calls();
+      const fetcher = (...args: any[]) => {
+        if ((++requestCount)%2) {
+          return Promise.resolve({
+            json: () => Promise.resolve(errorResponseWithCode),
+            text: () => Promise.resolve(errorResponseWithCode),
+            status,
+          });
+        }
+        return global.fetch.apply(null, args);
+      };
+      const link = createPersistedQuery({ sha256 }).concat(
+        createHttpLink({ fetch: fetcher } as any),
+      );
+
+      execute(link, { query, variables }).subscribe(result => {
+        expect(result.data).toEqual(data);
+        const [[,success]] = fetchMock.calls();
         expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
-        expect(JSON.parse(success!.body!.toString()).extensions).toBeUndefined();
-        resolve();
+        expect(JSON.parse(success!.body!.toString()).extensions.persistedQuery.sha256Hash).toBe(hash);
+        execute(link, { query, variables }).subscribe(secondResult => {
+          expect(secondResult.data).toEqual(data);
+          const [,[,success]] = fetchMock.calls();
+          expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
+          expect(JSON.parse(success!.body!.toString()).extensions.persistedQuery.sha256Hash).toBe(hash);
+          resolve();
+        }, reject);
       }, reject);
-    }, reject);
-  });
-
-  itAsync('handles a 400 network error and still retries', (resolve, reject) => {
-    let failed = false;
-    fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response})), { repeat: 1 });
+    });
 
-    // mock it again so we can verify it doesn't try anymore
+    it(`will fail on an unrelated ${status} network error, but still send a hash the next request`, async () => {
+      let failed = false;
       fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response})), { repeat: 1 });
 
-    const fetcher = (...args: any[]) => {
-      if (!failed) {
-        failed = true;
-        return Promise.resolve({
-          json: () => Promise.resolve('This will blow up'),
-          text: () => Promise.resolve('THIS WILL BLOW UP'),
-          status: 400,
-        });
-      }
-      return global.fetch.apply(null, args);
-    };
-    const link = createPersistedQuery({ sha256 }).concat(
-      createHttpLink({ fetch: fetcher } as any),
-    );
+      // mock it again so we can verify it doesn't try anymore
+      fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response })), { repeat: 1 });
+
+      const fetcher = (...args: any[]) => {
+        if (!failed) {
+          failed = true;
+          return Promise.resolve({
+            json: () => Promise.resolve('This will blow up'),
+            text: () => Promise.resolve('THIS WILL BLOW UP'),
+            status,
+          });
+        }
+        return global.fetch.apply(null, args);
+      };
+      const link = createPersistedQuery({ sha256 }).concat(
+        createHttpLink({ fetch: fetcher } as any),
+      );
 
-    execute(link, { query, variables }).subscribe(result => {
-      expect(result.data).toEqual(data);
+      const failingAttempt = toPromise(execute(link, { query, variables }))
+      await expect(failingAttempt).rejects.toThrow()
+      expect(fetchMock.calls().length).toBe(0)
+
+      const successfullAttempt = toPromise(execute(link, { query, variables }))
+      await expect(successfullAttempt).resolves.toEqual({data})
       const [[,success]] = fetchMock.calls();
-      expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
-      expect(JSON.parse(success!.body!.toString()).extensions).toBeUndefined();
-      execute(link, { query, variables }).subscribe(secondResult => {
-        expect(secondResult.data).toEqual(data);
-        const [,[,success]] = fetchMock.calls();
-        expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
-        expect(JSON.parse(success!.body!.toString()).extensions).toBeUndefined();
-        resolve();
-      }, reject);
-    }, reject);
-  });
-
-  itAsync('only retries a 400 network error once', (resolve, reject) => {
-    let fetchCalls = 0;
-    const fetcher = () => {
-      fetchCalls++;
-      return Promise.resolve({
-        json: () => Promise.resolve('This will blow up'),
-        text: () => Promise.resolve('THIS WILL BLOW UP'),
-        status: 400,
-      });
-    };
-    const link = createPersistedQuery({ sha256 }).concat(
-      createHttpLink({ fetch: fetcher } as any),
-    );
-
-    execute(link, { query, variables }).subscribe(
-      result => reject,
-      error => {
-        expect(fetchCalls).toBe(2);
-        resolve();
-      },
-    );
-  });
+      expect(JSON.parse(success!.body!.toString()).query).toBeUndefined();
+      expect(JSON.parse(success!.body!.toString()).extensions.persistedQuery.sha256Hash).toBe(hash);
+    });
 
-  itAsync('handles 400 response network error and graphql error without disabling persistedQuery support', (resolve, reject) => {
-    let failed = false;
-    fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response})), { repeat: 1 });
+    itAsync(`handles ${status} response network error and graphql error without disabling persistedQuery support`, (resolve, reject) => {
+      let failed = false;
+      fetchMock.post("/graphql", () => new Promise(resolve => resolve({ body: response})), { repeat: 1 });
 
-    const fetcher = (...args: any[]) => {
-      if (!failed) {
-        failed = true;
-        return Promise.resolve({
-          json: () => Promise.resolve(errorResponse),
-          text: () => Promise.resolve(errorResponse),
-          status: 400,
-        });
-      }
-      return global.fetch.apply(null, args);
-    };
+      const fetcher = (...args: any[]) => {
+        if (!failed) {
+          failed = true;
+          return Promise.resolve({
+            json: () => Promise.resolve(errorResponse),
+            text: () => Promise.resolve(errorResponse),
+            status,
+          });
+        }
+        return global.fetch.apply(null, args);
+      };
 
-    const link = createPersistedQuery({ sha256 }).concat(
-      createHttpLink({ fetch: fetcher } as any),
-    );
+      const link = createPersistedQuery({ sha256 }).concat(
+        createHttpLink({ fetch: fetcher } as any),
+      );
 
-    execute(link, { query, variables }).subscribe(result => {
-      expect(result.data).toEqual(data);
-      const [[,success]] = fetchMock.calls();
-      expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
-      expect(JSON.parse(success!.body!.toString()).extensions).not.toBeUndefined();
-      resolve();
-    }, reject);
+      execute(link, { query, variables }).subscribe(result => {
+        expect(result.data).toEqual(data);
+        const [[,success]] = fetchMock.calls();
+        expect(JSON.parse(success!.body!.toString()).query).toBe(queryString);
+        expect(JSON.parse(success!.body!.toString()).extensions).not.toBeUndefined();
+        resolve();
+      }, reject);
+    });
   });
 });
diff --git a/src/link/persisted-queries/index.ts b/src/link/persisted-queries/index.ts
@@ -25,20 +25,24 @@ export interface ErrorResponse {
   networkError?: NetworkError;
   response?: ExecutionResult;
   operation: Operation;
+  meta: ErrorMeta;
+}
+
+type ErrorMeta = {
+  persistedQueryNotSupported: boolean;
+  persistedQueryNotFound: boolean;
 }
 
 type SHA256Function = (...args: any[]) => string | PromiseLike<string>;
 type GenerateHashFunction = (document: DocumentNode) => string | PromiseLike<string>;
 
-const PERSISTED_QUERY_NOT_SUPPORTED = "PERSISTED_QUERY_NOT_SUPPORTED";
-const PERSISTED_QUERY_NOT_FOUND = "PERSISTED_QUERY_NOT_FOUND"
+interface BaseOptions {
+  disable?: (error: ErrorResponse) => boolean;
+  retry?: (error: ErrorResponse) => boolean;
+  useGETForHashedQueries?: boolean;
+};
 
 export namespace PersistedQueryLink {
-  interface BaseOptions {
-    disable?: (error: ErrorResponse, errorsByMessageAndCode: ErrorsByMessageAndCode) => boolean;
-    useGETForHashedQueries?: boolean;
-  };
-
   interface SHA256Options extends BaseOptions {
     sha256: SHA256Function;
     generateHash?: never;
@@ -52,55 +56,28 @@ export namespace PersistedQueryLink {
   export type Options = SHA256Options | GenerateHashOptions;
 }
 
-type ErrorsByMessageAndCode = { byMessage: Record<string, GraphQLError>; byCode: Record<string, GraphQLError> }
-
-function collectErrorsByMessageAndCode(
+function processErrors(
   graphQLErrors: GraphQLError[] | readonly GraphQLError[] | undefined
-): ErrorsByMessageAndCode {
-  const collected: ErrorsByMessageAndCode = {
-    byMessage: Object.create(null),
-    byCode: Object.create(null),
-  };
+): ErrorMeta {
+  const byMessage = Object.create(null),
+        byCode = Object.create(null);
+
   if (isNonEmptyArray(graphQLErrors)) {
     graphQLErrors.forEach((error) => {
-      collected.byMessage[error.message] = error;
-      if (typeof error.extensions?.code === "string")
-        collected.byCode[error.extensions.code] = error;
+      byMessage[error.message] = error;
+      if (typeof error.extensions?.code == "string")
+        byCode[error.extensions.code] = error;
     });
   }
-  return collected;
+  return {
+    persistedQueryNotSupported: !!(byMessage.PersistedQueryNotSupported || byCode.PERSISTED_QUERY_NOT_SUPPORTED),
+    persistedQueryNotFound: !!(byMessage.PersistedQueryNotFound || byCode.PERSISTED_QUERY_NOT_FOUND),
+  };
 }
 
-const defaultOptions = {
-  disable: ({ operation }: ErrorResponse, {byMessage, byCode}: ErrorsByMessageAndCode) => {
-    // if the server doesn't support persisted queries, don't try anymore
-    if (
-      byMessage.PersistedQueryNotSupported ||
-      byCode[PERSISTED_QUERY_NOT_SUPPORTED]
-    ) {
-      return true;
-    }
-
-    if (
-      byMessage.PersistedQueryNotFound ||
-      byCode[PERSISTED_QUERY_NOT_FOUND]
-    ) {
-      return false;
-    }
-
-    const { response } = operation.getContext();
-    // if the server responds with bad request
-    // Apollo Server responds with 400 for GET and 500 for POST when no query is found
-    if (
-      response &&
-      response.status &&
-      (response.status === 400 || response.status === 500)
-    ) {
-      return true;
-    }
-
-    return false;
-  },
+const defaultOptions: Required<BaseOptions> = {
+  disable: ({ meta }) => meta.persistedQueryNotSupported,
+  retry: ({ meta }) => meta.persistedQueryNotSupported || meta.persistedQueryNotFound,
   useGETForHashedQueries: false,
 };
 
@@ -139,10 +116,10 @@ export const createPersistedQueryLink = (
     // `sha256` option will be ignored. Developers can configure and
     // use any hashing approach they want in a custom `generateHash`
     // function; they aren't limited to SHA-256.
-    generateHash = (query: DocumentNode) =>
-      Promise.resolve<string>(sha256!(print(query))),
+    generateHash = (query: DocumentNode) => Promise.resolve<string>(sha256!(print(query))),
     disable,
-    useGETForHashedQueries
+    retry,
+    useGETForHashedQueries,
   } = compact(defaultOptions, options);
 
   let supportsPersistedQueries = true;
@@ -175,7 +152,7 @@ export const createPersistedQueryLink = (
       let retried = false;
       let originalFetchOptions: any;
       let setFetchOptions = false;
-      const retry = (
+      const maybeRetry = (
         {
           response,
           networkError,
@@ -204,24 +181,19 @@ export const createPersistedQueryLink = (
             graphQLErrors.push(...networkErrors);
           }
 
-          const disablePayload = {
+          const disablePayload: ErrorResponse = {
             response,
             networkError,
             operation,
             graphQLErrors: isNonEmptyArray(graphQLErrors) ? graphQLErrors : void 0,
+            meta: processErrors(graphQLErrors)
           };
 
-          const errorsByMessageAndCode = collectErrorsByMessageAndCode(graphQLErrors);
-
           // if the server doesn't support persisted queries, don't try anymore
-          supportsPersistedQueries = !disable(disablePayload, errorsByMessageAndCode);
+          supportsPersistedQueries = !disable(disablePayload);
 
           // if its not found, we can try it again, otherwise just report the error
-          if (
-            errorsByMessageAndCode.byMessage.PersistedQueryNotFound ||
-            errorsByMessageAndCode.byCode[PERSISTED_QUERY_NOT_FOUND] ||
-            !supportsPersistedQueries
-          ) {
+          if (retry(disablePayload)) {
             // need to recall the link chain
             if (subscription) subscription.unsubscribe();
             // actually send the query this time
@@ -249,10 +221,10 @@ export const createPersistedQueryLink = (
       };
       const handler = {
         next: (response: ExecutionResult) => {
-          retry({ response }, () => observer.next!(response));
+          maybeRetry({ response }, () => observer.next!(response));
         },
         error: (networkError: ServerError) => {
-          retry({ networkError }, () => observer.error!(networkError));
+          maybeRetry({ networkError }, () => observer.error!(networkError));
         },
         complete: observer.complete!.bind(observer),
       };