Skip to content

Commit 657e066

Browse files
authored
Backport catcher alignment for v20 (#2533)
Cherry-picking baa062d for v20. See #2525
1 parent 2d19495 commit 657e066

6 files changed

Lines changed: 141 additions & 31 deletions

File tree

CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,15 @@
22

33
## Version 20
44

5+
### v20.22.4
6+
7+
- Fixed inconsistency between the actual catcher behavior and the error handling documentation:
8+
- Removed conversion of non-`HttpError`s to `BadRequest` before passing them to `errorHandler`;
9+
- A `ResultHandler` configured as `errorHandler` is responsible to handling all errors and responding accordingly.
10+
- The default `errorHandler` is `defaultResultHandler`:
11+
- Using `ensureHttpError()` it coverts non-`HttpError`s to `InternalServerError` and responds with status code `500`;
12+
- The issue has occurred since [v19.0.0](#v1900).
13+
514
### v20.22.3
615

716
- Fixed: the output type of the `ez.raw()` schema (without an argument) was missing the `raw` property (since v19.0.0).

README.md

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -870,25 +870,25 @@ const endpointsFactory = new EndpointsFactory(yourResultHandler);
870870

871871
## Error handling
872872

873-
`ResultHandler` is designed to be the entity responsible for centralized error handling. By default, that center is
874-
the `defaultResultHandler`, however, since much can be customized, you should be aware that there are three possible
875-
origins of errors that could happen in runtime and be handled the following way:
876-
877-
- Ones related to `Endpoint` execution — handled by a `ResultHandler` assigned to the `EndpointsFactory` produced it:
878-
- The following proprietary classes are available to you for customizing error handling in your `ResultHandler`:
879-
- `InputValidationError` — when request payload does not match the `input` schema of the endpoint or middleware.
880-
The default response status code is `400`, `cause` property contains the original `ZodError`;
881-
- `OutputValidationError` — when returns of the endpoint's `handler` does not match its `output` schema (`500`);
882-
- Errors thrown within endpoint's `handler`:
883-
- `HttpError`, made by `createHttpError()` method of `http-errors` (required peer dependency). The default response
884-
status code is taken from `error.statusCode`;
885-
- Others, inheriting from `Error` class (`500`);
886-
- Ones related to routing, parsing and upload issues — handled by `ResultHandler` assigned to `errorHandler` in config:
887-
- Default is `defaultResultHandler` — it sets the response status code from the corresponding `HttpError`:
888-
`400` for parsing, `404` for routing, `config.upload.limitError.statusCode` for upload issues, or `500` for others.
889-
- `ResultHandler` must handle possible `error` and avoid throwing its own errors, otherwise:
890-
- Ones related to `ResultHandler` execution — handled by `LastResortHandler`:
891-
- Response status code is always `500` and the response itself is a plain text.
873+
All runtime errors are handled by a `ResultHandler`. The default is `defaultResultHandler`. Using `ensureHttpError()`
874+
it normalizes errors into consistent HTTP responses with sensible status codes. Errors can originate from three layers:
875+
876+
- `Endpoint` execution (including attached `Middleware`):
877+
- Handled by a `ResultHandler` used by `EndpointsFactory` (`defaultEndpointsFactory` uses `defaultResultHandler`);
878+
- `InputValidationError`: request violates `input` schema, the default status code is `400`;
879+
- `OutputValidationError`: handler violates `output` schema, the default status code is `500`;
880+
- `HttpError`: can be thrown in handlers with help of `createHttpError()`, its `.statusCode` is used for response;
881+
- For other errors the default status code is `500`;
882+
- Routing, parsing and upload issues:
883+
- Handled by `ResultHandler` configured as `errorHandler` (the defaults is `defaultResultHandler`);
884+
- Parsing errors: passed through as-is (typically `HttpError` with `4XX` code used for response by default);
885+
- Routing errors: `404` or `405`, based on `wrongMethodBehavior` configuration;
886+
- Upload issues: thrown only if `upload.limitError` is configured (`HttpError::statusCode` can be used for response);
887+
- For other errors the default status code is `500`;
888+
- `ResultHandler` failures:
889+
- Handled by `LastResortHandler` with status code `500` and a plain text response.
890+
891+
You can customize it by passing a custom `ResultHandler` to `EndpointsFactory` and by configuring `errorHandler`.
892892

893893
## Production mode
894894

src/server-helpers.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { AbstractResultHandler } from "./result-handler";
55
import { ActualLogger } from "./logger-helpers";
66
import { CommonConfig, ServerConfig } from "./config-type";
77
import { ErrorRequestHandler, RequestHandler, Request } from "express";
8-
import createHttpError, { isHttpError } from "http-errors";
8+
import createHttpError from "http-errors";
99
import { lastResortHandler } from "./last-resort";
1010
import { ResultHandlerError } from "./errors";
1111
import { ensureError } from "./common-helpers";
@@ -34,9 +34,7 @@ export const createParserFailureHandler =
3434
async (error, request, response, next) => {
3535
if (!error) return next();
3636
return errorHandler.execute({
37-
error: isHttpError(error)
38-
? error
39-
: createHttpError(400, ensureError(error).message),
37+
error: ensureError(error),
4038
request,
4139
response,
4240
input: null,

tests/system/__snapshots__/system.spec.ts.snap

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,19 +66,19 @@ exports[`App in production mode > Protocol > Should fail on invalid method 1`] =
6666
}
6767
`;
6868

69-
exports[`App in production mode > Protocol > Should fail on malformed body 1`] = `
69+
exports[`App in production mode > Protocol > Should fail when missing content type header 1`] = `
7070
{
7171
"error": {
72-
"message": StringMatching /\\(Unexpected end of JSON input\\|Unterminated string in JSON at position 25\\)/,
72+
"message": "key: Required",
7373
},
7474
"status": "error",
7575
}
7676
`;
7777

78-
exports[`App in production mode > Protocol > Should fail when missing content type header 1`] = `
78+
exports[`App in production mode > Protocol > Should handle JSON parser failures 1`] = `
7979
{
8080
"error": {
81-
"message": "key: Required",
81+
"message": StringMatching /\\(Unexpected end of JSON input\\|Unterminated string in JSON at position 25\\)/,
8282
},
8383
"status": "error",
8484
}

tests/system/system.spec.ts

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import cors from "cors";
22
import depd from "depd";
3+
import express from "express";
4+
import { readFile } from "node:fs/promises";
35
import { z } from "zod";
46
import {
57
EndpointsFactory,
@@ -9,6 +11,7 @@ import {
911
ResultHandler,
1012
BuiltinLogger,
1113
Middleware,
14+
ez,
1215
} from "../../src";
1316
import { givePort } from "../helpers";
1417
import { setTimeout } from "node:timers/promises";
@@ -102,12 +105,26 @@ describe("App in production mode", async () => {
102105
output: z.object({}),
103106
handler: async () => setTimeout(5000, {}),
104107
});
108+
const rawEndpoint = new EndpointsFactory(defaultResultHandler).build({
109+
method: "post",
110+
input: ez.raw(),
111+
output: z.object({ crc: z.number() }),
112+
handler: async ({ input: { raw } }) => ({ crc: raw.length }),
113+
});
114+
const uploadEndpoint = new EndpointsFactory(defaultResultHandler).build({
115+
method: "post",
116+
input: z.object({ avatar: ez.upload() }),
117+
output: z.object({}),
118+
handler: async () => ({}),
119+
});
105120
const routing = {
106121
v1: {
107122
corsed: corsedEndpoint,
108123
faulty: faultyEndpoint,
109124
test: testEndpoint,
110125
long: longEndpoint,
126+
raw: rawEndpoint,
127+
upload: uploadEndpoint,
111128
},
112129
};
113130
vi.spyOn(process.stdout, "write").mockImplementation(vi.fn()); // mutes logo output
@@ -117,11 +134,20 @@ describe("App in production mode", async () => {
117134
server: {
118135
listen: port,
119136
compression: { threshold: 1 },
137+
rawParser: express.raw({ limit: 20 }),
138+
upload: {
139+
beforeUpload: ({ request }) => {
140+
if ("trigger" in request.query)
141+
throw new Error("beforeUpload failure");
142+
},
143+
},
120144
beforeRouting: ({ app, getChildLogger }) => {
121145
depd("express")("Sample deprecation message");
122146
app.use((req, {}, next) => {
123147
const childLogger = getChildLogger(req);
124148
assert("isChild" in childLogger && childLogger.isChild);
149+
if (req.path === "/trigger/beforeRouting")
150+
return next(new Error("Failure of beforeRouting triggered"));
125151
next();
126152
});
127153
},
@@ -253,6 +279,35 @@ describe("App in production mode", async () => {
253279
"Content-Range,X-Content-Range",
254280
);
255281
});
282+
283+
test("Should handle raw request", async () => {
284+
const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, {
285+
method: "POST",
286+
headers: { "content-type": "application/octet-stream" },
287+
body: Buffer.from("testing"),
288+
});
289+
expect(response.status).toBe(200);
290+
const json = await response.json();
291+
expect(json).toEqual({ status: "success", data: { crc: 7 } });
292+
});
293+
294+
test("Should handle upload request", async () => {
295+
const filename = "logo.svg";
296+
const logo = await readFile(filename, "utf-8");
297+
const data = new FormData();
298+
data.append(
299+
"avatar",
300+
new Blob([logo], { type: "image/svg+xml" }),
301+
filename,
302+
);
303+
const response = await fetch(`http://localhost:${port}/v1/upload`, {
304+
method: "POST",
305+
body: data,
306+
});
307+
expect(response.status).toBe(200);
308+
const json = await response.json();
309+
expect(json).toEqual({ data: {}, status: "success" });
310+
});
256311
});
257312

258313
describe("Negative", () => {
@@ -303,6 +358,38 @@ describe("App in production mode", async () => {
303358
expect(text).toBe("Internal Server Error");
304359
expect(errorMethod.mock.lastCall).toMatchSnapshot();
305360
});
361+
362+
test("Should treat beforeRouting error as internal", async () => {
363+
const response = await fetch(
364+
`http://127.0.0.1:${port}/trigger/beforeRouting`,
365+
);
366+
expect(await response.json()).toEqual({
367+
status: "error",
368+
error: { message: "Internal Server Error" },
369+
});
370+
expect(response.status).toBe(500);
371+
});
372+
373+
test("Should treat beforeUpload error as internal", async () => {
374+
const filename = "logo.svg";
375+
const logo = await readFile(filename, "utf-8");
376+
const data = new FormData();
377+
data.append(
378+
"avatar",
379+
new Blob([logo], { type: "image/svg+xml" }),
380+
filename,
381+
);
382+
const response = await fetch(
383+
`http://localhost:${port}/v1/upload?trigger=beforeUpload`,
384+
{ method: "POST", body: data },
385+
);
386+
expect(response.status).toBe(500);
387+
const json = await response.json();
388+
expect(json).toEqual({
389+
error: { message: "Internal Server Error" },
390+
status: "error",
391+
});
392+
});
306393
});
307394

308395
describe("Protocol", () => {
@@ -322,7 +409,7 @@ describe("App in production mode", async () => {
322409
expect(json).toMatchSnapshot();
323410
});
324411

325-
test("Should fail on malformed body", async () => {
412+
test("Should handle JSON parser failures", async () => {
326413
const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
327414
method: "POST", // valid method this time
328415
headers: {
@@ -343,6 +430,20 @@ describe("App in production mode", async () => {
343430
});
344431
});
345432

433+
test("Should handle Raw parser failures", async () => {
434+
const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, {
435+
method: "POST",
436+
headers: { "content-type": "application/octet-stream" },
437+
body: Buffer.alloc(100),
438+
});
439+
expect(response.status).toBe(413);
440+
const json = await response.json();
441+
expect(json).toEqual({
442+
status: "error",
443+
error: { message: "request entity too large" },
444+
});
445+
});
446+
346447
test("Should fail when missing content type header", async () => {
347448
const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
348449
method: "POST",
@@ -452,7 +553,7 @@ describe("App in production mode", async () => {
452553
await setTimeout(500);
453554
process.emit("FAKE" as "SIGTERM");
454555
expect(infoMethod).toHaveBeenCalledWith("Graceful shutdown", {
455-
sockets: 1,
556+
sockets: expect.any(Number),
456557
timeout: 1000,
457558
});
458559
await setTimeout(1500);

tests/unit/server-helpers.spec.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,11 @@ describe("Server helpers", () => {
3535

3636
test.each([
3737
new SyntaxError("Unexpected end of JSON input"),
38+
new Error("Anything"),
3839
createHttpError(400, "Unexpected end of JSON input"),
40+
"just a text",
3941
])(
40-
"the handler should call error handler with correct error code %#",
42+
"the handler should call error handler with given error %#",
4143
async (error) => {
4244
const errorHandler = new ResultHandler({
4345
positive: vi.fn(),
@@ -57,7 +59,7 @@ describe("Server helpers", () => {
5759
);
5860
expect(spy).toHaveBeenCalledTimes(1);
5961
expect(spy.mock.calls[0][0].error).toEqual(
60-
createHttpError(400, "Unexpected end of JSON input"),
62+
error instanceof Error ? error : new Error(error),
6163
);
6264
},
6365
);

0 commit comments

Comments
 (0)