Skip to content

Commit

Permalink
Update AbuseLimitHandlerTest
Browse files Browse the repository at this point in the history
  • Loading branch information
@bitwiseman
bitwiseman committed Sep 17, 2024
1 parent 6609bd4 commit 0689914
Show file tree
Hide file tree
Showing 8 changed files with 160 additions and 115 deletions.
5 changes: 4 additions & 1 deletion src/main/java/org/kohsuke/github/AbuseLimitHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
}
};

// If "Retry-After" missing, wait for unambiguously over one minute per GitHub guidance
static long DEFAULT_WAIT_MILLIS = 61 * 1000;

/*
* 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.
Expand All @@ -116,7 +119,7 @@ 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;
return DEFAULT_WAIT_MILLIS;
}

try {
Expand Down
114 changes: 55 additions & 59 deletions src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import java.util.Map;

import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.core.IsInstanceOf.instanceOf;

Expand Down Expand Up @@ -71,8 +70,7 @@ public void testHandler_Fail() throws Exception {
snapshotNotAllowed();
final HttpURLConnection[] savedConnection = new HttpURLConnection[1];

gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
.withAbuseLimitHandler(new AbuseLimitHandler() {
gitHub = getGitHubWithAbuseLimitHandler(new TestAbuseLimitHandler() {
@Override
public void onError(IOException e, HttpURLConnection uc) throws IOException {
savedConnection[0] = uc;
Expand Down Expand Up @@ -224,8 +222,7 @@ public void testHandler_HttpStatus_Fail() throws Exception {
// Customized response that templates the date to keep things working
snapshotNotAllowed();

gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
.withAbuseLimitHandler(AbuseLimitHandler.FAIL)
gitHub = getGitHubWithAbuseLimitHandler(AbuseLimitHandler.FAIL)
.build();

gitHub.getMyself();
Expand Down Expand Up @@ -257,8 +254,7 @@ public void testHandler_Wait() throws Exception {
// Customized response that templates the date to keep things working
snapshotNotAllowed();

gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
.withAbuseLimitHandler(AbuseLimitHandler.WAIT)
gitHub = getGitHubWithAbuseLimitHandler(AbuseLimitHandler.WAIT)
.build();

gitHub.getMyself();
Expand All @@ -279,8 +275,7 @@ public void testHandler_WaitStuck() throws Exception {
// Customized response that templates the date to keep things working
snapshotNotAllowed();

gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
.withAbuseLimitHandler(new AbuseLimitHandler() {
gitHub = getGitHubWithAbuseLimitHandler(new TestAbuseLimitHandler() {
@Override
public void onError(IOException e, HttpURLConnection uc) throws IOException {
}
Expand Down Expand Up @@ -312,8 +307,7 @@ public void testHandler_Wait_Secondary_Limits() 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() {
gitHub = getGitHubWithAbuseLimitHandler(new TestAbuseLimitHandler() {
/**
* Overriding method because the actual method will wait for one minute causing slowness in unit
* tests
Expand Down Expand Up @@ -343,25 +337,26 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
assertThat(uc.getContentLength(), equalTo(-1));
assertThat(uc.getHeaderFields(), instanceOf(Map.class));
assertThat(uc.getHeaderFields().size(), greaterThan(25));

assertThat(AbuseLimitHandler.DEFAULT_WAIT_MILLIS, equalTo(61 * 1000l));
AbuseLimitHandler.DEFAULT_WAIT_MILLIS = 3210l;
long waitTime = parseWaitTime(uc);
assertThat(waitTime, equalTo(AbuseLimitHandler.DEFAULT_WAIT_MILLIS));

assertThat(uc.getHeaderField("Status"), equalTo("403 Forbidden"));

checkErrorMessageMatches(uc,
"You have exceeded a secondary rate limit. Please wait a few minutes before you try again");
AbuseLimitHandler.FAIL.onError(e, uc);
AbuseLimitHandler.WAIT.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));

getTempRepository();
assertThat(mockGitHub.getRequestCount(), equalTo(3));
}

/**
Expand All @@ -388,8 +383,7 @@ public void testHandler_Wait_Secondary_Limits_Too_Many_Requests() throws Excepti
// 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() {
gitHub = getGitHubWithAbuseLimitHandler(new TestAbuseLimitHandler() {
/**
* Overriding method because the actual method will wait for one minute causing slowness in unit
* tests
Expand All @@ -415,27 +409,20 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {

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));
assertThat(waitTime, equalTo(8 * 1000l));

AbuseLimitHandler.FAIL.onError(e, uc);
AbuseLimitHandler.WAIT.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));

getTempRepository();
assertThat(mockGitHub.getRequestCount(), equalTo(3));
}

/**
Expand All @@ -449,8 +436,7 @@ public void testHandler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After
// 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() {
gitHub = getGitHubWithAbuseLimitHandler(new TestAbuseLimitHandler() {
/**
* Overriding method because the actual method will wait for one minute causing slowness in unit
* tests
Expand All @@ -472,28 +458,23 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
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));
// The exact value here will depend on when the test is run
assertThat(waitTime, Matchers.lessThan(AbuseLimitHandler.DEFAULT_WAIT_MILLIS));
assertThat(waitTime, Matchers.greaterThan(3 * 1000l));

AbuseLimitHandler.FAIL.onError(e, uc);

AbuseLimitHandler.WAIT.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));

getTempRepository();
assertThat(mockGitHub.getRequestCount(), equalTo(3));
}

/**
Expand All @@ -507,8 +488,7 @@ public void testHandler_Wait_Secondary_Limits_Too_Many_Requests_No_Retry_After()
// 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() {
gitHub = getGitHubWithAbuseLimitHandler(new TestAbuseLimitHandler() {
/**
* Overriding method because the actual method will wait for one minute causing slowness in unit
* tests
Expand All @@ -533,13 +513,12 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
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);
AbuseLimitHandler.DEFAULT_WAIT_MILLIS = 3210l;
long waitTime = parseWaitTime(uc);
assertThat(waitTime, equalTo(AbuseLimitHandler.DEFAULT_WAIT_MILLIS));

AbuseLimitHandler.WAIT.onError(e, uc);
}
})
.build();
Expand All @@ -555,4 +534,21 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
}
assertThat(mockGitHub.getRequestCount(), equalTo(2));
}

private GitHubBuilder getGitHubWithAbuseLimitHandler(AbuseLimitHandler abuseLimitHandler) {
return getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
.withAbuseLimitHandler(abuseLimitHandler);
}

/**
* Test class wrapping the deprecated AbuseLimitHandler to make editing easier.
*/
public static abstract class TestAbuseLimitHandler extends AbuseLimitHandler {
/**
* Default Constructor
*/
public TestAbuseLimitHandler() {
}
}

}
13 changes: 5 additions & 8 deletions ...t/wiremock/testHandler_Wait_Secondary_Limits_Too_Many_Requests/mappings/2-r_h_t_fail.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,14 @@
}
},
"response": {
"status": 403,
"status": 429,
"body": "{\"message\":\"You have exceeded a secondary rate limit. Please wait a few minutes before you try again\"}",
"headers": {
"Date": "{{now timezone='GMT' format='EEE, dd MMM yyyy HH:mm:ss z'}}",
"Content-Type": "application/json; charset=utf-8",
"Server": "github.com",
"Status": "403 Forbidden",
"gh-limited-by": "search-elapsed-time-shared-grouped",
"X-RateLimit-Limit": "5000",
"X-RateLimit-Remaining": "4000",
"X-RateLimit-Reset": "{{testStartDate offset='3 seconds' format='unix'}}",
"Status": "429 Too Many Requests",
"Retry-After": "8",
"Cache-Control": "private, max-age=60, s-maxage=60",
"Vary": [
"Accept, Authorization, Cookie, X-GitHub-OTP",
Expand All @@ -45,8 +42,8 @@
},
"uuid": "79fb1092-8bf3-4274-bc8e-ca126c9d9261",
"persistent": true,
"scenarioName": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests",
"scenarioName": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits1",
"requiredScenarioState": "Started",
"newScenarioState": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests-2",
"newScenarioState": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits1-2",
"insertionIndex": 2
}
27 changes: 14 additions & 13 deletions ...t/wiremock/testHandler_Wait_Secondary_Limits_Too_Many_Requests/mappings/3-r_h_t_fail.json
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"id": "79fb1092-8bf3-4274-bc8e-ca126c9d9261",
"id": "574da117-6845-46d8-b2c1-4415546ca670",
"name": "repos_hub4j-test-org_temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests",
"request": {
"url": "/repos/hub4j-test-org/temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests",
Expand All @@ -11,21 +11,23 @@
}
},
"response": {
"status": 429,
"body": "{\"message\":\"You have exceeded a secondary rate limit. Please wait a few minutes before you try again\"}",
"status": 200,
"bodyFileName": "3-r_h_t_fail.json",
"headers": {
"Date": "{{now timezone='GMT' format='EEE, dd MMM yyyy HH:mm:ss z'}}",
"Content-Type": "application/json; charset=utf-8",
"Server": "github.com",
"Status": "429 Too Many Requests",
"Retry-After": "42",
"Status": "200 OK",
"X-RateLimit-Limit": "5000",
"X-RateLimit-Remaining": "4922",
"X-RateLimit-Reset": "{{testStartDate offset='3 seconds' format='unix'}}",
"Cache-Control": "private, max-age=60, s-maxage=60",
"Vary": [
"Accept, Authorization, Cookie, X-GitHub-OTP",
"Accept-Encoding"
],
"ETag": "W/\"7ff3c96399f7ddf6129622d675ca9935\"",
"Last-Modified": "Thu, 06 Feb 2020 18:33:37 GMT",
"ETag": "W/\"858224998ac7d1fd6dcd43f73d375297\"",
"Last-Modified": "Thu, 06 Feb 2020 18:33:43 GMT",
"X-OAuth-Scopes": "admin:org, admin:org_hook, admin:public_key, admin:repo_hook, delete_repo, gist, notifications, repo, user, write:discussion",
"X-Accepted-OAuth-Scopes": "repo",
"X-GitHub-Media-Type": "unknown, github.v3",
Expand All @@ -37,13 +39,12 @@
"X-XSS-Protection": "1; mode=block",
"Referrer-Policy": "origin-when-cross-origin, strict-origin-when-cross-origin",
"Content-Security-Policy": "default-src 'none'",
"X-GitHub-Request-Id": "CC37:2605:3F982:4E949:5E3C5BFC"
"X-GitHub-Request-Id": "CC37:2605:3FADC:4EA8C:5E3C5C02"
}
},
"uuid": "79fb1092-8bf3-4274-bc8e-ca126c9d9261",
"uuid": "574da117-6845-46d8-b2c1-4415546ca670",
"persistent": true,
"scenarioName": "scenario-4-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests",
"requiredScenarioState": "Started",
"newScenarioState": "scenario-4-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests-2",
"insertionIndex": 2
"scenarioName": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits1",
"requiredScenarioState": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits1-2",
"insertionIndex": 3
}
13 changes: 5 additions & 8 deletions ...ndler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After/mappings/2-r_h_t_fail.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,14 @@
}
},
"response": {
"status": 403,
"status": 429,
"body": "{\"message\":\"You have exceeded a secondary rate limit. Please wait a few minutes before you try again\"}",
"headers": {
"Date": "{{now timezone='GMT' format='EEE, dd MMM yyyy HH:mm:ss z'}}",
"Content-Type": "application/json; charset=utf-8",
"Server": "github.com",
"Status": "403 Forbidden",
"gh-limited-by": "search-elapsed-time-shared-grouped",
"X-RateLimit-Limit": "5000",
"X-RateLimit-Remaining": "4000",
"X-RateLimit-Reset": "{{testStartDate offset='3 seconds' format='unix'}}",
"Status": "429 Too Many Requests",
"Retry-After": "{{now offset='8 seconds' timezone='GMT' format='EEE, dd MMM yyyy HH:mm:ss z'}}",
"Cache-Control": "private, max-age=60, s-maxage=60",
"Vary": [
"Accept, Authorization, Cookie, X-GitHub-OTP",
Expand All @@ -45,8 +42,8 @@
},
"uuid": "79fb1092-8bf3-4274-bc8e-ca126c9d9261",
"persistent": true,
"scenarioName": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After",
"scenarioName": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits2",
"requiredScenarioState": "Started",
"newScenarioState": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits_Too_Many_Requests_Date_Retry_After-2",
"newScenarioState": "scenario-1-repos-hub4j-test-org-temp-testHandler_Wait_Secondary_Limits2-2",
"insertionIndex": 2
}
Loading

0 comments on commit 0689914

Please sign in to comment.