Allow stringification of StatusCodeError errors to be disabled

[Limess]

Hello,

We have quite strict data-protection requirements regarding what can be logged to disk. When a StatusCodeError is logged, the error is currently thrown with a message which contains the whole stringified body (

promise-core/lib/errors.js

Line 26 in c377e5c

this.message = statusCode + ' - ' + (JSON && JSON.stringify ? JSON.stringify(body) : body);

). This creates problems as we need to intercept this and remove it from the message.

Would it be possible to add an option to disable this behaviour (as the body is still acceptable on the error object)?

[analog-nico]

Hi @Limess good point. I assume you only log the error message and maybe the stack trace but not the other error attributes like response in particular. That means the message of the StatusCodeError is the only problematic attribute, right?

---

Let's talk about a quick fix first since your project probably demands a quick resolution. You may replace the whole StatusCodeError if you want:

var rp = require('request-promise') // request-promise may required first - no problem

// Replace the StatusCodeError during app boot time
var rpErrors = require('request-promise/errors')

rpErrors.StatusCodeError = MyCustomStatusCodeError

rp(...) // Your first request in your app - now uses your custom error implementation

For MyCustomStatusCodeError you can just copy and paste the original code and tweak it as you like as long as you keep the constructor's signature.

---

Now about the proper fix: I agree, the error message is not the most brilliant one. We could of course just change it to "The request got a 404 response" or similar. But then it is not very insightful when encountered during development. What message would you use in your case which is more telling?

[Limess]

Message is the only problematic attribute, we can easily control the logging of other attributes.

We've done a quick fix of wrapping the library (already the case) and catching/re-throwing StatusCodeErrors with a re-written message (currently just trimming after the status code).

With regards to what information to instead place on the message, it's a difficult one without having detailed knowledge of what's 'safe' to add to the message which is beyond this library's role. We could provide a transform of the response but that might be overkill. Adding the status code string doesn't add much information either.

I think we would leave this enabled in development, just disabled on production servers, so the obtuse message may not be a huge problem.

[analog-nico]

I thought about it for a while and suggest this:

• The message of the StatusCodeError is changed to just "Received a response".
• To not introduce any rarely required overhead I would add unit tests that ensure that replacing the Error types - like in my snippet above - will always be possible. So in advanced use cases custom Error types can be used.

Sounds good @Limess ?

[gurpreetatwal]

@analog-nico may I make a PR for this?

Also why is this.error labeled as a legacy attribute, should body just get set to this.body?

[analog-nico]

@gurpreetatwal Of course, go ahead. To potentially save you some rework how do you intent to design the feature in terms how a user will use it?

this.error is redundant to this.cause. this.cause was introduced in [email protected] and this.error is kept to not introduce a breaking change.

[gurpreetatwal]

@analog-nico I think I might have misunderstood the solution you suggested. From my understanding the only changes that would be required would be:

• changing the message property of StatusCodeError to be something vague like Request status code was not 2xx or something of the like
• adding unit tests that ensure that replacing the Error types always works

[analog-nico]

@gurpreetatwal Oh sorry, I forgot that I already proposed a solution. Absolutely, go ahead with that. Thanks for your contribution!

[gurpreetatwal]

In terms of the this.error I messed up the link in my original comment, but I meant to refer specifically to its usage in StatusCodeError. For TransformError and RequestError this.error is just an alias for this.cause but for StatusCodeError it's a unique identifier for the request body. I was curious as to why body wasn't just set to this.body.

Assigning the response body to this.error can lead to some puzzling looking code when the response body also contains an error property.

rp.get(...).catch(StatusCodeError, function(err) {
   const detail = err.error.error.detail;
})

Given that the request responds with

{
  "error": {
      "detail": "...."
   }
}

[analog-nico]

True, quirky. It’s also legacy from <0.4. I didn’t introduce a body attribute for StatusCodeError because you can just use .response.body. So the error attribute in all error types is not needed anymore and only still there for backwards compatibility.

[gurpreetatwal]

Ah gotcha, makes sense

Any preference for what StatusCodeError's message should be?

I was thinking it would be cool if the message matched the description for the error encountered, it provides some information useful information when logged, but doesn't have the potential to reveal anything sensitive. However, this would require having to maintain a mapping of code -> message and updating that as needed.

[analog-nico]

Good idea. "404 - Not Found" etc.

You may use require('http').STATUS_CODES for that. request is requiring http anyway so this means no extra dependency which would otherwise be bad for Browserify and Webpack users.

[awitherow]

Why is the error lib even modifying the message to begin with?

I am passing a specific message along with the error and then this library modifies the message.

I am having to do string manipulation just to get the original message I wanted out of it.

Is there a way to modify the error in a way that adds the information you are attempting to add to it, without modifying the message at all?

[analog-nico]

@awitherow Might the error properties make your life easier? E.g. StatusCodeError has a .response property which is the full response that also contains the body .response.body.

[awitherow]

very nice @analog-nico thanks!