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

Expose the time at which the Rate Limit will reset #219

Merged
merged 5 commits into from
Jun 24, 2020
Merged

Conversation

lbalmaceda
Copy link
Contributor

Changes

When too many requests have been made against the server, the next request will fail with a 429. In the response headers there will be extra information, such as the time at which this limit is reset.

With this PR:

  • 429 errors are raised as RateLimitError instead of Auth0Error.
  • They will expose a property reset_at with the value of the X-RateLimit-Reset header. This value represents the Unix timestamp at which the limit will be reset.
  • When this header is not set, a value of -1 will be set as default.

References

More info https://auth0.com/docs/policies/rate-limits

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@lbalmaceda lbalmaceda added CH: Added small Small review labels Jun 23, 2020
@lbalmaceda lbalmaceda requested review from adamjmcgrath and a team June 23, 2020 22:37
@lbalmaceda lbalmaceda added this to the v3-Next milestone Jun 23, 2020
@@ -122,7 +127,7 @@ def _error_message(self):

class EmptyResponse(Response):
def __init__(self, status_code):
super(EmptyResponse, self).__init__(status_code, '')
super(EmptyResponse, self).__init__(status_code, '', {})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like a good default for a successful but empty response


def content(self):
if self._is_error():
if self._status_code == 429:
reset_at = int(self._headers.get('x-ratelimit-reset', '-1'))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default of -1 is being set here


def content(self):
if self._is_error():
if self._status_code == 429:
reset_at = int(self._headers.get('x-ratelimit-reset', '-1'))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers once parsed by the library are put into a CaseInsensitiveDict. Also, headers are by spec case insensitive -> https://requests.readthedocs.io/en/master/user/quickstart/#response-headers

README.rst Outdated Show resolved Hide resolved
@@ -152,7 +160,7 @@ def _error_message(self):

class EmptyResponse(Response):
def __init__(self, status_code):
super(EmptyResponse, self).__init__(status_code, '')
super(EmptyResponse, self).__init__(status_code, '', {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you not expect the API endpoints that return empty responses to get rate limited?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see - so if there's no response.text we know it's been successful so no need to check the rate limit header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responses sent from the server could be:

  • 204 NO CONTENT (empty body, empty text)
  • Plain text response: e.g. "User is blocked"
  • JSON response: e.g. { "code": "success" }

Every Response instance of this SDK defines a method to check if they represent errors, by checking against the status code here
https://github.com/auth0/auth0-python/blob/master/auth0/v3/management/rest.py#L111-L112.

Since for now, the headers are only being used for rate limit purposes, and only in the case of an errored response, I think it makes sense to pass an empty dict here. In addition, this headers property in the Response is not accessible for developers since the parsed content is what is being returned after a successful call, or an exception is raised if this represents an error.

https://github.com/auth0/auth0-python/blob/master/auth0/v3/management/rest.py#L84-L87

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replying to your question, 429 responses have been returning a non-empty body. I don't expect empty responses to represent a rate limit exception ever. 👍

Co-authored-by: Adam Mcgrath <adam.mcgrath@auth0.com>
@adamjmcgrath adamjmcgrath self-requested a review June 24, 2020 15:28
@lbalmaceda lbalmaceda merged commit 13a87e7 into master Jun 24, 2020
@lbalmaceda lbalmaceda deleted the rate-lim branch June 24, 2020 19:17
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.11.0 Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added small Small review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants