Default error handler should be the last

[Diluka]

I want to create an error handler to format errors then use default handler to send. But the truth is the default error handler processing errors at the beginning. My handler gets the errors after sending.

Now I can only disable the default error handler, copy your codes from driver, put it at my error handler, and invoke them after error format. It's duplicated.

It's better putting the default error handler in the last order.

[pleerock]

adding @19majkel94

[NoNameProvided]

Now I can only disable the default error handler, copy your codes from driver, put it at my error handler, and invoke them after error format. It's duplicated.

You have to disable the default error handler and register one written by you. After that it should get the errors before sending any response.

[pleerock]

@Diluka does @NoNameProvided solution works for you?

[MichalLytek]

Now I can only disable the default error handler, copy your codes from driver, put it at my error handler, and invoke them after error format. It's duplicated.

But the default error handler just set status code, call processJsonError do the formatting and send the response. Why you can't write your own middleware to format and send your own error and you need the original code? I had something like this (before I made a PR to handle ValidationError in routing-controllers):

import { ErrorMiddlewareInterface, MiddlewareGlobalAfter, HttpError } from "routing-controllers";
import { ValidationError } from "class-validator";
import * as express from "express";

@MiddlewareGlobalAfter()
export class CustomErrorHandler implements ErrorMiddlewareInterface {

    public error(error: any, _req: express.Request, res: express.Response, _next: express.NextFunction) {
        const responseObject = {} as any;
        console.error(error);

        // if its an array of ValidationError
        if (Array.isArray(error) && error.every((element) => element instanceof ValidationError)) {
            res.status(400);
            responseObject.message = "You have an error in your request's body. Check 'errors' field for more details!";
            responseObject.errors = error;
        } else {
            // set http status
            if (error instanceof HttpError && error.httpCode) {
                res.status(error.httpCode);
            } else {
                res.status(500);
            }

            if (error instanceof Error) {
                const developmentMode: boolean = process.env.NODE_ENV === "development";

                // set response error fields
                if (error.name && (developmentMode || error.message)) { // show name only if in development mode and if error message exist too
                    responseObject.name = error.name;
                }
                if (error.message) {
                    responseObject.message = error.message;
                }
                if (error.stack && developmentMode) {
                    responseObject.stack = error.stack;
                }
            } else if (typeof error === "string") {
                responseObject.message = error;
            }
        }

        // send json only with error
        res.json(responseObject);
    }
}

[Diluka]

@pleerock Yes for now. Your code is what I need. I still want to use it without copying.

[MichalLytek]

I've tried to move the next call before the sending error code but there's no way to return value from middleware to place which called next(). So the default error handler should be implemented as a middleware but then it has no info about ActionMetadata so it couldn't apply action http headers. Leaving it as is also won't work with async middlewares.

We have interceptors for successful response but no way to easily format error response. Maybe we should expose and option to provide custom processJsonError and processTextError handlers? This should be enough to reduce common boilerplate. Adding @pleerock @NoNameProvided.

[NoNameProvided]

I dont understand what custom error handlers doesnt work for @Diluka

[MichalLytek]

It works, but you need to copy the code to create custom error handler and disable the built one. And as you can see, it also doesn't apply action header because middleware is not an iterceptor and don't have access to that info.

The common usage pattern is to only perform formatting the error response, eg. text error without stack trace or JSON with some info or other properties, like with interceptors do:

{
    correct: true,
    value: {
        // result from action
    }
}

{
    correct: false,
    value: {
        // error object goes here
    }
}

I would introduce ErrorInterceptor to use it for logging or formatting purpose and the default error handler for sending the response to client.

[NoNameProvided]

We have interceptors for successful response but no way to easily format error response. Maybe we should expose and option to provide custom processJsonError and processTextError handlers? This should be enough to reduce common boilerplate.

We need interceptors because multiple parts of the app can manipulate the response (controller / middleware) and interceptor is the step where every response must flow through.

However at errors there is only one flow so I dont like the idea of adding an interceptor for this. It gives you a sub-part of the functionality an error handler already can give. Writing that 5 line of boilerplate once wont kill anyone in my opinion.

[MichalLytek]

Writing that 5 line of boilerplate once wont kill anyone in my opinion.

If you look at this, it's not only 5 lines:

            if (error.httpCode) {
                response.status(error.httpCode);
            } else {
                response.status(500);
            }

            // apply http headers
            if (action) {
                Object.keys(action.headers).forEach(name => {
                    response.header(name, action.headers[name]);
                });
            }

            // send error content
            if (action && action.isJsonTyped) {
                response.json(this.processJsonError(error));
            } else {
                response.send(this.processTextError(error)); // todo: no need to do it because express by default does it
            }


    protected processJsonError(error: any) {
        if (!this.isDefaultErrorHandlingEnabled)
            return error;

        let processedError: any = {};
        if (error instanceof Error) {
            const name = error.name && error.name !== "Error" ? error.name : error.constructor.name;

            if (name && (this.developmentMode || error.message)) // show name only if in debug mode and if error message exist too
                processedError.name = name;
            if (error.message)
                processedError.message = error.message;
            if (error.stack && this.developmentMode)
                processedError.stack = error.stack;

            Object.keys(error)
                .filter(key => key !== "stack" && key !== "name" && key !== "message" && (!(error instanceof HttpError) || key !== "httpCode"))
                .forEach(key => processedError[key] = (error as any)[key]);

            if (this.errorOverridingMap)
                Object.keys(this.errorOverridingMap)
                    .filter(key => name === key)
                    .forEach(key => processedError = this.merge(processedError, this.errorOverridingMap[key]));

            return Object.keys(processedError).length > 0 ? processedError : undefined;
        }

        return error;
    }

    protected processTextError(error: any) {
        if (!this.isDefaultErrorHandlingEnabled)
            return error;

        if (error instanceof Error) {
            if (this.developmentMode && error.stack) {
                return error.stack;

            } else if (error.message) {
                return error.message;
            }
        }
        return error;
    }

Copy-paste lib internals is not a solution IMO 😠

Also, with custom error middleware you can't do this:

if (action && action.isJsonTyped) {
    response.json(this.processJsonError(error));
} else {
    response.send(this.processTextError(error)); // todo: no need to do it because express by default does it
}

So you will always return JSON, even for server-rendered templates or text response for JSON REST API.

Interceptors might be an overkill but I would like to expose an option to provide custom error processing handler which has access to ActionMetadata and could perform error formating and manipulation (rendering nice error page or formatting JSON response object with some additional info).

[NoNameProvided]

Copy-paste lib internals is not a solution IMO 😠

Nobody said that we should do that. What I talked about the custom error handler what aprox 6 lines of boilerplate and your custom added logic.

import {Middleware, ExpressErrorMiddlewareInterface} from "routing-controllers";

@Middleware({ type: "after" })
export class CustomErrorHandler implements ExpressErrorMiddlewareInterface {

    error(error: any, request: any, response: any, next: (err: any) => any) {
        // your logic
        next(); // or res.json or res.send
    }

}

Interceptors might be an overkill but I would like to expose an option to provide custom error processing handler which has access to ActionMetadata and could perform error formating and manipulation

Better to add it to the error handler middleware than. Agreed?

[MichalLytek]

or res.json or res.send

How you can determine that in your middleware? You don't have access to action.isJsonTyped like the default handler. You can blind return JSON so the front page of your website will receive nice json text instead of error page or you will return html in your JSON REST API.

Custom error handler is ok, you have freedom, you can format it whatever you want but without ActionMetadata the usage is limited - no action.isJsonTyped, so headers apply and other stuffs.

[NoNameProvided]

How you can determine that in your middleware?

As we build a rest apis we always use json().

We should expose the metadata to the error handler as you mentioned a few comments before but not adding a new interceptor with partial capabilities.

[MichalLytek]

As we build a rest apis we always use json().

But sometimes we mix JSON and text in one controllers (#236) so it's really possible that in the whole routing-controllers app some endpoints do rendering html, generating pdf or svg or streaming file so sending JSON always is a bad idea.

[NoNameProvided]

We should expose the metadata to the error handler as you mentioned a few comments before but not adding a new interceptor with partial capabilities.

As I wrote we should add the action metadata to the error handler, something like

public error(error: ApiError, req: Request, res: Response, next: NextFunction, metaData: any)

and not sending JSON all the time. That was our use case only.

[marshall007]

@NoNameProvided @19majkel94 we could consider exposing a slightly different ErrorMiddlewareInterface:

class ErrorHandlerMiddleware<T> {
  // returns `T` or `undefined`, which would be equivalent
  // to calling `next()` with or without an argument, respectively
  error(error: any, request: any, response: any): T | undefined;

  // generic formatter, called with the result of `.error()`
  format(error: T, meta: any): HttpError<T>;

  // common formatters which are not required to be implemented.
  // if defined, they will be called instead of the generic `.format()`
  // method based on the response content type
  text(error: T, meta: any): HttpError<T>;
  json(error: T, meta: any): HttpError<T>;
  html(error: T, meta: any): HttpError<T>;
}


interface HttpError<T extends Error> {
  status: number; // HTTP status code override
  content: string; // formatted response
  error: T; // capture original error (other things like logging middleware might need it down the chain)
}

[NoNameProvided]

@marshall007 this needs some more discussion about the details but I like the idea. @19majkel94 ?

[MichalLytek]

@marshall007 Oh god! We've jumped from "not complicate things - put all in your custom error middleware" to the big generic error handler with multiple methods and interfaces 😆

I like samples and examples, you need to show me the sample usage of this API. Right now it looks overengineered for me.

[NoNameProvided]

I like samples and examples

Having a default ErrorHandlerMiddleware what is used by default, and the easy way to only override specific parts of it. An example:

class CustomErrorHandler extends ErrorHandlerMiddleware {
  text(error: T, meta: any): HttpError<T> {
    // my very own implementation for text errors
   // while json and html errors still handled by the extended ErrorHandlerMiddleware class
  }

}

Anyway we use kind of the same functions just scattered away in the code base.

[MichalLytek]

Ok, now I get the idea. Instead of exposing in options functions to overwrite like processTextError we expose whole generic error handler class to extend it by user. So when we pass it in routing-controllers options as a error handler, the default would be replaced by custom extended one. Nice!

However the ErrorHandlerMiddleware interface needs some polishing. We shouldn't expose request: any, response: any but action like in interceptors (intercept(action: Action, result: any)).

And I think that formatters shouldn't have to return HttpError<T> with property error: T - we could capture original error at the framework level and then call the handler. Also json formatter should respond with content: object not string.

Also, do we need this two methods?

error(error: any, request: any, response: any): T | undefined;
format(error: T, meta: any): HttpError<T>;

I think that it would be better that we just have on method where we receive the error: any and in one place do the loging, transforming and other stuffs and just return the new transformed error (with meta from action data?) with statusCode.

And I don't know what generic T is for. I tought that we have one error handler where we do the check logic and formating. T means that for every error class there would be separate error handler class?

[MichalLytek]

@pleerock I think that problem with calling next(error) in default middleware and calling next() after route handler brings us to one conclusion - it's time to do big changes and drop off support for error/after middleware and replace it with this and this - basically it is about enhanced signature with access to action metadata and returned value as we don't need support for 3rd party middlewares using standard signatures (they don't exist, we only use custom ones, handwritten).

[IAMtheIAM]

I'm still confused after reading all this. What is the proper way to:

1. Return text HTMLtemplate when user requests a GET method and gets error (like 404)
2. Return JSON when user requests POST method and gets error (like 404)

When I implement this function which I saw above , I get a json response error on POST, but when visiting a non exist page, the error handler is not invoked. Instead, Cannot GET /hgfhgfhgfh is displayed

    public error(error: any, _req: express.Request, res: express.Response, next: express.NextFunction) {
        const responseObject = {} as any;
        console.error(error);

        // if its an array of ValidationError
        if (Array.isArray(error) && error.every((element) => element instanceof ValidationError)) {
            res.status(400);
            responseObject.message = "You have an error in your request's body. Check 'errors' field for more details!";
            responseObject.errors = error;
        } else {
            // set http status
            if (error instanceof HttpError && error.httpCode) {
                res.status(error.httpCode);
            } else {
                res.status(500);
            }

            if (error instanceof Error) {
                const developmentMode: boolean = process.env.NODE_ENV !== "production";

                // set response error fields
                if (error.name && (developmentMode && error.message)) { // show name only if in development mode and if error message exist too
                    responseObject.name = error.name;
                }
                if (error.message) {
                    responseObject.message = error.message;
                }
                if (error.stack && developmentMode) {
                    responseObject.stack = error.stack;
                }
            } else if (typeof error === "string") {
                responseObject.message = error;
            }
        }

        // send json only with error
        res.json(responseObject);
        
    }

Edited: added syntax highlighting - @NoNameProvided

[IAMtheIAM]

I got it working using express default error handler and routing.

export function customErrorHandler(err, req, res, next) {
    var isProduction = app.get('env') === 'production';

    // Error handlers
    // Development - will print stacktrace
    // Production - no stacktraces leaked to user
    if (req.method === 'POST') {

        if (res.headersSent) {
            return next(err) // if headers are already sent, don't render the template
        }

        const responseObject = {} as any;
        console.error(err);

        // if its an array of ValidationError
        if (Array.isArray(err) && err.every((element) => element instanceof ValidationError)) {
            res.status(400);
            responseObject.message = "You have an err in your request's body. Check 'errs' field for more details!";
            responseObject.errors = err;
        } else {
            // set http status
            if (err instanceof HttpError && err.status) {
                res.status(err.status);
            } else {
                res.status(404);
            }

            if (err instanceof Error) {
                const developmentMode: boolean = !isProduction;

                // set response error fields
                if (err.name && (developmentMode && err.message)) { // show name only if in development mode and if error message exist too
                    responseObject.name = err.name;
                }
                if (err.message) {
                    responseObject.message = err.message;
                }
                if (err.stack && developmentMode) {
                    responseObject.stack = err.stack;
                }
            } else if (typeof err === "string") {
                responseObject.message = err;
            }
        }

        // Return JSON for POST requests
        // send json only with error
        res.json(responseObject);

        // Return the rendered error.pug HTML template for GET requests
    } else if (req.method === 'GET') {
        // set http status
        if (err instanceof HttpError && err.status) {
            res.status(err.status);
        } else {
            res.status(404);
        }
        // if (err.message !== corsErrorMessage) {
        //     return next(err)
        // }

        // The route path is relative to whatever value is set for app.set('views', '/app/routes')
        res.render('error/error', {
            title  : err.message,
            message: err.message,
            status : err.status,
            stack  : isProduction ? '' : err.stack
        });
    }
}

But when I try to convert it to routing-controller framework, it does not register any error for page not found or for missing required bodyParams

import { Middleware, ExpressErrorMiddlewareInterface, ExpressMiddlewareInterface, HttpError } from "routing-controllers";
import { customErrorHandler } from "./error.middleware.express-router";

@Middleware({ type: "after" })
export class ErrorHandler implements ExpressErrorMiddlewareInterface {

    constructor(private customErrorHandler: CustomErrorHandler) {}

    // Custom error handlers
    error(err: any, req: any, res: any, next: (err: any) => any) {

        console.log("Request: ", req.originalUrl);
        var err = <any>new HttpError(404, 'Page Not Found');
        // err.status = 404;
        this.customErrorHandler.handleError(err, req, res, next);

    }
}

Edited: added syntax highlighting - @NoNameProvided

Update: Actually now it does register errors fine when used within routing-controller framework. Its a lot of code to handle this use case, but it works.

[github-actions[bot]]

Stale issue message