Raise specific error classes for 4xx and 5xx errors

[omkarmoghe]

Context/problem

When using the http.rb gem in production code, I often find myself writing boilerplate code to wrap the StatusError thrown by the gem and raise a more specific error based on the response code.

begin
  HTTP.post(...)
rescue HTTP::StatusError => e
  case e.response.code
  when 401
    raise UnauthorizedError
  when 409
    raise ConflictError
  ...
  end
end

This is often in some client class or shared HTTP classes that can be used across the codebase. This works, but has a few issues:

1. The custom error classes need to also implement a response instance var to be useful. You end up re-implementing StatusError in the app, or you inherit directly from the gem class.
2. This code is repetitive

Proposal (this PR)

I would like to propose that the HTTP gem enumerate the roughly ~40 or so 4xx and 5xx status codes. This would provide a clean interface that allows callers to catch errors at varying levels of granularity, while being fully backwards compatible.

To achieve this, I'd like to propose 2 subclasses of StatusError:

1. ClientError, an entry point for all 4xx errors
2. ServerError, an entry point for all 5xx errors

Because all specific error classes are ultimately instances of StatusError, this should not break any existing rescue blocks. Users can opt-in to more granular errors as they see fit.

Example migration

Existing code

begin
  HTTP.post
rescue StatusError => e
  if e.response.code >= 500 ||
    retry
  elsif e.response.code == 429
    sleep(1) # throttle
    retry
  else
    fail
  end
end

After change

Existing code still works as-is

begin
  HTTP.post
rescue ServerError # all 5xx
  retry
rescue TooManyRequestsError # 429
  sleep(1) # throttle
  retry
rescue ClientError # all other 4xx
  fail
end

Would love any feedback or thoughts, thanks.

[tarcieri]

Why are the tests failing? I can't even tell

[sferik]

@tarcieri Tests are all passing but exit non-zero because code coverage dropped below 100%. Also, RuboCop also found linting errors, Steep type check failed, and Mutant failed.

Assuming those issues are fixed, I’m curious what you think about this proposal, in general?

[omkarmoghe]

Yeah, happy to fix the Rubocop linter issues if the maintainers like the overall idea. It's fairly trivial (method/case statement too long). Happy to add more test coverage for each branch too.

[tarcieri]

This seems fine to me as a general direction

[omkarmoghe]

@tarcieri @sferik Are there any docs or guidance on how I should go about regenerating the http.rbs file?

I naively ran rbs prototype rb lib/**/*.rb > sig/http.rbs but that produced a huge diff and the subsequent call to steep check produced even more errors than before.

[ixti]

I don't think we should explode amount of exceptions. To be quite frank I also think we should get rid of predicates in response status. Because response.status == 422 is much more readable than guessing is it #unprocessable_entity? (good luck typing it correct with first attempt if you have sloppy finger like I do) or is it #unprocessable_content?.

HTTP::Response support deconstuction, so I think we should enhance StatusError:

class StatusError
  # ...

  def deconstruct(...) = response.deconstruct(...)
  def deconstruct_keys(...) = response.deconstruct_keys(...)
end

This will allow us writing code:

begin
  HTTP.post
rescue StatusError => e
  case e
  in status: 500..599
    retry
  in status: 429
    sleep(1) # throttle
    retry
  else
    fail
  end
end

Even without changes to StatusError:

begin
  HTTP.post(...)
rescue StatusError => e
  case e.response
  in status: 500..599
    retry
  in status: 429
    sleep(1) # throttle
    retry
  else
    fail
  end
end

Or without pattern matching:

begin
  HTTP.post(...)
rescue StatusError => e
  case e.response.status
  when 500..599
    retry
  when 429
    sleep(1) # throttle
    retry
  else
    fail
  end
end