Allow to retrieve Rate Limit headers by rvillablanca · Pull Request #153 · auth0/auth0-java

rvillablanca · 2018-09-22T07:20:51Z

This PR resolve #99 to allow get the rate limit values from X-RateLimit-* headers.
I have made some decisions for default values, for example, when an http response code with value 429 is received then the information about rate limits should be recovered from headers X-RateLimit-*, but I don't know if these values are always sent so, if I couldn't get some header then a default -1 will be assigned.
The primitive data type chosen was long since the X-RateLimit-Reset is a Unix timestamp and the others values are normal numbers.

lbalmaceda

@rvillablanca Thanks for your PR! I've left a few comments. 🎉

lbalmaceda · 2018-09-25T13:11:04Z

src/main/java/com/auth0/net/CustomRequest.java

    }

    protected Auth0Exception createResponseException(Response response) {
+        if (response.code() == 429) {


Move the 429 to a private constant like STATUS_CODE_TOO_MANY_REQUEST

lbalmaceda · 2018-09-25T13:12:00Z

src/main/java/com/auth0/net/CustomRequest.java

+            long limit = parseRateLimitHeader("X-RateLimit-Limit", response);
+            long remaining = parseRateLimitHeader("X-RateLimit-Remaining", response);
+            long reset = parseRateLimitHeader("X-RateLimit-Reset", response);
+            return new RateLimitException(limit, remaining, reset, "Rate limits reached");


I'd make the parseRateLimitHeader method parse all the headers at the same time and then throw the exception. Maybe something like this:

if (code == 429){ throw buildRateLimitException(response); }

Also, seems like a rate limit exception would always have the same message/description. Let's remove that argument from the constructor and just keep the numbers.

I'm not so sure about this, but I think the message should be "Rate limit reached". (no "limits")

In this case I didn't want to couple the exception and the response so I modified the method to parse the headers and create the exception

I didn't want to couple the exception and the response

That was a good call 👍

lbalmaceda · 2018-09-25T13:14:36Z

src/main/java/com/auth0/exception/RateLimitException.java

+    private final long reset;
+
+    public RateLimitException(long limit, long remaining, long reset, String message) {
+        super(message, 429, null);


Message can be "hardcoded" here. No need for an arg.

lbalmaceda · 2018-09-25T13:17:30Z

src/test/java/com/auth0/client/MockServer.java

 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Paths;
+import java.util.Date;


unused import

lbalmaceda · 2018-09-25T13:19:46Z

src/test/java/com/auth0/client/MockServer.java

        server.enqueue(response);
    }
+
+    public void rateLimitResponse() throws IOException {


I'd call this rateLimitReachedResponse and pass the 3 values on the args. This way, you can test how defaults are being assigned if the header is missing (not just for 1 header)

lbalmaceda · 2018-09-25T13:22:24Z

src/test/java/com/auth0/net/CustomRequestTest.java

+            exception = e;
+        }
+        assertThat(exception, is(notNullValue()));
+        assertThat(exception, is(instanceOf(APIException.class)));


No need to test for APIException since RateLimitException is already a subclass.

lbalmaceda · 2018-09-25T13:24:30Z

src/main/java/com/auth0/exception/RateLimitException.java

+
+/**
+ * Represents a server error when a rate limit has been exceeded.
+ */


Please add a line linking to the docs. Such as

"To learn more about rate limits, visit https://auth0.com/docs/policies/rate-limits"

lbalmaceda · 2018-09-25T13:25:22Z

src/main/java/com/auth0/exception/RateLimitException.java

+        this.reset = reset;
+    }
+
+    public long getLimit() {


Javadoc for this 3 getters. You can use the docs to take some input. The idea is to explain in no more than a line what each of this numbers represent and most important, what value would be returned when the header is missing.

lbalmaceda

A few small comments. Please add 1 more test that asserts that when headers are not set, the returned value is -1

lbalmaceda · 2018-09-25T15:43:35Z

src/main/java/com/auth0/exception/RateLimitException.java

+
+    /**
+     * Getter for the maximum number of requests available in the current time frame.
+     * @return The maximun number of requests.


typo: should be "maximum"

Please change to "The maximum number of requests or -1 if missing."

lbalmaceda · 2018-09-25T15:43:55Z

src/main/java/com/auth0/exception/RateLimitException.java

+ * Getters for {@code limit, remaining} and {@code reset} corresponds to {@code X-RateLimit-Limit, X-RateLimit-Remaining} and {@code X-RateLimit-Reset} HTTP headers.
+ * If the value of any headers is missing, then a default value -1 will assigned.
+ * <p>
+ * "To learn more about rate limits, visit <a href="https://auth0.com/docs/policies/rate-limits">https://auth0.com/docs/policies/rate-limits</a>


Remove the " at the beginning

lbalmaceda · 2018-09-25T15:44:31Z

src/main/java/com/auth0/exception/RateLimitException.java

+
+    /**
+     * Getter for the number of remaining requests in the current time frame.
+     * @return Number of remaining requests.


Please change to "The number of remaining requests or -1 if missing."

lbalmaceda · 2018-09-25T15:44:54Z

src/main/java/com/auth0/exception/RateLimitException.java

+
+    /**
+     * Getter for the UNIX timestamp of the expected time when the rate limit will reset.
+     * @return The UNIX timestamp.


Please change to "The UNIX timestamp or -1 if missing."

lbalmaceda · 2018-09-25T15:45:26Z

src/test/java/com/auth0/client/MockServer.java

+    public void rateLimitReachedResponse(long limit, long remaining, long reset) throws IOException {
+        MockResponse response = new MockResponse()
+                .setResponseCode(429)
+                .addHeader("X-RateLimit-Limit", String.valueOf(limit))


You might want to add a check whether the arg is "-1" to avoid setting the header, so you can test that.

All suggestions have been done!

rvillablanca added 2 commits September 22, 2018 04:11

support to retrieve rate limits values from response headers

845a77a

more testing

ddae769

lbalmaceda suggested changes Sep 25, 2018

View reviewed changes

rvillablanca added 2 commits September 25, 2018 12:28

improvements according to changes requested

e98fbab

fix test

7be11c7

lbalmaceda suggested changes Sep 25, 2018

View reviewed changes

rvillablanca and others added 3 commits September 25, 2018 12:53

fix typo and javadoc

ef380a5

more test cases

6146660

rename tests

567b31e

lbalmaceda added this to the 1.9.0 milestone Sep 25, 2018

lbalmaceda added the CH: Added label Sep 25, 2018

lbalmaceda approved these changes Sep 25, 2018

View reviewed changes

Merge branch 'master' into rate-limits

6cc592a

cocojoe merged commit 66d189e into auth0:master Sep 25, 2018

lbalmaceda changed the title ~~support to retrieve rate limits values from response headers #99~~ Allow to retrieve Rate Limit headers Sep 25, 2018

rvillablanca deleted the rate-limits branch September 25, 2018 17:51

damieng mentioned this pull request Nov 12, 2019

Replace ApiData with RateLimitException auth0/auth0.net#312

Merged

Conversation

rvillablanca commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbalmaceda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbalmaceda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rvillablanca commented Sep 22, 2018 •

edited

Loading