Added error context to the json

[wesleytodd]

We need to return extra metadata with our errors, and the existing functionality explicitly stripped the extra context. This is similar to the functionality that existed in in restify 4.x (although it was in a different format). Do you think this is a good addition?

CC: @ghermeto @m5m1th

[coveralls]

Coverage increased (+0.02%) to 98.206% when pulling ad2a959 on wesleytodd:add-context-to-json into ec07e40 on restify:master.

[coveralls]

Coverage increased (+0.02%) to 98.206% when pulling ad2a959 on wesleytodd:add-context-to-json into ec07e40 on restify:master.

[coveralls]

Coverage increased (+0.02%) to 98.206% when pulling ad2a959 on wesleytodd:add-context-to-json into ec07e40 on restify:master.

[DonutEspresso]

Thanks for the PR! I think we default to not returning it for security - frequently context can contain sensitive information.

However, you should able to override the Error instance's toJSON to specify your own custom payload. If this is for a custom error class, you can also pass in toJSON when you create the constructor.

[wesleytodd]

Hey! That is what we were going to do, but it is a pretty heavy handed approach which we were trying to avoid. What do you think about adding a new option for exposing this type of context in the response? Here are a few ideas we thought could work:

• metadata
• details or errorDetails
• publicContext

[DonutEspresso]

We just released 6.x today, which migrates context property to use VError's info property under the hood. With this move we now mirror VError's API in its entirety, so unfortunately we are very unlikely to introduce any additional fields at this point. That said, I still want to find a way to resolve your problem.

Maybe I should take a step back, I kind of assumed you meant to return the .context fields to a client (over the wire). If that is indeed case, then in the context of this library that feels like a serialization concern. On the server, you typically want to keep maintain all relevant context (sensitive or not) until the point of it going out the door. At the point of serialization, you would then decide how you want this to be represented over the wire, and/or do any filtering as necessary. Arguably, same concerns might apply to logging sensitive things like PII on the server side.

If you are using this library with a restify web server, then a response with a header a content-type of txt/plain would call cause restify to automatically call toString() on the error, and a content-type of application/json would call toJSON on the error (not directly, but via JSON.stringify). You could even take a hammer like approach and simply do a instanceof Error check on the way out, and replace it with string 'foobarbaz'. Hope that context sheds some light on why this library takes the toJSON/toString approach.

Would you mind sharing code snippets of your use case? That might help me to better understand why you feel toJSON feels heavy handed.

[wesleytodd]

Unfortunately, I am not familiar enough with the full context around this issue to know the right way to move forward. I just figured that I would open this PR because it was such an easy addition. Also since it seemed like a feature removal it seemed like it may not have been intentional, or that with the proper implementation you would be open to adding it back.

Unless @ghermeto wants to push this conversation forward, we can close this.

[DonutEspresso]

Bummer, sorry about the poor first PR experience! 😞 As under-resourced as we are we appreciate all the help we can get! The proactiveness is definitely 💯!

Here are some things you can try. If the use case is for a custom error, it's easy to provide a toJSON when you create the constructor itself:

restifyErrs.makeConstructor('MyCustomError', {
    statusCode: 500,
    toJSON: function toJSON() {
        const self = this;
        return {
            message: self.message,
            context: self.context
        };
    }
});

var myErr = new MyCustomError();
JSON.stringify(myErr); // => contains context field

If this is for any of the standard errors that restify-errors ships out of the box, and you happen to be using restify, you can implement a custom formatter to do last mile interception/formatting:

const server = restify.createServer({
    name: 'foo',
    formatters: {
        // all application/json responses will be funneled through this formatter. in restify,
        // formatters are a last mile serializer for preparing your payload based on content-type.
        'application/json': function(req, res, body) {
            // in formatters, body is the object passed to res.send()
            if (body instanceof Error) {
                // overwrite the built-in toJSON
                body.toJSON = function() {
                    return {
                        message: body.message,
                        context: body.context
                    };
                };
                // alternatively, you can also directly return a response:
                // return JSON.stringify({
                //     message: body.message,
                //     context: body.context
                // });
            }
            // for everything else, stringify as normal
            return JSON.stringify(body);
        }
    }
});

If neither of these work, let me know - would love to know if there's a use case we haven't addressed properly.

[ghermeto]

Hi @DonutEspresso,

that is exactly what we are doing:

errors.makeConstructor('RuleConflictError', {
    statusCode: 409,
    message: '... my message ...',
    toJSON: function () {
        const { body, context } = this;
        return JSON.stringify({ ...body, context });
    }
});

The question we had was if context was hidden by mistake.

I believe that it is valid to allow your APIs to return some sort of context to the clients, so that they can have important details for errors which the code and HTTP status code are not enough. If that is using context or some other key (like info), it doesn't matter much, but if every micro-service needs to override the serialization method, why not to just provide it inside the library?

Right now we will just build a wrapper around restify-errors to get the desired error payload.

[DonutEspresso]

I hear you! For historical context, this module came out of need for capturing errors through multiple server side layers in our production stacks - so the truth is that we use it to capture a ton of sensitive info that was never intended to go out the wire (and especially not back to our customers).

However, I understand that not every service is subject to those same restrictions, and it sounds like you're actually using the error fields to build up the payload that is returned to the client. That's definitely not a use case we had in mind, but sounds like fair game.

Rather than change the default serialization, which would just swing us from one end of the spectrum to the other, I think there's probably a way we can provide the ability to set a library wide defaults for all errors (of which serialization would be one of many). A strawman might be something along the lines of:

const errs = require('restify-errors');
errs.defaults.toJSON = function toJSON() { ... } . // => would get applied to prototype of all errors created by this module

Would that work?

[ghermeto]

yes, that would work!

[wesleytodd]

Closing since this is probably not moving forward.