Fixed error hanling with bad JSON

networkteam · Feb 8, 2024 · 7109571 · 7109571
1 parent 33757e9
commit 7109571
Show file tree

Hide file tree

Showing 3 changed files with 147 additions and 52 deletions.
diff --git a/src/server/utils/dataLoader.ts b/src/server/utils/dataLoader.ts
@@ -38,17 +38,13 @@ export const loadDocumentProps = async (
     next: opts?.next,
   });
 
-  if (!response.ok) {
-    if (response.status === 404) {
-      log.debug('content API returned 404 for path', path);
+  if (response.status === 404) {
+    log.debug('content API returned 404 for path', path);
 
-      return undefined;
-    }
-
-    await handleNotOkResponse(response, fetchUrl);
+    return undefined;
   }
 
-  const data: NeosData = await response.json();
+  const data: NeosData = await parseResponse(fetchUrl, response);
   const endTime = Date.now();
   log.debug('fetched data from content API for path', path, ', took', `${endTime - startTime}ms`);
 
@@ -79,42 +75,19 @@ export const loadPreviewDocumentProps = async (
     cache: 'no-store',
   });
 
-  if (!response.ok) {
-    if (response.status === 404) {
-      log.debug('content API returned 404 for context path', contextPath);
+  if (response.status === 404) {
+    log.debug('content API returned 404 for context path', contextPath);
 
-      return undefined;
-    }
-
-    await handleNotOkResponse(response, fetchUrl);
+    return undefined;
   }
 
-  const data: NeosData = await response.json();
+  const data: NeosData = await parseResponse(fetchUrl, response);
   const endTime = Date.now();
   log.debug('fetched data from content API for context path', contextPath, ', took', `${endTime - startTime}ms`);
 
   return data;
 };
 
-async function handleNotOkResponse(response: Response, fetchUrl: string): Promise<never> {
-  try {
-    const data: ApiErrors = await response.json();
-    if (data.errors) {
-      throw new ApiError('Content API responded with errors', response.status, fetchUrl, data.errors);
-    }
-  } catch (e) {
-    // Ignore any error if response is not JSON
-  }
-
-  throw new ApiError(
-    'Content API responded with unexpected error',
-    response.status,
-    fetchUrl,
-    undefined,
-    await response.text()
-  );
-}
-
 export const loadDocumentPropsCached = cache(
   (routePath: string | undefined, opts?: DataLoaderOptions & OptionalOption) => {
     if (!routePath) {
@@ -170,22 +143,50 @@ export const loadSiteProps = async <CustomSiteData extends SiteData = SiteData>(
     next: opts?.next,
   });
 
-  if (!response.ok) {
-    const data: ApiErrors = await response.json();
-    if (data.errors) {
-      const flatErrors = data.errors.map((e) => e.message).join(', ');
-      log.error('error fetching from content API with url', fetchUrl, ':', flatErrors);
-      throw new Error('Content API responded with error: ' + flatErrors);
-    }
-  }
-
-  const data: CustomSiteData = await response.json();
+  const data: CustomSiteData = await parseResponse(fetchUrl, response);
   const endTime = Date.now();
   log.debug('fetched data from content API with url', fetchUrl, ', took', `${endTime - startTime}ms`);
 
   return data;
 };
 
+async function parseResponse<T>(fetchUrl: string, response: Response): Promise<T> {
+  if (!response.ok) {
+    return await handleNotOkResponse(response, fetchUrl);
+  }
+
+  const responseBody = await response.text();
+  try {
+    return JSON.parse(responseBody);
+  } catch (e) {
+    throw new ApiError(
+      'Content API responded with invalid JSON: ' + e,
+      response.status,
+      fetchUrl,
+      undefined,
+      responseBody
+    );
+  }
+}
+
+async function handleNotOkResponse(response: Response, fetchUrl: string): Promise<never> {
+  const responseBody = await response.text();
+
+  if (response.headers.get('content-type')?.startsWith('application/json')) {
+    let data: ApiErrors | undefined = undefined;
+    try {
+      data = JSON.parse(responseBody);
+    } catch (e) {
+      // Ignore any error if response is not JSON
+    }
+    if (data?.errors) {
+      throw new ApiError('Content API responded with errors', response.status, fetchUrl, data.errors);
+    }
+  }
+
+  throw new ApiError('Content API responded with unexpected error', response.status, fetchUrl, undefined, responseBody);
+}
+
 export const buildNeosPreviewHeaders = () => {
   const _headers = nextHeaders();
 

diff --git a/test/server/utils/__snapshots__/dataLoader.test.ts.snap b/test/server/utils/__snapshots__/dataLoader.test.ts.snap
@@ -1,5 +1,15 @@
 // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
 
+exports[`loadDocumentProps > with NEOS_BASE_URL set > with error as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with errors, status: 500, url: http://neos:1234/neos/content-api/document?path=%2Ffoo, errors: Some error]`;
+
 exports[`loadDocumentProps > with NEOS_BASE_URL set > with error not as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with unexpected error, status: 500, url: http://neos:1234/neos/content-api/document?path=%2Ffoo]`;
 
+exports[`loadDocumentProps > with NEOS_BASE_URL set > with ok but not as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with invalid JSON: SyntaxError: Unexpected token '<', "<html><bod"... is not valid JSON, status: 200, url: http://neos:1234/neos/content-api/document?path=%2Ffoo]`;
+
+exports[`loadPreviewDocumentProps > with NEOS_BASE_URL set > with Neos session cookie > with error as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with errors, status: 500, url: http://neos:1234/neos/content-api/document?contextPath=foo%2Fbar%40user-me, errors: Some error]`;
+
 exports[`loadPreviewDocumentProps > with NEOS_BASE_URL set > with Neos session cookie > with error not as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with unexpected error, status: 500, url: http://neos:1234/neos/content-api/document?contextPath=foo%2Fbar%40user-me]`;
+
+exports[`loadSiteProps > with NEOS_BASE_URL set > with error as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with errors, status: 500, url: http://neos:1234/neos/content-api/site, errors: Some error]`;
+
+exports[`loadSiteProps > with NEOS_BASE_URL set > with error not as JSON > should throw an error with the response status text 1`] = `[Error: Content API responded with unexpected error, status: 500, url: http://neos:1234/neos/content-api/site]`;
diff --git a/test/server/utils/dataLoader.test.ts b/test/server/utils/dataLoader.test.ts
@@ -19,7 +19,10 @@ describe('loadDocumentProps', () => {
       vi.stubEnv('NEOS_BASE_URL', 'http://neos:1234');
     });
 
-    describe.each<{ opts?: DataLoaderOptions; expectedFetchConfig: RequestInit }>([
+    describe.each<{
+      opts?: DataLoaderOptions;
+      expectedFetchConfig: RequestInit & { next?: { tags: string[] } | undefined };
+    }>([
       // No options
       { expectedFetchConfig: { cache: 'no-store', headers: {}, next: undefined } },
       // Specify cache
@@ -45,9 +48,31 @@ describe('loadDocumentProps', () => {
       });
     });
 
+    describe('with ok but not as JSON', () => {
+      it('should throw an error with the response status text', async () => {
+        const fetch = vi.fn().mockResolvedValue(createOkayButNotJsonFetchResponse());
+        vi.stubGlobal('fetch', fetch);
+
+        await expect(loadDocumentProps({ slug: 'foo' })).rejects.toThrowErrorMatchingSnapshot();
+      });
+    });
+
     describe('with error not as JSON', () => {
       it('should throw an error with the response status text', async () => {
-        const fetch = vi.fn().mockResolvedValue(createNotOkJsonErrorFetchResponse());
+        const fetch = vi.fn().mockResolvedValue(createNotOkJsonErrorFetchResponse({ 'Content-Type': 'text/html' }));
+        vi.stubGlobal('fetch', fetch);
+
+        await expect(loadDocumentProps({ slug: 'foo' })).rejects.toThrowErrorMatchingSnapshot();
+      });
+    });
+
+    describe('with error as JSON', () => {
+      it('should throw an error with the response status text', async () => {
+        const fetch = vi
+          .fn()
+          .mockResolvedValue(
+            createNotOkJsonErrorFetchResponse({ 'content-type': 'application/json' }, '{"errors":["Some error"]}')
+          );
         vi.stubGlobal('fetch', fetch);
 
         await expect(loadDocumentProps({ slug: 'foo' })).rejects.toThrowErrorMatchingSnapshot();
@@ -143,7 +168,22 @@ describe('loadPreviewDocumentProps', () => {
 
       describe('with error not as JSON', () => {
         it('should throw an error with the response status text', async () => {
-          const fetch = vi.fn().mockResolvedValue(createNotOkJsonErrorFetchResponse());
+          const fetch = vi.fn().mockResolvedValue(createNotOkJsonErrorFetchResponse({ 'Content-Type': 'text/html' }));
+          vi.stubGlobal('fetch', fetch);
+
+          await expect(
+            loadPreviewDocumentProps({ 'node[__contextNodePath]': 'foo/bar@user-me' })
+          ).rejects.toThrowErrorMatchingSnapshot();
+        });
+      });
+
+      describe('with error as JSON', () => {
+        it('should throw an error with the response status text', async () => {
+          const fetch = vi
+            .fn()
+            .mockResolvedValue(
+              createNotOkJsonErrorFetchResponse({ 'content-type': 'application/json' }, '{"errors":["Some error"]}')
+            );
           vi.stubGlobal('fetch', fetch);
 
           await expect(
@@ -165,7 +205,12 @@ describe('loadSiteProps', () => {
       vi.stubEnv('NEOS_BASE_URL', 'http://neos:1234');
     });
 
-    describe.each<{ opts?: DataLoaderOptions; expectedFetchConfig: RequestInit }>([
+    describe.each<{
+      opts?: DataLoaderOptions;
+      expectedFetchConfig: RequestInit & {
+        next?: { tags: string[] } | undefined;
+      };
+    }>([
       // No options
       { expectedFetchConfig: { cache: 'no-store', headers: {}, next: undefined } },
       // Override cache
@@ -182,6 +227,28 @@ describe('loadSiteProps', () => {
         expect(fetch).toHaveBeenCalledWith('http://neos:1234/neos/content-api/site', expectedFetchConfig);
       });
     });
+
+    describe('with error not as JSON', () => {
+      it('should throw an error with the response status text', async () => {
+        const fetch = vi.fn().mockResolvedValue(createNotOkJsonErrorFetchResponse({ 'Content-Type': 'text/html' }));
+        vi.stubGlobal('fetch', fetch);
+
+        await expect(loadSiteProps()).rejects.toThrowErrorMatchingSnapshot();
+      });
+    });
+
+    describe('with error as JSON', () => {
+      it('should throw an error with the response status text', async () => {
+        const fetch = vi
+          .fn()
+          .mockResolvedValue(
+            createNotOkJsonErrorFetchResponse({ 'content-type': 'application/json' }, '{"errors":["Some error"]}')
+          );
+        vi.stubGlobal('fetch', fetch);
+
+        await expect(loadSiteProps()).rejects.toThrowErrorMatchingSnapshot();
+      });
+    });
   });
 
   describe('with optional option', () => {
@@ -236,16 +303,33 @@ afterEach(() => {
 });
 
 function createOkayFetchResponse(data: any) {
-  return { ok: true, status: 200, json: () => new Promise((resolve) => resolve(data)) };
+  return {
+    ok: true,
+    status: 200,
+    json: () => new Promise((resolve) => resolve(data)),
+    text: () => new Promise((resolve) => resolve(JSON.stringify(data))),
+  };
+}
+
+function createOkayButNotJsonFetchResponse() {
+  return {
+    ok: true,
+    status: 200,
+    json: async () => {
+      throw new SyntaxError('Unexpected token < in JSON at position 0');
+    },
+    text: () => new Promise((resolve) => resolve('<html><body>Some Bad Error</body></html>')),
+  };
 }
 
-function createNotOkJsonErrorFetchResponse() {
+function createNotOkJsonErrorFetchResponse(headers?: HeadersInit, responseBody?: string) {
   return {
     ok: false,
     status: 500,
+    headers: new Headers(headers),
     json: async () => {
       throw new SyntaxError('Unexpected token < in JSON at position 0');
     },
-    text: async () => '<html><body>Internal Server Error</body></html>',
+    text: async () => responseBody || '<html><body>Internal Server Error</body></html>',
   };
 }