opendev/gerrit
Code Issues Proposed changes

Merge "Fix a bug where _moreChanges is set to the wrong value" into stable-2.16

Browse Source
This commit is contained in:
David Pursehouse
2020-01-15 23:37:47 +00:00
committed by Gerrit Code Review
parent 522ffdfaad 57993866d8
commit f50bcf0df0
1 changed files with 19 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -296,7 +296,6 @@ public class ChangeJson {
Map<Change.Id, ChangeInfo> cache = Maps.newHashMapWithExpectedSize(in.size()); Map<Change.Id, ChangeInfo> cache = Maps.newHashMapWithExpectedSize(in.size());
for (QueryResult<ChangeData> r : in) { for (QueryResult<ChangeData> r : in) {
List<ChangeInfo> infos = toChangeInfos(r.entities(), cache); List<ChangeInfo> infos = toChangeInfos(r.entities(), cache);
infos.forEach(c -> cache.put(new Change.Id(c._number), c));
if (!infos.isEmpty() && r.more()) { if (!infos.isEmpty() && r.more()) {
infos.get(infos.size() - 1)._moreChanges = true; infos.get(infos.size() - 1)._moreChanges = true;
} }
@@ -417,14 +416,29 @@ public class ChangeJson {
List<ChangeData> changes, Map<Change.Id, ChangeInfo> cache) { List<ChangeData> changes, Map<Change.Id, ChangeInfo> cache) {
try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) { try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) {
List<ChangeInfo> changeInfos = new ArrayList<>(changes.size()); List<ChangeInfo> changeInfos = new ArrayList<>(changes.size());
for (ChangeData cd : changes) { for (int i = 0; i < changes.size(); i++) {
ChangeInfo i = cache.get(cd.getId()); // We can only cache and re-use an entity if it's not the last in the list. The last entity
if (i != null) { // 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; continue;
} }
// Compute and cache if possible
try { try {
ensureLoaded(Collections.singleton(cd)); 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(new Change.Id(info._number), info);
}
} catch (OrmException | RuntimeException e) { } catch (OrmException | RuntimeException e) {
logger.atWarning().withCause(e).log( logger.atWarning().withCause(e).log(
"Omitting corrupt change %s from results", cd.getId()); "Omitting corrupt change %s from results", cd.getId());