diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubClientTest.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubClientTest.java index 1c715b5..55b8672 100644 --- a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubClientTest.java +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubClientTest.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.List; +import org.eclipse.egit.pullrequest.internal.github.GitHubSearchQueries.Query; import org.junit.Test; /** @@ -42,32 +43,37 @@ public void testTokenOnlyClientConstruction() { @Test public void testAccessibleSearchCoversOwnedOrgsAndForeignRepos() { - List queries = GitHubSearchQueries + List queries = GitHubSearchQueries .accessiblePullRequestQueries("alice", //$NON-NLS-1$ Arrays.asList("acme", "friends"), //$NON-NLS-1$ //$NON-NLS-2$ Arrays.asList("alice/notes", "acme/app", //$NON-NLS-1$ //$NON-NLS-2$ "bob/tool"), //$NON-NLS-1$ null, null); - assertThat(queries, contains("is:pr user:alice is:open", //$NON-NLS-1$ - "is:pr org:acme is:open", //$NON-NLS-1$ - "is:pr org:friends is:open", //$NON-NLS-1$ - "is:pr repo:bob/tool is:open")); //$NON-NLS-1$ + assertThat(queries, hasSize(1)); + assertThat(queries.get(0).getText(), + equalTo("is:pr user:alice org:acme org:friends " //$NON-NLS-1$ + + "repo:bob/tool is:open")); //$NON-NLS-1$ + assertThat(queries.get(0).getScopes(), + contains("user:alice", "org:acme", "org:friends", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + "repo:bob/tool")); //$NON-NLS-1$ } @Test public void testAccessibleSearchAppliesAuthorFilterOnlyWhenSet() { - List allAuthors = GitHubSearchQueries + List allAuthors = GitHubSearchQueries .accessiblePullRequestQueries("alice", null, null, //$NON-NLS-1$ "OPEN", null); //$NON-NLS-1$ - List oneAuthor = GitHubSearchQueries + List oneAuthor = GitHubSearchQueries .accessiblePullRequestQueries("alice", null, null, //$NON-NLS-1$ "OPEN", "carol"); //$NON-NLS-1$ //$NON-NLS-2$ - assertThat(allAuthors, contains("is:pr user:alice is:open")); //$NON-NLS-1$ - assertThat(allAuthors.get(0), not(containsString("author:"))); //$NON-NLS-1$ - assertThat(oneAuthor, - contains("is:pr user:alice author:carol is:open")); //$NON-NLS-1$ + assertThat(allAuthors.get(0).getText(), + equalTo("is:pr user:alice is:open")); //$NON-NLS-1$ + assertThat(allAuthors.get(0).getText(), + not(containsString("author:"))); //$NON-NLS-1$ + assertThat(oneAuthor.get(0).getText(), + equalTo("is:pr user:alice author:carol is:open")); //$NON-NLS-1$ } @Test @@ -86,16 +92,37 @@ public void testAccessibleSearchMapsMergedAndAllStates() { @Test public void testParsePullRequestPathsFromSearch() { String json = "{\"total_count\":2,\"items\":[" //$NON-NLS-1$ - + "{\"pull_request\":{\"url\":\"https://api.github.com/repos/alice/one/pulls/7\"}}," //$NON-NLS-1$ - + "{\"pull_request\":{\"url\":\"https://api.github.com/repos/acme/two/pulls/12\"}}" //$NON-NLS-1$ + + "{\"updated_at\":\"2026-09-02T10:00:00Z\",\"pull_request\":" //$NON-NLS-1$ + + "{\"url\":\"https://api.github.com/repos/alice/one/pulls/7\"}}," //$NON-NLS-1$ + + "{\"updated_at\":\"2026-09-01T10:00:00Z\",\"pull_request\":" //$NON-NLS-1$ + + "{\"url\":\"https://api.github.com/repos/acme/two/pulls/12\"}}" //$NON-NLS-1$ + "]}"; //$NON-NLS-1$ - List paths = GitHubJsonParser - .parseSearchPullRequestPaths(json); + List hits = GitHubJsonParser.parseSearchHits(json); - assertThat(paths, hasSize(2)); - assertThat(paths.get(0), equalTo("/repos/alice/one/pulls/7")); //$NON-NLS-1$ - assertThat(paths.get(1), equalTo("/repos/acme/two/pulls/12")); //$NON-NLS-1$ + assertThat(hits, hasSize(2)); + assertThat(hits.get(0).path(), + equalTo("/repos/alice/one/pulls/7")); //$NON-NLS-1$ + assertThat(hits.get(1).path(), + equalTo("/repos/acme/two/pulls/12")); //$NON-NLS-1$ + assertThat(hits.get(0).updated(), notNullValue()); + assertThat(hits.get(0).updated() + .after(hits.get(1).updated()), equalTo(true)); + } + + @Test + public void testSearchResultsThatAreNotPullRequestsAreIgnored() { + String json = "{\"total_count\":2,\"items\":[" //$NON-NLS-1$ + + "{\"number\":3,\"title\":\"a plain issue\"}," //$NON-NLS-1$ + + "{\"updated_at\":\"2026-09-01T10:00:00Z\",\"pull_request\":" //$NON-NLS-1$ + + "{\"url\":\"https://api.github.com/repos/acme/two/pulls/12\"}}" //$NON-NLS-1$ + + "]}"; //$NON-NLS-1$ + + List hits = GitHubJsonParser.parseSearchHits(json); + + assertThat(hits, hasSize(1)); + assertThat(hits.get(0).path(), + equalTo("/repos/acme/two/pulls/12")); //$NON-NLS-1$ } @Test diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java new file mode 100644 index 0000000..34c8f91 --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java @@ -0,0 +1,372 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; + +import java.io.IOException; +import java.time.Instant; +import java.time.format.DateTimeFormatter; +import java.util.ArrayList; +import java.util.List; + +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics.Outcome; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics.Step; +import org.eclipse.egit.pullrequest.internal.github.StubGitHubServer.Response; +import org.eclipse.egit.pullrequest.internal.model.PullRequest; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Tests how {@link GitHubClient} lists pull requests across the scopes it is + * allowed to search, and how few requests that takes. + */ +public class GitHubConnectionTest { + + private static final String VALIDATION_FAILED = "{\"message\":" //$NON-NLS-1$ + + "\"Validation Failed\",\"errors\":[{\"message\":\"The listed " //$NON-NLS-1$ + + "users and repositories cannot be searched either because " //$NON-NLS-1$ + + "the resources do not exist or you do not have permission to " //$NON-NLS-1$ + + "view them.\",\"resource\":\"Search\",\"field\":\"q\"," //$NON-NLS-1$ + + "\"code\":\"invalid\"}],\"status\":\"422\"}"; //$NON-NLS-1$ + + private static final String RATE_LIMITED = "{\"message\":\"API rate " //$NON-NLS-1$ + + "limit exceeded for user ID 201185819.\",\"status\":\"403\"}"; //$NON-NLS-1$ + + private StubGitHubServer server; + + @Before + public void startServer() throws IOException { + server = new StubGitHubServer(); + server.on("/user", ok("{\"login\":\"philipp-kurrle_ADVNTST\"}")); //$NON-NLS-1$ //$NON-NLS-2$ + server.on("/user/orgs", ok("[{\"login\":\"acme\"}]")); //$NON-NLS-1$ //$NON-NLS-2$ + server.on("/user/repos", ok("[]")); //$NON-NLS-1$ //$NON-NLS-2$ + } + + @After + public void stopServer() throws IOException { + server.close(); + } + + @Test + public void testConfiguredScopesReplaceTheDiscoveredOnes() + throws Exception { + servePullRequest("/repos/Philipp0205/notes/pulls/1", //$NON-NLS-1$ + "Fix the parser"); //$NON-NLS-1$ + server.onSearch("is:pr user:Philipp0205 is:open", //$NON-NLS-1$ + searchResult("/repos/Philipp0205/notes/pulls/1")); //$NON-NLS-1$ + + List pullRequests = client("Philipp0205") //$NON-NLS-1$ + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Fix the parser")); //$NON-NLS-1$ + assertThat(server.searchQueries(), + contains("is:pr user:Philipp0205 is:open")); //$NON-NLS-1$ + assertThat(server.requestedPaths(), + not(hasItem("/user/orgs"))); //$NON-NLS-1$ + assertThat(server.requestedPaths(), not(hasItem("/user"))); //$NON-NLS-1$ + } + + @Test + public void testConfiguredScopesAcceptOwnersAndSingleRepositories() + throws Exception { + client("Philipp0205, acme, someone/their-repo") //$NON-NLS-1$ + .getPullRequests(null, null, null, 25, 0); + + assertThat(server.searchQueries(), + contains("is:pr user:Philipp0205 user:acme " //$NON-NLS-1$ + + "repo:someone/their-repo is:open")); //$NON-NLS-1$ + } + + @Test + public void testOnlyTheRequestedPageIsReadInFull() throws Exception { + List found = new ArrayList<>(); + for (int number = 1; number <= 30; number++) { + String path = "/repos/acme/app/pulls/" + number; //$NON-NLS-1$ + found.add(path); + servePullRequest(path, "Change " + number); //$NON-NLS-1$ + } + server.onSearch("is:pr user:acme is:open", //$NON-NLS-1$ + searchResult(found.toArray(new String[0]))); + + List pullRequests = client("acme") //$NON-NLS-1$ + .getPullRequests(null, null, null, 5, 0); + + assertThat(pullRequests, hasSize(5)); + assertThat(detailRequests(), hasSize(5)); + } + + @Test + public void testManyRepositoriesAreSearchedInBatchedRequests() + throws Exception { + StringBuilder repositories = new StringBuilder(); + for (int i = 0; i < 50; i++) { + repositories.append("owner").append(i).append("/repo "); //$NON-NLS-1$ //$NON-NLS-2$ + } + + client(repositories.toString()).getPullRequests(null, null, null, 25, + 0); + + assertThat(server.searchQueries(), hasSize(3)); + } + + /** + * A token that reaches many repositories used to cost one search per + * repository plus one request per pull request found anywhere, which is + * what exhausted the API rate limit. + * + * @throws Exception + * if the listing fails + */ + @Test + public void testListingManyRepositoriesCostsFewRequests() + throws Exception { + StringBuilder repositories = new StringBuilder(); + for (int i = 0; i < 60; i++) { + repositories.append("owner").append(i).append("/repo "); //$NON-NLS-1$ //$NON-NLS-2$ + } + List found = new ArrayList<>(); + for (int number = 1; number <= 600; number++) { + String path = "/repos/owner0/repo/pulls/" + number; //$NON-NLS-1$ + found.add(path); + servePullRequest(path, "Change " + number); //$NON-NLS-1$ + } + server.onAnySearch(searchResult(found.toArray(new String[0]))); + + List pullRequests = client(repositories.toString()) + .getPullRequests(null, null, null, 100, 0); + + assertThat(pullRequests, hasSize(100)); + assertThat(server.searchQueries(), hasSize(3)); + assertThat(detailRequests(), hasSize(100)); + assertThat(server.requestedPaths(), hasSize(103)); + } + + @Test + public void testScopeGitHubRefusesToSearchIsSkipped() throws Exception { + servePullRequest("/repos/philipp-kurrle_ADVNTST/notes/pulls/1", //$NON-NLS-1$ + "Fix the parser"); //$NON-NLS-1$ + server.onSearch( + "is:pr user:philipp-kurrle_ADVNTST org:acme is:open", //$NON-NLS-1$ + refused()); + server.onSearch("is:pr user:philipp-kurrle_ADVNTST is:open", //$NON-NLS-1$ + searchResult( + "/repos/philipp-kurrle_ADVNTST/notes/pulls/1")); //$NON-NLS-1$ + server.onSearch("is:pr org:acme is:open", refused()); //$NON-NLS-1$ + + List pullRequests = client(null) + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Fix the parser")); //$NON-NLS-1$ + } + + @Test + public void testDiagnosticsReportSkippedScopeAsWarningNotFailure() + throws Exception { + servePullRequest("/repos/philipp-kurrle_ADVNTST/notes/pulls/1", //$NON-NLS-1$ + "Fix the parser"); //$NON-NLS-1$ + server.onSearch( + "is:pr user:philipp-kurrle_ADVNTST org:acme is:open", //$NON-NLS-1$ + refused()); + server.onSearch("is:pr user:philipp-kurrle_ADVNTST is:open", //$NON-NLS-1$ + searchResult( + "/repos/philipp-kurrle_ADVNTST/notes/pulls/1")); //$NON-NLS-1$ + server.onSearch("is:pr org:acme is:open", refused()); //$NON-NLS-1$ + + ConnectionDiagnostics report = client(null).diagnoseConnection(); + + assertThat(report.toReport(), report.isSuccessful(), is(true)); + assertThat(step(report, "Authentication").getDetail(), //$NON-NLS-1$ + equalTo("Authenticated as philipp-kurrle_ADVNTST")); //$NON-NLS-1$ + assertThat(step(report, "Read pull requests").getDetail(), //$NON-NLS-1$ + equalTo("Returned 1 pull request(s)")); //$NON-NLS-1$ + Step scopes = step(report, "Search scopes"); //$NON-NLS-1$ + assertThat(scopes.getOutcome(), equalTo(Outcome.WARNING)); + assertThat(scopes.getDetail(), containsString("org:acme")); //$NON-NLS-1$ + } + + @Test + public void testDiagnosticsStaySilentWhenEveryScopeIsSearchable() + throws Exception { + servePullRequest("/repos/philipp-kurrle_ADVNTST/notes/pulls/1", //$NON-NLS-1$ + "Fix the parser"); //$NON-NLS-1$ + server.onSearch( + "is:pr user:philipp-kurrle_ADVNTST org:acme is:open", //$NON-NLS-1$ + searchResult( + "/repos/philipp-kurrle_ADVNTST/notes/pulls/1")); //$NON-NLS-1$ + + ConnectionDiagnostics report = client(null).diagnoseConnection(); + + assertThat(report.toReport(), report.isSuccessful(), is(true)); + assertThat(report.toReport(), not(containsString("Search scopes"))); //$NON-NLS-1$ + } + + @Test + public void testEveryScopeRefusedIsReportedWithTokenAdvice() { + server.onSearch("is:pr user:alice user:bob is:open", refused()); //$NON-NLS-1$ + server.onSearch("is:pr user:alice is:open", refused()); //$NON-NLS-1$ + server.onSearch("is:pr user:bob is:open", refused()); //$NON-NLS-1$ + + ConnectionDiagnostics report = client("alice, bob") //$NON-NLS-1$ + .diagnoseConnection(); + + assertThat(report.isSuccessful(), is(false)); + Step read = step(report, "Read pull requests"); //$NON-NLS-1$ + assertThat(read.getOutcome(), equalTo(Outcome.FAILED)); + assertThat(read.getDetail(), containsString("user:alice")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("user:bob")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("read:org")); //$NON-NLS-1$ + } + + @Test + public void testRateLimitIsReportedWithAdviceInsteadOfRawJson() { + server.onSearch("is:pr user:alice is:open", //$NON-NLS-1$ + new Response(403, "Forbidden", RATE_LIMITED)); //$NON-NLS-1$ + + ConnectionDiagnostics report = client("alice").diagnoseConnection(); //$NON-NLS-1$ + + Step read = step(report, "Read pull requests"); //$NON-NLS-1$ + assertThat(read.getOutcome(), equalTo(Outcome.FAILED)); + assertThat(read.getDetail(), + containsString("rate limit for this access token")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("preference page")); //$NON-NLS-1$ + } + + @Test + public void testOrganizationsThatCannotBeListedDoNotHideOwnPullRequests() + throws Exception { + server.on("/user/orgs", new Response(403, "Forbidden", //$NON-NLS-1$ //$NON-NLS-2$ + "{\"message\":\"Resource not accessible by integration\"}")); //$NON-NLS-1$ + servePullRequest("/repos/philipp-kurrle_ADVNTST/notes/pulls/1", //$NON-NLS-1$ + "Fix the parser"); //$NON-NLS-1$ + server.onSearch("is:pr user:philipp-kurrle_ADVNTST is:open", //$NON-NLS-1$ + searchResult( + "/repos/philipp-kurrle_ADVNTST/notes/pulls/1")); //$NON-NLS-1$ + + List pullRequests = client(null) + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Fix the parser")); //$NON-NLS-1$ + } + + @Test + public void testSearchFailuresOtherThanRefusedScopesAreReported() { + server.onSearch("is:pr user:alice is:open", //$NON-NLS-1$ + new Response(503, "Service Unavailable", //$NON-NLS-1$ + "{\"message\":\"No server is available\"}")); //$NON-NLS-1$ + + ConnectionDiagnostics report = client("alice").diagnoseConnection(); //$NON-NLS-1$ + + Step read = step(report, "Read pull requests"); //$NON-NLS-1$ + assertThat(read.getOutcome(), equalTo(Outcome.FAILED)); + assertThat(read.getDetail(), containsString("HTTP 503")); //$NON-NLS-1$ + } + + @Test + public void testPullRequestFoundInTwoScopesIsListedOnce() + throws Exception { + servePullRequest("/repos/acme/app/pulls/4", //$NON-NLS-1$ + "Bump the target platform"); //$NON-NLS-1$ + server.onSearch("is:pr user:acme repo:acme/app is:open", //$NON-NLS-1$ + searchResult("/repos/acme/app/pulls/4", //$NON-NLS-1$ + "/repos/acme/app/pulls/4")); //$NON-NLS-1$ + + List pullRequests = client("acme, acme/app") //$NON-NLS-1$ + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(detailRequests(), hasSize(1)); + } + + private GitHubClient client(String searchScopes) { + return new GitHubClient(null, null, "token", server.url(), //$NON-NLS-1$ + GitHubSearchQueries.parseScopes(searchScopes)); + } + + private List detailRequests() { + return server.requestedPaths().stream() + .filter(path -> path.contains("/pulls/")) //$NON-NLS-1$ + .toList(); + } + + private void servePullRequest(String path, String title) { + int number = Integer + .parseInt(path.substring(path.lastIndexOf('/') + 1)); + server.on(path, ok("{\"number\":" + number + ",\"title\":\"" + title //$NON-NLS-1$ //$NON-NLS-2$ + + "\",\"state\":\"open\",\"body\":\"\"," //$NON-NLS-1$ + + "\"created_at\":\"2026-09-01T10:00:00Z\"," //$NON-NLS-1$ + + "\"updated_at\":\"2026-09-01T10:00:00Z\"," //$NON-NLS-1$ + + "\"user\":{\"login\":\"Philipp0205\"}}")); //$NON-NLS-1$ + } + + private Response searchResult(String... pullPaths) { + StringBuilder items = new StringBuilder(); + int index = 0; + for (String pullPath : pullPaths) { + if (items.length() > 0) { + items.append(','); + } + items.append("{\"updated_at\":\"").append(timestamp(index)) //$NON-NLS-1$ + .append("\",\"pull_request\":{\"url\":\"") //$NON-NLS-1$ + .append(server.url()).append(pullPath).append("\"}}"); //$NON-NLS-1$ + index++; + } + return ok("{\"total_count\":" + pullPaths.length + ",\"items\":[" //$NON-NLS-1$ //$NON-NLS-2$ + + items + "]}"); //$NON-NLS-1$ + } + + /** + * Builds an update timestamp that decreases with the index, so that + * search results come back most recently updated first. + * + * @param index + * position in the result list + * @return an ISO 8601 timestamp + */ + private static String timestamp(int index) { + return DateTimeFormatter.ISO_INSTANT.format( + Instant.parse("2026-09-01T00:00:00Z") //$NON-NLS-1$ + .minusSeconds(index * 60L)); + } + + private static Response refused() { + return new Response(422, "Unprocessable Entity", VALIDATION_FAILED); //$NON-NLS-1$ + } + + private static Response ok(String body) { + return new Response(200, "OK", body); //$NON-NLS-1$ + } + + private static Step step(ConnectionDiagnostics report, String name) { + for (Step step : report.getSteps()) { + if (step.getName().equals(name)) { + return step; + } + } + throw new AssertionError( + "No step " + name + " in\n" + report.toReport()); //$NON-NLS-1$ //$NON-NLS-2$ + } +} diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java new file mode 100644 index 0000000..be66bfa --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java @@ -0,0 +1,294 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.eclipse.egit.pullrequest.internal.github.GitHubPullRequestSearch.PageReader; +import org.eclipse.egit.pullrequest.internal.github.GitHubPullRequestSearch.Result; +import org.eclipse.egit.pullrequest.internal.github.GitHubSearchQueries.Query; +import org.junit.Test; + +/** + * Tests for {@link GitHubPullRequestSearch} + */ +public class GitHubPullRequestSearchTest { + + private static final String VALIDATION_FAILED = "GitHub API request " //$NON-NLS-1$ + + "failed: HTTP 422 - {\"message\":\"Validation Failed\"," //$NON-NLS-1$ + + "\"errors\":[{\"message\":\"The listed users and " //$NON-NLS-1$ + + "repositories cannot be searched either because the " //$NON-NLS-1$ + + "resources do not exist or you do not have permission to " //$NON-NLS-1$ + + "view them.\"}]}"; //$NON-NLS-1$ + + private static final String RATE_LIMITED = "GitHub API request failed: " //$NON-NLS-1$ + + "HTTP 403 - {\"message\":\"API rate limit exceeded for user " //$NON-NLS-1$ + + "ID 201185819.\"}"; //$NON-NLS-1$ + + private final List requestedQueries = new ArrayList<>(); + + /** + * A reader that answers each query from canned results and fails every + * query that was registered as failing. + */ + private class StubReader implements PageReader { + + private final Map> results = + new HashMap<>(); + + private final Map failures = new HashMap<>(); + + StubReader returning(String query, String... paths) { + List hits = new ArrayList<>(); + for (String path : paths) { + hits.add(new GitHubSearchHit(path, new Date(0))); + } + results.put(query, hits); + return this; + } + + StubReader failingWith(String query, String message) { + failures.put(query, new IOException(message)); + return this; + } + + @Override + public List read(String query, int page, + int pageSize) throws IOException { + requestedQueries.add(query); + IOException failure = failures.get(query); + if (failure != null) { + throw failure; + } + if (page > 1) { + return Collections.emptyList(); + } + return results.getOrDefault(query, Collections.emptyList()); + } + } + + private static Query query(String... scopes) { + return new Query(Arrays.asList(scopes), null, null); + } + + private static List paths(Result result) { + List paths = new ArrayList<>(); + for (GitHubSearchHit hit : result.getHits()) { + paths.add(hit.path()); + } + return paths; + } + + @Test + public void testRefusedScopeIsSkippedAndOthersStillReturnResults() + throws Exception { + StubReader reader = new StubReader() + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .returning("is:pr repo:bob/tool is:open", //$NON-NLS-1$ + "/repos/bob/tool/pulls/9"); //$NON-NLS-1$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice"), query("org:acme"), //$NON-NLS-1$ //$NON-NLS-2$ + query("repo:bob/tool")), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(paths(result), contains("/repos/alice/notes/pulls/1", //$NON-NLS-1$ + "/repos/bob/tool/pulls/9")); //$NON-NLS-1$ + assertThat(result.getUnsearchableScopes(), contains("org:acme")); //$NON-NLS-1$ + } + + @Test + public void testRefusedScopeDoesNotStopLaterQueries() throws Exception { + StubReader reader = new StubReader() + .failingWith("is:pr user:alice is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .returning("is:pr org:acme is:open", //$NON-NLS-1$ + "/repos/acme/app/pulls/3"); //$NON-NLS-1$ + + GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice"), query("org:acme")), //$NON-NLS-1$ //$NON-NLS-2$ + 1, 1, reader); + + assertThat(requestedQueries, hasSize(2)); + } + + @Test + public void testRefusedBatchIsRetriedScopeByScope() throws Exception { + StubReader reader = new StubReader() + .failingWith( + "is:pr user:alice org:acme repo:bob/tool is:open", //$NON-NLS-1$ + VALIDATION_FAILED) + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .returning("is:pr repo:bob/tool is:open", //$NON-NLS-1$ + "/repos/bob/tool/pulls/9"); //$NON-NLS-1$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList( + query("user:alice", "org:acme", "repo:bob/tool")), //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + 1, 1, reader); + + assertThat(paths(result), contains("/repos/alice/notes/pulls/1", //$NON-NLS-1$ + "/repos/bob/tool/pulls/9")); //$NON-NLS-1$ + assertThat(result.getUnsearchableScopes(), contains("org:acme")); //$NON-NLS-1$ + } + + @Test + public void testBatchIsSentAsOneRequestWhenGitHubAcceptsIt() + throws Exception { + StubReader reader = new StubReader().returning( + "is:pr user:alice org:acme is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ + + GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice", "org:acme")), //$NON-NLS-1$ //$NON-NLS-2$ + 1, 1, reader); + + assertThat(requestedQueries, + contains("is:pr user:alice org:acme is:open")); //$NON-NLS-1$ + } + + @Test + public void testAllScopesRefusedFailsWithActionableMessage() { + StubReader reader = new StubReader() + .failingWith("is:pr user:alice is:open", VALIDATION_FAILED) //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", VALIDATION_FAILED); //$NON-NLS-1$ + + try { + GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice"), query("org:acme")), //$NON-NLS-1$ //$NON-NLS-2$ + 1, 1, reader); + fail("Expected the search to fail"); //$NON-NLS-1$ + } catch (IOException e) { + assertThat(e.getMessage(), containsString("user:alice")); //$NON-NLS-1$ + assertThat(e.getMessage(), containsString("org:acme")); //$NON-NLS-1$ + assertThat(e.getMessage(), containsString("read:org")); //$NON-NLS-1$ + assertThat(e.getCause().getMessage(), + containsString("HTTP 422")); //$NON-NLS-1$ + } + } + + @Test + public void testRateLimitAbortsTheSearchWithAdvice() { + StubReader reader = new StubReader() + .failingWith("is:pr user:alice is:open", RATE_LIMITED) //$NON-NLS-1$ + .returning("is:pr org:acme is:open", //$NON-NLS-1$ + "/repos/acme/app/pulls/3"); //$NON-NLS-1$ + + try { + GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice"), query("org:acme")), //$NON-NLS-1$ //$NON-NLS-2$ + 1, 1, reader); + fail("Expected the search to fail"); //$NON-NLS-1$ + } catch (IOException e) { + assertThat(e.getMessage(), containsString("rate limit")); //$NON-NLS-1$ + assertThat(e.getMessage(), + containsString("preference page")); //$NON-NLS-1$ + } + assertThat(requestedQueries, hasSize(1)); + } + + @Test + public void testFailuresOtherThanRefusedScopesArePropagated() { + StubReader reader = new StubReader() + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .failingWith("is:pr org:acme is:open", //$NON-NLS-1$ + "GitHub API request failed: HTTP 503 - unavailable"); //$NON-NLS-1$ + + try { + GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice"), query("org:acme")), //$NON-NLS-1$ //$NON-NLS-2$ + 1, 1, reader); + fail("Expected the search to fail"); //$NON-NLS-1$ + } catch (IOException e) { + assertThat(e.getMessage(), containsString("HTTP 503")); //$NON-NLS-1$ + } + } + + @Test + public void testPullRequestFoundByTwoScopesIsReportedOnce() + throws Exception { + StubReader reader = new StubReader() + .returning("is:pr user:alice is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1") //$NON-NLS-1$ + .returning("is:pr repo:alice/notes is:open", //$NON-NLS-1$ + "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice"), //$NON-NLS-1$ + query("repo:alice/notes")), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(paths(result), + contains("/repos/alice/notes/pulls/1")); //$NON-NLS-1$ + } + + @Test + public void testSuccessfulSearchReportsNoUnsearchableScopes() + throws Exception { + StubReader reader = new StubReader().returning( + "is:pr user:alice is:open", "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ //$NON-NLS-2$ + + Result result = GitHubPullRequestSearch.run( + Arrays.asList(query("user:alice")), 1, 1, reader); //$NON-NLS-1$ + + assertThat(result.getUnsearchableScopes(), empty()); + } + + @Test + public void testValidationFailureIsRecognizedAsRefusedScope() { + assertThat( + GitHubPullRequestSearch.isRefusedScope( + new IOException(VALIDATION_FAILED)), + equalTo(true)); + assertThat( + GitHubPullRequestSearch.isRefusedScope(new IOException( + "GitHub API request failed: HTTP 401 - Bad token")), //$NON-NLS-1$ + equalTo(false)); + assertThat( + GitHubPullRequestSearch.isRefusedScope(new IOException()), + equalTo(false)); + } + + @Test + public void testRateLimitIsRecognizedButPlainForbiddenIsNot() { + assertThat( + GitHubPullRequestSearch + .isRateLimited(new IOException(RATE_LIMITED)), + equalTo(true)); + assertThat(GitHubPullRequestSearch.isRateLimited(new IOException( + "GitHub API request failed: HTTP 403 - " //$NON-NLS-1$ + + "{\"message\":\"Resource not accessible\"}")), //$NON-NLS-1$ + equalTo(false)); + assertThat( + GitHubPullRequestSearch + .isRateLimited(new IOException(VALIDATION_FAILED)), + equalTo(false)); + } +} diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueriesTest.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueriesTest.java new file mode 100644 index 0000000..3e72652 --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueriesTest.java @@ -0,0 +1,128 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.eclipse.egit.pullrequest.internal.github.GitHubSearchQueries.Query; +import org.junit.Test; + +/** + * Tests for {@link GitHubSearchQueries} + */ +public class GitHubSearchQueriesTest { + + @Test + public void testScopesAreReadFromCommasAndWhitespace() { + assertThat( + GitHubSearchQueries + .parseScopes(" alice, acme\nbob/tool , "), //$NON-NLS-1$ + contains("alice", "acme", "bob/tool")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + } + + @Test + public void testScopesAreDeduplicatedAndEmptyValuesAreIgnored() { + assertThat(GitHubSearchQueries.parseScopes("alice, alice"), //$NON-NLS-1$ + contains("alice")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.parseScopes(" "), empty()); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.parseScopes(null), empty()); + } + + @Test + public void testBareNameCoversEverythingAnOwnerHas() { + assertThat(GitHubSearchQueries.qualifierFor("alice"), //$NON-NLS-1$ + equalTo("user:alice")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.qualifierFor("bob/tool"), //$NON-NLS-1$ + equalTo("repo:bob/tool")); //$NON-NLS-1$ + } + + @Test + public void testAlreadyQualifiedScopesAreKeptAsWritten() { + assertThat(GitHubSearchQueries.qualifierFor("org:acme"), //$NON-NLS-1$ + equalTo("org:acme")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.qualifierFor("user:alice"), //$NON-NLS-1$ + equalTo("user:alice")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.qualifierFor("repo:bob/tool"), //$NON-NLS-1$ + equalTo("repo:bob/tool")); //$NON-NLS-1$ + } + + @Test + public void testConfiguredScopesShareOneQuery() { + List queries = GitHubSearchQueries + .configuredPullRequestQueries( + Arrays.asList("alice", "acme", "bob/tool"), null, //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + null); + + assertThat(queries, hasSize(1)); + assertThat(queries.get(0).getText(), + equalTo("is:pr user:alice user:acme repo:bob/tool " //$NON-NLS-1$ + + "is:open")); //$NON-NLS-1$ + } + + @Test + public void testManyScopesAreSplitIntoSeveralQueries() { + List scopes = new ArrayList<>(); + for (int i = 0; i < 45; i++) { + scopes.add("owner" + i + "/repo"); //$NON-NLS-1$ //$NON-NLS-2$ + } + + List queries = GitHubSearchQueries + .configuredPullRequestQueries(scopes, null, null); + + assertThat(queries, hasSize(3)); + assertThat(queries.get(0).getScopes(), hasSize(20)); + assertThat(queries.get(1).getScopes(), hasSize(20)); + assertThat(queries.get(2).getScopes(), hasSize(5)); + } + + @Test + public void testSplitTurnsABatchIntoOneQueryPerScope() { + Query batch = new Query( + Arrays.asList("user:alice", "repo:bob/tool"), "MERGED", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + "carol"); //$NON-NLS-1$ + + List parts = batch.split(); + + assertThat(parts, hasSize(2)); + assertThat(parts.get(0).getText(), + equalTo("is:pr user:alice author:carol is:merged")); //$NON-NLS-1$ + assertThat(parts.get(1).getText(), + equalTo("is:pr repo:bob/tool author:carol is:merged")); //$NON-NLS-1$ + } + + @Test + public void testSplittingASingleScopeQueryKeepsIt() { + Query single = new Query(Arrays.asList("user:alice"), null, null); //$NON-NLS-1$ + + assertThat(single.split(), contains(single)); + } + + @Test + public void testRepositoriesCoveredByAnOwnerScopeAreNotSearchedTwice() { + List queries = GitHubSearchQueries + .accessiblePullRequestQueries("alice", //$NON-NLS-1$ + Arrays.asList("acme"), //$NON-NLS-1$ + Arrays.asList("alice/notes", "acme/app", //$NON-NLS-1$ //$NON-NLS-2$ + "bob/tool"), //$NON-NLS-1$ + null, null); + + assertThat(queries.get(0).getScopes(), + contains("user:alice", "org:acme", "repo:bob/tool")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + } +} diff --git a/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java new file mode 100644 index 0000000..9366385 --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java @@ -0,0 +1,218 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +/** + * Minimal HTTP server on the loopback interface that answers canned responses, + * used to exercise {@link GitHubClient} without talking to github.com. + *

+ * Responses are registered per request path, and searches additionally per + * decoded search query, so that the individual queries the client sends to + * {@code /search/issues} can be answered and counted separately. + */ +class StubGitHubServer implements AutoCloseable { + + /** A canned response. */ + static class Response { + + private final int status; + + private final String reason; + + private final String body; + + Response(int status, String reason, String body) { + this.status = status; + this.reason = reason; + this.body = body; + } + } + + private static final String SEARCH_PATH = "/search/issues"; //$NON-NLS-1$ + + private final ServerSocket serverSocket; + + private final Map byPath = new ConcurrentHashMap<>(); + + private final Map bySearch = new ConcurrentHashMap<>(); + + private volatile Response anySearch; + + private final List requestedPaths = new CopyOnWriteArrayList<>(); + + private final List searchQueries = new CopyOnWriteArrayList<>(); + + private volatile boolean running = true; + + StubGitHubServer() throws IOException { + serverSocket = new ServerSocket(0, 0, InetAddress.getLoopbackAddress()); + Thread thread = new Thread(this::serve, "stub-github"); //$NON-NLS-1$ + thread.setDaemon(true); + thread.start(); + } + + /** + * @return the API base URL of this server + */ + String url() { + return "http://" //$NON-NLS-1$ + + serverSocket.getInetAddress().getHostAddress() + ':' + + serverSocket.getLocalPort(); + } + + /** + * Registers the response for a path, ignoring any query string. + * + * @param path + * the request path + * @param response + * the response to send + */ + void on(String path, Response response) { + byPath.put(path, response); + } + + /** + * Registers the response for one search query. + * + * @param query + * the decoded value of the {@code q} parameter + * @param response + * the response to send + */ + void onSearch(String query, Response response) { + bySearch.put(query, response); + } + + /** + * Registers the response for every search that has no exact match. + * + * @param response + * the response to send + */ + void onAnySearch(Response response) { + anySearch = response; + } + + /** + * @return the paths that were requested, in order, without their query + * strings + */ + List requestedPaths() { + return requestedPaths; + } + + /** + * @return the decoded search queries that were sent, in order + */ + List searchQueries() { + return searchQueries; + } + + @Override + public void close() throws IOException { + running = false; + serverSocket.close(); + } + + private void serve() { + while (running) { + try (Socket client = serverSocket.accept()) { + handle(client); + } catch (IOException e) { + return; + } + } + } + + private void handle(Socket client) throws IOException { + BufferedReader in = new BufferedReader(new InputStreamReader( + client.getInputStream(), StandardCharsets.UTF_8)); + String requestLine = in.readLine(); + if (requestLine == null) { + return; + } + String line; + while ((line = in.readLine()) != null && !line.isEmpty()) { + // Headers are not inspected, but must be drained. + } + String[] parts = requestLine.split(" "); //$NON-NLS-1$ + String target = parts.length > 1 ? parts[1] : ""; //$NON-NLS-1$ + write(client.getOutputStream(), responseFor(target)); + } + + private Response responseFor(String target) { + int mark = target.indexOf('?'); + String path = mark < 0 ? target : target.substring(0, mark); + String query = mark < 0 ? "" : target.substring(mark + 1); //$NON-NLS-1$ + requestedPaths.add(path); + + if (SEARCH_PATH.equals(path)) { + String search = decodeParameter(query, "q"); //$NON-NLS-1$ + searchQueries.add(search); + Response response = bySearch.get(search); + if (response == null) { + response = anySearch; + } + if (response != null) { + return response; + } + return new Response(200, "OK", //$NON-NLS-1$ + "{\"total_count\":0,\"items\":[]}"); //$NON-NLS-1$ + } + + Response response = byPath.get(path); + if (response != null) { + return response; + } + return new Response(404, "Not Found", //$NON-NLS-1$ + "{\"message\":\"Not Found\"}"); //$NON-NLS-1$ + } + + private static String decodeParameter(String query, String name) { + for (String pair : query.split("&")) { //$NON-NLS-1$ + int equals = pair.indexOf('='); + if (equals > 0 && pair.substring(0, equals).equals(name)) { + return URLDecoder.decode(pair.substring(equals + 1), + StandardCharsets.UTF_8); + } + } + return ""; //$NON-NLS-1$ + } + + private void write(OutputStream out, Response response) + throws IOException { + byte[] body = response.body.getBytes(StandardCharsets.UTF_8); + StringBuilder head = new StringBuilder(); + head.append("HTTP/1.1 ").append(response.status).append(' ') //$NON-NLS-1$ + .append(response.reason).append("\r\n"); //$NON-NLS-1$ + head.append("Content-Type: application/json\r\n"); //$NON-NLS-1$ + head.append("Content-Length: ").append(body.length).append("\r\n"); //$NON-NLS-1$ //$NON-NLS-2$ + head.append("Connection: close\r\n\r\n"); //$NON-NLS-1$ + out.write(head.toString().getBytes(StandardCharsets.UTF_8)); + out.write(body); + out.flush(); + } +} diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/PRPreferences.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/PRPreferences.java index 6620d23..900420a 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/PRPreferences.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/PRPreferences.java @@ -42,6 +42,12 @@ public class PRPreferences { /** GitHub access token */ public static final String GITHUB_ACCESS_TOKEN = "github_access_token"; //$NON-NLS-1$ + /** + * Owners and repositories whose pull requests to list, separated by + * commas or whitespace. Empty means everything the token can reach. + */ + public static final String GITHUB_SEARCH_SCOPES = "github_search_scopes"; //$NON-NLS-1$ + /** Sash weights for changes view */ public static final String PULLREQUEST_CHANGES_SASH_WEIGHTS = "pullrequest_changes_sash_weights"; //$NON-NLS-1$ diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/client/PullRequestClientFactory.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/client/PullRequestClientFactory.java index 727e4de..6a453a6 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/client/PullRequestClientFactory.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/client/PullRequestClientFactory.java @@ -57,6 +57,12 @@ public static class ClientConfig { * GitHub access token */ public String githubAccessToken; + + /** + * Owners and repositories whose pull requests to list, separated by + * commas or whitespace. Empty means everything the token can reach. + */ + public String githubSearchScopes; } /** @@ -148,7 +154,8 @@ public static IPullRequestClient createClient(ClientConfig config) { if (isBlank(config.githubAccessToken)) { return null; } - return new GitHubClient(config.githubAccessToken); + return new GitHubClient(config.githubAccessToken, + config.githubSearchScopes); default: return null; @@ -185,6 +192,8 @@ public static ClientConfig loadConfig() { config.githubOwner = prefs.get(PRPreferences.GITHUB_OWNER, ""); //$NON-NLS-1$ config.githubRepo = prefs.get(PRPreferences.GITHUB_REPO, ""); //$NON-NLS-1$ config.githubAccessToken = prefs.get(PRPreferences.GITHUB_ACCESS_TOKEN, ""); //$NON-NLS-1$ + config.githubSearchScopes = prefs + .get(PRPreferences.GITHUB_SEARCH_SCOPES, ""); //$NON-NLS-1$ return config; } diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java index 3280fea..5d59cb5 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java @@ -11,7 +11,6 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Comparator; -import java.util.LinkedHashSet; import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -20,6 +19,8 @@ import org.eclipse.egit.pullrequest.internal.model.PullRequest; import org.eclipse.egit.pullrequest.internal.model.PullRequestComment; import org.eclipse.egit.pullrequest.internal.model.PullRequestCommit; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics; +import org.eclipse.egit.pullrequest.internal.client.ConnectionDiagnostics.Outcome; import org.eclipse.egit.pullrequest.internal.client.IPullRequestClient; import org.eclipse.egit.pullrequest.internal.client.PullRequestProviderCapabilities; import org.eclipse.egit.pullrequest.Activator; @@ -34,8 +35,6 @@ public class GitHubClient implements IPullRequestClient { private static final String API_BASE_URL = "https://api.github.com"; //$NON-NLS-1$ - private static final String GRAPHQL_URL = "https://api.github.com/graphql"; //$NON-NLS-1$ - private static final int DEFAULT_TIMEOUT = 30000; // 30 seconds private String owner; @@ -44,8 +43,22 @@ public class GitHubClient implements IPullRequestClient { private final String token; + private final String apiBaseUrl; + private final PullRequestProviderCapabilities capabilities; + /** + * Owners and repositories to search, as configured by the user. Empty + * means every scope the authenticated user can reach. + */ + private final List searchScopes; + + /** + * Owner scopes the last token-scoped listing had to skip because GitHub + * refused to search them. Reported by {@link #diagnoseConnection()}. + */ + private List unsearchableScopes = List.of(); + /** * Creates a new GitHub client * @@ -58,11 +71,7 @@ public class GitHubClient implements IPullRequestClient { */ public GitHubClient(@NonNull String owner, @NonNull String repo, @NonNull String token) { - this.owner = owner; - this.repo = repo; - this.token = token; - this.capabilities = PullRequestProviderCapabilities - .forProvider(PullRequestProviderType.GITHUB); + this(owner, repo, token, API_BASE_URL, List.of()); } /** @@ -73,9 +82,54 @@ public GitHubClient(@NonNull String owner, @NonNull String repo, * the GitHub access token */ public GitHubClient(@NonNull String token) { - this.owner = null; - this.repo = null; + this(null, null, token, API_BASE_URL, List.of()); + } + + /** + * Creates a token-scoped GitHub client that only searches the given + * owners and repositories. + *

+ * Restricting the scopes is what makes the listing predictable for a + * token that can reach hundreds of repositories: it decides which pull + * requests appear, and it keeps the number of search requests small + * enough to stay within the API rate limit. + * + * @param token + * the GitHub access token + * @param searchScopes + * owners and repositories separated by commas or whitespace, + * {@code null} or empty to search every reachable scope. An + * entry is either {@code owner/name} for a single repository + * or a bare name for everything a user or organization owns + */ + public GitHubClient(@NonNull String token, + @Nullable String searchScopes) { + this(null, null, token, API_BASE_URL, + GitHubSearchQueries.parseScopes(searchScopes)); + } + + /** + * Creates a client against a specific GitHub REST API endpoint. + * + * @param owner + * the repository owner, or {@code null} to list pull requests + * from every accessible repository + * @param repo + * the repository name, or {@code null} + * @param token + * the GitHub access token + * @param apiBaseUrl + * the REST API base URL, without a trailing slash + * @param searchScopes + * owners and repositories to search, empty for all reachable + */ + GitHubClient(String owner, String repo, @NonNull String token, + @NonNull String apiBaseUrl, @NonNull List searchScopes) { + this.owner = owner; + this.repo = repo; this.token = token; + this.apiBaseUrl = apiBaseUrl; + this.searchScopes = List.copyOf(searchScopes); this.capabilities = PullRequestProviderCapabilities .forProvider(PullRequestProviderType.GITHUB); } @@ -195,68 +249,131 @@ public void setActivePullRequest(PullRequest pullRequest) { return pulls; } + /** + * Lists pull requests across every searched scope. + *

+ * The search already reports when each pull request was last updated, so + * the results of all scopes are ordered and cut down to the requested page + * first. Only that page is then read in full: reading every hit instead + * costs one request per pull request found in any scope, which exhausts + * the API rate limit as soon as a token can reach a few dozen + * repositories. + * + * @param state + * pull request state filter, or {@code null} for open + * @param authorUsername + * author filter, or {@code null} for every author + * @param limit + * how many pull requests to return + * @param start + * how many pull requests to skip + * @return the requested page, most recently updated first + * @throws IOException + * if the pull requests cannot be listed + */ private List getUserPullRequests(String state, String authorUsername, int limit, int start) throws IOException { - List queries = GitHubSearchQueries - .accessiblePullRequestQueries(getCurrentUser(), - listOrganizationLogins(), - listCollaboratorRepositories(), state, - authorUsername); + List queries = pullRequestQueries(state, + authorUsername); int needed = Math.max(1, start + Math.max(1, limit)); int pageSize = Math.min(100, needed); - LinkedHashSet pullRequestPaths = new LinkedHashSet<>(); - for (String query : queries) { - int collected = 0; - int page = 1; - while (collected < needed) { - String path = "/search/issues?q=" //$NON-NLS-1$ - + URLEncoder.encode(query, StandardCharsets.UTF_8) - + "&per_page=" + pageSize //$NON-NLS-1$ - + "&page=" + page //$NON-NLS-1$ - + "&sort=updated&order=desc"; //$NON-NLS-1$ - List pagePaths = GitHubJsonParser - .parseSearchPullRequestPaths(doGet(path)); - if (pagePaths.isEmpty()) { - break; - } - pullRequestPaths.addAll(pagePaths); - collected += pagePaths.size(); - if (pagePaths.size() < pageSize) { - break; - } - page++; - } - } + GitHubPullRequestSearch.Result search = GitHubPullRequestSearch.run( + queries, needed, pageSize, + (query, page, size) -> GitHubJsonParser.parseSearchHits( + doGet(searchPath(query, page, size)))); + unsearchableScopes = search.getUnsearchableScopes(); + + List hits = new ArrayList<>(search.getHits()); + hits.sort(Comparator.comparing(GitHubSearchHit::updated, + Comparator.nullsLast(Comparator.reverseOrder()))); + int from = Math.min(Math.max(0, start), hits.size()); + int to = Math.min(from + Math.max(0, limit), hits.size()); List result = new ArrayList<>(); - for (String pullRequestPath : pullRequestPaths) { + for (GitHubSearchHit hit : hits.subList(from, to)) { PullRequest pullRequest = GitHubJsonParser - .parseSinglePullRequest(doGet(pullRequestPath)); + .parseSinglePullRequest(doGet(hit.path())); if (pullRequest != null) { result.add(pullRequest); } } - result.sort(Comparator.comparing(PullRequest::getUpdatedDate, - Comparator.nullsLast(Comparator.reverseOrder()))); - int from = Math.min(Math.max(0, start), result.size()); - int to = Math.min(from + Math.max(0, limit), result.size()); - return new ArrayList<>(result.subList(from, to)); + return result; + } + + /** + * Builds the searches to run, from the configured scopes when there are + * any and from everything the token can reach otherwise. + * + * @param state + * pull request state filter, or {@code null} for open + * @param authorUsername + * author filter, or {@code null} for every author + * @return the queries to run + * @throws IOException + * if the authenticated user cannot be read + */ + private List pullRequestQueries(String state, + String authorUsername) throws IOException { + if (!searchScopes.isEmpty()) { + return GitHubSearchQueries.configuredPullRequestQueries( + searchScopes, state, authorUsername); + } + return GitHubSearchQueries.accessiblePullRequestQueries( + getCurrentUser(), listOrganizationLogins(), + listCollaboratorRepositories(), state, authorUsername); + } + + private static String searchPath(String query, int page, int pageSize) { + return "/search/issues?q=" //$NON-NLS-1$ + + URLEncoder.encode(query, StandardCharsets.UTF_8) + + "&per_page=" + pageSize //$NON-NLS-1$ + + "&page=" + page //$NON-NLS-1$ + + "&sort=updated&order=desc"; //$NON-NLS-1$ } - private List listOrganizationLogins() throws IOException { + /** + * Lists the organizations of the authenticated user. + *

+ * A token without the {@code read:org} scope cannot read them. That only + * narrows the set of searched scopes, so it is reported as a warning + * instead of failing the whole listing. + * + * @return the organization logins, empty if they cannot be read + */ + private List listOrganizationLogins() { List logins = new ArrayList<>(); - for (String page : doGetAllPages("/user/orgs?per_page=100")) { //$NON-NLS-1$ - logins.addAll(GitHubJsonParser.parseLogins(page)); + try { + for (String page : doGetAllPages("/user/orgs?per_page=100")) { //$NON-NLS-1$ + logins.addAll(GitHubJsonParser.parseLogins(page)); + } + } catch (IOException e) { + Activator.logWarning( + "Cannot list GitHub organizations; their pull requests " //$NON-NLS-1$ + + "are not listed. " + e.getMessage()); //$NON-NLS-1$ } return logins; } - private List listCollaboratorRepositories() throws IOException { + /** + * Lists the repositories the authenticated user collaborates on. + * + * @return the {@code owner/name} repositories, empty if they cannot be + * read + */ + private List listCollaboratorRepositories() { List fullNames = new ArrayList<>(); - for (String page : doGetAllPages( - "/user/repos?affiliation=collaborator&per_page=100")) { //$NON-NLS-1$ - fullNames.addAll(GitHubJsonParser.parseRepositoryFullNames(page)); + try { + for (String page : doGetAllPages( + "/user/repos?affiliation=collaborator&per_page=100")) { //$NON-NLS-1$ + fullNames.addAll( + GitHubJsonParser.parseRepositoryFullNames(page)); + } + } catch (IOException e) { + Activator.logWarning( + "Cannot list GitHub collaborator repositories; their " //$NON-NLS-1$ + + "pull requests are not listed. " //$NON-NLS-1$ + + e.getMessage()); } return fullNames; } @@ -816,6 +933,20 @@ public boolean testConnection() { return GitHubJsonParser.parseCurrentUser(json); } + @Override + public @NonNull ConnectionDiagnostics diagnoseConnection() { + ConnectionDiagnostics report = IPullRequestClient.super + .diagnoseConnection(); + if (!unsearchableScopes.isEmpty()) { + report.add("Search scopes", Outcome.WARNING, //$NON-NLS-1$ + "GitHub refused to search " //$NON-NLS-1$ + + String.join(", ", unsearchableScopes) //$NON-NLS-1$ + + ".\n" //$NON-NLS-1$ + + GitHubPullRequestSearch.TOKEN_SCOPE_HINT); + } + return report; + } + @Override public @NonNull List getPullRequestCommits( long pullRequestId) throws IOException { @@ -881,7 +1012,7 @@ private String doGet(String path) throws IOException { */ private List doGetAllPages(String path) throws IOException { List pages = new ArrayList<>(); - String nextUrl = API_BASE_URL + path; + String nextUrl = apiBaseUrl + path; while (nextUrl != null) { HttpURLConnection conn = null; @@ -1134,7 +1265,7 @@ private String executeRequest(String path, String method, private String executeGraphQL(String query) throws IOException { HttpURLConnection conn = null; try { - URL url = new URL(GRAPHQL_URL); + URL url = new URL(apiBaseUrl + "/graphql"); //$NON-NLS-1$ conn = (HttpURLConnection) url.openConnection(); conn.setRequestMethod("POST"); //$NON-NLS-1$ conn.setConnectTimeout(DEFAULT_TIMEOUT); @@ -1184,7 +1315,7 @@ private String executeGraphQL(String query) throws IOException { */ private HttpURLConnection createConnection(String path, String method) throws IOException { - URL url = new URL(API_BASE_URL + path); + URL url = new URL(apiBaseUrl + path); HttpURLConnection conn = (HttpURLConnection) url.openConnection(); // HttpURLConnection doesn't support PATCH by default in Java diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java index fbd8305..e0dd55f 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubJsonParser.java @@ -24,30 +24,46 @@ class GitHubJsonParser { private static final String ISO8601_FORMAT = "yyyy-MM-dd'T'HH:mm:ss'Z'"; //$NON-NLS-1$ - private static final Pattern SEARCH_PULL_URL = Pattern.compile( - "\"pull_request\"\\s*:\\s*\\{[^}]*\"url\"\\s*:\\s*" //$NON-NLS-1$ - + "\"https://api\\.github\\.com([^\"?]+)\"", //$NON-NLS-1$ - Pattern.DOTALL); + private static final Pattern PULL_REQUEST_PATH = Pattern + .compile("https?://[^\"]+?(/repos/[^\"?]+)"); //$NON-NLS-1$ /** - * Extracts REST pull request paths from a GitHub issue search response. + * Extracts the pull requests of a GitHub issue search response. * * @param json * search response JSON - * @return API paths for the pull requests in result order + * @return the pull requests in result order, without the issues the + * response may also contain */ - static List parseSearchPullRequestPaths(String json) { - List result = new ArrayList<>(); - if (json == null || json.isBlank()) { + static List parseSearchHits(String json) { + List result = new ArrayList<>(); + String items = extractArray(json, "items"); //$NON-NLS-1$ + if (items == null) { return result; } - Matcher matcher = SEARCH_PULL_URL.matcher(json); - while (matcher.find()) { - result.add(matcher.group(1)); + for (String item : splitTopLevelObjects(items)) { + String pullRequest = extractObject(item, "pull_request"); //$NON-NLS-1$ + if (pullRequest == null) { + continue; + } + String path = pullRequestPath( + extractString(pullRequest, "url")); //$NON-NLS-1$ + if (path != null) { + result.add(new GitHubSearchHit(path, + extractDate(item, "updated_at"))); //$NON-NLS-1$ + } } return result; } + private static String pullRequestPath(String url) { + if (url == null) { + return null; + } + Matcher matcher = PULL_REQUEST_PATH.matcher(url); + return matcher.matches() ? matcher.group(1) : null; + } + /** * Parses {@code login} values from a GitHub user or organization array. * @@ -956,7 +972,7 @@ private static String extractNumberString(String json, String key) { return json.substring(startIndex, endIndex); } - private static Date extractDate(String json, String key) { + static Date extractDate(String json, String key) { String dateStr = extractString(json, key); if (dateStr == null) { return null; diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java new file mode 100644 index 0000000..df547cd --- /dev/null +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java @@ -0,0 +1,240 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import java.io.IOException; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Deque; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +import org.eclipse.egit.pullrequest.Activator; +import org.eclipse.egit.pullrequest.internal.github.GitHubSearchQueries.Query; + +/** + * Searches GitHub for pull requests and merges what the queries return. + *

+ * GitHub answers {@code GET /search/issues} with HTTP 422 as soon as a single + * qualifier in the query names a user, organization or repository the access + * token may not search. That happens for organizations the token was never + * SSO-authorized for, for private repositories when the token lacks the + * {@code repo} scope, and for repositories that were renamed or deleted after + * they were listed. A rejected query is therefore retried scope by scope, and + * only the scopes GitHub really refuses are dropped; the listing fails only + * when no scope is searchable at all. + */ +final class GitHubPullRequestSearch { + + /** Advice shown whenever GitHub refuses to search an owner scope. */ + static final String TOKEN_SCOPE_HINT = "Use a classic personal access " //$NON-NLS-1$ + + "token with the 'repo' and 'read:org' scopes, and authorize " //$NON-NLS-1$ + + "it for every organization that enforces SAML single " //$NON-NLS-1$ + + "sign-on. Fine-grained tokens can only search the " //$NON-NLS-1$ + + "repositories they were granted."; //$NON-NLS-1$ + + /** Advice shown when GitHub throttles the token. */ + static final String RATE_LIMIT_HINT = "The GitHub API rate limit for " //$NON-NLS-1$ + + "this access token is exhausted. Wait for the limit to " //$NON-NLS-1$ + + "reset, or search fewer repositories by listing the wanted " //$NON-NLS-1$ + + "owners on the pull request preference page."; //$NON-NLS-1$ + + private GitHubPullRequestSearch() { + // No instantiation + } + + /** + * Reads one page of search results for a single query. + */ + @FunctionalInterface + interface PageReader { + + /** + * Runs one search request. + * + * @param query + * the search query + * @param page + * the one-based page number + * @param pageSize + * the number of results per page + * @return the pull requests on that page, in result order + * @throws IOException + * if the request fails + */ + List read(String query, int page, int pageSize) + throws IOException; + } + + /** Merged outcome of searching every scope. */ + static final class Result { + + private final List hits; + + private final List unsearchableScopes; + + Result(List hits, List unsearchableScopes) { + this.hits = hits; + this.unsearchableScopes = unsearchableScopes; + } + + /** + * @return the found pull requests, without duplicates + */ + List getHits() { + return hits; + } + + /** + * @return the owner qualifiers GitHub refused to search + */ + List getUnsearchableScopes() { + return unsearchableScopes; + } + } + + /** + * Runs every query and merges the pull requests they return. + * + * @param queries + * the queries covering the scopes to search + * @param needed + * how many results are needed per query + * @param pageSize + * how many results to request per page + * @param reader + * runs the individual search requests + * @return the merged result + * @throws IOException + * if a search fails for any reason other than a refused + * scope, or if GitHub refuses to search every scope + */ + static Result run(List queries, int needed, int pageSize, + PageReader reader) throws IOException { + Map hits = new LinkedHashMap<>(); + List unsearchable = new ArrayList<>(); + Deque pending = new ArrayDeque<>(queries); + int scopeCount = 0; + for (Query query : queries) { + scopeCount += query.getScopes().size(); + } + + IOException lastRejection = null; + while (!pending.isEmpty()) { + Query query = pending.removeFirst(); + try { + collect(query.getText(), needed, pageSize, reader, hits); + } catch (IOException e) { + if (isRateLimited(e)) { + throw new IOException(RATE_LIMIT_HINT + " " //$NON-NLS-1$ + + e.getMessage(), e); + } + if (!isRefusedScope(e)) { + throw e; + } + List parts = query.split(); + if (parts.size() > 1) { + // Find out which of the batched scopes is the bad one. + for (int i = parts.size() - 1; i >= 0; i--) { + pending.addFirst(parts.get(i)); + } + continue; + } + String scope = query.getScopes().get(0); + unsearchable.add(scope); + lastRejection = e; + Activator.logWarning("GitHub cannot search " + scope //$NON-NLS-1$ + + "; its pull requests are not listed. " //$NON-NLS-1$ + + e.getMessage()); + } + } + + if (lastRejection != null && unsearchable.size() == scopeCount) { + throw new IOException(allScopesRefused(unsearchable), + lastRejection); + } + return new Result(new ArrayList<>(hits.values()), + Collections.unmodifiableList(unsearchable)); + } + + private static void collect(String query, int needed, int pageSize, + PageReader reader, Map hits) + throws IOException { + int collected = 0; + int page = 1; + while (collected < needed) { + List pageHits = reader.read(query, page, + pageSize); + if (pageHits.isEmpty()) { + return; + } + for (GitHubSearchHit hit : pageHits) { + hits.putIfAbsent(hit.path(), hit); + } + collected += pageHits.size(); + if (pageHits.size() < pageSize) { + return; + } + page++; + } + } + + /** + * Tells whether GitHub rejected the query because one of its qualifiers + * names something the token may not search. + * + * @param e + * the failure reported by the search request + * @return true if the scope should be skipped rather than propagated + */ + static boolean isRefusedScope(IOException e) { + String message = e.getMessage(); + return message != null && message.contains("HTTP 422"); //$NON-NLS-1$ + } + + /** + * Tells whether GitHub throttled the token. + *

+ * Retrying the remaining scopes would only produce the same answer, so + * this aborts the listing instead of dropping scopes. + * + * @param e + * the failure reported by the search request + * @return true if GitHub reported a primary or secondary rate limit + */ + static boolean isRateLimited(IOException e) { + String message = e.getMessage(); + if (message == null) { + return false; + } + if (!message.contains("HTTP 403") //$NON-NLS-1$ + && !message.contains("HTTP 429")) { //$NON-NLS-1$ + return false; + } + return message.toLowerCase(Locale.ROOT).contains("rate limit"); //$NON-NLS-1$ + } + + /** + * Builds the message for the case where no scope could be searched. + * + * @param unsearchableScopes + * the owner qualifiers GitHub refused to search + * @return an actionable error message + */ + static String allScopesRefused(List unsearchableScopes) { + return "GitHub refused to search " //$NON-NLS-1$ + + String.join(", ", unsearchableScopes) //$NON-NLS-1$ + + ". " + TOKEN_SCOPE_HINT; //$NON-NLS-1$ + } +} diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchHit.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchHit.java new file mode 100644 index 0000000..ed20d72 --- /dev/null +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchHit.java @@ -0,0 +1,30 @@ +/******************************************************************************* + * Copyright (C) 2026, Philipp0205 and others + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.egit.pullrequest.internal.github; + +import java.util.Date; + +/** + * One pull request in a GitHub issue search response. + *

+ * The search tells which pull requests exist and when they changed, which is + * all that is needed to order them and to pick the page the caller asked for. + * Only that page is then read in full. + * + * @param path + * REST path of the pull request, for example + * {@code /repos/owner/name/pulls/7} + * @param updated + * when the pull request was last updated, or {@code null} if the + * search did not report it + */ +record GitHubSearchHit(String path, Date updated) { +} diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java index 8708de8..909f6e5 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java @@ -11,22 +11,89 @@ package org.eclipse.egit.pullrequest.internal.github; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; /** - * Builds GitHub issue-search queries that cover pull requests in repositories - * the authenticated user owns or belongs to. + * Builds GitHub issue-search queries that cover pull requests in the + * repositories a token may search. + *

+ * GitHub combines repeated owner qualifiers of the same kind with OR, so + * several scopes fit into one request. Batching them matters: a user who + * collaborates on a few hundred repositories would otherwise need a few + * hundred search requests per refresh and exhaust the API rate limit. */ final class GitHubSearchQueries { + /** + * How many owner qualifiers to put into one query. Kept well below the + * length at which GitHub starts rejecting queries. + */ + private static final int SCOPES_PER_QUERY = 20; + private GitHubSearchQueries() { // No instantiation } + /** One search request, covering one or more owner scopes. */ + static final class Query { + + private final List scopes; + + private final String state; + + private final String authorUsername; + + private final String text; + + Query(List scopes, String state, String authorUsername) { + this.scopes = List.copyOf(scopes); + this.state = state; + this.authorUsername = authorUsername; + this.text = pullRequestQuery(String.join(" ", scopes), state, //$NON-NLS-1$ + authorUsername); + } + + /** + * @return the owner qualifiers this query covers + */ + List getScopes() { + return scopes; + } + + /** + * @return the query string for {@code GET /search/issues} + */ + String getText() { + return text; + } + + /** + * Splits a batched query so that every scope is searched on its own. + *

+ * Used after GitHub rejected the batch, to find out which of its + * scopes is the unsearchable one instead of dropping them all. + * + * @return one query per scope, or this query if it has only one + */ + List split() { + if (scopes.size() < 2) { + return List.of(this); + } + List parts = new ArrayList<>(); + for (String scope : scopes) { + parts.add(new Query(List.of(scope), state, authorUsername)); + } + return parts; + } + } + /** - * Builds one search query per accessible owner scope. + * Builds the queries covering every scope the authenticated user can + * reach. *

* Personal repositories use {@code user:login}, organization membership * uses {@code org:name}, and remaining collaborator repositories use @@ -43,16 +110,15 @@ private GitHubSearchQueries() { * pull request state filter, or {@code null} for open * @param authorUsername * author filter, or {@code null} for every author - * @return search query strings for {@code GET /search/issues} + * @return the queries to run */ - static List accessiblePullRequestQueries(String login, + static List accessiblePullRequestQueries(String login, List organizationLogins, List collaboratorFullNames, String state, String authorUsername) { - List queries = new ArrayList<>(); + List scopes = new ArrayList<>(); if (login != null && !login.isBlank()) { - queries.add(pullRequestQuery("user:" + login, state, //$NON-NLS-1$ - authorUsername)); + scopes.add("user:" + login); //$NON-NLS-1$ } Set organizations = new HashSet<>(); @@ -62,8 +128,7 @@ static List accessiblePullRequestQueries(String login, continue; } organizations.add(organization); - queries.add(pullRequestQuery("org:" + organization, state, //$NON-NLS-1$ - authorUsername)); + scopes.add("org:" + organization); //$NON-NLS-1$ } } @@ -72,11 +137,92 @@ static List accessiblePullRequestQueries(String login, if (isCoveredByUserOrOrg(fullName, login, organizations)) { continue; } - queries.add(pullRequestQuery("repo:" + fullName, state, //$NON-NLS-1$ - authorUsername)); + scopes.add("repo:" + fullName); //$NON-NLS-1$ + } + } + return batch(scopes, state, authorUsername); + } + + /** + * Builds the queries for an explicitly configured set of scopes. + * + * @param scopes + * entries as returned by {@link #parseScopes(String)} + * @param state + * pull request state filter, or {@code null} for open + * @param authorUsername + * author filter, or {@code null} for every author + * @return the queries to run + */ + static List configuredPullRequestQueries(List scopes, + String state, String authorUsername) { + List qualifiers = new ArrayList<>(); + for (String scope : scopes) { + qualifiers.add(qualifierFor(scope)); + } + return batch(qualifiers, state, authorUsername); + } + + /** + * Reads the configured search scopes from their preference value. + *

+ * Entries are separated by commas or whitespace. An entry may already be + * a {@code user:}, {@code org:} or {@code repo:} qualifier; otherwise + * {@code owner/name} means a single repository and a bare name means + * everything owned by that user or organization. + * + * @param value + * the preference value, may be {@code null} + * @return the scope entries in the order they were written, without + * duplicates + */ + static List parseScopes(String value) { + if (value == null || value.isBlank()) { + return List.of(); + } + Set scopes = new LinkedHashSet<>(); + for (String entry : value.split("[,\\s]+")) { //$NON-NLS-1$ + if (!entry.isBlank()) { + scopes.add(entry.trim()); } } - return queries; + return List.copyOf(scopes); + } + + /** + * Turns a configured scope entry into a search qualifier. + *

+ * A bare name becomes {@code user:name}, which GitHub also accepts for + * organizations. + * + * @param scope + * the configured entry + * @return the qualifier to search with + */ + static String qualifierFor(String scope) { + String trimmed = scope.trim(); + if (trimmed.startsWith("user:") //$NON-NLS-1$ + || trimmed.startsWith("org:") //$NON-NLS-1$ + || trimmed.startsWith("repo:")) { //$NON-NLS-1$ + return trimmed; + } + if (trimmed.indexOf('/') > 0) { + return "repo:" + trimmed; //$NON-NLS-1$ + } + return "user:" + trimmed; //$NON-NLS-1$ + } + + private static List batch(List qualifiers, String state, + String authorUsername) { + List unique = new ArrayList<>( + new LinkedHashSet<>(qualifiers)); + List queries = new ArrayList<>(); + for (int from = 0; from < unique.size(); from += SCOPES_PER_QUERY) { + int to = Math.min(from + SCOPES_PER_QUERY, unique.size()); + queries.add(new Query(unique.subList(from, to), state, + authorUsername)); + } + return Collections.unmodifiableList(queries); } static String pullRequestQuery(String ownerQualifier, String state, diff --git a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java index 2c0c404..3de61bc 100644 --- a/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/ui/PullRequestPreferencePage.java @@ -55,6 +55,8 @@ public class PullRequestPreferencePage extends PreferencePage private Text githubTokenText; + private Text githubSearchScopesText; + private Button showInlineCommentsCheckbox; private Button animateInlineCommentsCheckbox; @@ -230,13 +232,37 @@ private Composite createGitHubConfiguration(Composite parent) { Label infoLabel = new Label(group, SWT.WRAP); infoLabel.setText( "Authenticate as a GitHub user with a personal access token " //$NON-NLS-1$ - + "(classic) that has the 'repo' scope:\n" //$NON-NLS-1$ + + "(classic) that has the 'repo' and 'read:org' " //$NON-NLS-1$ + + "scopes:\n" //$NON-NLS-1$ + "https://github.com/settings/tokens\n\n" //$NON-NLS-1$ - + "The plugin lists pull requests from every repository " //$NON-NLS-1$ - + "you own or belong to."); //$NON-NLS-1$ + + "The pull requests you see are the ones the token's " //$NON-NLS-1$ + + "own account can see. Use Test Connection to check " //$NON-NLS-1$ + + "which account that is."); //$NON-NLS-1$ GridDataFactory.fillDefaults().span(2, 1).hint(400, SWT.DEFAULT) .indent(0, 5).applyTo(infoLabel); + // Search scopes + Label scopesLabel = new Label(group, SWT.NONE); + scopesLabel.setText("&Search only:"); //$NON-NLS-1$ + + githubSearchScopesText = new Text(group, SWT.BORDER); + githubSearchScopesText.setToolTipText( + "Owners and repositories to list pull requests from"); //$NON-NLS-1$ + GridDataFactory.fillDefaults().grab(true, false) + .applyTo(githubSearchScopesText); + + Label scopesInfoLabel = new Label(group, SWT.WRAP); + scopesInfoLabel.setText( + "Owners and repositories separated by commas, for example " //$NON-NLS-1$ + + "'my-user, my-org, someone/their-repo'. A bare name " //$NON-NLS-1$ + + "covers everything that user or organization owns.\n" //$NON-NLS-1$ + + "Leave empty to search every repository the token " //$NON-NLS-1$ + + "can reach. Naming the owners you care about makes " //$NON-NLS-1$ + + "refreshing much faster and avoids running into the " //$NON-NLS-1$ + + "GitHub API rate limit."); //$NON-NLS-1$ + GridDataFactory.fillDefaults().span(2, 1).hint(400, SWT.DEFAULT) + .indent(0, 5).applyTo(scopesInfoLabel); + // Test connection button Button testButton = new Button(group, SWT.PUSH); testButton.setText("&Test Connection"); //$NON-NLS-1$ @@ -346,6 +372,8 @@ private void loadValues() { // Load GitHub values githubTokenText .setText(store.getString(PRPreferences.GITHUB_ACCESS_TOKEN)); + githubSearchScopesText + .setText(store.getString(PRPreferences.GITHUB_SEARCH_SCOPES)); // Load display options showInlineCommentsCheckbox.setSelection(store @@ -369,6 +397,7 @@ protected void performDefaults() { bitbucketTokenText.setText(""); //$NON-NLS-1$ githubTokenText.setText(""); //$NON-NLS-1$ + githubSearchScopesText.setText(""); //$NON-NLS-1$ showInlineCommentsCheckbox.setSelection(true); animateInlineCommentsCheckbox.setSelection(true); @@ -404,6 +433,8 @@ public boolean performOk() { // Save GitHub values store.setValue(PRPreferences.GITHUB_ACCESS_TOKEN, githubTokenText.getText().trim()); + store.setValue(PRPreferences.GITHUB_SEARCH_SCOPES, + githubSearchScopesText.getText().trim()); // Save display options store.setValue(PRPreferences.PULLREQUEST_SHOW_INLINE_COMMENTS, @@ -433,6 +464,7 @@ private void testGitHubConnection() { PullRequestClientFactory.ClientConfig config = new PullRequestClientFactory.ClientConfig(); config.providerType = PullRequestProviderType.GITHUB; config.githubAccessToken = githubTokenText.getText().trim(); + config.githubSearchScopes = githubSearchScopesText.getText().trim(); runConnectionTest(config); }