diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 5c1ffdfc51..50410fb20d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -139,12 +139,10 @@ public class ChangeJson { private final DynamicMap downloadCommands; private final DynamicMap> changeViews; private final Revisions revisions; + private final Provider webLinks; + private final EnumSet options; - private EnumSet options; private AccountInfo.Loader accountLoader; - private ChangeControl lastControl; - private Set reviewed; - private Provider webLinks; @Inject ChangeJson( @@ -179,7 +177,6 @@ public class ChangeJson { this.changeViews = changeViews; this.revisions = revisions; this.webLinks = webLinks; - options = EnumSet.noneOf(ListChangesOption.class); } @@ -212,10 +209,11 @@ public class ChangeJson { private ChangeInfo format(ChangeData cd, Optional limitToPsId) throws OrmException { accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); + Set reviewed = Sets.newHashSet(); if (has(REVIEWED)) { - ensureReviewedLoaded(Collections.singleton(cd)); + reviewed = loadReviewed(Collections.singleton(cd)); } - ChangeInfo res = toChangeInfo(cd, limitToPsId); + ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId); accountLoader.fill(); return res; } @@ -235,15 +233,16 @@ public class ChangeJson { } else { ChangeData.ensureCurrentPatchSetLoaded(all); } + Set reviewed = Sets.newHashSet(); if (has(REVIEWED)) { - ensureReviewedLoaded(all); + reviewed = loadReviewed(all); } ChangeData.ensureCurrentApprovalsLoaded(all); List> res = Lists.newArrayListWithCapacity(in.size()); Map out = Maps.newHashMap(); for (List changes : in) { - res.add(toChangeInfo(out, changes)); + res.add(toChangeInfo(out, changes, reviewed)); } accountLoader.fill(); return res; @@ -254,13 +253,13 @@ public class ChangeJson { } private List toChangeInfo(Map out, - List changes) throws OrmException { + List changes, Set reviewed) throws OrmException { List info = Lists.newArrayListWithCapacity(changes.size()); for (ChangeData cd : changes) { ChangeInfo i = out.get(cd.getId()); if (i == null) { try { - i = toChangeInfo(cd, Optional. absent()); + i = toChangeInfo(cd, reviewed, Optional. absent()); } catch (OrmException e) { log.warn( "Omitting corrupt change " + cd.getId() + " from results", e); @@ -273,8 +272,9 @@ public class ChangeJson { return info; } - private ChangeInfo toChangeInfo(ChangeData cd, + private ChangeInfo toChangeInfo(ChangeData cd, Set reviewed, Optional limitToPsId) throws OrmException { + ChangeControl ctl = cd.changeControl().forUser(userProvider.get()); ChangeInfo out = new ChangeInfo(); Change in = cd.change(); out.project = in.getProject().get(); @@ -300,28 +300,28 @@ public class ChangeJson { out.reviewed = in.getStatus().isOpen() && has(REVIEWED) && reviewed.contains(cd.getId()) ? true : null; - out.labels = labelsFor(cd, has(LABELS), has(DETAILED_LABELS)); + out.labels = labelsFor(ctl, cd, has(LABELS), has(DETAILED_LABELS)); if (out.labels != null && has(DETAILED_LABELS)) { // If limited to specific patch sets but not the current patch set, don't // list permitted labels, since users can't vote on those patch sets. if (!limitToPsId.isPresent() || limitToPsId.get().equals(in.currentPatchSetId())) { - out.permittedLabels = permittedLabels(cd); + out.permittedLabels = permittedLabels(ctl, cd); } - out.removableReviewers = removableReviewers(cd, out.labels.values()); + out.removableReviewers = removableReviewers(ctl, cd, out.labels.values()); } Map src = loadPatchSets(cd, limitToPsId); if (has(MESSAGES)) { - out.messages = messages(cd, src); + out.messages = messages(ctl, cd, src); } out.finish(); if (has(ALL_REVISIONS) || has(CURRENT_REVISION) || limitToPsId.isPresent()) { - out.revisions = revisions(cd, limitToPsId, out.project, src); + out.revisions = revisions(ctl, cd, limitToPsId, out.project, src); if (out.revisions != null) { for (Map.Entry entry : out.revisions.entrySet()) { if (entry.getValue().isCurrent) { @@ -336,28 +336,19 @@ public class ChangeJson { out.actions = Maps.newTreeMap(); for (UiAction.Description d : UiActions.from( changeViews, - changes.parse(control(cd)), + changes.parse(ctl), userProvider)) { out.actions.put(d.getId(), new ActionInfo(d)); } } - lastControl = null; return out; } - private ChangeControl control(ChangeData cd) throws OrmException { - if (lastControl != null - && cd.getId().equals(lastControl.getChange().getId())) { - return lastControl; - } - return lastControl = cd.changeControl().forUser(userProvider.get()); - } - - private List submitRecords(ChangeData cd) throws OrmException { + private List submitRecords(ChangeControl ctl, ChangeData cd) + throws OrmException { if (cd.getSubmitRecords() != null) { return cd.getSubmitRecords(); } - ChangeControl ctl = control(cd); if (ctl == null) { return ImmutableList.of(); } @@ -369,31 +360,30 @@ public class ChangeJson { return cd.getSubmitRecords(); } - private Map labelsFor(ChangeData cd, boolean standard, + private Map labelsFor(ChangeControl ctl, ChangeData cd, boolean standard, boolean detailed) throws OrmException { if (!standard && !detailed) { return null; } - ChangeControl ctl = control(cd); if (ctl == null) { return null; } LabelTypes labelTypes = ctl.getLabelTypes(); if (cd.change().getStatus().isOpen()) { - return labelsForOpenChange(cd, labelTypes, standard, detailed); + return labelsForOpenChange(ctl, cd, labelTypes, standard, detailed); } else { return labelsForClosedChange(cd, labelTypes, standard, detailed); } } - private Map labelsForOpenChange(ChangeData cd, - LabelTypes labelTypes, boolean standard, boolean detailed) + private Map labelsForOpenChange(ChangeControl ctl, + ChangeData cd, LabelTypes labelTypes, boolean standard, boolean detailed) throws OrmException { - Map labels = initLabels(cd, labelTypes, standard); + Map labels = initLabels(ctl, cd, labelTypes, standard); if (detailed) { - setAllApprovals(cd, labels); + setAllApprovals(ctl, cd, labels); } for (Map.Entry e : labels.entrySet()) { LabelType type = labelTypes.byLabel(e.getKey()); @@ -416,11 +406,11 @@ public class ChangeJson { return labels; } - private Map initLabels(ChangeData cd, + private Map initLabels(ChangeControl ctl, ChangeData cd, LabelTypes labelTypes, boolean standard) throws OrmException { // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167. Map labels = new TreeMap<>(labelTypes.nameComparator()); - for (SubmitRecord rec : submitRecords(cd)) { + for (SubmitRecord rec : submitRecords(ctl, cd)) { if (rec.labels == null) { continue; } @@ -478,13 +468,8 @@ public class ChangeJson { } } - private void setAllApprovals(ChangeData cd, + private void setAllApprovals(ChangeControl baseCtrl, ChangeData cd, Map labels) throws OrmException { - ChangeControl baseCtrl = control(cd); - if (baseCtrl == null) { - return; - } - // Include a user in the output for this label if either: // - They are an explicit reviewer. // - They ever voted on this change. @@ -618,16 +603,15 @@ public class ChangeJson { } } - private Map> permittedLabels(ChangeData cd) + private Map> permittedLabels(ChangeControl ctl, ChangeData cd) throws OrmException { - ChangeControl ctl = control(cd); if (ctl == null) { return null; } LabelTypes labelTypes = ctl.getLabelTypes(); SetMultimap permitted = LinkedHashMultimap.create(); - for (SubmitRecord rec : submitRecords(cd)) { + for (SubmitRecord rec : submitRecords(ctl, cd)) { if (rec.labels == null) { continue; } @@ -658,14 +642,9 @@ public class ChangeJson { return permitted.asMap(); } - private Collection messages(ChangeData cd, + private Collection messages(ChangeControl ctl, ChangeData cd, Map map) throws OrmException { - ChangeControl ctl = control(cd); - if (ctl == null) { - return null; - } - List messages = db.get().changeMessages().byChange(cd.getId()).toList(); if (messages.isEmpty()) { @@ -698,13 +677,8 @@ public class ChangeJson { return result; } - private Collection removableReviewers(ChangeData cd, + private Collection removableReviewers(ChangeControl ctl, ChangeData cd, Collection labels) throws OrmException { - ChangeControl ctl = control(cd); - if (ctl == null) { - return null; - } - Set fixed = Sets.newHashSetWithExpectedSize(labels.size()); Set removable = Sets.newHashSetWithExpectedSize(labels.size()); for (LabelInfo label : labels) { @@ -728,9 +702,9 @@ public class ChangeJson { return result; } - private void ensureReviewedLoaded(Iterable all) + private Set loadReviewed(Iterable all) throws OrmException { - reviewed = Sets.newHashSet(); + Set reviewed = Sets.newHashSet(); if (userProvider.get().isIdentifiedUser()) { Account.Id self = ((IdentifiedUser) userProvider.get()).getAccountId(); for (List batch : Iterables.partition(all, 50)) { @@ -751,6 +725,7 @@ public class ChangeJson { } } } + return reviewed; } private boolean isChangeReviewed(Account.Id self, ChangeData cd, @@ -774,20 +749,15 @@ public class ChangeJson { return false; } - private Map revisions(ChangeData cd, + private Map revisions(ChangeControl ctl, ChangeData cd, Optional limitToPsId, String project, Map map) throws OrmException { - ChangeControl ctl = control(cd); - if (ctl == null) { - return null; - } - Map res = Maps.newLinkedHashMap(); for (PatchSet in : map.values()) { if ((has(ALL_REVISIONS) || in.getId().equals(cd.change().currentPatchSetId())) && ctl.isPatchVisible(in, db.get())) { - res.put(in.getRevision().get(), toRevisionInfo(cd, in, project)); + res.put(in.getRevision().get(), toRevisionInfo(ctl, cd, in, project)); } } return res; @@ -821,13 +791,13 @@ public class ChangeJson { return map; } - private RevisionInfo toRevisionInfo(ChangeData cd, PatchSet in, String project) - throws OrmException { + private RevisionInfo toRevisionInfo(ChangeControl ctl, ChangeData cd, + PatchSet in, String project) throws OrmException { RevisionInfo out = new RevisionInfo(); out.isCurrent = in.getId().equals(cd.change().currentPatchSetId()); out._number = in.getId().get(); out.draft = in.isDraft() ? true : null; - out.fetch = makeFetchMap(cd, in); + out.fetch = makeFetchMap(ctl, cd, in); if (has(ALL_COMMITS) || (out.isCurrent && has(CURRENT_COMMIT))) { try { @@ -852,7 +822,7 @@ public class ChangeJson { out.actions = Maps.newTreeMap(); for (UiAction.Description d : UiActions.from( revisions, - new RevisionResource(changes.parse(control(cd)), in), + new RevisionResource(changes.parse(ctl), in), userProvider)) { out.actions.put(d.getId(), new ActionInfo(d)); } @@ -897,7 +867,7 @@ public class ChangeJson { return commit; } - private Map makeFetchMap(ChangeData cd, PatchSet in) + private Map makeFetchMap(ChangeControl ctl, ChangeData cd, PatchSet in) throws OrmException { Map r = Maps.newLinkedHashMap(); @@ -909,7 +879,6 @@ public class ChangeJson { continue; } - ChangeControl ctl = control(cd); if (!scheme.isAuthSupported() && !ctl.forUser(anonymous).isPatchVisible(in, db.get())) { continue;