search

optoro / served

MIT License
0 stars 0 forks source link

Unable to handle HTTP error that are not defined (e.g. 504) #26

Open clupprich opened 7 years ago

clupprich commented 7 years ago

We're come across the situation where we're receiving an HTTP error that's not defined within served (e.g. a 504 Gateway Timeout error). Currently, served will blow up in Served::Resource::Requestable.handle_response, cause 504 doesn't have a registered handler.

I'd really like to submit a PR for this issue, but I'm unsure about the strategy. Adding all/most of the supported HTTP codes as subclasses of HttpError doesn't really seem feasible?

Thoughts on this @fugufish @nevern02?

bherrup commented 7 years ago

I think that the solution here is there should be a fallback handler for all othr statuses that will just properly propagate the fact that the server returned somthing. In all reality there should not be a reason that served cannot handle ANY response code 2xx-5xx. if we are truly going to be using this for all service communication, using RESTful endpoints, we should be able to leverage all codes for their appropriate usage...see HTTP Status Code 418.

clupprich commented 7 years ago

I totally agree, but with the current HttpError base class, we would need to create separate classes for every HTTP status code. I wanna get clearance if that's what we want before I invest time on doing so.

openmailbox commented 7 years ago

I think Brad's suggestion is more that the HttpError base class should have a default handler so in the absence of a specialty subclass it can handle anything.

Given that the subclasses also don't really have any behavior and are just basically blobs of data with names and codes, I'd actually be on board with just dynamically instantiating a meaningful object of some kind while reading the server error. And then getting rid of all the empty class definitions.

fugufish commented 7 years ago

The empty class definitions are present for error specific rescue base error handling. The intent being that you can handle Timeout error differently than a 404 not found in a rescue statement rather than a case statement checking error codes.

clupprich commented 7 years ago

Maybe this approach could limit the # of error classes we need? https://github.com/rails/activeresource/blob/505825bd1ae5c4100f1571d0842a9a99f471f87a/lib/active_resource/base.rb#L191-L206

fugufish commented 7 years ago

I like that and the handler code actually should support that without much effort as it can support ranges.