Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify http error responses #5595

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Unify http error responses #5595

wants to merge 2 commits into from

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Jan 10, 2025

when proxying requests
this in order to make it easier to debug when setting up companion/transloadit integration, and lessen support burden on us.

remove outdated err.status checks. this was added 7+ years ago and we now use got which doesn't provide err.status
Instead, for any other unhandled proxied HTTP request error responses, be nice and forward the JSON response to the client for easier debugging

mifi added 2 commits January 10, 2025 20:48
when proxying requests
this in order to make it easier to debug when setting up companion/transloadit integration, and lessen support burden on us.
remove outdated `err.status` checks. this was added [7+ years ago](https://github.com/transloadit/uppy/blame/cf18689c1055055fc73a33fb9fe18e1046dfc8e4/packages/%40uppy/companion/src/standalone/index.js#L143) and we now use `got` which doesn't provide err.status
Instead, for any other unhandled proxied HTTP request error responses, be nice and forward the JSON response to the client for easier debugging
Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/companion/lib/server/controllers/get.js b/packages/@uppy/companion/lib/server/controllers/get.js
index f69f049..ab3b42c 100644
--- a/packages/@uppy/companion/lib/server/controllers/get.js
+++ b/packages/@uppy/companion/lib/server/controllers/get.js
@@ -2,6 +2,7 @@
 Object.defineProperty(exports, "__esModule", { value: true });
 const logger = require("../logger");
 const { startDownUpload } = require("../helpers/upload");
+const { respondWithError } = require("../provider/error");
 async function get(req, res) {
   const { id } = req.params;
   const { providerUserSession } = req.companion;
@@ -15,6 +16,9 @@ async function get(req, res) {
     await startDownUpload({ req, res, getSize, download });
   } catch (err) {
     logger.error(err, "controller.get.error", req.id);
+    if (respondWithError(err, res)) {
+      return;
+    }
     res.status(500).json({ message: "Failed to download file" });
   }
 }
diff --git a/packages/@uppy/companion/lib/server/controllers/googlePicker.js b/packages/@uppy/companion/lib/server/controllers/googlePicker.js
index 1b4ce4e..60e11b7 100644
--- a/packages/@uppy/companion/lib/server/controllers/googlePicker.js
+++ b/packages/@uppy/companion/lib/server/controllers/googlePicker.js
@@ -8,6 +8,7 @@ const { getURLMeta } = require("../helpers/request");
 const logger = require("../logger");
 const { downloadURL } = require("../download");
 const { getGoogleFileSize, streamGoogleFile } = require("../provider/google/drive");
+const { respondWithError } = require("../provider/error");
 const getAuthHeader = (token) => ({ authorization: `Bearer ${token}` });
 /**
  * @param {object} req expressJS request object
@@ -39,7 +40,10 @@ const get = async (req, res) => {
     await startDownUpload({ req, res, getSize, download });
   } catch (err) {
     logger.error(err, "controller.googlePicker.error", req.id);
-    res.status(err.status || 500).json({ message: "failed to fetch Google Picker URL" });
+    if (respondWithError(err, res)) {
+      return;
+    }
+    res.status(500).json({ message: "failed to fetch Google Picker URL" });
   }
 };
 module.exports = () =>
diff --git a/packages/@uppy/companion/lib/server/controllers/url.js b/packages/@uppy/companion/lib/server/controllers/url.js
index 6a0b3be..d83a456 100644
--- a/packages/@uppy/companion/lib/server/controllers/url.js
+++ b/packages/@uppy/companion/lib/server/controllers/url.js
@@ -6,6 +6,7 @@ const { downloadURL } = require("../download");
 const { validateURL } = require("../helpers/request");
 const { getURLMeta } = require("../helpers/request");
 const logger = require("../logger");
+const { respondWithError } = require("../provider/error");
 /**
  * @callback downloadCallback
  * @param {Error} err
@@ -23,13 +24,17 @@ const meta = async (req, res) => {
     const { allowLocalUrls } = req.companion.options;
     if (!validateURL(req.body.url, allowLocalUrls)) {
       logger.debug("Invalid request body detected. Exiting url meta handler.", null, req.id);
-      return res.status(400).json({ error: "Invalid request body" });
+      res.status(400).json({ error: "Invalid request body" });
+      return;
     }
     const urlMeta = await getURLMeta(req.body.url, allowLocalUrls);
-    return res.json(urlMeta);
+    res.json(urlMeta);
   } catch (err) {
     logger.error(err, "controller.url.meta.error", req.id);
-    return res.status(err.status || 500).json({ message: "failed to fetch URL metadata" });
+    if (respondWithError(err, res)) {
+      return;
+    }
+    res.status(500).json({ message: "failed to fetch URL metadata" });
   }
 };
 /**
@@ -56,7 +61,10 @@ const get = async (req, res) => {
     await startDownUpload({ req, res, getSize, download });
   } catch (err) {
     logger.error(err, "controller.url.error", req.id);
-    res.status(err.status || 500).json({ message: "failed to fetch URL" });
+    if (respondWithError(err, res)) {
+      return;
+    }
+    res.status(500).json({ message: "failed to fetch URL" });
   }
 };
 module.exports = () =>
diff --git a/packages/@uppy/companion/lib/server/helpers/upload.js b/packages/@uppy/companion/lib/server/helpers/upload.js
index 4d60174..e461c57 100644
--- a/packages/@uppy/companion/lib/server/helpers/upload.js
+++ b/packages/@uppy/companion/lib/server/helpers/upload.js
@@ -2,44 +2,31 @@
 Object.defineProperty(exports, "__esModule", { value: true });
 const Uploader = require("../Uploader");
 const logger = require("../logger");
-const { respondWithError } = require("../provider/error");
 async function startDownUpload({ req, res, getSize, download }) {
-  try {
-    logger.debug("Starting download stream.", null, req.id);
-    const { stream, size: maybeSize } = await download();
-    let size;
-    // if the provider already knows the size, we can use that
-    if (typeof maybeSize === "number" && !Number.isNaN(maybeSize) && maybeSize > 0) {
-      size = maybeSize;
-    }
-    // if not we need to get the size
-    if (size == null) {
-      size = await getSize();
-    }
-    const { clientSocketConnectTimeout } = req.companion.options;
-    logger.debug("Instantiating uploader.", null, req.id);
-    const uploader = new Uploader(Uploader.reqToOptions(req, size));
-    (async () => {
-      // wait till the client has connected to the socket, before starting
-      // the download, so that the client can receive all download/upload progress.
-      logger.debug("Waiting for socket connection before beginning remote download/upload.", null, req.id);
-      await uploader.awaitReady(clientSocketConnectTimeout);
-      logger.debug("Socket connection received. Starting remote download/upload.", null, req.id);
-      await uploader.tryUploadStream(stream, req);
-    })().catch((err) => logger.error(err));
-    // Respond the request
-    // NOTE: the Uploader will continue running after the http request is responded
-    res.status(200).json({ token: uploader.token });
-  } catch (err) {
-    if (err.name === "ValidationError") {
-      logger.debug(err.message, "uploader.validator.fail");
-      res.status(400).json({ message: err.message });
-      return;
-    }
-    if (respondWithError(err, res)) {
-      return;
-    }
-    throw err;
+  logger.debug("Starting download stream.", null, req.id);
+  const { stream, size: maybeSize } = await download();
+  let size;
+  // if the provider already knows the size, we can use that
+  if (typeof maybeSize === "number" && !Number.isNaN(maybeSize) && maybeSize > 0) {
+    size = maybeSize;
   }
+  // if not we need to get the size
+  if (size == null) {
+    size = await getSize();
+  }
+  const { clientSocketConnectTimeout } = req.companion.options;
+  logger.debug("Instantiating uploader.", null, req.id);
+  const uploader = new Uploader(Uploader.reqToOptions(req, size));
+  (async () => {
+    // wait till the client has connected to the socket, before starting
+    // the download, so that the client can receive all download/upload progress.
+    logger.debug("Waiting for socket connection before beginning remote download/upload.", null, req.id);
+    await uploader.awaitReady(clientSocketConnectTimeout);
+    logger.debug("Socket connection received. Starting remote download/upload.", null, req.id);
+    await uploader.tryUploadStream(stream, req);
+  })().catch((err) => logger.error(err));
+  // Respond the request
+  // NOTE: the Uploader will continue running after the http request is responded
+  res.status(200).json({ token: uploader.token });
 }
 module.exports = { startDownUpload };
diff --git a/packages/@uppy/companion/lib/server/helpers/utils.d.ts b/packages/@uppy/companion/lib/server/helpers/utils.d.ts
index cca8131..3409e4c 100644
--- a/packages/@uppy/companion/lib/server/helpers/utils.d.ts
+++ b/packages/@uppy/companion/lib/server/helpers/utils.d.ts
@@ -19,6 +19,9 @@ export function getBucket({ bucketOrFn, req, metadata, filename }: {
     metadata?: Record<string, string>;
     filename?: string;
 }): string;
+/**
+ * Our own HttpError in cases where we can't use `got`'s `HTTPError`
+ */
 export class HttpError extends Error {
     constructor({ statusCode, responseJson }: {
         statusCode: any;
diff --git a/packages/@uppy/companion/lib/server/helpers/utils.js b/packages/@uppy/companion/lib/server/helpers/utils.js
index 34de6ed..490e448 100644
--- a/packages/@uppy/companion/lib/server/helpers/utils.js
+++ b/packages/@uppy/companion/lib/server/helpers/utils.js
@@ -128,6 +128,9 @@ module.exports.decrypt = (encrypted, secret) => {
   return decrypted;
 };
 module.exports.defaultGetKey = ({ filename }) => `${crypto.randomUUID()}-${filename}`;
+/**
+ * Our own HttpError in cases where we can't use `got`'s `HTTPError`
+ */
 class HttpError extends Error {
   statusCode;
   responseJson;
@@ -153,7 +156,7 @@ module.exports.prepareStream = async (stream) =>
       })
       .on("error", (err) => {
         // In this case the error object is not a normal GOT HTTPError where json is already parsed,
-        // we create our own HttpError error for this case
+        // we use our own HttpError error for this scenario.
         if (typeof err.response?.body === "string" && typeof err.response?.statusCode === "number") {
           let responseJson;
           try {
diff --git a/packages/@uppy/companion/lib/server/provider/error.d.ts b/packages/@uppy/companion/lib/server/provider/error.d.ts
index d7ec4f1..5c2b5ee 100644
--- a/packages/@uppy/companion/lib/server/provider/error.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/error.d.ts
@@ -1,6 +1,8 @@
 /**
  * AuthError is error returned when an adapter encounters
  * an authorization error while communication with its corresponding provider
+ * this signals to the client that the access token is invalid and needs to be
+ * refreshed or the user needs to re-authenticate
  */
 export class ProviderAuthError extends ProviderApiError {
     constructor();
@@ -26,3 +28,7 @@ export class ProviderUserError extends ProviderApiError {
     json: any;
 }
 export function respondWithError(err: any, res: any): boolean;
+export function parseHttpError(err: any): {
+    statusCode: any;
+    body: any;
+};
diff --git a/packages/@uppy/companion/lib/server/provider/error.js b/packages/@uppy/companion/lib/server/provider/error.js
index 48bd1d2..e4cebf4 100644
--- a/packages/@uppy/companion/lib/server/provider/error.js
+++ b/packages/@uppy/companion/lib/server/provider/error.js
@@ -30,6 +30,8 @@ class ProviderUserError extends ProviderApiError {
 /**
  * AuthError is error returned when an adapter encounters
  * an authorization error while communication with its corresponding provider
+ * this signals to the client that the access token is invalid and needs to be
+ * refreshed or the user needs to re-authenticate
  */
 class ProviderAuthError extends ProviderApiError {
   constructor() {
@@ -38,16 +40,35 @@ class ProviderAuthError extends ProviderApiError {
     this.isAuthError = true;
   }
 }
+function parseHttpError(err) {
+  if (err?.name === "HTTPError") {
+    return {
+      statusCode: err.response?.statusCode,
+      body: err.response?.body,
+    };
+  }
+  if (err?.name === "HttpError") {
+    return {
+      statusCode: err.statusCode,
+      body: err.responseJson,
+    };
+  }
+  return undefined;
+}
 /**
  * Convert an error instance to an http response if possible
  *
  * @param {Error | ProviderApiError} err the error instance to convert to an http json response
+ * @returns {object | undefined} an object with a code and json field if the error can be converted to a response
  */
 function errorToResponse(err) {
   // @ts-ignore
   if (err?.isAuthError) {
     return { code: 401, json: { message: err.message } };
   }
+  if (err?.name === "ValidationError") {
+    return { code: 400, json: { message: err.message } };
+  }
   if (err?.name === "ProviderUserError") {
     // @ts-ignore
     return { code: 400, json: err.json };
@@ -68,6 +89,10 @@ function errorToResponse(err) {
       return { code: 424, json: { message: err.message } };
     }
   }
+  const httpError = parseHttpError(err);
+  if (httpError) {
+    return { code: 500, json: { statusCode: httpError.statusCode, body: httpError.body } };
+  }
   return undefined;
 }
 function respondWithError(err, res) {
@@ -78,4 +103,4 @@ function respondWithError(err, res) {
   }
   return false;
 }
-module.exports = { ProviderAuthError, ProviderApiError, ProviderUserError, respondWithError };
+module.exports = { ProviderAuthError, ProviderApiError, ProviderUserError, respondWithError, parseHttpError };
diff --git a/packages/@uppy/companion/lib/server/provider/providerErrors.d.ts b/packages/@uppy/companion/lib/server/provider/providerErrors.d.ts
index 9e37f86..0fa0f0d 100644
--- a/packages/@uppy/companion/lib/server/provider/providerErrors.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/providerErrors.d.ts
@@ -25,3 +25,5 @@ export function withProviderErrorHandling({ fn, tag, providerName, isAuthError,
     getJsonErrorMessage: (a: object) => string;
 }): Promise<any>;
 export function withGoogleErrorHandling(providerName: any, tag: any, fn: any): Promise<any>;
+import { parseHttpError } from "./error";
+export { parseHttpError };
diff --git a/packages/@uppy/companion/lib/server/provider/providerErrors.js b/packages/@uppy/companion/lib/server/provider/providerErrors.js
index 25213ab..5be77aa 100644
--- a/packages/@uppy/companion/lib/server/provider/providerErrors.js
+++ b/packages/@uppy/companion/lib/server/provider/providerErrors.js
@@ -1,7 +1,7 @@
 "use strict";
 Object.defineProperty(exports, "__esModule", { value: true });
 const logger = require("../logger");
-const { ProviderApiError, ProviderUserError, ProviderAuthError } = require("./error");
+const { ProviderApiError, ProviderUserError, ProviderAuthError, parseHttpError } = require("./error");
 /**
  * @param {{
  *   fn: () => any,
@@ -31,16 +31,10 @@ async function withProviderErrorHandling(
   try {
     return await fn();
   } catch (err) {
-    let statusCode;
-    let body;
-    if (err?.name === "HTTPError") {
-      statusCode = err.response?.statusCode;
-      body = err.response?.body;
-    } else if (err?.name === "HttpError") {
-      statusCode = err.statusCode;
-      body = err.responseJson;
-    }
-    if (statusCode != null) {
+    const httpError = parseHttpError(err);
+    // Wrap all HTTP errors according to the provider's desired error handling
+    if (httpError) {
+      const { statusCode, body } = httpError;
       let knownErr;
       if (isAuthError({ statusCode, body })) {
         knownErr = new ProviderAuthError();
@@ -52,6 +46,7 @@ async function withProviderErrorHandling(
       logger.error(knownErr, tag);
       throw knownErr;
     }
+    // non HTTP errors will be passed through
     logger.error(err, tag);
     throw err;
   }
@@ -68,4 +63,4 @@ async function withGoogleErrorHandling(providerName, tag, fn) {
     getJsonErrorMessage: (body) => body?.error?.message,
   });
 }
-module.exports = { withProviderErrorHandling, withGoogleErrorHandling };
+module.exports = { withProviderErrorHandling, withGoogleErrorHandling, parseHttpError };
diff --git a/packages/@uppy/companion/lib/standalone/index.js b/packages/@uppy/companion/lib/standalone/index.js
index e3b6c1f..b5153e8 100644
--- a/packages/@uppy/companion/lib/standalone/index.js
+++ b/packages/@uppy/companion/lib/standalone/index.js
@@ -167,10 +167,10 @@ module.exports = function server(inputCompanionOptions) {
       } else {
         logger.error(err, "root.error", req.id);
       }
-      res.status(err.status || 500).json({ message: "Something went wrong", requestId: req.id });
+      res.status(500).json({ message: "Something went wrong", requestId: req.id });
     } else {
       logger.error(err, "root.error", req.id);
-      res.status(err.status || 500).json({ message: err.message, error: err, requestId: req.id });
+      res.status(500).json({ message: err.message, error: err, requestId: req.id });
     }
   });
   return { app, companionOptions };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant