Fix a bug where _moreChanges is set to the wrong value
The query changes REST endpoint supports sending multiple queries in a
single request. The backend implementation uses a cache (a simple map)
to spare computing a ChangeInfo object more than once in case it shows
up in more than a single query result.
The implementation contained a bug that would set _moreChanges on the
last entity of one query that has more results than get sent. However,
through the reuse of the entity, this boolean would also end up in other
entities, where it's simply wrong.
There are multiple, alternative ways how this can be fixed:
1) Remove the cache: We could simply remove the map. This has an unknown
performance impact.
2) Deep-copy objects instead of plainly reusing them: This would add
boiler-plate to a lot of *Info objects that ChangeInfo depends on.
Hopefully we can consider moving to immutable API objects (AutoValue
or Protobuf) in the future, which would obsolete this code. It still
seems undesirable to add this now. Notably, ActionJson already
contains what looks like brittle copy logic.
3) Rework the JSON response to be a List<QueryResponse> where
QueryResponse is an object that has List<ChangeInfo> and _more: Would
be the right path design-wise, but is a non-starter because it's a
major change to a heavily used API.
4) Never reuse the last entity in a result set: In this way, the last
entity is always unique. Setting _moreChanges on it has no side
effects.
This commit implements (4), which seems like the least evil.
Change-Id: I6b007d442249d0445ea49f125c45d8f9022e104e
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index d50e740..b0477cd 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -298,7 +298,6 @@
Map<Change.Id, ChangeInfo> cache = Maps.newHashMapWithExpectedSize(in.size());
for (QueryResult<ChangeData> r : in) {
List<ChangeInfo> infos = toChangeInfos(r.entities(), cache);
- infos.forEach(c -> cache.put(Change.id(c._number), c));
if (!infos.isEmpty() && r.more()) {
infos.get(infos.size() - 1)._moreChanges = true;
}
@@ -415,15 +414,29 @@
List<ChangeData> changes, Map<Change.Id, ChangeInfo> cache) {
try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) {
List<ChangeInfo> changeInfos = new ArrayList<>(changes.size());
- for (ChangeData cd : changes) {
- ChangeInfo i = cache.get(cd.getId());
- if (i != null) {
- changeInfos.add(i);
+ for (int i = 0; i < changes.size(); i++) {
+ // We can only cache and re-use an entity if it's not the last in the list. The last entity
+ // may later get _moreChanges set. If it was cached or re-used, that setting would propagate
+ // to the original entity yielding wrong results.
+ // This problem has two sides where 'last in the list' has to be respected:
+ // (1) Caching
+ // (2) Reusing
+ boolean isCacheable = i != changes.size() - 1;
+ ChangeData cd = changes.get(i);
+ ChangeInfo info = cache.get(cd.getId());
+ if (info != null && isCacheable) {
+ changeInfos.add(info);
continue;
}
+
+ // Compute and cache if possible
try {
ensureLoaded(Collections.singleton(cd));
- changeInfos.add(format(cd, Optional.empty(), false, ChangeInfo::new));
+ info = format(cd, Optional.empty(), false, ChangeInfo::new);
+ changeInfos.add(info);
+ if (isCacheable) {
+ cache.put(Change.id(info._number), info);
+ }
} catch (RuntimeException e) {
logger.atWarning().withCause(e).log(
"Omitting corrupt change %s from results", cd.getId());
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangeIT.java
index 76166e1..92f914b 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangeIT.java
@@ -44,8 +44,8 @@
QueryChanges queryChanges = queryChangesProvider.get();
- queryChanges.addQuery("is:open");
- queryChanges.addQuery("is:wip");
+ queryChanges.addQuery("is:open repo:" + project.get());
+ queryChanges.addQuery("is:wip repo:" + project.get());
List<List<ChangeInfo>> result =
(List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
@@ -58,4 +58,48 @@
assertThat(firstResultIds).containsExactly(numericId1, numericId2);
assertThat(result.get(1).get(0)._number).isEqualTo(numericId2);
}
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void moreChangesIndicatorDoesNotWronglyCopyToUnrelatedChanges() throws Exception {
+ String queryWithMoreChanges = "is:wip limit:1 repo:" + project.get();
+ String queryWithNoMoreChanges = "is:open limit:10 repo:" + project.get();
+ createChange().getChangeId();
+ String cId2 = createChange().getChangeId();
+ String cId3 = createChange().getChangeId();
+ gApi.changes().id(cId2).setWorkInProgress();
+ gApi.changes().id(cId3).setWorkInProgress();
+
+ // Run the capped query first
+ QueryChanges queryChanges = queryChangesProvider.get();
+ queryChanges.addQuery(queryWithMoreChanges);
+ queryChanges.addQuery(queryWithNoMoreChanges);
+ List<List<ChangeInfo>> result =
+ (List<List<ChangeInfo>>) queryChanges.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result).hasSize(2);
+ assertThat(result.get(0)).hasSize(1);
+ assertThat(result.get(1)).hasSize(3);
+ // _moreChanges is set on the first response, but not on the second.
+ assertThat(result.get(0).get(0)._moreChanges).isTrue();
+ assertNoChangeHasMoreChangesSet(result.get(1));
+
+ // Run the capped query second
+ QueryChanges queryChanges2 = queryChangesProvider.get();
+ queryChanges2.addQuery(queryWithNoMoreChanges);
+ queryChanges2.addQuery(queryWithMoreChanges);
+ List<List<ChangeInfo>> result2 =
+ (List<List<ChangeInfo>>) queryChanges2.apply(TopLevelResource.INSTANCE).value();
+ assertThat(result2).hasSize(2);
+ assertThat(result2.get(0)).hasSize(3);
+ assertThat(result2.get(1)).hasSize(1);
+ // _moreChanges is set on the second response, but not on the first.
+ assertNoChangeHasMoreChangesSet(result2.get(0));
+ assertThat(result2.get(1).get(0)._moreChanges).isTrue();
+ }
+
+ private static void assertNoChangeHasMoreChangesSet(List<ChangeInfo> results) {
+ for (ChangeInfo info : results) {
+ assertThat(info._moreChanges).isNull();
+ }
+ }
}
