Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 9c80b7c

Browse filesBrowse files
bitwisemanbitwiseman
committed
Various clean up and fixes for GHRateLimit
1 parent 1ecad70 commit 9c80b7c
Copy full SHA for 9c80b7c

File tree

Expand file treeCollapse file tree

44 files changed

+63
-286
lines changed
Filter options

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Dismiss banner
Expand file treeCollapse file tree

44 files changed

+63
-286
lines changed

‎src/main/java/org/kohsuke/github/GHRateLimit.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHRateLimit.java
+12-14Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import com.fasterxml.jackson.annotation.JsonCreator;
44
import com.fasterxml.jackson.annotation.JsonProperty;
5-
import edu.umd.cs.findbugs.annotations.CheckForNull;
65
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
76
import org.apache.commons.lang3.StringUtils;
87

@@ -64,17 +63,16 @@ public class GHRateLimit {
6463
private final long resetEpochSeconds;
6564

6665
/**
67-
* EpochSeconds time (UTC) at which this response was updated.
68-
* Will be updated to match {@link this.updatedAt} if that is not null.
66+
* EpochSeconds time (UTC) at which this instance was created.
6967
*/
7068
private final long createdAtEpochSeconds = System.currentTimeMillis() / 1000;
7169

7270
/**
7371
* The calculated time at which the rate limit will reset.
74-
* Only calculated if {@link #getResetDate} is called.
72+
* Recalculated if {@link #recalculateResetDate} is called.
7573
*/
76-
@CheckForNull
77-
private Date resetDate = null;
74+
@Nonnull
75+
private Date resetDate;
7876

7977
/**
8078
* Gets a placeholder instance that can be used when we fail to get one from the server.
@@ -96,24 +94,26 @@ public GHRateLimit(@JsonProperty("limit") int limit,
9694
this(limit, remaining, resetEpochSeconds, null);
9795
}
9896

97+
@SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD",
98+
justification = "Deprecated")
9999
public GHRateLimit(int limit, int remaining, long resetEpochSeconds, String updatedAt) {
100100
this.limitCount = limit;
101101
this.remainingCount = remaining;
102102
this.resetEpochSeconds = resetEpochSeconds;
103-
setUpdatedAt(updatedAt);
103+
this.resetDate = recalculateResetDate(updatedAt);
104104

105105
// Deprecated fields
106106
this.remaining = remaining;
107107
this.limit = limit;
108108
this.reset = new Date(resetEpochSeconds);
109-
110109
}
111110

112111
/**
113112
*
114113
* @param updatedAt a string date in RFC 1123
114+
* @return reset date based on the passed date
115115
*/
116-
void setUpdatedAt(String updatedAt) {
116+
Date recalculateResetDate(String updatedAt) {
117117
long updatedAtEpochSeconds = createdAtEpochSeconds;
118118
if (!StringUtils.isBlank(updatedAt)) {
119119
try {
@@ -126,10 +126,10 @@ void setUpdatedAt(String updatedAt) {
126126
}
127127
}
128128

129-
long calculatedSecondsUntilReset = resetEpochSeconds - updatedAtEpochSeconds;
130-
131129
// This may seem odd but it results in an accurate or slightly pessimistic reset date
132-
resetDate = new Date((createdAtEpochSeconds + calculatedSecondsUntilReset) * 1000);
130+
// based on system time rather than on the system being in sync with the server
131+
long calculatedSecondsUntilReset = resetEpochSeconds - updatedAtEpochSeconds;
132+
return resetDate = new Date((createdAtEpochSeconds + calculatedSecondsUntilReset) * 1000);
133133
}
134134

135135
/**
@@ -173,8 +173,6 @@ public boolean isExpired() {
173173
*
174174
* @return the calculated date at which the rate limit has or will reset.
175175
*/
176-
@SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR",
177-
justification = "The value comes from JSON deserialization")
178176
@Nonnull
179177
public Date getResetDate() {
180178
return new Date(resetDate.getTime());

‎src/main/java/org/kohsuke/github/GitHub.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GitHub.java
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ public GHRateLimit getRateLimit() throws IOException {
334334
synchronized (headerRateLimitLock) {
335335
if (headerRateLimit == null
336336
|| headerRateLimit.getRemaining() > observed.getRemaining()
337-
|| headerRateLimit.getResetDate().getTime() < observed.getResetDate().getTime()) {
337+
|| headerRateLimit.getResetEpochSeconds() < observed.getResetEpochSeconds()) {
338338
headerRateLimit = observed;
339339
LOGGER.log(FINE, "Rate limit now: {0}", headerRateLimit);
340340
}

‎src/main/java/org/kohsuke/github/Requester.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/Requester.java
+10-2Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,15 @@
4444
import java.net.SocketTimeoutException;
4545
import java.net.URL;
4646
import java.net.URLEncoder;
47-
import java.util.*;
47+
import java.util.ArrayList;
48+
import java.util.Collection;
49+
import java.util.HashMap;
50+
import java.util.Iterator;
51+
import java.util.LinkedHashMap;
52+
import java.util.List;
53+
import java.util.Locale;
54+
import java.util.Map;
55+
import java.util.NoSuchElementException;
4856
import java.util.function.Consumer;
4957
import java.util.logging.Logger;
5058
import java.util.regex.Matcher;
@@ -702,7 +710,7 @@ private <T> T setResponseHeaders(T readValue) {
702710
setResponseHeaders((GHObject) readValue);
703711
} else if (readValue instanceof JsonRateLimit) {
704712
// if we're getting a GHRateLimit it needs the server date
705-
((JsonRateLimit)readValue).rate.setUpdatedAt(uc.getHeaderField("Date"));
713+
((JsonRateLimit)readValue).rate.recalculateResetDate(uc.getHeaderField("Date"));
706714
}
707715
return readValue;
708716
}

‎src/test/java/org/kohsuke/github/GitHubConnectionTest.java

Copy file name to clipboardExpand all lines: src/test/java/org/kohsuke/github/GitHubConnectionTest.java
-229Lines changed: 0 additions & 229 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@
66
import java.lang.reflect.Field;
77
import java.util.*;
88

9-
import static org.hamcrest.CoreMatchers.*;
10-
119
/**
1210
* Unit test for {@link GitHub}.
1311
*/
@@ -97,233 +95,6 @@ public void testGithubBuilderWithAppInstallationToken() throws Exception{
9795
assertEquals("",github.login);
9896
}
9997

100-
@Test
101-
public void testGitHubRateLimit() throws Exception {
102-
assertThat(mockGitHub.getRequestCount(), equalTo(0));
103-
GHRateLimit rateLimit = null;
104-
GitHub hub = null;
105-
Date lastReset = new Date(System.currentTimeMillis() / 1000L);
106-
int lastRemaining = 5000;
107-
108-
// Give this a moment
109-
Thread.sleep(1000);
110-
111-
// -------------------------------------------------------------
112-
// /user gets response with rate limit information
113-
hub = getGitHubBuilder()
114-
.withEndpoint(mockGitHub.apiServer().baseUrl()).build();
115-
hub.getMyself();
116-
117-
assertThat(mockGitHub.getRequestCount(), equalTo(1));
118-
119-
// Since we already had rate limit info these don't request again
120-
rateLimit = hub.lastRateLimit();
121-
assertThat(rateLimit, notNullValue());
122-
assertThat(rateLimit.limit, equalTo(5000));
123-
lastRemaining = rateLimit.remaining;
124-
// Because we're gettting this from old mocked info, it will be an older date
125-
//assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(-1));
126-
lastReset = rateLimit.getResetDate();
127-
128-
GHRateLimit headerRateLimit = rateLimit;
129-
130-
// Give this a moment
131-
Thread.sleep(1000);
132-
133-
// ratelimit() uses headerRateLimit if available
134-
assertThat(hub.rateLimit(), equalTo(headerRateLimit));
135-
136-
assertThat(mockGitHub.getRequestCount(), equalTo(1));
137-
138-
// Give this a moment
139-
Thread.sleep(1000);
140-
141-
// Always requests new info
142-
rateLimit = hub.getRateLimit();
143-
assertThat(mockGitHub.getRequestCount(), equalTo(2));
144-
145-
assertThat(rateLimit, notNullValue());
146-
assertThat(rateLimit.limit, equalTo(5000));
147-
// rate limit request is free
148-
assertThat(rateLimit.remaining, equalTo(lastRemaining));
149-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
150-
151-
// Give this a moment
152-
Thread.sleep(1000);
153-
154-
// Always requests new info
155-
rateLimit = hub.getRateLimit();
156-
assertThat(mockGitHub.getRequestCount(), equalTo(3));
157-
158-
assertThat(rateLimit, notNullValue());
159-
assertThat(rateLimit.limit, equalTo(5000));
160-
// rate limit request is free
161-
assertThat(rateLimit.remaining, equalTo(lastRemaining));
162-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
163-
164-
165-
hub.getOrganization(GITHUB_API_TEST_ORG);
166-
assertThat(mockGitHub.getRequestCount(), equalTo(4));
167-
168-
169-
assertThat(hub.lastRateLimit(), not(equalTo(headerRateLimit)));
170-
rateLimit = hub.lastRateLimit();
171-
assertThat(rateLimit, notNullValue());
172-
assertThat(rateLimit.limit, equalTo(5000));
173-
// Org costs limit to query
174-
assertThat(rateLimit.remaining, equalTo(lastRemaining - 1));
175-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
176-
lastReset = rateLimit.getResetDate();
177-
headerRateLimit = rateLimit;
178-
179-
// ratelimit() should prefer headerRateLimit when it is most recent
180-
assertThat(hub.rateLimit(), equalTo(headerRateLimit));
181-
182-
assertThat(mockGitHub.getRequestCount(), equalTo(4));
183-
184-
// Always requests new info
185-
rateLimit = hub.getRateLimit();
186-
assertThat(mockGitHub.getRequestCount(), equalTo(5));
187-
188-
assertThat(rateLimit, notNullValue());
189-
assertThat(rateLimit.limit, equalTo(5000));
190-
// Org costs limit to query
191-
assertThat(rateLimit.remaining, equalTo(lastRemaining - 1));
192-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
193-
194-
// ratelimit() should prefer headerRateLimit when getRateLimit() fails
195-
// BUG: When getRateLimit() succeeds, it should reset the ratelimit() to the new value
196-
// assertThat(hub.rateLimit(), equalTo(rateLimit));
197-
// assertThat(hub.rateLimit(), not(equalTo(headerRateLimit)));
198-
assertThat(hub.rateLimit(), equalTo(headerRateLimit));
199-
200-
assertThat(mockGitHub.getRequestCount(), equalTo(5));
201-
}
202-
203-
@Test
204-
public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
205-
// Customized response that results in file not found the same as GitHub Enterprise
206-
snapshotNotAllowed();
207-
assertThat(mockGitHub.getRequestCount(), equalTo(0));
208-
GHRateLimit rateLimit = null;
209-
GitHub hub = null;
210-
211-
212-
Date lastReset = new Date(System.currentTimeMillis() / 1000L);
213-
214-
// Give this a moment
215-
Thread.sleep(1000);
216-
217-
// -------------------------------------------------------------
218-
// Before any queries, rate limit starts as null but may be requested
219-
hub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus");
220-
assertThat(mockGitHub.getRequestCount(), equalTo(0));
221-
222-
assertThat(hub.lastRateLimit(), nullValue());
223-
224-
rateLimit = hub.rateLimit();
225-
assertThat(rateLimit, notNullValue());
226-
assertThat(rateLimit.limit, equalTo(1000000));
227-
assertThat(rateLimit.remaining, equalTo(1000000));
228-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
229-
lastReset = rateLimit.getResetDate();
230-
231-
assertThat(mockGitHub.getRequestCount(), equalTo(1));
232-
233-
// last is still null, because it actually means lastHeaderRateLimit
234-
assertThat(hub.lastRateLimit(), nullValue());
235-
236-
assertThat(mockGitHub.getRequestCount(), equalTo(1));
237-
238-
// Give this a moment
239-
Thread.sleep(1000);
240-
241-
// -------------------------------------------------------------
242-
// First call to /user gets response without rate limit information
243-
hub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus");
244-
hub.getMyself();
245-
assertThat(mockGitHub.getRequestCount(), equalTo(2));
246-
247-
assertThat(hub.lastRateLimit(), nullValue());
248-
249-
rateLimit = hub.rateLimit();
250-
assertThat(rateLimit, notNullValue());
251-
assertThat(rateLimit.limit, equalTo(1000000));
252-
assertThat(rateLimit.remaining, equalTo(1000000));
253-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
254-
lastReset = rateLimit.getResetDate();
255-
256-
assertThat(mockGitHub.getRequestCount(), equalTo(3));
257-
258-
// Give this a moment
259-
Thread.sleep(1000);
260-
261-
// Always requests new info
262-
rateLimit = hub.getRateLimit();
263-
assertThat(mockGitHub.getRequestCount(), equalTo(4));
264-
265-
assertThat(rateLimit, notNullValue());
266-
assertThat(rateLimit.limit, equalTo(1000000));
267-
assertThat(rateLimit.remaining, equalTo(1000000));
268-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
269-
270-
// Give this a moment
271-
Thread.sleep(1000);
272-
273-
274-
// last is still null, because it actually means lastHeaderRateLimit
275-
assertThat(hub.lastRateLimit(), nullValue());
276-
277-
// ratelimit() tries not to make additional requests, uses queried rate limit since header not available
278-
Thread.sleep(1000);
279-
assertThat(hub.rateLimit(), equalTo(rateLimit));
280-
281-
// -------------------------------------------------------------
282-
// Second call to /user gets response with rate limit information
283-
hub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus");
284-
hub.getMyself();
285-
assertThat(mockGitHub.getRequestCount(), equalTo(5));
286-
287-
// Since we already had rate limit info these don't request again
288-
rateLimit = hub.lastRateLimit();
289-
assertThat(rateLimit, notNullValue());
290-
assertThat(rateLimit.limit, equalTo(5000));
291-
assertThat(rateLimit.remaining, equalTo(4978));
292-
// Because we're gettting this from old mocked info, it will be an older date
293-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(-1));
294-
lastReset = rateLimit.getResetDate();
295-
296-
GHRateLimit headerRateLimit = rateLimit;
297-
298-
// Give this a moment
299-
Thread.sleep(1000);
300-
301-
// ratelimit() uses headerRateLimit if available
302-
assertThat(hub.rateLimit(), equalTo(headerRateLimit));
303-
304-
assertThat(mockGitHub.getRequestCount(), equalTo(5));
305-
306-
// Give this a moment
307-
Thread.sleep(1000);
308-
309-
// Always requests new info
310-
rateLimit = hub.getRateLimit();
311-
assertThat(mockGitHub.getRequestCount(), equalTo(6));
312-
313-
assertThat(rateLimit, notNullValue());
314-
assertThat(rateLimit.limit, equalTo(1000000));
315-
assertThat(rateLimit.remaining, equalTo(1000000));
316-
assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
317-
318-
// Give this a moment
319-
Thread.sleep(1000);
320-
321-
// ratelimit() should prefer headerRateLimit when getRateLimit fails
322-
assertThat(hub.rateLimit(), equalTo(headerRateLimit));
323-
324-
assertThat(mockGitHub.getRequestCount(), equalTo(6));
325-
}
326-
32798
@Test
32899
public void testGitHubIsApiUrlValid() throws IOException {
329100
GitHub hub = GitHub.connectAnonymously();

‎src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/orgs_github-api-test-org-3-ec2931.json

Copy file name to clipboardExpand all lines: src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/orgs_github-api-test-org-3-ec2931.json
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
"Status": "200 OK",
1616
"X-RateLimit-Limit": "5000",
1717
"X-RateLimit-Remaining": "4969",
18-
"X-RateLimit-Reset": "1569875630",
18+
"X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
1919
"Cache-Control": "private, max-age=60, s-maxage=60",
2020
"Vary": [
2121
"Accept, Authorization, Cookie, X-GitHub-OTP",

‎src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/rate_limit-2-36588a.json

Copy file name to clipboardExpand all lines: src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/rate_limit-2-36588a.json
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
"Status": "200 OK",
1616
"X-RateLimit-Limit": "5000",
1717
"X-RateLimit-Remaining": "4970",
18-
"X-RateLimit-Reset": "1569875630",
18+
"X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
1919
"Cache-Control": "no-cache",
2020
"X-OAuth-Scopes": "gist, notifications, read:org, read:public_key, read:repo_hook, repo",
2121
"X-Accepted-OAuth-Scopes": "",

‎src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-10-10-5432b2.json

Copy file name to clipboardExpand all lines: src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-10-10-5432b2.json
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
"Status": "200 OK",
1616
"X-RateLimit-Limit": "5000",
1717
"X-RateLimit-Remaining": "4966",
18-
"X-RateLimit-Reset": "1569875630",
18+
"X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
1919
"Cache-Control": "private, max-age=60, s-maxage=60",
2020
"Vary": [
2121
"Accept, Authorization, Cookie, X-GitHub-OTP",

‎src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-11-11-2e2a6e.json

Copy file name to clipboardExpand all lines: src/test/resources/org/kohsuke/github/extras/OkHttpConnectorTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-11-11-2e2a6e.json
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
"Status": "304 Not Modified",
1919
"X-RateLimit-Limit": "5000",
2020
"X-RateLimit-Remaining": "4966",
21-
"X-RateLimit-Reset": "1569875630",
21+
"X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
2222
"Cache-Control": "private, max-age=60, s-maxage=60",
2323
"Vary": [
2424
"Accept, Authorization, Cookie, X-GitHub-OTP",

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.