Accept 429 response codes as an indication of the secondary rate limi…

…t being exceeded (#1895)

* Accept 429 as an indication of the rate limit

Co-Authored-By: Liam Newman <bitwiseman@gmail.com>

* Remove assertion which is mostly asserting about the configuration of wiremock

* Add extra test to bring coverage above threshold, also handle case where retry-after is a date, and light refactoring

* Reformat header to resolve warning

* Update src/main/java/org/kohsuke/github/AbuseLimitHandler.java

* Update src/main/java/org/kohsuke/github/AbuseLimitHandler.java

* Apply suggestions from code review

---------

Co-authored-by: Liam Newman <bitwiseman@gmail.com>

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

@@ -5,6 +5,9 @@
 import java.io.IOException;
 import java.io.InterruptedIOException;
 import java.net.HttpURLConnection;
+import java.time.ZonedDateTime;
+import java.time.format.DateTimeFormatter;
+import java.time.temporal.ChronoUnit;
 
 import javax.annotation.Nonnull;
 
@@ -86,13 +89,6 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
             }
         }
 
-        private long parseWaitTime(HttpURLConnection uc) {
-            String v = uc.getHeaderField("Retry-After");
-            if (v == null)
-                return 60 * 1000; // can't tell, return 1 min
-
-            return Math.max(1000, Long.parseLong(v) * 1000);
-        }
     };
 
     /**
@@ -105,4 +101,25 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
             throw e;
         }
     };
+
+    /*
+     * Exposed for testability. Given an http response, find the retry-after header field and parse it as either a
+     * number or a date (the spec allows both). If no header is found, wait for a reasonably amount of time.
+     */
+    long parseWaitTime(HttpURLConnection uc) {
+        String v = uc.getHeaderField("Retry-After");
+        if (v == null) {
+            // can't tell, wait for unambiguously over one minute per GitHub guidance
+            return 61 * 1000;
+        }
+
+        try {
+            return Math.max(1000, Long.parseLong(v) * 1000);
+        } catch (NumberFormatException nfe) {
+            // The retry-after header could be a number in seconds, or an http-date
+            ZonedDateTime zdt = ZonedDateTime.parse(v, DateTimeFormatter.RFC_1123_DATE_TIME);
+            return ChronoUnit.MILLIS.between(ZonedDateTime.now(), zdt);
+        }
+    }
+
 }

src/main/java/org/kohsuke/github/GitHubAbuseLimitHandler.java

@@ -29,7 +29,19 @@ public abstract class GitHubAbuseLimitHandler extends GitHubConnectorResponseErr
      */
     @Override
     boolean isError(@Nonnull GitHubConnectorResponse connectorResponse) {
-        return isForbidden(connectorResponse) && hasRetryOrLimitHeader(connectorResponse);
+        return isTooManyRequests(connectorResponse)
+                || (isForbidden(connectorResponse) && hasRetryOrLimitHeader(connectorResponse));
+    }
+
+    /**
+     * Checks if the response status code is TOO_MANY_REQUESTS (429).
+     *
+     * @param connectorResponse
+     *            the response from the GitHub connector
+     * @return true if the status code is TOO_MANY_REQUESTS
+     */
+    private boolean isTooManyRequests(GitHubConnectorResponse connectorResponse) {
+        return connectorResponse.statusCode() == TOO_MANY_REQUESTS;
     }
 
     /**

src/main/java/org/kohsuke/github/GitHubConnectorResponseErrorHandler.java

@@ -23,6 +23,17 @@
  */
 abstract class GitHubConnectorResponseErrorHandler {
 
+    /**
+     * The HTTP 429 Too Many Requests response status code indicates the user has sent too many requests in a given
+     * amount of time ("rate limiting").
+     *
+     * A Retry-After header might be included to this response indicating how long to wait before making a new request.
+     *
+     * Why is this hardcoded here? The HttpURLConnection class is missing the status codes above 415, so the constant
+     * needs to be sourced from elsewhere.
+     */
+    public static final int TOO_MANY_REQUESTS = 429;
+
     /**
      * Called to detect an error handled by this handler.
      *

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java

@@ -16,6 +16,7 @@
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.core.IsInstanceOf.instanceOf;
 
 // TODO: Auto-generated Javadoc
@@ -98,22 +99,14 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
                         // getting an input stream in an error case should throw
                         IOException ioEx = Assert.assertThrows(IOException.class, () -> uc.getInputStream());
 
-                        try (InputStream errorStream = uc.getErrorStream()) {
-                            assertThat(errorStream, notNullValue());
-                            String errorString = IOUtils.toString(errorStream, StandardCharsets.UTF_8);
-                            assertThat(errorString, containsString("Must have push access to repository"));
-                        }
+                        checkErrorMessageMatches(uc, "Must have push access to repository");
 
                         // calling again should still error
                         ioEx = Assert.assertThrows(IOException.class, () -> uc.getInputStream());
 
                         // calling again on a GitHubConnectorResponse should yield the same value
                         if (uc.toString().contains("GitHubConnectorResponseHttpUrlConnectionAdapter")) {
-                            try (InputStream errorStream = uc.getErrorStream()) {
-                                assertThat(errorStream, notNullValue());
-                                String errorString = IOUtils.toString(errorStream, StandardCharsets.UTF_8);
-                                assertThat(errorString, containsString("Must have push access to repository"));
-                            }
+                            checkErrorMessageMatches(uc, "Must have push access to repository");
                         } else {
                             try (InputStream errorStream = uc.getErrorStream()) {
                                 assertThat(errorStream, notNullValue());
@@ -126,7 +119,7 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
                         }
 
                         assertThat(uc.getHeaderFields(), instanceOf(Map.class));
-                        assertThat(uc.getHeaderFields().size(), Matchers.greaterThan(25));
+                        assertThat(uc.getHeaderFields().size(), greaterThan(25));
                         assertThat(uc.getHeaderField("Status"), equalTo("403 Forbidden"));
 
                         String key = uc.getHeaderFieldKey(1);
@@ -349,16 +342,203 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
                         assertThat(uc.getContentType(), equalTo("application/json; charset=utf-8"));
                         assertThat(uc.getContentLength(), equalTo(-1));
                         assertThat(uc.getHeaderFields(), instanceOf(Map.class));
-                        assertThat(uc.getHeaderFields().size(), Matchers.greaterThan(25));
+                        assertThat(uc.getHeaderFields().size(), greaterThan(25));
                         assertThat(uc.getHeaderField("Status"), equalTo("403 Forbidden"));
 
-                        try (InputStream errorStream = uc.getErrorStream()) {
-                            assertThat(errorStream, notNullValue());
-                            String errorString = IOUtils.toString(errorStream, StandardCharsets.UTF_8);
-                            assertThat(errorString,
-                                    containsString(
-                                            "You have exceeded a secondary rate limit. Please wait a few minutes before you try again"));
-                        }
+                        checkErrorMessageMatches(uc,
+                                "You have exceeded a secondary rate limit. Please wait a few minutes before you try again");
+                        AbuseLimitHandler.FAIL.onError(e, uc);
+                    }
+                })
+                .build();
+
+        gitHub.getMyself();
+        assertThat(mockGitHub.getRequestCount(), equalTo(1));
+        try {
+            getTempRepository();
+            fail();
+        } catch (Exception e) {
+            assertThat(e, instanceOf(HttpException.class));
+            assertThat(e.getMessage(), equalTo("Abuse limit reached"));
+        }
+        assertThat(mockGitHub.getRequestCount(), equalTo(2));
+    }
+
+    /**
+     * This is making an assertion about the behaviour of the mock, so it's useful for making sure we're on the right
+     * mock, but should not be used to validate assumptions about the behaviour of the actual GitHub API.
+     */
+    private static void checkErrorMessageMatches(HttpURLConnection uc, String substring) throws IOException {
+        try (InputStream errorStream = uc.getErrorStream()) {
+            assertThat(errorStream, notNullValue());
+            String errorString = IOUtils.toString(errorStream, StandardCharsets.UTF_8);
+            assertThat(errorString, containsString(substring));
+        }
+    }
+
+    /**
+     * Tests the behavior of the GitHub API client when the abuse limit handler is set to WAIT then the handler waits
+     * appropriately when secondary rate limits are encountered.
+     *
+     * @throws Exception
+     *             if any error occurs during the test execution.
+     */
+    @Test
+    public void testHandler_Wait_Secondary_Limits_Too_Many_Requests() throws Exception {
+        // Customized response that templates the date to keep things working
+        snapshotNotAllowed();
+        final HttpURLConnection[] savedConnection = new HttpURLConnection[1];
+        gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
+                .withAbuseLimitHandler(new AbuseLimitHandler() {
+                    /**
+                     * Overriding method because the actual method will wait for one minute causing slowness in unit
+                     * tests
+                     */
+                    @Override
+                    public void onError(IOException e, HttpURLConnection uc) throws IOException {
+                        savedConnection[0] = uc;
+                        // Verify the test data is what we expected it to be for this test case
+                        assertThat(uc.getDate(), Matchers.greaterThanOrEqualTo(new Date().getTime() - 10000));
+                        assertThat(uc.getExpiration(), equalTo(0L));
+                        assertThat(uc.getIfModifiedSince(), equalTo(0L));
+                        assertThat(uc.getLastModified(), equalTo(1581014017000L));
+                        assertThat(uc.getRequestMethod(), equalTo("GET"));
+                        assertThat(uc.getResponseCode(), equalTo(429));
+                        assertThat(uc.getResponseMessage(), containsString("Many"));
+                        assertThat(uc.getURL().toString(),
+                                endsWith(
+                                        "/repos/hub4j-test-org/temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests"));
+                        assertThat(uc.getContentLength(), equalTo(-1));
+                        assertThat(uc.getHeaderFields(), instanceOf(Map.class));
+                        assertThat(uc.getHeaderField("Status"), equalTo("429 Too Many Requests"));
+                        assertThat(uc.getHeaderField("Retry-After"), equalTo("42"));
+
+                        checkErrorMessageMatches(uc,
+                                "You have exceeded a secondary rate limit. Please wait a few minutes before you try again");
+                        // Because we've overridden onError to bypass the wait, we don't cover the wait calculation
+                        // logic
+                        // Manually invoke it to make sure it's what we intended
+                        long waitTime = parseWaitTime(uc);
+                        assertThat(waitTime, equalTo(42 * 1000l));
+
+                        AbuseLimitHandler.FAIL.onError(e, uc);
+                    }
+                })
+                .build();
+
+        gitHub.getMyself();
+        assertThat(mockGitHub.getRequestCount(), equalTo(1));
+        try {
+            getTempRepository();
+            fail();
+        } catch (Exception e) {
+            assertThat(e, instanceOf(HttpException.class));
+            assertThat(e.getMessage(), equalTo("Abuse limit reached"));
+        }
+        assertThat(mockGitHub.getRequestCount(), equalTo(2));
+    }
+
+    /**
+     * Tests the behavior of the GitHub API client when the abuse limit handler with a date retry.
+     *
+     * @throws Exception
+     *             if any error occurs during the test execution.
+     */
+    @Test
+    public void testHandler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After() throws Exception {
+        // Customized response that templates the date to keep things working
+        snapshotNotAllowed();
+        final HttpURLConnection[] savedConnection = new HttpURLConnection[1];
+        gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
+                .withAbuseLimitHandler(new AbuseLimitHandler() {
+                    /**
+                     * Overriding method because the actual method will wait for one minute causing slowness in unit
+                     * tests
+                     */
+                    @Override
+                    public void onError(IOException e, HttpURLConnection uc) throws IOException {
+                        savedConnection[0] = uc;
+                        // Verify the test data is what we expected it to be for this test case
+                        assertThat(uc.getRequestMethod(), equalTo("GET"));
+                        assertThat(uc.getResponseCode(), equalTo(429));
+                        assertThat(uc.getResponseMessage(), containsString("Many"));
+                        assertThat(uc.getURL().toString(),
+                                endsWith(
+                                        "/repos/hub4j-test-org/temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After"));
+                        assertThat(uc.getContentLength(), equalTo(-1));
+                        assertThat(uc.getHeaderField("Status"), equalTo("429 Too Many Requests"));
+                        assertThat(uc.getHeaderField("Retry-After"), startsWith("Mon"));
+
+                        checkErrorMessageMatches(uc,
+                                "You have exceeded a secondary rate limit. Please wait a few minutes before you try again");
+
+                        // Because we've overridden onError to bypass the wait, we don't cover the wait calculation
+                        // logic
+                        // Manually invoke it to make sure it's what we intended
+                        long waitTime = parseWaitTime(uc);
+                        // The exact value here will depend on when the test is run, but it should be positive, and huge
+                        assertThat(waitTime, greaterThan(1000 * 1000l));
+
+                        AbuseLimitHandler.FAIL.onError(e, uc);
+                    }
+                })
+                .build();
+
+        gitHub.getMyself();
+        assertThat(mockGitHub.getRequestCount(), equalTo(1));
+        try {
+            getTempRepository();
+            fail();
+        } catch (Exception e) {
+            assertThat(e, instanceOf(HttpException.class));
+            assertThat(e.getMessage(), equalTo("Abuse limit reached"));
+        }
+        assertThat(mockGitHub.getRequestCount(), equalTo(2));
+    }
+
+    /**
+     * Tests the behavior of the GitHub API client when the abuse limit handler with a no retry after header.
+     *
+     * @throws Exception
+     *             if any error occurs during the test execution.
+     */
+    @Test
+    public void testHandler_Wait_Secondary_Limits_Too_Many_Requests_No_Retry_After() throws Exception {
+        // Customized response that templates the date to keep things working
+        snapshotNotAllowed();
+        final HttpURLConnection[] savedConnection = new HttpURLConnection[1];
+        gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
+                .withAbuseLimitHandler(new AbuseLimitHandler() {
+                    /**
+                     * Overriding method because the actual method will wait for one minute causing slowness in unit
+                     * tests
+                     */
+                    @Override
+                    public void onError(IOException e, HttpURLConnection uc) throws IOException {
+                        savedConnection[0] = uc;
+                        // Verify the test data is what we expected it to be for this test case
+                        assertThat(uc.getRequestMethod(), equalTo("GET"));
+                        assertThat(uc.getResponseCode(), equalTo(429));
+                        assertThat(uc.getResponseMessage(), containsString("Many"));
+                        assertThat(uc.getURL().toString(),
+                                endsWith(
+                                        "/repos/hub4j-test-org/temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests_No_Retry_After"));
+                        assertThat(uc.getContentEncoding(), nullValue());
+                        assertThat(uc.getContentType(), equalTo("application/json; charset=utf-8"));
+                        assertThat(uc.getContentLength(), equalTo(-1));
+                        assertThat(uc.getHeaderFields(), instanceOf(Map.class));
+                        assertThat(uc.getHeaderField("Status"), equalTo("429 Too Many Requests"));
+                        assertThat(uc.getHeaderField("Retry-After"), nullValue());
+
+                        checkErrorMessageMatches(uc,
+                                "You have exceeded a secondary rate limit. Please wait a few minutes before you try again");
+
+                        // Because we've overridden onError to bypass the wait, we don't cover the wait calculation
+                        // logic
+                        // Manually invoke it to make sure it's what we intended
+                        long waitTime = parseWaitTime(uc);
+                        assertThat(waitTime, greaterThan(60000l));
+
                         AbuseLimitHandler.FAIL.onError(e, uc);
                     }
                 })

src/test/resources/org/kohsuke/github/AbuseLimitHandlerTest/wiremock/testHandler_Wait_Secondary_Limits_Too_Many_Requests/__files/1-user.json

@@ -0,0 +1,45 @@
+{
+  "login": "bitwiseman",
+  "id": 1958953,
+  "node_id": "MDQ6VXNlcjE5NTg5NTM=",
+  "avatar_url": "https://avatars3.githubusercontent.com/u/1958953?v=4",
+  "gravatar_id": "",
+  "url": "https://api.github.com/users/bitwiseman",
+  "html_url": "https://github.com/bitwiseman",
+  "followers_url": "https://api.github.com/users/bitwiseman/followers",
+  "following_url": "https://api.github.com/users/bitwiseman/following{/other_user}",
+  "gists_url": "https://api.github.com/users/bitwiseman/gists{/gist_id}",
+  "starred_url": "https://api.github.com/users/bitwiseman/starred{/owner}{/repo}",
+  "subscriptions_url": "https://api.github.com/users/bitwiseman/subscriptions",
+  "organizations_url": "https://api.github.com/users/bitwiseman/orgs",
+  "repos_url": "https://api.github.com/users/bitwiseman/repos",
+  "events_url": "https://api.github.com/users/bitwiseman/events{/privacy}",
+  "received_events_url": "https://api.github.com/users/bitwiseman/received_events",
+  "type": "User",
+  "site_admin": false,
+  "name": "Liam Newman",
+  "company": "Cloudbees, Inc.",
+  "blog": "",
+  "location": "Seattle, WA, USA",
+  "email": "bitwiseman@gmail.com",
+  "hireable": null,
+  "bio": "https://twitter.com/bitwiseman",
+  "public_repos": 181,
+  "public_gists": 7,
+  "followers": 146,
+  "following": 9,
+  "created_at": "2012-07-11T20:38:33Z",
+  "updated_at": "2020-02-06T17:29:39Z",
+  "private_gists": 8,
+  "total_private_repos": 10,
+  "owned_private_repos": 0,
+  "disk_usage": 33697,
+  "collaborators": 0,
+  "two_factor_authentication": true,
+  "plan": {
+    "name": "free",
+    "space": 976562499,
+    "collaborators": 0,
+    "private_repos": 10000
+  }
+}