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
diff --git a/java/com/google/gerrit/server/change/ChangeFinder.java b/java/com/google/gerrit/server/change/ChangeFinder.java
index 71d7ba0..ba104d8 100644
--- a/java/com/google/gerrit/server/change/ChangeFinder.java
+++ b/java/com/google/gerrit/server/change/ChangeFinder.java
@@ -100,7 +100,9 @@
}
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 @@
* @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 @@
// 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 95b74f8..572f704 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 @@
@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 bc52681..06e24ab 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 @@
}
@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()));
