return error object with status and response #37

mcbouslog · 2017-03-10T22:25:11Z

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

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!")

How can I test how this handles a 5** error? Or more specifically does this solve Jim's panoptes-javascript-client Issue 25?

mcbouslog · 2017-03-10T23:28:54Z

src/json-api-client.coffee

@@ -28,6 +28,7 @@ class JSONAPIClient extends Model
    mergeInto this, mixins

  beforeEveryRequest: ->
+    console.log 'using local json-api-client'


will remove, keeping momentarily while try to stage branch of PFE with dependency as GitHub branch that itself has a dependency as GitHub branch.

mcbouslog · 2017-03-13T20:01:48Z

src/json-api-client.coffee

@@ -28,6 +28,7 @@ class JSONAPIClient extends Model
    mergeInto this, mixins

  beforeEveryRequest: ->
+    console.log 'using GitHub branch of json-api-client'


will remove, keeping while linking dependencies via GitHub branches

srallen

As a side note, I totally forgot that we already switch to using superagent as a dep about a year ago and that it's just superagent-cache that is the open PR, whoops! Be aware that the version that json-api-client is using is very old when looking at the superagent docs.

The changes on the json-api-client itself looks ok to me, though we may want to revisit this and at least bump the version to 1.8.2 and rework it to use the native Promise support superagent had since 1.3.0.

I'm of the opinion that panoptes-javascript-client should be doing very minimal handling and should usually just pass the error object along. This way we have access to the full error object, we can resolve zooniverse/panoptes-javascript-client#15 with smarter handling, ignore 404s for not found avatars, but redirect users to a 404 page if it's a not found project, etc. Throwing strings into the console is very noisy and not very helpful.

eatyourgreens · 2017-03-14T10:35:46Z

src/json-api-client.coffee

    # Override this as necessary.
-    Promise.reject arguments...
+    Promise.reject err


Previously this rejected with the arguments passed to the handler, so I'm not sure what this change does?

yep, previous way preferred, I'll revert.
I did unnecessarily while attempting to understand flow of responses/rejections.

eatyourgreens · 2017-03-14T10:36:42Z

src/make-http-request.coffee

@@ -43,9 +43,7 @@ makeHTTPRequest = (method, url, data, headers = {}, modify) ->
      if error?.status is 408
        resolve makeHTTPRequest.apply null, originalArguments


Why does the promise resolve if the error status is 408?

408 is Request Timeout so retry immediately?

That's how I'm reading it.

Also how I interpreted. Not sure if we want to move this out of here now that we'll be passing along error with status in case of 408s, so relevant handler (in panoptes-client or something in PFE or other app) can decide what it wants to do?

My opinion is that the JSON API client shouldn't know anything about how to handle a response. I think its responsibility should be to make requests to a JSON API and pass the response back to a parent app. The parent app can make decisions based on the response.

So, I think retrying on timeouts is perhaps better handled by the Panoptes API client, or by PFE.

I agree that JSON API client should be low level and not make decisions on how to handle responses. My concern with moving it to the Panoptes JS client is if we move this handler to the Panoptes JS client and if we decide to merge the JSON API client and Panoptes JS client, then that separation of concerns wouldn't be present anymore. With that in mind, moving this handler to PFE or moving it to the Panoptes JS client and keeping the clients separate would be best. If moved to PFE, I also don't want us spending a lot of time adding this kind of handling to most requests since I think it might be likely we would want this behavior most of the time. What do you think the best approach here is?

My first impression is that the Panoptes client is the best place to make decisions like this. I'm not 100% convinced that the client should repeatedly hammer the API after timeouts, though.

👍 for the Panoptes client.

I like the behavior that node-request-retry has: https://github.com/FGRibreau/node-request-retry
I added it to the Panoptes subject uploader some time ago and use its default behavior. It'll attempt only up to 5 times with 5 sec intervals depending on the HTTP error.

That does sound a lot better. Retrying failed classifications is currently handled in PFE, but I can see how this would be generally useful in the Panoptes client instead.

mcbouslog · 2017-03-14T20:05:37Z

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

eatyourgreens · 2017-03-21T15:21:16Z

src/make-http-request.coffee

-        # Prefer rejecting with the response, since it'll have more specific information.
-        # TODO: Reject with the error as expected and access the response through `error.response` in the handler.
-        reject response ? error
+        reject error


I've been looking at the error responses returned by Panoptes. They only seem to contain an error message, so we might have to add in error.status and error.statusText here, unless that's already been done elsewhere in the client.

this highlights another good question. currently the panoptes-javascript-client (p-j-c) just returns an error message, but with this PR and the related p-j-c PR it'll return an error object that includes status and response attributes. the response attribute includes the verbose error message currently returned.

recently I think there was an error related to If-Then headers and the API I didn't quite understand, but the point was made that maybe our error messages should be less verbose or more generic?

if this PR and the related p-j-c PR are merged as is, the error message the PFE will throw to the console will be a less verbose version, but the more verbose explanation will be available if we want to access it.

the example I've been playing with is requesting a project by ID that doesn't exist. here's the error.response attribute:

currently the "Could not find project with id-'1693777'" message is returned, but with these changes just "404: Not Found" would be console logged, though the more verbose one would be available as shown above.

If we want to go the route of returning the verbose error message just as we have been, I can clean this p-j-c PR up that I closed?

At the moment, this version of the client always errors with 'Unknown error (bad response) in the Panoptes client, because error is not an instance of Error ie. not a JavaScript error.

I think it got lost in the noise on Slack, but I've set up a copy of PFE using these branches of the client libs on https://api-client-errors.pfe-preview.zooniverse.org/

Turns out there's a build step I'd missed when deploying the coffeescript changes. Should be sorted out now.

eatyourgreens · 2017-03-22T11:59:55Z

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 · 2017-03-22T14:40:20Z

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 · 2017-03-22T15:28:50Z

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 · 2017-03-22T16:03:35Z

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.

return error object with status and response

0e29529

mcbouslog mentioned this pull request Mar 10, 2017

[WIP] edit error handling for object with status and response zooniverse/panoptes-javascript-client#61

Closed

mcbouslog commented Mar 10, 2017

View reviewed changes

edit console log

ff7e241

mcbouslog requested review from eatyourgreens and srallen March 13, 2017 19:47

mcbouslog commented Mar 13, 2017

View reviewed changes

srallen reviewed Mar 13, 2017

View reviewed changes

eatyourgreens reviewed Mar 14, 2017

View reviewed changes

clean-up logs, revert unnecessary changes

dec4bc2

mcbouslog force-pushed the refactor-error-handling branch from 3264ae3 to dec4bc2 Compare March 14, 2017 19:22

mcbouslog mentioned this pull request Mar 14, 2017

improve API error handling #36

Closed

mcbouslog changed the title ~~[WIP] return error object with status and response~~ return error object with status and response Mar 14, 2017

mcbouslog mentioned this pull request Mar 14, 2017

simplify handleError with new error object zooniverse/panoptes-javascript-client#62

Closed

srallen approved these changes Mar 20, 2017

View reviewed changes

eatyourgreens reviewed Mar 21, 2017

View reviewed changes

eatyourgreens closed this Mar 22, 2017

eatyourgreens deleted the refactor-error-handling branch March 16, 2020 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return error object with status and response #37

return error object with status and response #37

mcbouslog commented Mar 10, 2017 •

edited

Loading

mcbouslog Mar 10, 2017

mcbouslog Mar 13, 2017

srallen left a comment

eatyourgreens Mar 14, 2017

mcbouslog Mar 14, 2017

eatyourgreens Mar 14, 2017

eatyourgreens Mar 14, 2017

srallen Mar 14, 2017

mcbouslog Mar 14, 2017

eatyourgreens Mar 21, 2017

srallen Mar 21, 2017

eatyourgreens Mar 21, 2017

srallen Mar 21, 2017

eatyourgreens Mar 21, 2017

mcbouslog commented Mar 14, 2017

eatyourgreens Mar 21, 2017

mcbouslog Mar 21, 2017

eatyourgreens Mar 21, 2017

eatyourgreens Mar 21, 2017

eatyourgreens commented Mar 22, 2017 •

edited

Loading

mcbouslog commented Mar 22, 2017

eatyourgreens commented Mar 22, 2017

mcbouslog commented Mar 22, 2017

		@@ -43,9 +43,7 @@ makeHTTPRequest = (method, url, data, headers = {}, modify) ->
		if error?.status is 408
		resolve makeHTTPRequest.apply null, originalArguments

return error object with status and response #37

return error object with status and response #37

Conversation

mcbouslog commented Mar 10, 2017 • edited Loading

Please confirm my understanding:

Changes

Initial Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcbouslog commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Mar 22, 2017 • edited Loading

mcbouslog commented Mar 22, 2017

eatyourgreens commented Mar 22, 2017

mcbouslog commented Mar 22, 2017

mcbouslog commented Mar 10, 2017 •

edited

Loading

eatyourgreens commented Mar 22, 2017 •

edited

Loading