From c452ed23ac4865de4f256f73498bb580b521f53c Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 27 May 2014 09:16:24 +0200 Subject: [PATCH] ChangeJson: Make it semi stateless This is prerequisite to move REST endpoints that are using ChangeJson to singleton scope. Note the class ChangeJson itself canot be made singleton because of options attribute. Change-Id: Ib305ba37fa279c65df6c85a261baad07b32ff3db --- .../gerrit/server/change/ChangeJson.java | 117 +++++++----------- 1 file changed, 43 insertions(+), 74 deletions(-) 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;