Skip to content

Commit 12cac40

Browse files
DakEnviyDakEnviy
authored
fix: fix closing context for middlewares and route handlers and fix error handling (#88)
* fix: fix closing context for middleware * chore: fix typo * chore: remove unnecessary type casting for AuthPolicy * fix: fix closing context for route handler * fix: fix try-catch for error handlers * test: add tests for context lifecycle
1 parent 554de8f commit 12cac40

File tree

9 files changed

+380
-58
lines changed

9 files changed

+380
-58
lines changed

‎src/base-middleware.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ export function setupBaseMiddleware(ctx: AppContext, expressApp: Express) {
8686
next();
8787
return;
8888
} catch (error) {
89-
ctx.logError('Error during server setup', error);
89+
ctx.logError('Error during request setup', error);
9090
if (req.ctx) {
9191
req.ctx.setTag('error', true);
9292
req.ctx.end();

‎src/csp/middleware.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type {CSPPreset} from '.';
77

88
export interface CSPMiddlewareParams extends Omit<ExpressCSPParams, 'presets'> {
99
appPresets: CSPPreset;
10-
routPresets?:
10+
routePresets?:
1111
| CSPPreset
1212
| ((params: {
1313
getDefaultPresets: typeof getDefaultPresets;
@@ -17,11 +17,11 @@ export interface CSPMiddlewareParams extends Omit<ExpressCSPParams, 'presets'> {
1717

1818
export function cspMiddleware(options: CSPMiddlewareParams) {
1919
let presets: CSPPreset = options.appPresets;
20-
if (options.routPresets) {
20+
if (options.routePresets) {
2121
presets =
22-
typeof options.routPresets === 'function'
23-
? options.routPresets({getDefaultPresets, appPresets: presets})
24-
: options.routPresets;
22+
typeof options.routePresets === 'function'
23+
? options.routePresets({getDefaultPresets, appPresets: presets})
24+
: options.routePresets;
2525
}
2626

2727
return expressCspHeader({...options, presets});

‎src/csrf.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ function getUnixTime() {
99
return Math.floor(Number(new Date()) / 1000);
1010
}
1111

12-
export function prepareCSRFMiddleware(ctx: AppContext, routeAuthPolicy: AuthPolicy) {
12+
export function prepareCSRFMiddleware(ctx: AppContext, routeAuthPolicy: `${AuthPolicy}`) {
1313
const {
1414
appCsrfSecret: secret,
1515
appCsrfLifetime: lifetime = MONTH_SECONDS,

‎src/error-handlers.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import {AppContext} from '@gravity-ui/nodekit';
2+
import {Express} from 'express';
3+
4+
import {AppErrorHandler} from './types';
5+
import {validationErrorMiddleware} from './validator';
6+
7+
export function setupErrorHandlers(ctx: AppContext, expressApp: Express) {
8+
const validationErrorHandler = ctx.config.appValidationErrorHandler
9+
? ctx.config.appValidationErrorHandler(ctx)
10+
: validationErrorMiddleware;
11+
const validationRequestHandler: AppErrorHandler = async (error, req, res, next) => {
12+
try {
13+
await validationErrorHandler(error, req, res, next);
14+
} catch (err) {
15+
next(err);
16+
return;
17+
}
18+
};
19+
expressApp.use(validationRequestHandler);
20+
21+
const appFinalErrorHandler = ctx.config.appFinalErrorHandler;
22+
if (appFinalErrorHandler) {
23+
const appFinalRequestHandler: AppErrorHandler = async (error, req, res, next) => {
24+
try {
25+
await appFinalErrorHandler(error, req, res, next);
26+
} catch (err) {
27+
next(err);
28+
return;
29+
}
30+
};
31+
expressApp.use(appFinalRequestHandler);
32+
}
33+
34+
const finalRequestHandler: AppErrorHandler = (error, _req, res, _next) => {
35+
const errorDescription = 'Unhandled error during request processing';
36+
ctx.logError(errorDescription, error);
37+
const statusCode = (error && error.statusCode) || 500;
38+
res.status(statusCode).send(statusCode === 400 ? 'Bad request' : 'Internal server error');
39+
};
40+
expressApp.use(finalRequestHandler);
41+
}

‎src/expresskit.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {setupParsers} from './parsers';
1111
import {setupRoutes} from './router';
1212
import type {AppRoutes} from './types';
1313
import {setupLangMiddleware} from './lang/lang-middleware';
14+
import {setupErrorHandlers} from './error-handlers';
1415

1516
const DEFAULT_PORT = 3030;
1617

@@ -38,6 +39,7 @@ export class ExpressKit {
3839
setupLangMiddleware(this.nodekit.ctx, this.express);
3940
setupParsers(this.nodekit.ctx, this.express);
4041
setupRoutes(this.nodekit.ctx, this.express, routes);
42+
setupErrorHandlers(this.nodekit.ctx, this.express);
4143

4244
const appSocket = this.getAppSocket();
4345
const listenTarget = this.getListenTarget(appSocket);

‎src/router.ts‎

Lines changed: 26 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,17 @@ import {Express, Router} from 'express';
33

44
import {cspMiddleware, getAppPresets} from './csp/middleware';
55
import {
6-
AppErrorHandler,
76
AppMiddleware,
87
AppMountDescription,
98
AppRouteDescription,
109
AppRouteHandler,
1110
AppRoutes,
1211
AuthPolicy,
13-
ExpressFinalError,
1412
HTTP_METHODS,
1513
HttpMethod,
1614
} from './types';
1715
import {prepareCSRFMiddleware} from './csrf';
1816

19-
import {validationErrorMiddleware} from './validator';
20-
2117
function isAllowedMethod(method: string): method is HttpMethod | 'mount' {
2218
// eslint-disable-next-line @typescript-eslint/no-explicit-any
2319
return HTTP_METHODS.includes(method as any) || method === 'mount';
@@ -26,21 +22,24 @@ function isAllowedMethod(method: string): method is HttpMethod | 'mount' {
2622
function wrapMiddleware(fn: AppMiddleware, i?: number): AppMiddleware {
2723
const result: AppMiddleware = async (req, res, next) => {
2824
const reqCtx = req.ctx;
25+
const ctx = reqCtx.create(`${fn.name || `noname-${i}`} middleware`);
26+
2927
let ended = false;
28+
req.ctx = ctx;
29+
3030
try {
31-
return await reqCtx.call(`${fn.name || `noname-${i}`} middleware`, async (ctx) => {
32-
req.ctx = ctx;
33-
return await fn(req, res, (...args: unknown[]) => {
34-
req.ctx = reqCtx;
35-
ended = true;
36-
next(...args);
37-
});
31+
await fn(req, res, (...args: unknown[]) => {
32+
ctx.end();
33+
req.ctx = reqCtx;
34+
ended = true;
35+
next(...args);
3836
});
3937
} catch (error) {
40-
return next(error);
41-
} finally {
4238
if (!ended) {
39+
ctx.fail(error);
4340
req.ctx = reqCtx;
41+
next(error);
42+
return;
4443
}
4544
}
4645
};
@@ -53,7 +52,7 @@ const UNNAMED_CONTROLLER = 'unnamedController';
5352
function wrapRouteHandler(fn: AppRouteHandler, handlerName?: string) {
5453
const handlerNameLocal = handlerName || fn.name || UNNAMED_CONTROLLER;
5554

56-
const handler: AppMiddleware = (req, res, next) => {
55+
const handler: AppMiddleware = async (req, res, next) => {
5756
req.ctx = req.originalContext.create(handlerNameLocal);
5857
if (req.routeInfo.handlerName !== handlerNameLocal) {
5958
if (req.routeInfo.handlerName === UNNAMED_CONTROLLER) {
@@ -62,12 +61,16 @@ function wrapRouteHandler(fn: AppRouteHandler, handlerName?: string) {
6261
req.routeInfo.handlerName = `${req.routeInfo.handlerName}(${handlerNameLocal})`;
6362
}
6463
}
65-
Promise.resolve(fn(req, res))
66-
.catch(next)
67-
.finally(() => {
68-
req.ctx.end();
69-
req.ctx = req.originalContext;
70-
});
64+
try {
65+
await fn(req, res);
66+
req.ctx.end();
67+
req.ctx = req.originalContext;
68+
} catch (error) {
69+
req.ctx.fail(error);
70+
req.ctx = req.originalContext;
71+
next(error);
72+
return;
73+
}
7174
};
7275

7376
Object.defineProperty(handler, 'name', {value: handlerNameLocal});
@@ -102,7 +105,7 @@ export function setupRoutes(ctx: AppContext, expressApp: Express, routes: AppRou
102105
} = route;
103106

104107
const handlerName = routeHandlerName || route.handler.name || UNNAMED_CONTROLLER;
105-
const authPolicy = routeAuthPolicy || ctx.config.appAuthPolicy || AuthPolicy.disabled;
108+
const authPolicy = routeAuthPolicy || ctx.config.appAuthPolicy || `${AuthPolicy.disabled}`;
106109
const enableCaching =
107110
typeof routeEnableCaching === 'boolean'
108111
? routeEnableCaching
@@ -155,7 +158,7 @@ export function setupRoutes(ctx: AppContext, expressApp: Express, routes: AppRou
155158
routeMiddleware.push(
156159
cspMiddleware({
157160
appPresets,
158-
routPresets: cspPresets,
161+
routePresets: cspPresets,
159162
reportOnly: ctx.config.expressCspReportOnly,
160163
reportTo: ctx.config.expressCspReportTo,
161164
reportUri: ctx.config.expressCspReportUri,
@@ -175,7 +178,7 @@ export function setupRoutes(ctx: AppContext, expressApp: Express, routes: AppRou
175178
routeMiddleware.push(authHandler);
176179

177180
if (ctx.config.appCsrfSecret) {
178-
routeMiddleware.push(prepareCSRFMiddleware(ctx, authPolicy as AuthPolicy));
181+
routeMiddleware.push(prepareCSRFMiddleware(ctx, authPolicy));
179182
}
180183
}
181184

@@ -194,25 +197,4 @@ export function setupRoutes(ctx: AppContext, expressApp: Express, routes: AppRou
194197
expressApp[method](routePath, wrappedMiddleware, handler);
195198
}
196199
});
197-
198-
const errorHandler = ctx.config.appValidationErrorHandler
199-
? ctx.config.appValidationErrorHandler(ctx)
200-
: validationErrorMiddleware;
201-
202-
expressApp.use(errorHandler);
203-
204-
if (ctx.config.appFinalErrorHandler) {
205-
const appFinalRequestHandler: AppErrorHandler = (error, req, res, next) =>
206-
Promise.resolve(ctx.config.appFinalErrorHandler?.(error, req, res, next)).catch(next);
207-
expressApp.use(appFinalRequestHandler);
208-
}
209-
210-
const finalRequestHandler: AppErrorHandler = (error: ExpressFinalError, _, res, __) => {
211-
const errorDescription = 'Unhandled error during request processing';
212-
ctx.logError(errorDescription, error);
213-
const statusCode = (error && error.statusCode) || 500;
214-
res.status(statusCode).send(statusCode === 400 ? 'Bad request' : 'Internal server error');
215-
};
216-
217-
expressApp.use(finalRequestHandler);
218200
}

0 commit comments

Comments
 (0)