Expose rate limiting information

[philomory]

Changes

This PR adds a new exception class, Auth0::RateLimitEncountered. In addition to the message and backtrace, this exception has attributes to hold the values of the Auth0 Rate Limiting Headers.

Thus, you can get access to this information when you encountered rate limiting, for example:

try_again = true
begin
  auth0.clients
rescue Auth0::RateLimitEncountered => e
  if try_again
    try_again = false
    warn("Rate limiting encountered, waiting until #{Time.at(e.reset)} to try again")
    sleep(e.reset - Time.now.to_i)
    retry
  else
    raise e
  end
end

References

This pull request address #158

A future improvement might incorporate (optional) automatic retry-after-wait into the gem itself, but for now exposing the required information to library users is enough.

Testing

This test adds unit tests, but not integration tests.

• This change adds unit test coverage
• This change adds integration test coverage
• This change has been tested on the latest version of Ruby

Checklist

• I have read the Auth0 general contribution guidelines
• I have read the Auth0 Code of Conduct
• All existing and new tests complete without errors
• Rubocop passes on all added/modified files
• All active GitHub checks have passed

[damieng]

Hi @philomory thanks for this. We have some internal discussions happening about how we want to handle and expose rate limiting across our various SDKs and want to make sure they are aligned. We'll let you know once we have a decision and if we need any tweaks to fit this PR in with that final design.

[joshcanhelp]

@philomory - While I definitely want to make sure we do this correctly (as in, not make any decisions on this now that we have to back out of later), do you think it would make sense to pass the response headers to all the exceptions and allow for that additional param in Auth0::Exception?

module Auth0
    class Exception < StandardError
        attr_reader :headers
        def initialize(message, headers)
            super(message)
            @headers = headers
        end
    end
end

[philomory]

That seems like a fairly sensible idea to me. I'm not sure entirely how much utility it has for most exception types, but it's certainly a more flexible implementation than one that is specific to the rate-limiting case, and thus probably more future-proof as well.

That said, if you are going to go that route, it may also be worth exposing the HTTP response code along with the headers; some of the exceptions pair 1-to-1 with a specific response code, but in the case of Auth0::Unsupported, there's currently no way to know what response code you received, since it's basically just "other". And if you're going to expose it on the Auth0::Unsupported exception, it probably makes sense to expose it on all of them for consistency.

The one caution in both of these cases is that it implies that all Auth0::Exception subclasses are going to always be about HTTP problems, and not, say, a failure of JWT verification (I know the library doesn't do JWT verification right now, but it might someday, I don't know). So if you add HTTP-related features to an exception base-class, it might be a good idea for it to be Auth0::HTTPError or Auth0::TransportError or something, which would then be the parent of all the current exceptions raised by Auth0::Mixins::HttpProxy, while itself being a subclass of Auth0::Exception.

At that point it begins to feel like you should just expose the RestClient::Response object, but I actually don't like that plan, since it would make RestClient part of the gem's API (and I'm not super keen on RestClient anymore, myself).

Let me know how you'd like to proceed, and whether you want me to tweak my PR, or whether you want to close this PR and implement the changes yourselves.

[joshcanhelp]

@philomory - Thank you for your thoughtful reply. As you mentioned, those exceptions are not always HTTP-related. Maybe that second parameter could be additional information or similar? In the case of JWT validation, say, that could be expected values that aren't in danger of being exposed.

I like the idea of including the HTTP code as well. Could be just a generic error_code, which could include error responses from our API. Feels like that makes sense as the second parameter and the headers/other as the third.

At that point it begins to feel like you should just expose the RestClient::Response object

We'll be replacing RestClient in a future version, likely a major. I do like the idea of an HttpError parent class, though.

Again, thank you for all this. We're working on a release for this week so if you're able to make these changes, I'd be happy to merge and get this out as part of that.

[philomory]

So, a basic sketch of what you've outlined above might look like this:

  class Exception < StandardError
    attr_reader :error_code, :additional_information
    def initialize(message,error_code=nil,additional_information={})
      super(message)
      @error_code, @additional_information = error_code, additional_information
    end
  end
  class HTTPError < Auth0::Exception; end
  class Unauthorized < Auth0::HTTPError; end
  ...

And get used like this:

          case result.code
          when 200...226 then safe_parse_json(result.body)
          when 400       then raise Auth0::BadRequest.new(result.body, result.code, headers: result.headers)
          when 401       then raise Auth0::Unauthorized.new(result.body, result.code, headers: result.headers)
          when 403       then raise Auth0::AccessDenied.new(result.body, result.code, headers: result.headers)
          when 404       then raise Auth0::NotFound.new(result.body, result.code, headers: result.headers)
          when 429       then raise Auth0::RateLimitEncountered.new(result.body, result.code, headers: result.headers)
          when 500       then raise Auth0::ServerError.new(result.body, result.code, headers: result.headers)
          else                raise Auth0::Unsupported.new(result.body, result.code, headers: result.headers)
          end

In this case the consumer of the exception could get access to the headers via something like:

begin
  auth0.some_call(args)
rescue Auth0::RateLimitEncountered => e
  reset = e.additional_information[:headers]['X-RateLimit-Reset']
  wait_until(reset)
  retry
end

You could add a convenience method to Auth0::HTTPError that returned the headers, e.g.

class HTTPError < Auth::Exception
  def headers
    additional_information[:headers]
  end
end

... so it could be called more cleanly, like e.headers['X-RateLimit-Reset'], for users who know what kind of exception they are working with.

The only awkward thing about this design is that, for non-HTTP errors that have additional information to provide, but nothing analogous to an HTTP response code, code raising that exception has to explicitly pass nil as the second argument; you could use keyword arguments to clean that up, but it does get a little cumbersome with long keyword names like additional_information, e.g.

raise Auth0::RateLimitEncountered.new(result.body, error_code: result.code, additional_information: { headers: result.headers })

Anyway, with the holiday in the US this week I'm not sure I'll have the time to put together an updated version of the PR this week, not if we also want corresponding unit tests and acceptance tests.

[joshcanhelp]

@philomory - Thanks again for your work here. I totally forgot about the short week so next week is totally fine. I'm not sure if we'll be able to get the rest of the changes in place before then on our end either so no rush. I'd rather wait and get this in.

Everything is looking great here. I think the long var name and missing code make a case for just using a single var for all that data we're getting back, like error_data:

class Exception < StandardError
  attr_reader :error_data
  def initialize(message,error_data={})
    super(message)
    @error_data = error_data
  end
end

# ...

case result.code
when 200...226 then safe_parse_json result.body
when 400 then Auth0::BadRequest.new result.body, { code: result.code, headers: result.headers }

# ...

class HTTPError < Auth::Exception
  def headers
    error_data[:headers]
  end

  def http_code
    error_data[:code]
  end
end

[philomory]

I've pushed a patch which adds the changes we discussed in terms of an error_data attribute on Auth0::Exception, headers and http_code convenience methods on Auth0::HTTPError, and a reset convenience method on Auth0::RateLimitEncountered that basically returns Time.at(headers['X-RateLimit-Reset']).utc.

I also tweaked the new unit tests I had already added, so that they would work with the new setup. I did not add any additional unit tests for the general changes to Auth0::Exception or Auth0::HTTPError, only for the new Auth0::RateLimitEncountered error - honestly, I wasn't sure where those sorts of tests would go or what they would look like. I also didn't add any integration tests.

[joshcanhelp]

@philomory - Thanks once again! This looks great at first glance. I'm going to take a closer look Monday and get back to you with feedback, if I have any. The tests you've already added look fine, I don't know that integration tests for internals like this would do any more good than the stubbed ones.

[joshcanhelp]

A couple of minor things here. Once again, thank you for this, I think it will be a solid improvement to the error handling experience in this lib.

[lbalmaceda]

@joshcanhelp @philomory is there anything I can do to unblock this one? Feels like a good, needed contribution to this SDK. Happy to resolve any doubts.

[joshcanhelp]

@philomory - Thanks for the update here! I'm out of office for a bit but I'll pull this down and run through it when I'm back to get it in the next release.

[philomory]

Apropos of very little, what does the CH: Added label mean?

[joshcanhelp]

@philomory - Indicates a new feature of some kind. We use that label to generate the changelog.

I'm pulling this down and testing today/tomorrow and will get this in a release this week 👍

[joshcanhelp]

One possible change and one question for you. I pulled this down locally and everything looks good!

[joshcanhelp]

Should we extend Auth0::Unsupported here instead of Auth0::HTTPError? Without that, we're not backwards-compatible (we're now throwing a different exception and, for apps that are rescuing Auth0::Unsupported in the case of a rate limit hit, that exception will bubble up).

[philomory]

Ah, hm, interesting point. From an API-compatibility point, yes; from a semantic standpoint, no?

Actually, I wonder if Auth0::Unsupported should just become an alias for Auth0::HTTPError. After all, what is Auth0::Unsupported if not "An HTTP error that this gem doesn't have any more specific classification for"?

E.g., instead of making Auth0::RateLimitEncountered inherit from Auth0::Unsupported, just have this line in there:

Unsupported = Auth0::HTTPError

[philomory]

Hm. Actually, maybe that's not great either, since it would broaden the scope of Auth0::Unsupported; rescue Auth0::Unsupported would now rescue error types that it didn't previously.

[joshcanhelp]

I'm more worried about breaking folks on a minor than semantics, at this point. We're due for a major on this SDK (a number of deprecated methods and addressing the HTTP client we're using) and could remove that extension then.

[philomory]

Ok, I've made Auth0::RateLimitEncountered a subclass of Auth0::Unsupported, with a comment explaining the reason and a TODO item to move it at a major version change.

[philomory]

The snyk CI check is now failing, although from what I can tell (I'm not very familiar with snyk) it seems to be unrelated to this PR - if I'm reading correctly, a vulnerability apparently exists in YARD, which this gem has as a development dependency. Specifically, a new vulnerability seems to have been published a few days ago.

[joshcanhelp]

@philomory - Deps fixed in master. Just rebase your base and push and we should be good to go here!

[philomory]

All checks now pass 😄

[joshcanhelp]

@philomory - Thanks once again for this great addition and sticking through all the changes 🙇