fix(api-gateway): Fallback to global error middelware for async handlers #7520

ovr · 2023-12-12T13:10:01Z

Hello!

Thanks

vercel · 2023-12-12T13:10:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-react-dashboard	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Dec 12, 2023 2:28pm
examples-react-pivot-table	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Dec 12, 2023 2:28pm

6 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-angular-dashboard	⬜️ Ignored (Inspect)	Visit Preview	Dec 12, 2023 2:28pm
examples-react-d3	⬜️ Ignored (Inspect)	Visit Preview	Dec 12, 2023 2:28pm
examples-react-data-table	⬜️ Ignored (Inspect)	Visit Preview	Dec 12, 2023 2:28pm
examples-react-highcharts	⬜️ Ignored (Inspect)	Visit Preview	Dec 12, 2023 2:28pm
examples-react-material-ui	⬜️ Ignored (Inspect)	Visit Preview	Dec 12, 2023 2:28pm
examples-vue-query-builder	⬜️ Ignored (Inspect)	Visit Preview	Dec 12, 2023 2:28pm

packages/cubejs-api-gateway/src/gateway.ts

    );

    /** **************************************************************
     * data scope                                                    *
     *************************************************************** */

-    app.get(`${this.basePath}/v1/load`, userMiddlewares, (async (req, res) => {
+    app.get(`${this.basePath}/v1/load`, userMiddlewares, userAsyncHandler(async (req: any, res) => {


packages/cubejs-api-gateway/src/gateway.ts

@@ -254,7 +264,7 @@
    }));

    const jsonParser = bodyParser.json({ limit: '1mb' });
-    app.post(`${this.basePath}/v1/load`, jsonParser, userMiddlewares, (async (req, res) => {
+    app.post(`${this.basePath}/v1/load`, jsonParser, userMiddlewares, userAsyncHandler(async (req, res) => {


packages/cubejs-api-gateway/src/gateway.ts

@@ -263,7 +273,7 @@
      });
    }));

-    app.get(`${this.basePath}/v1/subscribe`, userMiddlewares, (async (req, res) => {
+    app.get(`${this.basePath}/v1/subscribe`, userMiddlewares, userAsyncHandler(async (req: any, res) => {


packages/cubejs-api-gateway/src/gateway.ts

@@ -272,31 +282,31 @@
      });
    }));

-    app.get(`${this.basePath}/v1/sql`, userMiddlewares, (async (req, res) => {
+    app.get(`${this.basePath}/v1/sql`, userMiddlewares, userAsyncHandler(async (req: any, res) => {


packages/cubejs-api-gateway/src/gateway.ts

      await this.sql({
        query: req.query.query,
        context: req.context,
        res: this.resToResultFn(res)
      });
    }));

-    app.post(`${this.basePath}/v1/sql`, jsonParser, userMiddlewares, (async (req, res) => {
+    app.post(`${this.basePath}/v1/sql`, jsonParser, userMiddlewares, userAsyncHandler(async (req, res) => {


packages/cubejs-api-gateway/src/gateway.ts

        this.resToResultFn(res)({
          timezones: this.scheduledRefreshTimeZones || []
        });
      }));

-      app.post('/cubejs-system/v1/pre-aggregations/partitions', jsonParser, systemMiddlewares, (async (req, res) => {
+      app.post('/cubejs-system/v1/pre-aggregations/partitions', jsonParser, systemMiddlewares, systemAsyncHandler(async (req, res) => {


packages/cubejs-api-gateway/src/gateway.ts

        await this.getPreAggregationPartitions({
          query: req.body.query,
          context: req.context,
          res: this.resToResultFn(res)
        });
      }));

-      app.post('/cubejs-system/v1/pre-aggregations/preview', jsonParser, systemMiddlewares, (async (req, res) => {
+      app.post('/cubejs-system/v1/pre-aggregations/preview', jsonParser, systemMiddlewares, systemAsyncHandler(async (req, res) => {


packages/cubejs-api-gateway/src/gateway.ts

        await this.getPreAggregationPreview({
          query: req.body.query,
          context: req.context,
          res: this.resToResultFn(res)
        });
      }));

-      app.post('/cubejs-system/v1/pre-aggregations/build', jsonParser, systemMiddlewares, (async (req, res) => {
+      app.post('/cubejs-system/v1/pre-aggregations/build', jsonParser, systemMiddlewares, systemAsyncHandler(async (req, res) => {


packages/cubejs-api-gateway/src/gateway.ts

        await this.buildPreAggregations({
          query: req.body.query,
          context: req.context,
          res: this.resToResultFn(res)
        });
      }));

-      app.post('/cubejs-system/v1/pre-aggregations/queue', jsonParser, systemMiddlewares, (async (req, res) => {
+      app.post('/cubejs-system/v1/pre-aggregations/queue', jsonParser, systemMiddlewares, systemAsyncHandler(async (req, res) => {


packages/cubejs-api-gateway/src/gateway.ts

        await this.getPreAggregationsInQueue({
          context: req.context,
          res: this.resToResultFn(res)
        });
      }));

-      app.post('/cubejs-system/v1/pre-aggregations/cancel', jsonParser, systemMiddlewares, (async (req, res) => {
+      app.post('/cubejs-system/v1/pre-aggregations/cancel', jsonParser, systemMiddlewares, systemAsyncHandler(async (req, res) => {


tenphi · 2023-12-12T13:20:13Z

packages/cubejs-api-gateway/src/gateway.ts

- * API gateway server class.
- */
+function userAsyncHandler(handler: (req: Request & { context: ExtendedRequestContext }, res: ExpressResponse) => Promise<void>) {
+  return (req: ExpressRequest, res: ExpressResponse, next: NextFunction) => {


We need to add async before the function to make sure it will not throw a sync error.

handler is limited with TS by returning type = => Promise<void>.

function returns wrapper = sync handler = express.Handler

You can't validate async functions using Typescript:

https://www.typescriptlang.org/play?#code/C4TwDgpgBAggziAdgYwGIFcVQLxQBQCWiAJhAB4BcUi6AtgEYQBOAlDgHxQAKTA9rQTgQAPAENEIdgG4AUDOS9EcYFFEIUGFFXhI0mZDnxFSZNtk4BvGVBtQCAMyMlyUYVAAMbK7Z9RgACz4Ad2oIEIBRJj4mPAByUWAE0ViWWR8AX2tbJghgdCZEbj4BIQA6HLheABsANwhCZ1NZdNkgA

This snippet does create a function that throws a sync error despite being TS-validated as fully async.

Technically it's possible. For example, It's possible to do process.exit, but we don't monkey patch process.exit to protect it, because this approach is incorrect and we cannot imagine that someone will write code like this and it will pass review.

tenphi · 2023-12-12T13:20:22Z

packages/cubejs-api-gateway/src/gateway.ts

+}
+
+function systemAsyncHandler(handler: (req: Request & { context: ExtendedRequestContext }, res: ExpressResponse) => Promise<void>) {
+  return (req: ExpressRequest, res: ExpressResponse, next: NextFunction) => {


Here as well.

codecov · 2023-12-12T14:36:41Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (55422a1) 48.01% compared to head (26fabbc) 48.02%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
packages/cubejs-api-gateway/src/gateway.ts	94.11%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7520      +/-   ##
==========================================
+ Coverage   48.01%   48.02%   +0.01%     
==========================================
  Files         154      154              
  Lines       20897    20903       +6     
  Branches     5382     5382              
==========================================
+ Hits        10033    10039       +6     
  Misses      10122    10122              
  Partials      742      742

Flag	Coverage Δ
cube-backend	`48.02% <94.11%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ovr requested a review from a team as a code owner December 12, 2023 13:10

MikeNitsenko approved these changes Dec 12, 2023

View reviewed changes

github-advanced-security bot found potential problems Dec 12, 2023

View reviewed changes

RusovDmitriy approved these changes Dec 12, 2023

View reviewed changes

tenphi reviewed Dec 12, 2023

View reviewed changes

fix(api-gateway): Fallback to global error middelware for async handlers

26fabbc

ovr force-pushed the fix/api-gateway-fallback-error-middelware branch from f53655a to 26fabbc Compare December 12, 2023 14:28

ovr merged commit 74b0862 into master Dec 12, 2023
28 of 29 checks passed

ovr deleted the fix/api-gateway-fallback-error-middelware branch December 12, 2023 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api-gateway): Fallback to global error middelware for async handlers #7520

fix(api-gateway): Fallback to global error middelware for async handlers #7520

ovr commented Dec 12, 2023

vercel bot commented Dec 12, 2023 •

edited

Loading

tenphi Dec 12, 2023

ovr Dec 12, 2023

tenphi Dec 12, 2023 •

edited

Loading

ovr Dec 12, 2023

tenphi Dec 12, 2023

codecov bot commented Dec 12, 2023

fix(api-gateway): Fallback to global error middelware for async handlers #7520

fix(api-gateway): Fallback to global error middelware for async handlers #7520

Conversation

ovr commented Dec 12, 2023

vercel bot commented Dec 12, 2023 • edited Loading

tenphi Dec 12, 2023

Choose a reason for hiding this comment

ovr Dec 12, 2023

Choose a reason for hiding this comment

tenphi Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

ovr Dec 12, 2023

Choose a reason for hiding this comment

tenphi Dec 12, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2023

Codecov Report

vercel bot commented Dec 12, 2023 •

edited

Loading

tenphi Dec 12, 2023 •

edited

Loading