Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create exception hierarchy based on status code classes #4

Open
mfrister opened this issue Dec 10, 2015 · 2 comments
Open

Create exception hierarchy based on status code classes #4

mfrister opened this issue Dec 10, 2015 · 2 comments
Milestone

Comments

@mfrister
Copy link
Contributor

Currently, there is no way to catch all exceptions for a specific class of status codes.

A client might e.g. want to retry requests based on the status code class. It might e.g. make sense to retry a request returning a 5xx server error, but not to retry the request for a 4xx client error (that would fail again anyway with the same parameters).

Such a hierarchy would also allow us to better process status codes that don't have a specific exception for.

Suggestions for a hierarchy:

  • Base - allow catching all Grac-related exceptions
    • ClientException - already exists, keep for compatibility, currently inherits from StandardError
      • ClientError (4xx)
        • BadRequest (400)
        • Forbidden (403)
        • NotFound (404)
        • Conflict (409)
      • ServerError (5xx)
        • ServiceError - we already have this and don't want to break applications catching these (might have to add it above ServerError)
          • InternalServerError (500)
    • RequestFailed - already exists, currently inherits from StandardError
      • ServiceTimeout - already exists
    • InvalidContent - already exists, currently inherits from StandardError

We'll have to extend this hierarchy when adding more status codes (see #1). We don't explicitly deal with redirects yet, so nothing on 3xx codes here.

Still missing:

  • Handling unknown 2xx and 3xx status codes
mfrister added a commit that referenced this issue Jan 7, 2016
As required by HTTP:

> [..] However, a client MUST
> understand the class of any status code, as indicated by the first
> digit, and treat an unrecognized status code as being equivalent to
> the x00 status code of that class, with the exception that a
> recipient MUST NOT cache a response with an unrecognized status code.

See RFC 7231, section 6.

Error status handling will be improved with #4.
@mfrister
Copy link
Contributor Author

mfrister commented Jan 30, 2017

With #13 improving error handling in Grac 2, it might make sense to move this to the next major version, so we can properly refactor exceptions.

Here are a few further ideas:

No separate classes for individual error codes

In a distributed service environment, HTTP error codes are almost never enough. There are especially lot's of different cases on why a response could be a 'bad request', ranging from an invalid request format via invalid attribute values in the correct format, to a server resource being in a state that's incompatible with the request.

We pretty much always (except maybe for 404s) check an error code in the response (which gets easier with #13). Thus, we catch the Grac::Exception::ClientException that's valid for all 4xx error codes and just check the error code in the returned JSON.

Covering all known HTTP error codes would result in a lot of classes we probably don't need. Also, making this usable for people outside Barzahlen might require a way to support additional HTTP status codes via some kind of extension mechanism that would make exception handling even more complicated or annoying to use. That's probably not worth it.

So, instead of creating lots of individual exceptions for status codes, we could create one exception class per HTTP status code class, at least for the error classes 4xx and 5xx. We can then expose the status code via the exception, so people can use it, if they want.

Rename ClientException to ClientError

The name ClientException indicates some kind of problem with making the request on the client, although it represents a response from the server. The HTTP spec calls the error Client Error, so that's what we should use.

Prefix exceptions representing HTTP errors with HTTP?

While ClientError is already better than ClientException, it's still not fully unambiguous. Renaming these to HTTPClientError and HTTPServerError would make the layer of these levels more clear.

No Exception namespace?

I currently see no good reason for putting exceptions into their own namespace. It's some kind of grouping, but with Error or Exception in all the class names and no longer having per-status code exception classes, I don't see any issues with potential collisions or confusion about what a class represents.

Having the additional module just makes users write one additional word.

@tobischo Thoughts?

@mfrister mfrister added this to the v3 milestone Jan 30, 2017
@tobischo
Copy link
Member

I have no objections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants