Backport catcher alignment for v21

CHANGELOG.md

@@ -2,6 +2,15 @@
 
 ## Version 21
 
+### v21.11.6
+
+- Fixed inconsistency between the actual catcher behavior and the error handling documentation:
+  - Removed conversion of non-`HttpError`s to `BadRequest` before passing them to `errorHandler`;
+  - A `ResultHandler` configured as `errorHandler` is responsible to handling all errors and responding accordingly.
+  - The default `errorHandler` is `defaultResultHandler`:
+    - Using `ensureHttpError()` it coverts non-`HttpError`s to `InternalServerError` and responds with status code `500`;
+  - The issue has occurred since [v19.0.0](#v1900).
+
 ### v21.11.5
 
 - Fixed: the output type of the `ez.raw()` schema (without an argument) was missing the `raw` property (since v19.0.0).

README.md

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

src/server-helpers.ts

@@ -5,7 +5,7 @@ import { AbstractResultHandler } from "./result-handler";
 import { ActualLogger } from "./logger-helpers";
 import { CommonConfig, ServerConfig } from "./config-type";
 import { ErrorRequestHandler, RequestHandler, Request } from "express";
-import createHttpError, { isHttpError } from "http-errors";
+import createHttpError from "http-errors";
 import { lastResortHandler } from "./last-resort";
 import { ResultHandlerError } from "./errors";
 import { ensureError } from "./common-helpers";
@@ -32,9 +32,7 @@ export const createParserFailureHandler =
   async (error, request, response, next) => {
     if (!error) return next();
     return errorHandler.execute({
-      error: isHttpError(error)
-        ? error
-        : createHttpError(400, ensureError(error).message),
+      error: ensureError(error),
       request,
       response,
       input: null,

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

@@ -66,19 +66,19 @@ exports[`App in production mode > Protocol > Should fail on invalid method 1`] =
 }
 `;
 
-exports[`App in production mode > Protocol > Should fail on malformed body 1`] = `
+exports[`App in production mode > Protocol > Should fail when missing content type header 1`] = `
 {
   "error": {
-    "message": StringMatching /\\(Unexpected end of JSON input\\|Unterminated string in JSON at position 25\\)/,
+    "message": "key: Required",
   },
   "status": "error",
 }
 `;
 
-exports[`App in production mode > Protocol > Should fail when missing content type header 1`] = `
+exports[`App in production mode > Protocol > Should handle JSON parser failures 1`] = `
 {
   "error": {
-    "message": "key: Required",
+    "message": StringMatching /\\(Unexpected end of JSON input\\|Unterminated string in JSON at position 25\\)/,
   },
   "status": "error",
 }

tests/system/system.spec.ts

@@ -1,5 +1,7 @@
 import cors from "cors";
 import depd from "depd";
+import express from "express";
+import { readFile } from "node:fs/promises";
 import { z } from "zod";
 import {
   EndpointsFactory,
@@ -10,6 +12,7 @@ import {
   ResultHandler,
   BuiltinLogger,
   Middleware,
+  ez,
 } from "../../src";
 import { givePort } from "../helpers";
 import { setTimeout } from "node:timers/promises";
@@ -100,23 +103,44 @@ describe("App in production mode", async () => {
     output: z.object({}),
     handler: async () => setTimeout(5000, {}),
   });
+  const rawEndpoint = new EndpointsFactory(defaultResultHandler).build({
+    method: "post",
+    input: ez.raw(),
+    output: z.object({ crc: z.number() }),
+    handler: async ({ input: { raw } }) => ({ crc: raw.length }),
+  });
+  const uploadEndpoint = new EndpointsFactory(defaultResultHandler).buildVoid({
+    method: "post",
+    input: z.object({ avatar: ez.upload() }),
+    handler: vi.fn(),
+  });
   const routing = {
     v1: {
       corsed: corsedEndpoint,
       faulty: faultyEndpoint,
       test: testEndpoint,
       long: longEndpoint,
+      raw: rawEndpoint,
+      upload: uploadEndpoint,
     },
   };
   vi.spyOn(process.stdout, "write").mockImplementation(vi.fn()); // mutes logo output
   const config = createConfig({
     http: { listen: port },
     compression: { threshold: 1 },
+    rawParser: express.raw({ limit: 20 }),
+    upload: {
+      beforeUpload: ({ request }) => {
+        if ("trigger" in request.query) throw new Error("beforeUpload failure");
+      },
+    },
     beforeRouting: ({ app, getLogger }) => {
       depd("express")("Sample deprecation message");
       app.use((req, {}, next) => {
         const childLogger = getLogger(req);
         assert("isChild" in childLogger && childLogger.isChild);
+        if (req.path === "/trigger/beforeRouting")
+          return next(new Error("Failure of beforeRouting triggered"));
         next();
       });
     },
@@ -247,6 +271,35 @@ describe("App in production mode", async () => {
         "Content-Range,X-Content-Range",
       );
     });
+
+    test("Should handle raw request", async () => {
+      const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, {
+        method: "POST",
+        headers: { "content-type": "application/octet-stream" },
+        body: Buffer.from("testing"),
+      });
+      expect(response.status).toBe(200);
+      const json = await response.json();
+      expect(json).toEqual({ status: "success", data: { crc: 7 } });
+    });
+
+    test("Should handle upload request", async () => {
+      const filename = "logo.svg";
+      const logo = await readFile(filename, "utf-8");
+      const data = new FormData();
+      data.append(
+        "avatar",
+        new Blob([logo], { type: "image/svg+xml" }),
+        filename,
+      );
+      const response = await fetch(`http://localhost:${port}/v1/upload`, {
+        method: "POST",
+        body: data,
+      });
+      expect(response.status).toBe(200);
+      const json = await response.json();
+      expect(json).toEqual({ data: {}, status: "success" });
+    });
   });
 
   describe("Negative", () => {
@@ -297,6 +350,38 @@ describe("App in production mode", async () => {
       expect(text).toBe("Internal Server Error");
       expect(errorMethod.mock.lastCall).toMatchSnapshot();
     });
+
+    test("Should treat beforeRouting error as internal", async () => {
+      const response = await fetch(
+        `http://127.0.0.1:${port}/trigger/beforeRouting`,
+      );
+      expect(await response.json()).toEqual({
+        status: "error",
+        error: { message: "Internal Server Error" },
+      });
+      expect(response.status).toBe(500);
+    });
+
+    test("Should treat beforeUpload error as internal", async () => {
+      const filename = "logo.svg";
+      const logo = await readFile(filename, "utf-8");
+      const data = new FormData();
+      data.append(
+        "avatar",
+        new Blob([logo], { type: "image/svg+xml" }),
+        filename,
+      );
+      const response = await fetch(
+        `http://localhost:${port}/v1/upload?trigger=beforeUpload`,
+        { method: "POST", body: data },
+      );
+      expect(response.status).toBe(500);
+      const json = await response.json();
+      expect(json).toEqual({
+        error: { message: "Internal Server Error" },
+        status: "error",
+      });
+    });
   });
 
   describe("Protocol", () => {
@@ -316,7 +401,7 @@ describe("App in production mode", async () => {
       expect(json).toMatchSnapshot();
     });
 
-    test("Should fail on malformed body", async () => {
+    test("Should handle JSON parser failures", async () => {
       const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
         method: "POST", // valid method this time
         headers: {
@@ -337,6 +422,20 @@ describe("App in production mode", async () => {
       });
     });
 
+    test("Should handle Raw parser failures", async () => {
+      const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, {
+        method: "POST",
+        headers: { "content-type": "application/octet-stream" },
+        body: Buffer.alloc(100),
+      });
+      expect(response.status).toBe(413);
+      const json = await response.json();
+      expect(json).toEqual({
+        status: "error",
+        error: { message: "request entity too large" },
+      });
+    });
+
     test("Should fail when missing content type header", async () => {
       const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
         method: "POST",
@@ -446,7 +545,7 @@ describe("App in production mode", async () => {
       await setTimeout(500);
       process.emit("FAKE" as "SIGTERM");
       expect(infoMethod).toHaveBeenCalledWith("Graceful shutdown", {
-        sockets: 1,
+        sockets: expect.any(Number),
         timeout: 1000,
       });
       await setTimeout(1500);

tests/unit/server-helpers.spec.ts

@@ -35,9 +35,11 @@ describe("Server helpers", () => {
 
     test.each([
       new SyntaxError("Unexpected end of JSON input"),
+      new Error("Anything"),
       createHttpError(400, "Unexpected end of JSON input"),
+      "just a text",
     ])(
-      "the handler should call error handler with correct error code %#",
+      "the handler should call error handler with given error %#",
       async (error) => {
         const errorHandler = new ResultHandler({
           positive: vi.fn(),
@@ -57,7 +59,7 @@ describe("Server helpers", () => {
         );
         expect(spy).toHaveBeenCalledTimes(1);
         expect(spy.mock.calls[0][0].error).toEqual(
-          createHttpError(400, "Unexpected end of JSON input"),
+          error instanceof Error ? error : new Error(error),
         );
       },
     );