ErrorHandler: invoke class specific toJSON transformation

[twittwer]

Hi,
it would be great to have the ability to define a custom toJSON transformation of Errors before they are sent to the client.

UseCase:
We want to censor our custom Errors (extending HttpError) before they are leaving the server.
Therefore we thought to simply implement a toJSON method, like you can override the toString.

export class CustomError extends HttpError {
    public publicData: string;
    public secretData: string;

    constructor( httpCode: number, publicData: string, publicData: string ) {
        super( httpCode );
        Object.setPrototypeOf(this, CustomError.prototype);
        this.publicData = publicData;
        this.secretData = secretData;
    }

    toJSON () {
        return {
            httpCode: this.httpCode,
            publicData: this.publicData
        };
    }
}

Best Regards

[MichalLytek]

AFAIK, express uses JSON.stringify under the hood:
https://github.com/expressjs/express/blob/master/lib/response.js#L1103

And according to MDN:

If an object being stringified has a property named toJSON whose value is a function, then the toJSON() method customizes JSON stringification behavior: instead of the object being serialized, the value returned by the toJSON() method when called will be serialized

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify

So I think that the added feature is not needed at all. If toJSON() by the unknown reason doesn't work for you, the easiest way is to create your own error middleware which custom logic of hidding internal data or transforming the structure of error.

[twittwer]

I think that the toJSON is currently not invoked, because it's wiped out by the error processing in processJsonError before express "search" for it.

The processedError does not contain the toJSON any more, the incoming error does.

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

        if (typeof error.toJSON === "function")
            return error.toJSON();
        
        let processedError: any = {};
        if (error instanceof Error) {
            const name = error.name && error.name !== "Error" ? error.name : error.constructor.name;
            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;
    }

[twittwer]

In my scenario it also works if I add this lines, before the return:

if (typeof error.toJSON === 'function')
    processedError.toJSON = error.toJSON;

See commit 0d94f8e on branch patch-2

[MichalLytek]

I think that the first version with return makes more sense. It unnecessary to process error field if it will be overwritten by calling toJSON method.

However please write a test for your case to prove that it is working and prevent regression in the future (we will be doing huge refactor of error handling part) 😉

[twittwer]

sure, I'll do it during the next days

[twittwer]

I think the standard error processing is covered by the existing test, so just added one for the case of an existing toJSON function.
@19majkel94 I hope it fullfills your expectations 🙂

[twittwer]

Just added a little sample for the documentation. Can be delete, if you don't like it.

[MichalLytek]

LGTM!

[twittwer]

@pleerock @NoNameProvided
Could one of you please publish the 0.7.7 to npm? - It would be great 🙂

[NoNameProvided]

Hey @twittwer!

Currently, only @pleerock has publish rights. I can understand this feature may be important for you, so if you need this right now you can install a package from GitHub as well via:

"routing-controllers": "git+https://github.com/pleerock/routing-controllers.git"

I hope this helps.

[MichalLytek]

@NoNameProvided Are you sure about that? "main" in package.json looks incorrect so I don't think that it will work 😕

[twittwer]

@19majkel94 You're right, it does not work. I've tried it, but the structure in npm is different to the repository. During your publish process the dist/ content is moved to the root.

[NoNameProvided]

In that case, I suggest you to fork the repo and make the required modifications. @pleerock mentioned multiple times, he wants to move this repo and others under the @typestack organization. Then both @19majkel94 and I will be able to publish new versions.

[twittwer]

That's the way to go currently - just wanted to ask 🙂
Maybe we can but it on a branch here, so not everyone has to fork and "build".

[NoNameProvided]

I would rather talk with @pleerock to make the move as soon as he can but he is very busy with TypeORM right now.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.