opendev/gerrit
Code Issues Proposed changes

ChangeJson: Make it semi stateless

Browse Source 
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
This commit is contained in:
David Ostrovsky
2014-05-27 09:16:24 +02:00
parent c9597c3ad2
commit c452ed23ac
1 changed files with 43 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

117
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

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