Merge "Fix a bug where _moreChanges is set to the wrong value"
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();
+ }
+ }
}
