diff options
author | Luca Milanesio <luca.milanesio@gmail.com> | 2021-07-13 21:54:38 +0100 |
---|---|---|
committer | Luca Milanesio <luca.milanesio@gmail.com> | 2021-07-14 18:17:22 +0100 |
commit | b1242861dd00195ece9a355009f01a7fe5157ed3 (patch) | |
tree | e2ad8d65495279ba1cfea606ddfb9d0c3a086aca | |
parent | de44654afccc2fd4b5f2e263871d17e930adcb09 (diff) |
Limit returned changes for a triplet without Change-Id
When looking for a single change, do not perform a full scan
of all changes on a repository/branch as they would not identify
a single entry but rather limit the result to just 2 elements,
which would be enough to identify the response.
Fix a bug where invoking the REST-API /changes/repo~branch~ without
mentioning the change-id resulted in the full scan of all changes
on a repository/branch causing a high CPU overload and occupying
the request threads for a long time.
Requesting a single change by NOT specifying its change-id is not
a valid use-case, and it should always return a 404, regardless of
how many changes exist on the project/branch.
Bug: Issue 14785
Change-Id: Ief77d199c917fea8ebaa6ecc44a7b4b2c59d6f4e
3 files changed, 26 insertions, 2 deletions
diff --git a/java/com/google/gerrit/server/change/ChangeFinder.java b/java/com/google/gerrit/server/change/ChangeFinder.java index 71d7ba01e8..ba104d8cca 100644 --- a/java/com/google/gerrit/server/change/ChangeFinder.java +++ b/java/com/google/gerrit/server/change/ChangeFinder.java @@ -100,7 +100,9 @@ public class ChangeFinder { } public Optional<ChangeNotes> findOne(String id) { - List<ChangeNotes> ctls = find(id); + // Limit the maximum number of results to just 2 items for saving CPU cycles + // in reading change-notes. + List<ChangeNotes> ctls = find(id, 2); if (ctls.size() != 1) { return Optional.empty(); } @@ -114,6 +116,17 @@ public class ChangeFinder { * @return possibly-empty list of notes for all matching changes; may or may not be visible. */ public List<ChangeNotes> find(String id) { + return find(id, 0); + } + + /** + * Find at most N changes matching the given identifier. + * + * @param id change identifier. + * @param queryLimit maximum number of changes to be returned + * @return possibly-empty list of notes for all matching changes; may or may not be visible. + */ + public List<ChangeNotes> find(String id, int queryLimit) { if (id.isEmpty()) { return Collections.emptyList(); } @@ -141,6 +154,9 @@ public class ChangeFinder { // Use the index to search for changes, but don't return any stored fields, // to force rereading in case the index is stale. InternalChangeQuery query = queryProvider.get().noFields(); + if (queryLimit > 0) { + query.setLimit(queryLimit); + } // Try commit hash if (id.matches("^([0-9a-fA-F]{" + ObjectIds.ABBREV_STR_LEN + "," + ObjectIds.STR_LEN + "})$")) { diff --git a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java index 95b74f8595..572f704834 100644 --- a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java +++ b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java @@ -82,7 +82,7 @@ public class ChangesCollection implements RestCollection<TopLevelResource, Chang @Override public ChangeResource parse(TopLevelResource root, IdString id) throws RestApiException, PermissionBackendException, IOException { - List<ChangeNotes> notes = changeFinder.find(id.encoded()); + List<ChangeNotes> notes = changeFinder.find(id.encoded(), 2); if (notes.isEmpty()) { throw new ResourceNotFoundException(id); } else if (notes.size() != 1) { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java index bc52681fdb..06e24ab81b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java @@ -81,6 +81,14 @@ public class ChangeIdIT extends AbstractDaemonTest { } @Test + public void tripletWithoutChangeIdReturnsNotFound() throws Exception { + createChange().assertOkStatus(); + createChange().assertOkStatus(); + RestResponse res = adminRestSession.get(changeDetail(project.get() + "~master~")); + res.assertNotFound(); + } + + @Test public void changeIdReturnsChange() throws Exception { PushOneCommit.Result c = createChange(); RestResponse res = adminRestSession.get(changeDetail(c.getChangeId())); |