From 257ae6f5c8b1849956b9ea8b540d5a23650c49c6 Mon Sep 17 00:00:00 2001 From: Philipp0205 <36603238+Philipp0205@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:06:38 +0000 Subject: [PATCH 1/2] Skip GitHub owner scopes the access token may not search Listing pull requests across every accessible repository searches one owner scope at a time, but a single rejected scope aborted the whole listing. GitHub answers GET /search/issues with HTTP 422 "The listed users and repositories cannot be searched" as soon as one qualifier names something the token may not search, which happens for organizations the token was never SSO-authorized for, for private repositories when the token lacks the 'repo' scope, and for repositories renamed or deleted after they were listed. The connection test then reported "FAILED Read pull requests" even though every other scope would have returned results. Run the per-scope searches through GitHubPullRequestSearch, which skips a scope GitHub refuses and only fails when no scope is searchable at all, in which case the message names the scopes and how to fix the token. Failures that are not scope rejections still propagate, so a server or network error is never mistaken for partial results. Organizations and collaborator repositories that cannot be listed no longer hide the user's own pull requests either, and the connection test reports the skipped scopes as a warning. Co-authored-by: Kurrle, Philipp --- .../internal/github/GitHubConnectionTest.java | 238 ++++++++++++++++++ .../github/GitHubPullRequestSearchTest.java | 216 ++++++++++++++++ .../internal/github/StubGitHubServer.java | 172 +++++++++++++ .../internal/github/GitHubClient.java | 144 +++++++---- .../internal/github/GitHubJsonParser.java | 2 +- .../github/GitHubPullRequestSearch.java | 186 ++++++++++++++ .../internal/github/GitHubSearchQueries.java | 23 ++ .../ui/PullRequestPreferencePage.java | 7 +- 8 files changed, 941 insertions(+), 47 deletions(-) create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java create mode 100644 org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java 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..69d522a --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubConnectionTest.java @@ -0,0 +1,238 @@ +/******************************************************************************* + * 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.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; + +import java.io.IOException; +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 every accessible + * owner scope when GitHub refuses to search some of them. + */ +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 StubGitHubServer server; + + @Before + public void startServer() throws IOException { + server = new StubGitHubServer(); + server.on("/user", ok("{\"login\":\"Philipp0205\"}")); //$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$ + server.on("/repos/Philipp0205/notes/pulls/1", //$NON-NLS-1$ + ok(pullRequest(1, "Fix the parser"))); //$NON-NLS-1$ + server.on("/repos/acme/app/pulls/4", //$NON-NLS-1$ + ok(pullRequest(4, "Bump the target platform"))); //$NON-NLS-1$ + } + + @After + public void stopServer() throws IOException { + server.close(); + } + + @Test + public void testScopeGitHubRefusesToSearchIsSkipped() throws Exception { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + + List pullRequests = client() + .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 { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + assertThat(report.toReport(), report.isSuccessful(), is(true)); + 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$ + assertThat(scopes.getDetail(), containsString("read:org")); //$NON-NLS-1$ + } + + @Test + public void testDiagnosticsStaySilentWhenEveryScopeIsSearchable() + throws Exception { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + assertThat(report.toReport(), report.isSuccessful(), is(true)); + assertThat(report.toReport(), not(containsString("Search scopes"))); //$NON-NLS-1$ + } + + @Test + public void testEveryScopeRefusedIsReportedWithTokenAdvice() { + searchFailsWithValidationError("user%3APhilipp0205"); //$NON-NLS-1$ + searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + + ConnectionDiagnostics report = client().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:Philipp0205")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("org:acme")); //$NON-NLS-1$ + assertThat(read.getDetail(), containsString("read:org")); //$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$ + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + + List pullRequests = client() + .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() { + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + server.onQueryContaining("org%3Aacme", //$NON-NLS-1$ + new Response(503, "Service Unavailable", //$NON-NLS-1$ + "{\"message\":\"No server is currently available\"}")); //$NON-NLS-1$ + + ConnectionDiagnostics report = client().diagnoseConnection(); + + 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 testEveryOwnerScopeIsSearchedSeparately() throws Exception { + server.on("/user/repos", //$NON-NLS-1$ + ok("[{\"full_name\":\"bob/tool\"}]")); //$NON-NLS-1$ + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ + searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + searchFailsWithValidationError("repo%3Abob%2Ftool"); //$NON-NLS-1$ + + List pullRequests = client() + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(2)); + assertThat(searchTargets(), hasSize(3)); + } + + @Test + public void testPullRequestFoundInTwoScopesIsListedOnce() + throws Exception { + server.on("/user/repos", //$NON-NLS-1$ + ok("[{\"full_name\":\"acme/app\"}]")); //$NON-NLS-1$ + searchReturns("user%3APhilipp0205", //$NON-NLS-1$ + "/repos/acme/app/pulls/4"); //$NON-NLS-1$ + searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + + List pullRequests = client() + .getPullRequests(null, null, null, 25, 0); + + assertThat(pullRequests, hasSize(1)); + assertThat(pullRequests.get(0).getTitle(), + equalTo("Bump the target platform")); //$NON-NLS-1$ + } + + private GitHubClient client() { + return new GitHubClient(null, null, "token", server.url()); //$NON-NLS-1$ + } + + private void searchReturns(String encodedScope, String... pullPaths) { + StringBuilder items = new StringBuilder(); + for (String pullPath : pullPaths) { + if (items.length() > 0) { + items.append(','); + } + items.append("{\"pull_request\":{\"url\":\"") //$NON-NLS-1$ + .append(server.url()).append(pullPath).append("\"}}"); //$NON-NLS-1$ + } + server.onQueryContaining(encodedScope, + ok("{\"total_count\":" + pullPaths.length + ",\"items\":[" //$NON-NLS-1$ //$NON-NLS-2$ + + items + "]}")); //$NON-NLS-1$ + } + + private void searchFailsWithValidationError(String encodedScope) { + server.onQueryContaining(encodedScope, new Response(422, + "Unprocessable Entity", VALIDATION_FAILED)); //$NON-NLS-1$ + } + + private List searchTargets() { + return server.requestedTargets().stream() + .filter(target -> target.startsWith("/search/issues")) //$NON-NLS-1$ + .toList(); + } + + private static Response ok(String body) { + return new Response(200, "OK", body); //$NON-NLS-1$ + } + + private static String pullRequest(int number, String title) { + return "{\"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-0" + number //$NON-NLS-1$ + + "T10:00:00Z\",\"user\":{\"login\":\"Philipp0205\"}}"; //$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..57e5979 --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearchTest.java @@ -0,0 +1,216 @@ +/******************************************************************************* + * 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.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.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 final List requestedQueries = new ArrayList<>(); + + /** + * A reader that answers each query from a canned list of paths and fails + * every query that was registered as rejected. + */ + private class StubReader implements PageReader { + + private final Map> results = new HashMap<>(); + + private final Map failures = new HashMap<>(); + + StubReader returning(String query, String... paths) { + results.put(query, Arrays.asList(paths)); + 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()); + } + } + + @Test + public void testRejectedScopeIsSkippedAndOthersStillReturnResults() + 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("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open", //$NON-NLS-1$ + "is:pr repo:bob/tool is:open"), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(result.getPullRequestPaths(), + 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 testRejectedScopeDoesNotStopLaterQueries() 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("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open"), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(requestedQueries, hasSize(2)); + } + + @Test + public void testAllScopesRejectedFailsWithActionableMessage() { + 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("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open"), //$NON-NLS-1$ + 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 testFailuresOtherThanRejectedScopesArePropagated() { + 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("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr org:acme is:open"), //$NON-NLS-1$ + 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("is:pr user:alice is:open", //$NON-NLS-1$ + "is:pr repo:alice/notes is:open"), //$NON-NLS-1$ + 1, 1, reader); + + assertThat(result.getPullRequestPaths(), + 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("is:pr user:alice is:open"), 1, 1, reader); //$NON-NLS-1$ + + assertThat(result.getUnsearchableScopes(), empty()); + } + + @Test + public void testValidationFailureIsRecognizedAsUnsearchableScope() { + assertThat( + GitHubPullRequestSearch.isUnsearchableScope( + new IOException(VALIDATION_FAILED)), + equalTo(true)); + assertThat( + GitHubPullRequestSearch.isUnsearchableScope(new IOException( + "GitHub API request failed: HTTP 401 - Bad token")), //$NON-NLS-1$ + equalTo(false)); + assertThat(GitHubPullRequestSearch + .isUnsearchableScope(new IOException()), equalTo(false)); + } + + @Test + public void testScopeOfExtractsTheOwnerQualifier() { + assertThat(GitHubSearchQueries.scopeOf("is:pr user:alice is:open"), //$NON-NLS-1$ + equalTo("user:alice")); //$NON-NLS-1$ + assertThat( + GitHubSearchQueries.scopeOf( + "is:pr org:acme author:carol is:merged"), //$NON-NLS-1$ + equalTo("org:acme")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries + .scopeOf("is:pr repo:bob/tool is:closed is:unmerged"), //$NON-NLS-1$ + equalTo("repo:bob/tool")); //$NON-NLS-1$ + assertThat(GitHubSearchQueries.scopeOf("is:pr"), equalTo("is:pr")); //$NON-NLS-1$ //$NON-NLS-2$ + } +} 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..441fc4f --- /dev/null +++ b/org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/StubGitHubServer.java @@ -0,0 +1,172 @@ +/******************************************************************************* + * 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.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 either for a request path or for a fragment of the + * query string, so that the different searches sent to + * {@code /search/issues} can be answered individually. + */ +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 final ServerSocket serverSocket; + + private final Map byPath = new ConcurrentHashMap<>(); + + private final Map byQuery = new ConcurrentHashMap<>(); + + private final List requestedTargets = 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 every request whose target contains the given + * fragment. Checked before the path responses. + * + * @param fragment + * a fragment of the encoded request target + * @param response + * the response to send + */ + void onQueryContaining(String fragment, Response response) { + byQuery.put(fragment, response); + } + + /** + * @return the request targets that were received, in order + */ + List requestedTargets() { + return requestedTargets; + } + + @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$ + requestedTargets.add(target); + write(client.getOutputStream(), responseFor(target)); + } + + private Response responseFor(String target) { + for (Map.Entry entry : byQuery.entrySet()) { + if (target.contains(entry.getKey())) { + return entry.getValue(); + } + } + int query = target.indexOf('?'); + String path = query < 0 ? target : target.substring(0, query); + 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 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/github/GitHubClient.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubClient.java index 3280fea..66837c4 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,16 @@ public class GitHubClient implements IPullRequestClient { private final String token; + private final String apiBaseUrl; + private final PullRequestProviderCapabilities capabilities; + /** + * 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 +65,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); } /** @@ -73,9 +76,28 @@ 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); + } + + /** + * 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 + */ + GitHubClient(String owner, String repo, @NonNull String token, + @NonNull String apiBaseUrl) { + this.owner = owner; + this.repo = repo; this.token = token; + this.apiBaseUrl = apiBaseUrl; this.capabilities = PullRequestProviderCapabilities .forProvider(PullRequestProviderType.GITHUB); } @@ -205,32 +227,15 @@ private List getUserPullRequests(String state, 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 + .parseSearchPullRequestPaths( + doGet(searchPath(query, page, size)))); + unsearchableScopes = search.getUnsearchableScopes(); List result = new ArrayList<>(); - for (String pullRequestPath : pullRequestPaths) { + for (String pullRequestPath : search.getPullRequestPaths()) { PullRequest pullRequest = GitHubJsonParser .parseSinglePullRequest(doGet(pullRequestPath)); if (pullRequest != null) { @@ -244,19 +249,56 @@ private List getUserPullRequests(String state, return new ArrayList<>(result.subList(from, to)); } - private List listOrganizationLogins() throws IOException { + 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$ + } + + /** + * 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 +858,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 +937,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 +1190,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 +1240,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..e00e653 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 @@ -26,7 +26,7 @@ class GitHubJsonParser { 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$ + + "\"https?://[^\"]+?(/repos/[^\"?]+)\"", //$NON-NLS-1$ Pattern.DOTALL); /** 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..4e01e50 --- /dev/null +++ b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubPullRequestSearch.java @@ -0,0 +1,186 @@ +/******************************************************************************* + * 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.ArrayList; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; + +import org.eclipse.egit.pullrequest.Activator; + +/** + * Runs one GitHub issue search per owner scope and merges the results. + *

+ * 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. Because every scope is searched with its own query, such a + * scope is skipped instead of failing the whole listing; only when GitHub + * rejects every scope is the failure reported to the caller. + */ +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$ + + private GitHubPullRequestSearch() { + // No instantiation + } + + /** + * Reads one page of pull request paths for a single search 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 REST pull request paths in result order + * @throws IOException + * if the request fails + */ + List read(String query, int page, int pageSize) + throws IOException; + } + + /** Merged outcome of searching every owner scope. */ + static final class Result { + + private final List pullRequestPaths; + + private final List unsearchableScopes; + + Result(List pullRequestPaths, + List unsearchableScopes) { + this.pullRequestPaths = pullRequestPaths; + this.unsearchableScopes = unsearchableScopes; + } + + /** + * @return REST pull request paths without duplicates + */ + List getPullRequestPaths() { + return pullRequestPaths; + } + + /** + * @return the owner qualifiers GitHub refused to search + */ + List getUnsearchableScopes() { + return unsearchableScopes; + } + } + + /** + * Searches every query and merges the pull requests they return. + * + * @param queries + * one search query per owner scope + * @param needed + * how many results are needed per scope + * @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 an unsearchable + * scope, or if GitHub refuses to search every scope + */ + static Result run(List queries, int needed, int pageSize, + PageReader reader) throws IOException { + LinkedHashSet paths = new LinkedHashSet<>(); + List unsearchable = new ArrayList<>(); + IOException lastRejection = null; + for (String query : queries) { + try { + collect(query, needed, pageSize, reader, paths); + } catch (IOException e) { + if (!isUnsearchableScope(e)) { + throw e; + } + String scope = GitHubSearchQueries.scopeOf(query); + 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() == queries.size()) { + throw new IOException(allScopesRejected(unsearchable), + lastRejection); + } + return new Result(new ArrayList<>(paths), + Collections.unmodifiableList(unsearchable)); + } + + private static void collect(String query, int needed, int pageSize, + PageReader reader, LinkedHashSet paths) + throws IOException { + int collected = 0; + int page = 1; + while (collected < needed) { + List pagePaths = reader.read(query, page, pageSize); + if (pagePaths.isEmpty()) { + return; + } + paths.addAll(pagePaths); + collected += pagePaths.size(); + if (pagePaths.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 isUnsearchableScope(IOException e) { + String message = e.getMessage(); + return message != null && message.contains("HTTP 422"); //$NON-NLS-1$ + } + + /** + * Builds the message for the case where no scope could be searched at all. + * + * @param unsearchableScopes + * the owner qualifiers GitHub refused to search + * @return an actionable error message + */ + static String allScopesRejected(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/GitHubSearchQueries.java b/org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueries.java index 8708de8..55bdcdc 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 @@ -96,6 +96,29 @@ static String pullRequestQuery(String ownerQualifier, String state, return query.toString(); } + /** + * Returns the owner qualifier a query was built for. + * + * @param query + * a query produced by + * {@link #pullRequestQuery(String, String, String)} + * @return the {@code user:}, {@code org:} or {@code repo:} qualifier, or + * the whole query if it carries none + */ + static String scopeOf(String query) { + if (query == null) { + return ""; //$NON-NLS-1$ + } + for (String token : query.split(" ")) { //$NON-NLS-1$ + if (token.startsWith("user:") //$NON-NLS-1$ + || token.startsWith("org:") //$NON-NLS-1$ + || token.startsWith("repo:")) { //$NON-NLS-1$ + return token; + } + } + return query; + } + static boolean isCoveredByUserOrOrg(String fullName, String login, Set organizations) { if (fullName == null || fullName.isBlank()) { 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..6e4739b 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 @@ -230,10 +230,13 @@ 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$ + + "you own or belong to. Organizations that enforce " //$NON-NLS-1$ + + "SAML single sign-on are only searched once the token " //$NON-NLS-1$ + + "is authorized for them."); //$NON-NLS-1$ GridDataFactory.fillDefaults().span(2, 1).hint(400, SWT.DEFAULT) .indent(0, 5).applyTo(infoLabel); From 48427910842fba7b7a83654d9b43323b686fafb0 Mon Sep 17 00:00:00 2001 From: Philipp0205 <36603238+Philipp0205@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:51:22 +0000 Subject: [PATCH 2/2] Let the searched GitHub owners be configured, and stop flooding the API Two problems showed up once a token could reach many repositories. The listing searched every reachable owner scope and then read every pull request any of those searches returned, one request each, before sorting and cutting the result down to the requested page. A token with sixty repositories therefore spent sixty search requests and six hundred detail requests on a single refresh and ran into "API rate limit exceeded". The search response already says when each pull request was last updated, so order and page the results first and read only that page in full: the same case now costs three searches and a hundred detail requests. Owner scopes are also batched into shared queries, since GitHub combines repeated qualifiers of the same kind with OR. A batch GitHub refuses is retried scope by scope so that only the truly unsearchable scope is dropped. Which pull requests appear was also decided entirely by whichever account the token belongs to, with no way to narrow it down. Add a "Search only" preference listing the owners and repositories to search; when it is set the plugin searches exactly those and skips discovering organizations and collaborator repositories altogether. A bare name covers everything a user or organization owns, and owner/name covers a single repository. Rate limit responses are now recognized as such: they abort the listing with an explanation and a pointer to the preference, instead of the raw JSON error, and the remaining scopes are not retried into the same wall. Co-authored-by: Kurrle, Philipp --- .../internal/github/GitHubClientTest.java | 63 ++-- .../internal/github/GitHubConnectionTest.java | 296 +++++++++++++----- .../github/GitHubPullRequestSearchTest.java | 160 +++++++--- .../github/GitHubSearchQueriesTest.java | 128 ++++++++ .../internal/github/StubGitHubServer.java | 86 +++-- .../pullrequest/internal/PRPreferences.java | 6 + .../client/PullRequestClientFactory.java | 11 +- .../internal/github/GitHubClient.java | 111 +++++-- .../internal/github/GitHubJsonParser.java | 42 ++- .../github/GitHubPullRequestSearch.java | 138 +++++--- .../internal/github/GitHubSearchHit.java | 30 ++ .../internal/github/GitHubSearchQueries.java | 195 +++++++++--- .../ui/PullRequestPreferencePage.java | 37 ++- 13 files changed, 1029 insertions(+), 274 deletions(-) create mode 100644 org.eclipse.egit.pullrequest.test/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchQueriesTest.java create mode 100644 org.eclipse.egit.pullrequest/src/org/eclipse/egit/pullrequest/internal/github/GitHubSearchHit.java 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 index 69d522a..34c8f91 100644 --- 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 @@ -11,13 +11,18 @@ 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; @@ -30,8 +35,8 @@ import org.junit.Test; /** - * Tests how {@link GitHubClient} lists pull requests across every accessible - * owner scope when GitHub refuses to search some of them. + * Tests how {@link GitHubClient} lists pull requests across the scopes it is + * allowed to search, and how few requests that takes. */ public class GitHubConnectionTest { @@ -42,18 +47,17 @@ public class GitHubConnectionTest { + "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\":\"Philipp0205\"}")); //$NON-NLS-1$ //$NON-NLS-2$ + 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$ - server.on("/repos/Philipp0205/notes/pulls/1", //$NON-NLS-1$ - ok(pullRequest(1, "Fix the parser"))); //$NON-NLS-1$ - server.on("/repos/acme/app/pulls/4", //$NON-NLS-1$ - ok(pullRequest(4, "Bump the target platform"))); //$NON-NLS-1$ } @After @@ -62,45 +66,157 @@ public void stopServer() throws IOException { } @Test - public void testScopeGitHubRefusesToSearchIsSkipped() throws Exception { - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ - searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + 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() + 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 testDiagnosticsReportSkippedScopeAsWarningNotFailure() + public void testConfiguredScopesAcceptOwnersAndSingleRepositories() throws Exception { - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ - searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + 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$ + } - ConnectionDiagnostics report = client().diagnoseConnection(); + 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$ - assertThat(scopes.getDetail(), containsString("read:org")); //$NON-NLS-1$ } @Test public void testDiagnosticsStaySilentWhenEveryScopeIsSearchable() throws Exception { - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ - searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + 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().diagnoseConnection(); + ConnectionDiagnostics report = client(null).diagnoseConnection(); assertThat(report.toReport(), report.isSuccessful(), is(true)); assertThat(report.toReport(), not(containsString("Search scopes"))); //$NON-NLS-1$ @@ -108,28 +224,47 @@ public void testDiagnosticsStaySilentWhenEveryScopeIsSearchable() @Test public void testEveryScopeRefusedIsReportedWithTokenAdvice() { - searchFailsWithValidationError("user%3APhilipp0205"); //$NON-NLS-1$ - searchFailsWithValidationError("org%3Aacme"); //$NON-NLS-1$ + 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().diagnoseConnection(); + 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:Philipp0205")); //$NON-NLS-1$ - assertThat(read.getDetail(), containsString("org:acme")); //$NON-NLS-1$ + 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$ - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/Philipp0205/notes/pulls/1"); //$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() + List pullRequests = client(null) .getPullRequests(null, null, null, 25, 0); assertThat(pullRequests, hasSize(1)); @@ -139,93 +274,92 @@ public void testOrganizationsThatCannotBeListedDoNotHideOwnPullRequests() @Test public void testSearchFailuresOtherThanRefusedScopesAreReported() { - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ - server.onQueryContaining("org%3Aacme", //$NON-NLS-1$ + server.onSearch("is:pr user:alice is:open", //$NON-NLS-1$ new Response(503, "Service Unavailable", //$NON-NLS-1$ - "{\"message\":\"No server is currently available\"}")); //$NON-NLS-1$ + "{\"message\":\"No server is available\"}")); //$NON-NLS-1$ - ConnectionDiagnostics report = client().diagnoseConnection(); + 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 testEveryOwnerScopeIsSearchedSeparately() throws Exception { - server.on("/user/repos", //$NON-NLS-1$ - ok("[{\"full_name\":\"bob/tool\"}]")); //$NON-NLS-1$ - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/Philipp0205/notes/pulls/1"); //$NON-NLS-1$ - searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ - searchFailsWithValidationError("repo%3Abob%2Ftool"); //$NON-NLS-1$ - - List pullRequests = client() - .getPullRequests(null, null, null, 25, 0); - - assertThat(pullRequests, hasSize(2)); - assertThat(searchTargets(), hasSize(3)); - } - @Test public void testPullRequestFoundInTwoScopesIsListedOnce() throws Exception { - server.on("/user/repos", //$NON-NLS-1$ - ok("[{\"full_name\":\"acme/app\"}]")); //$NON-NLS-1$ - searchReturns("user%3APhilipp0205", //$NON-NLS-1$ - "/repos/acme/app/pulls/4"); //$NON-NLS-1$ - searchReturns("org%3Aacme", "/repos/acme/app/pulls/4"); //$NON-NLS-1$ //$NON-NLS-2$ + 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() + List pullRequests = client("acme, acme/app") //$NON-NLS-1$ .getPullRequests(null, null, null, 25, 0); assertThat(pullRequests, hasSize(1)); - assertThat(pullRequests.get(0).getTitle(), - equalTo("Bump the target platform")); //$NON-NLS-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 GitHubClient client() { - return new GitHubClient(null, null, "token", server.url()); //$NON-NLS-1$ + 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 void searchReturns(String encodedScope, String... pullPaths) { + private Response searchResult(String... pullPaths) { StringBuilder items = new StringBuilder(); + int index = 0; for (String pullPath : pullPaths) { if (items.length() > 0) { items.append(','); } - items.append("{\"pull_request\":{\"url\":\"") //$NON-NLS-1$ + 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++; } - server.onQueryContaining(encodedScope, - ok("{\"total_count\":" + pullPaths.length + ",\"items\":[" //$NON-NLS-1$ //$NON-NLS-2$ - + items + "]}")); //$NON-NLS-1$ + return ok("{\"total_count\":" + pullPaths.length + ",\"items\":[" //$NON-NLS-1$ //$NON-NLS-2$ + + items + "]}"); //$NON-NLS-1$ } - private void searchFailsWithValidationError(String encodedScope) { - server.onQueryContaining(encodedScope, new Response(422, - "Unprocessable Entity", VALIDATION_FAILED)); //$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 List searchTargets() { - return server.requestedTargets().stream() - .filter(target -> target.startsWith("/search/issues")) //$NON-NLS-1$ - .toList(); + 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 String pullRequest(int number, String title) { - return "{\"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-0" + number //$NON-NLS-1$ - + "T10:00:00Z\",\"user\":{\"login\":\"Philipp0205\"}}"; //$NON-NLS-1$ - } - private static Step step(ConnectionDiagnostics report, String name) { for (Step step : report.getSteps()) { if (step.getName().equals(name)) { 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 index 57e5979..be66bfa 100644 --- 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 @@ -22,12 +22,14 @@ 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; /** @@ -42,20 +44,29 @@ public class GitHubPullRequestSearchTest { + "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 a canned list of paths and fails - * every query that was registered as rejected. + * 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> results = + new HashMap<>(); private final Map failures = new HashMap<>(); StubReader returning(String query, String... paths) { - results.put(query, Arrays.asList(paths)); + List hits = new ArrayList<>(); + for (String path : paths) { + hits.add(new GitHubSearchHit(path, new Date(0))); + } + results.put(query, hits); return this; } @@ -65,8 +76,8 @@ StubReader failingWith(String query, String message) { } @Override - public List read(String query, int page, int pageSize) - throws IOException { + public List read(String query, int page, + int pageSize) throws IOException { requestedQueries.add(query); IOException failure = failures.get(query); if (failure != null) { @@ -79,8 +90,20 @@ public List read(String query, int page, int pageSize) } } + 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 testRejectedScopeIsSkippedAndOthersStillReturnResults() + public void testRefusedScopeIsSkippedAndOthersStillReturnResults() throws Exception { StubReader reader = new StubReader() .returning("is:pr user:alice is:open", //$NON-NLS-1$ @@ -90,42 +113,75 @@ public void testRejectedScopeIsSkippedAndOthersStillReturnResults() "/repos/bob/tool/pulls/9"); //$NON-NLS-1$ Result result = GitHubPullRequestSearch.run( - Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ - "is:pr org:acme is:open", //$NON-NLS-1$ - "is:pr repo:bob/tool is:open"), //$NON-NLS-1$ + 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(result.getPullRequestPaths(), - contains("/repos/alice/notes/pulls/1", //$NON-NLS-1$ - "/repos/bob/tool/pulls/9")); //$NON-NLS-1$ + 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 testRejectedScopeDoesNotStopLaterQueries() throws Exception { + 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("is:pr user:alice is:open", //$NON-NLS-1$ - "is:pr org:acme is:open"), //$NON-NLS-1$ + Arrays.asList(query("user:alice"), query("org:acme")), //$NON-NLS-1$ //$NON-NLS-2$ 1, 1, reader); assertThat(requestedQueries, hasSize(2)); } @Test - public void testAllScopesRejectedFailsWithActionableMessage() { + 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("is:pr user:alice is:open", //$NON-NLS-1$ - "is:pr org:acme is:open"), //$NON-NLS-1$ + 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) { @@ -138,7 +194,27 @@ public void testAllScopesRejectedFailsWithActionableMessage() { } @Test - public void testFailuresOtherThanRejectedScopesArePropagated() { + 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$ @@ -147,8 +223,7 @@ public void testFailuresOtherThanRejectedScopesArePropagated() { try { GitHubPullRequestSearch.run( - Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ - "is:pr org:acme is:open"), //$NON-NLS-1$ + 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) { @@ -166,11 +241,11 @@ public void testPullRequestFoundByTwoScopesIsReportedOnce() "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ Result result = GitHubPullRequestSearch.run( - Arrays.asList("is:pr user:alice is:open", //$NON-NLS-1$ - "is:pr repo:alice/notes is:open"), //$NON-NLS-1$ + Arrays.asList(query("user:alice"), //$NON-NLS-1$ + query("repo:alice/notes")), //$NON-NLS-1$ 1, 1, reader); - assertThat(result.getPullRequestPaths(), + assertThat(paths(result), contains("/repos/alice/notes/pulls/1")); //$NON-NLS-1$ } @@ -181,36 +256,39 @@ public void testSuccessfulSearchReportsNoUnsearchableScopes() "is:pr user:alice is:open", "/repos/alice/notes/pulls/1"); //$NON-NLS-1$ //$NON-NLS-2$ Result result = GitHubPullRequestSearch.run( - Arrays.asList("is:pr user:alice is:open"), 1, 1, reader); //$NON-NLS-1$ + Arrays.asList(query("user:alice")), 1, 1, reader); //$NON-NLS-1$ assertThat(result.getUnsearchableScopes(), empty()); } @Test - public void testValidationFailureIsRecognizedAsUnsearchableScope() { + public void testValidationFailureIsRecognizedAsRefusedScope() { assertThat( - GitHubPullRequestSearch.isUnsearchableScope( + GitHubPullRequestSearch.isRefusedScope( new IOException(VALIDATION_FAILED)), equalTo(true)); assertThat( - GitHubPullRequestSearch.isUnsearchableScope(new IOException( + GitHubPullRequestSearch.isRefusedScope(new IOException( "GitHub API request failed: HTTP 401 - Bad token")), //$NON-NLS-1$ equalTo(false)); - assertThat(GitHubPullRequestSearch - .isUnsearchableScope(new IOException()), equalTo(false)); + assertThat( + GitHubPullRequestSearch.isRefusedScope(new IOException()), + equalTo(false)); } @Test - public void testScopeOfExtractsTheOwnerQualifier() { - assertThat(GitHubSearchQueries.scopeOf("is:pr user:alice is:open"), //$NON-NLS-1$ - equalTo("user:alice")); //$NON-NLS-1$ + 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( - GitHubSearchQueries.scopeOf( - "is:pr org:acme author:carol is:merged"), //$NON-NLS-1$ - equalTo("org:acme")); //$NON-NLS-1$ - assertThat(GitHubSearchQueries - .scopeOf("is:pr repo:bob/tool is:closed is:unmerged"), //$NON-NLS-1$ - equalTo("repo:bob/tool")); //$NON-NLS-1$ - assertThat(GitHubSearchQueries.scopeOf("is:pr"), equalTo("is:pr")); //$NON-NLS-1$ //$NON-NLS-2$ + 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 index 441fc4f..9366385 100644 --- 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 @@ -17,6 +17,7 @@ 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; @@ -27,9 +28,9 @@ * Minimal HTTP server on the loopback interface that answers canned responses, * used to exercise {@link GitHubClient} without talking to github.com. *

- * Responses are registered either for a request path or for a fragment of the - * query string, so that the different searches sent to - * {@code /search/issues} can be answered individually. + * 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 { @@ -49,13 +50,19 @@ static class Response { } } + private static final String SEARCH_PATH = "/search/issues"; //$NON-NLS-1$ + private final ServerSocket serverSocket; private final Map byPath = new ConcurrentHashMap<>(); - private final Map byQuery = new ConcurrentHashMap<>(); + private final Map bySearch = new ConcurrentHashMap<>(); + + private volatile Response anySearch; + + private final List requestedPaths = new CopyOnWriteArrayList<>(); - private final List requestedTargets = new CopyOnWriteArrayList<>(); + private final List searchQueries = new CopyOnWriteArrayList<>(); private volatile boolean running = true; @@ -88,23 +95,40 @@ void on(String path, Response response) { } /** - * Registers the response for every request whose target contains the given - * fragment. Checked before the path responses. + * 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 fragment - * a fragment of the encoded request target * @param response * the response to send */ - void onQueryContaining(String fragment, Response response) { - byQuery.put(fragment, response); + void onAnySearch(Response response) { + anySearch = response; } /** - * @return the request targets that were received, in order + * @return the paths that were requested, in order, without their query + * strings */ - List requestedTargets() { - return requestedTargets; + List requestedPaths() { + return requestedPaths; + } + + /** + * @return the decoded search queries that were sent, in order + */ + List searchQueries() { + return searchQueries; } @Override @@ -136,18 +160,29 @@ private void handle(Socket client) throws IOException { } String[] parts = requestLine.split(" "); //$NON-NLS-1$ String target = parts.length > 1 ? parts[1] : ""; //$NON-NLS-1$ - requestedTargets.add(target); write(client.getOutputStream(), responseFor(target)); } private Response responseFor(String target) { - for (Map.Entry entry : byQuery.entrySet()) { - if (target.contains(entry.getKey())) { - return entry.getValue(); + 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$ } - int query = target.indexOf('?'); - String path = query < 0 ? target : target.substring(0, query); + Response response = byPath.get(path); if (response != null) { return response; @@ -156,6 +191,17 @@ private Response responseFor(String target) { "{\"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); 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 66837c4..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 @@ -47,6 +47,12 @@ public class GitHubClient implements IPullRequestClient { 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()}. @@ -65,7 +71,7 @@ public class GitHubClient implements IPullRequestClient { */ public GitHubClient(@NonNull String owner, @NonNull String repo, @NonNull String token) { - this(owner, repo, token, API_BASE_URL); + this(owner, repo, token, API_BASE_URL, List.of()); } /** @@ -76,7 +82,30 @@ public GitHubClient(@NonNull String owner, @NonNull String repo, * the GitHub access token */ public GitHubClient(@NonNull String token) { - this(null, null, token, API_BASE_URL); + 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)); } /** @@ -91,13 +120,16 @@ public GitHubClient(@NonNull String 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 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); } @@ -217,36 +249,79 @@ 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); GitHubPullRequestSearch.Result search = GitHubPullRequestSearch.run( queries, needed, pageSize, - (query, page, size) -> GitHubJsonParser - .parseSearchPullRequestPaths( - doGet(searchPath(query, page, size)))); + (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 : search.getPullRequestPaths()) { + 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) { 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 e00e653..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?://[^\"]+?(/repos/[^\"?]+)\"", //$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 index 4e01e50..df547cd 100644 --- 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 @@ -11,24 +11,29 @@ 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.LinkedHashSet; +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; /** - * Runs one GitHub issue search per owner scope and merges the results. + * 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. Because every scope is searched with its own query, such a - * scope is skipped instead of failing the whole listing; only when GitHub - * rejects every scope is the failure reported to the caller. + * 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 { @@ -39,12 +44,18 @@ final class GitHubPullRequestSearch { + "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 pull request paths for a single search query. + * Reads one page of search results for a single query. */ @FunctionalInterface interface PageReader { @@ -58,32 +69,31 @@ interface PageReader { * the one-based page number * @param pageSize * the number of results per page - * @return REST pull request paths in result order + * @return the pull requests on that page, in result order * @throws IOException * if the request fails */ - List read(String query, int page, int pageSize) + List read(String query, int page, int pageSize) throws IOException; } - /** Merged outcome of searching every owner scope. */ + /** Merged outcome of searching every scope. */ static final class Result { - private final List pullRequestPaths; + private final List hits; private final List unsearchableScopes; - Result(List pullRequestPaths, - List unsearchableScopes) { - this.pullRequestPaths = pullRequestPaths; + Result(List hits, List unsearchableScopes) { + this.hits = hits; this.unsearchableScopes = unsearchableScopes; } /** - * @return REST pull request paths without duplicates + * @return the found pull requests, without duplicates */ - List getPullRequestPaths() { - return pullRequestPaths; + List getHits() { + return hits; } /** @@ -95,63 +105,85 @@ List getUnsearchableScopes() { } /** - * Searches every query and merges the pull requests they return. + * Runs every query and merges the pull requests they return. * * @param queries - * one search query per owner scope + * the queries covering the scopes to search * @param needed - * how many results are needed per scope + * 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 an unsearchable + * 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, + static Result run(List queries, int needed, int pageSize, PageReader reader) throws IOException { - LinkedHashSet paths = new LinkedHashSet<>(); + 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; - for (String query : queries) { + while (!pending.isEmpty()) { + Query query = pending.removeFirst(); try { - collect(query, needed, pageSize, reader, paths); + collect(query.getText(), needed, pageSize, reader, hits); } catch (IOException e) { - if (!isUnsearchableScope(e)) { + if (isRateLimited(e)) { + throw new IOException(RATE_LIMIT_HINT + " " //$NON-NLS-1$ + + e.getMessage(), e); + } + if (!isRefusedScope(e)) { throw e; } - String scope = GitHubSearchQueries.scopeOf(query); + 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()); + 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() == queries.size()) { - throw new IOException(allScopesRejected(unsearchable), + + if (lastRejection != null && unsearchable.size() == scopeCount) { + throw new IOException(allScopesRefused(unsearchable), lastRejection); } - return new Result(new ArrayList<>(paths), + return new Result(new ArrayList<>(hits.values()), Collections.unmodifiableList(unsearchable)); } private static void collect(String query, int needed, int pageSize, - PageReader reader, LinkedHashSet paths) + PageReader reader, Map hits) throws IOException { int collected = 0; int page = 1; while (collected < needed) { - List pagePaths = reader.read(query, page, pageSize); - if (pagePaths.isEmpty()) { + List pageHits = reader.read(query, page, + pageSize); + if (pageHits.isEmpty()) { return; } - paths.addAll(pagePaths); - collected += pagePaths.size(); - if (pagePaths.size() < pageSize) { + for (GitHubSearchHit hit : pageHits) { + hits.putIfAbsent(hit.path(), hit); + } + collected += pageHits.size(); + if (pageHits.size() < pageSize) { return; } page++; @@ -166,19 +198,41 @@ private static void collect(String query, int needed, int pageSize, * the failure reported by the search request * @return true if the scope should be skipped rather than propagated */ - static boolean isUnsearchableScope(IOException e) { + static boolean isRefusedScope(IOException e) { String message = e.getMessage(); return message != null && message.contains("HTTP 422"); //$NON-NLS-1$ } /** - * Builds the message for the case where no scope could be searched at all. + * 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 allScopesRejected(List unsearchableScopes) { + 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 55bdcdc..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 queries; + 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 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, @@ -96,29 +242,6 @@ static String pullRequestQuery(String ownerQualifier, String state, return query.toString(); } - /** - * Returns the owner qualifier a query was built for. - * - * @param query - * a query produced by - * {@link #pullRequestQuery(String, String, String)} - * @return the {@code user:}, {@code org:} or {@code repo:} qualifier, or - * the whole query if it carries none - */ - static String scopeOf(String query) { - if (query == null) { - return ""; //$NON-NLS-1$ - } - for (String token : query.split(" ")) { //$NON-NLS-1$ - if (token.startsWith("user:") //$NON-NLS-1$ - || token.startsWith("org:") //$NON-NLS-1$ - || token.startsWith("repo:")) { //$NON-NLS-1$ - return token; - } - } - return query; - } - static boolean isCoveredByUserOrOrg(String fullName, String login, Set organizations) { if (fullName == null || fullName.isBlank()) { 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 6e4739b..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; @@ -233,13 +235,34 @@ private Composite createGitHubConfiguration(Composite parent) { + "(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. Organizations that enforce " //$NON-NLS-1$ - + "SAML single sign-on are only searched once the token " //$NON-NLS-1$ - + "is authorized for them."); //$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$ @@ -349,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 @@ -372,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); @@ -407,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, @@ -436,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); }