feat: better error-handling for non-JSON responses (#538)

* chore(deps): bring mime-types into the mix We already have this package installed as part of `form-data`, but now we're going to use it to parse content-type headers. * feat: add non-JSON handling to `handleRes` This is mostly functionally equivalent, but now we check the Content-Type of the response before parsing its body as JSON. That way we can inspect the body properly. * refactor(openapi): use handleRes for error handling * test: update tests to reflect new error conditions * Update __tests__/cmds/openapi.test.js Co-authored-by: Jon Ursenbach <erunion@users.noreply.github.com> Co-authored-by: Jon Ursenbach <erunion@users.noreply.github.com>
readmeio · Jul 21, 2022 · 60b7f8f · 60b7f8f
1 parent dbd6930
commit 60b7f8f
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 39 deletions.
diff --git a/__tests__/cmds/openapi.test.js b/__tests__/cmds/openapi.test.js
@@ -535,7 +535,11 @@ describe('rdme openapi', () => {
 
       await expect(
         openapi.run({ spec: require.resolve('@readme/oas-examples/2.0/json/petstore.json'), key, version })
-      ).rejects.toStrictEqual(new Error('There was an error uploading!'));
+      ).rejects.toStrictEqual(
+        new Error(
+          'Yikes, something went wrong! Please try uploading your spec again and if the problem persists, get in touch with our support team at support@readme.io.'
+        )
+      );
 
       return mock.done();
     });
@@ -555,7 +559,7 @@ describe('rdme openapi', () => {
         .post('/api/v1/api-specification', { registryUUID })
         .delayConnection(1000)
         .basicAuth({ user: key })
-        .reply(400, '<html></html>');
+        .reply(500, '<title>Application Error</title>');
 
       await expect(
         openapi.run({ spec: require.resolve('@readme/oas-examples/2.0/json/petstore.json'), key, version })

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -47,6 +47,7 @@
     "form-data": "^4.0.0",
     "gray-matter": "^4.0.1",
     "isemail": "^3.1.3",
+    "mime-types": "^2.1.35",
     "node-fetch": "^2.6.1",
     "oas-normalize": "^6.0.0",
     "open": "^8.2.1",

diff --git a/src/cmds/openapi.js b/src/cmds/openapi.js
@@ -5,6 +5,7 @@ const config = require('config');
 const fs = require('fs');
 const { debug, oraOptions } = require('../lib/logger');
 const fetch = require('../lib/fetch');
+const { handleRes } = require('../lib/fetch');
 const { getProjectVersion } = require('../lib/versionSelect');
 const ora = require('ora');
 const parse = require('parse-link-header');
@@ -108,24 +109,29 @@ module.exports = class OpenAPICommand {
         );
       }
 
-      async function error(err) {
-        debug(`error response received with status code ${err.status}`);
-        try {
-          const parsedError = await err.json();
-          debug(`full error response: ${JSON.stringify(parsedError)}`);
-          return Promise.reject(new APIError(parsedError));
-        } catch (e) {
-          debug(`error parsing JSON with message: ${e.message}`);
-          if (e.message.includes('Unexpected token < in JSON')) {
-            return Promise.reject(
-              new Error(
-                "We're sorry, your upload request timed out. Please try again or split your file up into smaller chunks."
-              )
+      async function error(res) {
+        return handleRes(res).catch(err => {
+          // If we receive an APIError, no changes needed! Throw it as is.
+          if (err instanceof APIError) {
+            throw err;
+          }
+          // If we receive certain text responses, it's likely a 5xx error from our server.
+          if (
+            typeof err === 'string' &&
+            (err.includes('<title>Application Error</title>') || // Heroku error
+              err.includes('520: Web server is returning an unknown error</title>')) // Cloudflare error
+          ) {
+            throw new Error(
+              "We're sorry, your upload request timed out. Please try again or split your file up into smaller chunks."
             );
           }
-
-          return Promise.reject(new Error('There was an error uploading!'));
-        }
+          // As a fallback, we throw a more generic error.
+          throw new Error(
+            `Yikes, something went wrong! Please try uploading your spec again and if the problem persists, get in touch with our support team at ${chalk.underline(
+              'support@readme.io'
+            )}.`
+          );
+        });
       }
 
       const registryUUID = await streamSpecToRegistry(bundledSpec);

diff --git a/src/lib/fetch.js b/src/lib/fetch.js
@@ -2,6 +2,7 @@
 const { debug } = require('./logger');
 const fetch = require('node-fetch');
 const isGHA = require('./isGitHub');
+const mime = require('mime-types');
 const pkg = require('../../package.json');
 const APIError = require('./apiError');
 
@@ -41,18 +42,30 @@ module.exports.getUserAgent = function getUserAgent() {
 };
 
 /**
- * Small handler for transforming responses from our API into JSON and if there's errors, throwing
- * an APIError exception.
+ * Small handler for handling responses from our API.
+ *
+ * If we receive JSON errors, we throw an APIError exception.
+ *
+ * If we receive non-JSON responses, we consider them errors and throw them.
  *
  * @param {Response} res
  */
 module.exports.handleRes = async function handleRes(res) {
-  const body = await res.json();
-  debug(`received status code ${res.status} with response body: ${JSON.stringify(body)}`);
-  if (body.error) {
-    return Promise.reject(new APIError(body));
+  const contentType = res.headers.get('content-type');
+  const extension = mime.extension(contentType);
+  if (extension === 'json') {
+    const body = await res.json();
+    debug(`received status code ${res.status} with JSON response: ${JSON.stringify(body)}`);
+    if (body.error) {
+      return Promise.reject(new APIError(body));
+    }
+    return body;
   }
-  return body;
+  // If we receive a non-JSON response, it's likely an error.
+  // Let's debug the raw response body and throw it.
+  const body = await res.text();
+  debug(`received status code ${res.status} with non-JSON response: ${body}`);
+  return Promise.reject(body);
 };
 
 /**