Skip to content

Commit 7c04c2f

Browse files
committed
Cherry-picking 557cdbe for v21.
1 parent f408df8 commit 7c04c2f

6 files changed

Lines changed: 139 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 21
44

5+
### v21.11.6
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
### v21.11.5
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
@@ -858,25 +858,25 @@ const resultHandler = new ResultHandler({
858858

859859
## Error handling
860860

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

881881
## Production mode
882882

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";
@@ -32,9 +32,7 @@ export const createParserFailureHandler =
3232
async (error, request, response, next) => {
3333
if (!error) return next();
3434
return errorHandler.execute({
35-
error: isHttpError(error)
36-
? error
37-
: createHttpError(400, ensureError(error).message),
35+
error: ensureError(error),
3836
request,
3937
response,
4038
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: 101 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,
@@ -10,6 +12,7 @@ import {
1012
ResultHandler,
1113
BuiltinLogger,
1214
Middleware,
15+
ez,
1316
} from "../../src";
1417
import { givePort } from "../helpers";
1518
import { setTimeout } from "node:timers/promises";
@@ -100,23 +103,44 @@ describe("App in production mode", async () => {
100103
output: z.object({}),
101104
handler: async () => setTimeout(5000, {}),
102105
});
106+
const rawEndpoint = new EndpointsFactory(defaultResultHandler).build({
107+
method: "post",
108+
input: ez.raw(),
109+
output: z.object({ crc: z.number() }),
110+
handler: async ({ input: { raw } }) => ({ crc: raw.length }),
111+
});
112+
const uploadEndpoint = new EndpointsFactory(defaultResultHandler).buildVoid({
113+
method: "post",
114+
input: z.object({ avatar: ez.upload() }),
115+
handler: vi.fn(),
116+
});
103117
const routing = {
104118
v1: {
105119
corsed: corsedEndpoint,
106120
faulty: faultyEndpoint,
107121
test: testEndpoint,
108122
long: longEndpoint,
123+
raw: rawEndpoint,
124+
upload: uploadEndpoint,
109125
},
110126
};
111127
vi.spyOn(process.stdout, "write").mockImplementation(vi.fn()); // mutes logo output
112128
const config = createConfig({
113129
http: { listen: port },
114130
compression: { threshold: 1 },
131+
rawParser: express.raw({ limit: 20 }),
132+
upload: {
133+
beforeUpload: ({ request }) => {
134+
if ("trigger" in request.query) throw new Error("beforeUpload failure");
135+
},
136+
},
115137
beforeRouting: ({ app, getLogger }) => {
116138
depd("express")("Sample deprecation message");
117139
app.use((req, {}, next) => {
118140
const childLogger = getLogger(req);
119141
assert("isChild" in childLogger && childLogger.isChild);
142+
if (req.path === "/trigger/beforeRouting")
143+
return next(new Error("Failure of beforeRouting triggered"));
120144
next();
121145
});
122146
},
@@ -247,6 +271,35 @@ describe("App in production mode", async () => {
247271
"Content-Range,X-Content-Range",
248272
);
249273
});
274+
275+
test("Should handle raw request", async () => {
276+
const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, {
277+
method: "POST",
278+
headers: { "content-type": "application/octet-stream" },
279+
body: Buffer.from("testing"),
280+
});
281+
expect(response.status).toBe(200);
282+
const json = await response.json();
283+
expect(json).toEqual({ status: "success", data: { crc: 7 } });
284+
});
285+
286+
test("Should handle upload request", async () => {
287+
const filename = "logo.svg";
288+
const logo = await readFile(filename, "utf-8");
289+
const data = new FormData();
290+
data.append(
291+
"avatar",
292+
new Blob([logo], { type: "image/svg+xml" }),
293+
filename,
294+
);
295+
const response = await fetch(`http://localhost:${port}/v1/upload`, {
296+
method: "POST",
297+
body: data,
298+
});
299+
expect(response.status).toBe(200);
300+
const json = await response.json();
301+
expect(json).toEqual({ data: {}, status: "success" });
302+
});
250303
});
251304

252305
describe("Negative", () => {
@@ -297,6 +350,38 @@ describe("App in production mode", async () => {
297350
expect(text).toBe("Internal Server Error");
298351
expect(errorMethod.mock.lastCall).toMatchSnapshot();
299352
});
353+
354+
test("Should treat beforeRouting error as internal", async () => {
355+
const response = await fetch(
356+
`http://127.0.0.1:${port}/trigger/beforeRouting`,
357+
);
358+
expect(await response.json()).toEqual({
359+
status: "error",
360+
error: { message: "Internal Server Error" },
361+
});
362+
expect(response.status).toBe(500);
363+
});
364+
365+
test("Should treat beforeUpload error as internal", async () => {
366+
const filename = "logo.svg";
367+
const logo = await readFile(filename, "utf-8");
368+
const data = new FormData();
369+
data.append(
370+
"avatar",
371+
new Blob([logo], { type: "image/svg+xml" }),
372+
filename,
373+
);
374+
const response = await fetch(
375+
`http://localhost:${port}/v1/upload?trigger=beforeUpload`,
376+
{ method: "POST", body: data },
377+
);
378+
expect(response.status).toBe(500);
379+
const json = await response.json();
380+
expect(json).toEqual({
381+
error: { message: "Internal Server Error" },
382+
status: "error",
383+
});
384+
});
300385
});
301386

302387
describe("Protocol", () => {
@@ -316,7 +401,7 @@ describe("App in production mode", async () => {
316401
expect(json).toMatchSnapshot();
317402
});
318403

319-
test("Should fail on malformed body", async () => {
404+
test("Should handle JSON parser failures", async () => {
320405
const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
321406
method: "POST", // valid method this time
322407
headers: {
@@ -337,6 +422,20 @@ describe("App in production mode", async () => {
337422
});
338423
});
339424

425+
test("Should handle Raw parser failures", async () => {
426+
const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, {
427+
method: "POST",
428+
headers: { "content-type": "application/octet-stream" },
429+
body: Buffer.alloc(100),
430+
});
431+
expect(response.status).toBe(413);
432+
const json = await response.json();
433+
expect(json).toEqual({
434+
status: "error",
435+
error: { message: "request entity too large" },
436+
});
437+
});
438+
340439
test("Should fail when missing content type header", async () => {
341440
const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
342441
method: "POST",
@@ -446,7 +545,7 @@ describe("App in production mode", async () => {
446545
await setTimeout(500);
447546
process.emit("FAKE" as "SIGTERM");
448547
expect(infoMethod).toHaveBeenCalledWith("Graceful shutdown", {
449-
sockets: 1,
548+
sockets: expect.any(Number),
450549
timeout: 1000,
451550
});
452551
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)