Include err.message for debug data

[davidcheung]

Because err.message is also not enumerable,
therefore needs to be explicited added to the data obj

[davidcheung]

@bajtos PTAL

[bajtos]

We don't check whether err.message is set in other fill functions, is there any reason why we can't do the same here?

I'd also prefer to set the message among the first items, because JSON serializers preserve the order in which properties were defined.

data.message = err.message;

for (var p in err) {
  if ((p in data)) continue;
  data[p] = err[p];
}

data.stack = err.stack;

Could you please check that err.name is included in debug-mode data too?

[davidcheung]

if we do data.message = err.message;
the result will come back with
"message" : ""

---

and we need to handle scenario where a non-error is thrown (eg. throw 'string' )
https://github.com/strongloop/strong-error-handler/blob/ff2af3b5555ee84bdb110fef74effbf891a176fa/lib/data-builder.js#L20

do you think data.message = data.message || err.message; would work?

[bajtos]

the result will come back with "message" : ""

Hmm, I did was not aware of this edge case. In that case, I guess the following would be best?

if (err.message && !data.message) 
  data.message = err.message;

[bajtos]

I think we need to handle this edge case in fillBadRequestError too, shouldn't we?

Another option, which may be better, is to change the code handling non-errors so that it modified the err object instead of the data object:

 if (typeof err !== 'object') {
   err = {
      statusCode: 500,
      message: '' + err,
  }
}

And then we can do data.message = err.message everywhere else.

@davidcheung Thoughts?

[bajtos]

@davidcheung could you please add a testcase (or two?) to cover "the result will come back with "message" : """

Also cross-posting from collapsed #11 (diff)

the result will come back with "message" : ""

Hmm, I did was not aware of this edge case. In that case, perhaps the following would be better?

if (err.message && !data.message) 
  data.message = err.message;

I think we need to handle this edge case in fillBadRequestError too, shouldn't we?

Another option, which may be even better, is to change the code handling non-errors so that it modified the err object instead of the data object:

 if (typeof err !== 'object') {
   err = {
      statusCode: 500,
      message: '' + err,
  }
}

And then we can do data.message = err.message everywhere else, at least I think so.

@davidcheung Thoughts?

[davidcheung]

agreed, i think this approach is better too

Another option, which may be better, is to change the code handling non-errors so that it modified the err object instead of the data object:

 if (typeof err !== 'object') {
   err = {
      statusCode: 500,
      message: '' + err,
  }
}

---

this test case should handle the throw('STRING') scenario, now with err converted to an object with the property the message: "" should not be an issue anymore (sorry edited message a few times the notification you got is probably different)
https://github.com/strongloop/strong-error-handler/blob/a5129595ef672151d6bfd274af51cfcbe224a60b/test/handler.test.js#L319

@davidcheung could you please add a testcase (or two?) to cover "the result will come back with "message" : """

[davidcheung]

another issue to bring up is we are nolonger removing the statusCode from Array Error's details
is this intentional? since we are doing it in strong-remoting
https://github.com/strongloop/strong-remoting/blob/aa391d2e65460196ae2a59493f9dd4844622f35d/lib/rest-adapter.js#L380
I guess this could be addressed in a separate issue / PR

[bajtos]

another issue to bring up is we are nolonger removing the statusCode from Array Error's details
is this intentional? since we are doing it in strong-remoting
https://github.com/strongloop/strong-remoting/blob/aa391d2e65460196ae2a59493f9dd4844622f35d/lib/rest-adapter.js#L380
I guess this could be addressed in a separate issue / PR

Probably not intentional, please open a new pull request.

[bajtos]

Can we omit name in this case? Will it cause any failures in existing tests, either here or in strong-remoting?

[davidcheung]

@bajtos I have addressed the comments, PTAL

[bajtos]

expect(res.body).to.have.property('error', 'Error');

[bajtos]

Two more nitpicks, the rest LGTM. No further reviews are necessary, just wait for CI pass and squash the commits before landing.