zooniverse / json-api-client

Apache License 2.0
10 stars 5 forks source link

return error object with status and response #37

Closed mcbouslog closed 7 years ago

mcbouslog commented 7 years ago

See Issue #36.

This PR would be the 1st PR to be merged of general error handling improvements.

Please confirm my understanding:

Currently, if the error is a 4 (other than 408), 5 or other error it would be caught in make-http-request line 45 and the handleError function in json-api-client would reject without returning anything.

However, the panoptes-javascript-client/api-client handleError function overrides the json-api-client error handler, checks if what it received was an error and if so throw it, otherwise it received a response (meaning a 4 [but not 408] or 5) or something else, and based on that response or something else it crafts an error message to attempt to explain the error and then throws that error message (without status code or including the response object if there was one).

Changes

The error we're getting via superagent has a status and response field as they document here and had been helpfully mentioned in the comments (thanks!). This PR changes it so the error object and only the error is returned, never the response, as the response (if present) is included as a field on the error object. Then in a related panoptes-javascript-client PR it should throw the error object, including status and response, or a generic error message.

Initial Questions

  1. The panoptes-javascript-client has additional error handling logic, or mostly just attempts to craft a more helpful error message, but we could:

A. move any/all error handling logic and message crafting to the json-api-client B. get rid of any/all error handling logic from the panoptes-javascript-client and just return the new error object and let PFE or whatever app is consuming the error handle error handling C. improve the error handling logic and message in one the these repos based on status code (400 = "your request is bad," 500 = "your request might be good, but there's something wrong server-side," else = "good luck!")

  1. How can I test how this handles a 5** error? Or more specifically does this solve Jim's panoptes-javascript-client Issue 25?
mcbouslog commented 7 years ago

I cringe every time I open this and see my initial ramblings. Could we move more general planning and discussion to Issue #36?

eatyourgreens commented 7 years ago

Closing this because the JSON API client is passing back status codes etc. but it's the Panoptes client that's swallowing that info in its handleError method.

We might need to reopen this for 500 errors and the weird built-in 408 handling.

mcbouslog commented 7 years ago

currently for 4 and 5 errors, per make-http-request, the json-api-client is returning response when I think we want it to return error, which is the superagent javascript Error object.

eatyourgreens commented 7 years ago

If we return error, then we should add errors to it, from the response body, so that we capture any API error objects generated by Panoptes. PFE displays the messages from those objects.

I don't think there's any other info in the response that we want to preserve for the client, but if there is, that should also be attached to the object that's passed back. Maybe Object.assign(error, response) and capture everything?

mcbouslog commented 7 years ago

The response, including the body with all errors and related messages is included in the superagent Error object as error.response, so I think superagent is already doing the Object.assign(error, response) for us.