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 52472e9

Browse filesBrowse files
bitwisemanbitwiseman
committed
Simplified rate limit record selection
Here we have another example of trying to do something clever when simplicity is the better choice. Rather than trying to guess the rate limit record for a request based on the url path, I added an enumeration which can be set on the request to say which rate limit record to applies. This is simpler, safer, and faster than trying to guess the rate limit from the url path.
1 parent 4ef0d00 commit 52472e9
Copy full SHA for 52472e9

File tree

Expand file treeCollapse file tree

14 files changed

+3227
-55
lines changed
Filter options
Expand file treeCollapse file tree

14 files changed

+3227
-55
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
+24-18Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
*/
2727
@SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification = "JSON API")
2828
public class GHRateLimit {
29-
3029
/**
3130
* Remaining calls that can be made.
3231
*
@@ -75,32 +74,35 @@ static GHRateLimit Default() {
7574
}
7675

7776
@Nonnull
78-
static GHRateLimit Unknown(String urlPath) {
79-
return fromHeaderRecord(UnknownLimitRecord.current(), urlPath);
77+
static GHRateLimit Unknown(@Nonnull GitHubRateLimitSpecifier endpoint) {
78+
return fromHeaderRecord(UnknownLimitRecord.current(), endpoint);
8079
}
8180

8281
@Nonnull
83-
static GHRateLimit fromHeaderRecord(@Nonnull Record header, String urlPath) {
84-
if (urlPath.startsWith("/search")) {
82+
static GHRateLimit fromHeaderRecord(@Nonnull Record header, @Nonnull GitHubRateLimitSpecifier rateLimitSpecifier) {
83+
if (rateLimitSpecifier == GitHubRateLimitSpecifier.CORE
84+
|| rateLimitSpecifier == GitHubRateLimitSpecifier.NONE) {
85+
return new GHRateLimit(header,
86+
UnknownLimitRecord.DEFAULT,
87+
UnknownLimitRecord.DEFAULT,
88+
UnknownLimitRecord.DEFAULT);
89+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.SEARCH) {
8590
return new GHRateLimit(UnknownLimitRecord.DEFAULT,
8691
header,
8792
UnknownLimitRecord.DEFAULT,
8893
UnknownLimitRecord.DEFAULT);
89-
} else if (urlPath.startsWith("/graphql")) {
94+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.GRAPHQL) {
9095
return new GHRateLimit(UnknownLimitRecord.DEFAULT,
9196
UnknownLimitRecord.DEFAULT,
9297
header,
9398
UnknownLimitRecord.DEFAULT);
94-
} else if (urlPath.startsWith("/app-manifests")) {
99+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.INTEGRATION_MANIFEST) {
95100
return new GHRateLimit(UnknownLimitRecord.DEFAULT,
96101
UnknownLimitRecord.DEFAULT,
97102
UnknownLimitRecord.DEFAULT,
98103
header);
99104
} else {
100-
return new GHRateLimit(header,
101-
UnknownLimitRecord.DEFAULT,
102-
UnknownLimitRecord.DEFAULT,
103-
UnknownLimitRecord.DEFAULT);
105+
throw new IllegalArgumentException("Unknown rate limit specifier: " + rateLimitSpecifier.toString());
104106
}
105107
}
106108

@@ -272,20 +274,24 @@ GHRateLimit getMergedRateLimit(@Nonnull GHRateLimit newLimit) {
272274
/**
273275
* Gets the appropriate {@link Record} for a particular url path.
274276
*
275-
* @param urlPath
276-
* the url path of the request
277+
* @param endpoint
278+
* the rate limit endpoint specifier
277279
* @return the {@link Record} for a url path.
278280
*/
279281
@Nonnull
280-
Record getRecordForUrlPath(@Nonnull String urlPath) {
281-
if (urlPath.startsWith("/search")) {
282+
Record getRecordForUrlPath(@Nonnull GitHubRateLimitSpecifier endpoint) {
283+
if (endpoint == GitHubRateLimitSpecifier.CORE) {
284+
return getCore();
285+
} else if (endpoint == GitHubRateLimitSpecifier.SEARCH) {
282286
return getSearch();
283-
} else if (urlPath.startsWith("/graphql")) {
287+
} else if (endpoint == GitHubRateLimitSpecifier.GRAPHQL) {
284288
return getGraphQL();
285-
} else if (urlPath.startsWith("/app-manifests")) {
289+
} else if (endpoint == GitHubRateLimitSpecifier.INTEGRATION_MANIFEST) {
286290
return getIntegrationManifest();
291+
} else if (endpoint == GitHubRateLimitSpecifier.NONE) {
292+
return UnknownLimitRecord.DEFAULT;
287293
} else {
288-
return getCore();
294+
throw new IllegalArgumentException("Unknown rate limit specifier: " + endpoint.toString());
289295
}
290296
}
291297

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

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHSearchBuilder.java
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ public abstract class GHSearchBuilder<T> extends GHQueryBuilder<T> {
2525
super(root);
2626
this.receiverType = receiverType;
2727
req.withUrlPath(getApiUrl());
28+
req.rateLimit(GitHubRateLimitSpecifier.SEARCH);
2829
}
2930

3031
/**

‎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
@@ -416,7 +416,7 @@ public GHRateLimit lastRateLimit() {
416416
@Nonnull
417417
@Deprecated
418418
public GHRateLimit rateLimit() throws IOException {
419-
return client.rateLimit("");
419+
return client.rateLimit(GitHubRateLimitSpecifier.CORE);
420420
}
421421

422422
/**

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

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GitHubClient.java
+18-10Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ abstract class GitHubClient {
143143
}
144144

145145
private <T> T fetch(Class<T> type, String urlPath) throws IOException {
146+
// TODO: make this work for rate limit
146147
return this
147148
.sendRequest(GitHubRequest.newBuilder().withApiUrl(getApiUrl()).withUrlPath(urlPath).build(),
148149
(responseInfo) -> GitHubResponse.parseBody(responseInfo, type))
@@ -224,22 +225,29 @@ public boolean isAnonymous() {
224225
*/
225226
@Nonnull
226227
public GHRateLimit getRateLimit() throws IOException {
227-
return getRateLimit("/rate_limit");
228+
return getRateLimit(GitHubRateLimitSpecifier.NONE);
228229
}
229230

230231
@Nonnull
231-
GHRateLimit getRateLimit(@Nonnull String urlPath) throws IOException {
232+
GHRateLimit getRateLimit(@Nonnull GitHubRateLimitSpecifier rateLimitSpecifier) throws IOException {
232233
GHRateLimit result;
233234
try {
234-
result = fetch(JsonRateLimit.class, "/rate_limit").resources;
235+
GitHubRequest request = GitHubRequest.newBuilder()
236+
.rateLimit(GitHubRateLimitSpecifier.NONE)
237+
.withApiUrl(getApiUrl())
238+
.withUrlPath("/rate_limit")
239+
.build();
240+
result = this
241+
.sendRequest(request, (responseInfo) -> GitHubResponse.parseBody(responseInfo, JsonRateLimit.class))
242+
.body().resources;
235243
} catch (FileNotFoundException e) {
236244
// For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404.
237245
LOGGER.log(FINE, "/rate_limit returned 404 Not Found.");
238246

239247
// However some newer versions of GHE include rate limit header information
240248
// If the header info is missing and the endpoint returns 404, fill the rate limit
241249
// with unknown
242-
result = GHRateLimit.Unknown(urlPath);
250+
result = GHRateLimit.Unknown(rateLimitSpecifier);
243251
}
244252
return updateRateLimit(result);
245253
}
@@ -270,18 +278,18 @@ GHRateLimit lastRateLimit() {
270278
* {@link GHRateLimit.Record} for {@code urlPath} is expired, {@link #getRateLimit()} will be called to get the
271279
* current rate limit.
272280
*
273-
* @param urlPath
274-
* the path for the endpoint to get the rate limit for.
281+
* @param rateLimitSpecifier
282+
* the specifier for the endpoint to get the rate limit for.
275283
*
276284
* @return the current rate limit data. {@link GHRateLimit.Record}s in this instance may be expired when returned.
277285
* @throws IOException
278286
* if there was an error getting current rate limit data.
279287
*/
280288
@Nonnull
281-
GHRateLimit rateLimit(@Nonnull String urlPath) throws IOException {
289+
GHRateLimit rateLimit(@Nonnull GitHubRateLimitSpecifier rateLimitSpecifier) throws IOException {
282290
synchronized (rateLimitLock) {
283-
if (rateLimit.getRecordForUrlPath(urlPath).isExpired()) {
284-
getRateLimit(urlPath);
291+
if (rateLimit.getRecordForUrlPath(rateLimitSpecifier).isExpired()) {
292+
getRateLimit(rateLimitSpecifier);
285293
}
286294
return rateLimit;
287295
}
@@ -561,7 +569,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) {
561569
remaining = Integer.parseInt(remainingString);
562570
reset = Long.parseLong(resetString);
563571
GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo);
564-
updateRateLimit(GHRateLimit.fromHeaderRecord(observed, responseInfo.request().urlPath()));
572+
updateRateLimit(GHRateLimit.fromHeaderRecord(observed, responseInfo.request().rateLimitSpecifier()));
565573
} catch (NumberFormatException | NullPointerException e) {
566574
if (LOGGER.isLoggable(FINEST)) {
567575
LOGGER.log(FINEST, "Missing or malformed X-RateLimit header: ", e);

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

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java
+17-15Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,14 @@ class GitHubRateLimitChecker {
8888
* if there is an I/O error
8989
*/
9090
void checkRateLimit(GitHubClient client, GitHubRequest request) throws IOException {
91-
RateLimitChecker guard = selectChecker(request.urlPath());
91+
RateLimitChecker guard = selectChecker(request.rateLimitSpecifier());
9292
if (guard == RateLimitChecker.NONE) {
9393
return;
9494
}
9595

9696
// For the first rate limit, accept the current limit if a valid one is already present.
97-
GHRateLimit rateLimit = client.rateLimit(request.urlPath());
98-
GHRateLimit.Record rateLimitRecord = rateLimit.getRecordForUrlPath(request.urlPath());
97+
GHRateLimit rateLimit = client.rateLimit(request.rateLimitSpecifier());
98+
GHRateLimit.Record rateLimitRecord = rateLimit.getRecordForUrlPath(request.rateLimitSpecifier());
9999
long waitCount = 0;
100100
try {
101101
while (guard.checkRateLimit(rateLimitRecord, waitCount)) {
@@ -108,8 +108,8 @@ void checkRateLimit(GitHubClient client, GitHubRequest request) throws IOExcepti
108108
Thread.sleep(1000);
109109

110110
// After the first wait, always request a new rate limit from the server.
111-
rateLimit = client.getRateLimit(request.urlPath());
112-
rateLimitRecord = rateLimit.getRecordForUrlPath(request.urlPath());
111+
rateLimit = client.getRateLimit(request.rateLimitSpecifier());
112+
rateLimitRecord = rateLimit.getRecordForUrlPath(request.rateLimitSpecifier());
113113
}
114114
} catch (InterruptedException e) {
115115
throw (IOException) new InterruptedIOException(e.getMessage()).initCause(e);
@@ -118,24 +118,26 @@ void checkRateLimit(GitHubClient client, GitHubRequest request) throws IOExcepti
118118

119119
/**
120120
* Gets the appropriate {@link RateLimitChecker} for a particular url path. Similar to
121-
* {@link GHRateLimit#getRecordForUrlPath(String)}.
121+
* {@link GHRateLimit#getRecordForUrlPath(GitHubRateLimitSpecifier)}.
122122
*
123-
* @param urlPath
124-
* the url path of the request
125-
* @return the {@link RateLimitChecker} for a url path.
123+
* @param rateLimitSpecifier
124+
* the rate limit endpoint specifier
125+
* @return the {@link RateLimitChecker} for a particular specifier
126126
*/
127127
@Nonnull
128-
private RateLimitChecker selectChecker(@Nonnull String urlPath) {
129-
if (urlPath.equals("/rate_limit")) {
128+
private RateLimitChecker selectChecker(@Nonnull GitHubRateLimitSpecifier rateLimitSpecifier) {
129+
if (rateLimitSpecifier == GitHubRateLimitSpecifier.NONE) {
130130
return RateLimitChecker.NONE;
131-
} else if (urlPath.startsWith("/search")) {
131+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.CORE) {
132+
return core;
133+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.SEARCH) {
132134
return search;
133-
} else if (urlPath.startsWith("/graphql")) {
135+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.GRAPHQL) {
134136
return graphql;
135-
} else if (urlPath.startsWith("/app-manifests")) {
137+
} else if (rateLimitSpecifier == GitHubRateLimitSpecifier.INTEGRATION_MANIFEST) {
136138
return integrationManifest;
137139
} else {
138-
return core;
140+
throw new IllegalArgumentException("Unknown rate limit specifier: " + rateLimitSpecifier.toString());
139141
}
140142
}
141143
}

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

Copy file name to clipboard
+34Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package org.kohsuke.github;
2+
3+
/**
4+
* Specifies which rate limit record is used by a request
5+
*/
6+
enum GitHubRateLimitSpecifier {
7+
/**
8+
* Selects or updates the {@link GHRateLimit#getCore()} record.
9+
*/
10+
CORE,
11+
12+
/**
13+
* Selects or updates the {@link GHRateLimit#getSearch()} record.
14+
*/
15+
SEARCH,
16+
17+
/**
18+
* Selects or updates the {@link GHRateLimit#getGraphQL()} record.
19+
*/
20+
GRAPHQL,
21+
22+
/**
23+
* Selects or updates the {@link GHRateLimit#getIntegrationManifest()} record.
24+
*/
25+
INTEGRATION_MANIFEST,
26+
27+
/**
28+
* Selects no rate limit.
29+
*
30+
* This request uses no rate limit. If the response header includes rate limit information, it will apply to
31+
* {@link #CORE}.
32+
*/
33+
NONE
34+
}

0 commit comments

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