opendev/gerrit
Code Issues Proposed changes

Remove limited patch set view of ChangeData

Browse Source 
This required extra logic and state and complicated the semantics of a
ChangeData. It was only used by ChangeJson to limit results to a
single patch set. Instead of supporting arbitrary subsets of
revisions, just have ChangeJson do the limiting itself. The only
additional fetching this might require is for a single non-current
patch set, we might fetch all patch set rows for the change from the
db.

Change-Id: I7a35369d11da3b4b4d7f134766991f33c4a0b65c
This commit is contained in:
Dave Borowitz
2014-01-21 14:10:00 -08:00
parent d81ba62cdf
commit e759cd16ab
4 changed files with 67 additions and 105 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
View File

@@ -87,7 +87,7 @@ public class ApprovalCopier {
try {
ProjectState project =
projectCache.checkedGet(cd.change().getDest().getParentKey());
ListMultimap<PatchSet.Id, PatchSetApproval> all = cd.allApprovalsMap();
ListMultimap<PatchSet.Id, PatchSetApproval> all = cd.approvalsMap();
Table<String, Account.Id, PatchSetApproval> byUser =
HashBasedTable.create();

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

@@ -31,6 +31,7 @@ import static com.google.gerrit.common.changes.ListChangesOption.REVIEWED;
import com.google.common.base.Joiner;
import com.google.common.base.Objects;
import com.google.common.base.Optional;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
@@ -215,14 +216,20 @@ public class ChangeJson {
}
public ChangeInfo format(ChangeData cd) throws OrmException {
List<ChangeData> tmp = ImmutableList.of(cd);
return formatList2(ImmutableList.of(tmp)).get(0).get(0);
return format(cd, Optional.<PatchSet.Id> absent());
}
private ChangeInfo format(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
throws OrmException {
accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
ChangeInfo res = toChangeInfo(cd, limitToPsId);
accountLoader.fill();
return res;
}
public ChangeInfo format(RevisionResource rsrc) throws OrmException {
ChangeData cd = changeDataFactory.create(db.get(), rsrc.getControl());
cd.limitToPatchSets(ImmutableList.of(rsrc.getPatchSet().getId()));
return format(cd);
return format(cd, Optional.of(rsrc.getPatchSet().getId()));
}
public List<List<ChangeInfo>> formatList2(List<List<ChangeData>> in)
@@ -259,7 +266,7 @@ public class ChangeJson {
for (ChangeData cd : changes) {
ChangeInfo i = out.get(cd.getId());
if (i == null) {
i = toChangeInfo(cd);
i = toChangeInfo(cd, Optional.<PatchSet.Id> absent());
out.put(cd.getId(), i);
}
info.add(i);
@@ -267,7 +274,8 @@ public class ChangeJson {
return info;
}
private ChangeInfo toChangeInfo(ChangeData cd) throws OrmException {
private ChangeInfo toChangeInfo(ChangeData cd,
Optional<PatchSet.Id> limitToPsId) throws OrmException {
ChangeInfo out = new ChangeInfo();
Change in = cd.change();
out.project = in.getProject().get();
@@ -295,11 +303,11 @@ public class ChangeJson {
&& reviewed.contains(cd.getId()) ? true : null;
out.labels = labelsFor(cd, has(LABELS), has(DETAILED_LABELS));
Collection<PatchSet.Id> limited = cd.getLimitedPatchSets();
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 (limited == null || limited.contains(in.currentPatchSetId())) {
if (!limitToPsId.isPresent()
|| limitToPsId.get().equals(in.currentPatchSetId())) {
out.permitted_labels = permittedLabels(cd);
}
out.removable_reviewers = removableReviewers(cd, out.labels.values());
@@ -309,8 +317,10 @@ public class ChangeJson {
}
out.finish();
if (has(ALL_REVISIONS) || has(CURRENT_REVISION) || limited != null) {
out.revisions = revisions(cd);
if (has(ALL_REVISIONS)
|| has(CURRENT_REVISION)
|| limitToPsId.isPresent()) {
out.revisions = revisions(cd, limitToPsId);
if (out.revisions != null) {
for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) {
if (entry.getValue().isCurrent) {
@@ -491,7 +501,7 @@ public class ChangeJson {
// All users ever added, even if they can't vote on one or all labels.
Set<Account.Id> allUsers = Sets.newHashSet();
ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals =
cd.allApprovalsMap();
cd.approvalsMap();
for (PatchSetApproval psa : allApprovals.values()) {
allUsers.add(psa.getAccountId());
}
@@ -537,7 +547,7 @@ public class ChangeJson {
LabelTypes labelTypes, boolean standard, boolean detailed)
throws OrmException {
Set<Account.Id> allUsers = Sets.newHashSet();
for (PatchSetApproval psa : cd.allApprovals()) {
for (PatchSetApproval psa : cd.approvals()) {
allUsers.add(psa.getAccountId());
}
@@ -771,18 +781,33 @@ public class ChangeJson {
return false;
}
private Map<String, RevisionInfo> revisions(ChangeData cd) throws OrmException {
private Map<String, RevisionInfo> revisions(ChangeData cd,
Optional<PatchSet.Id> limitToPsId) throws OrmException {
ChangeControl ctl = control(cd);
if (ctl == null) {
return null;
}
Collection<PatchSet> src;
if (cd.getLimitedPatchSets() != null || has(ALL_REVISIONS)) {
if (has(ALL_REVISIONS)) {
src = cd.patches();
} else {
src = Collections.singletonList(cd.currentPatchSet());
PatchSet ps;
if (limitToPsId.isPresent()) {
ps = cd.patch(limitToPsId.get());
if (ps == null) {
throw new OrmException("missing patch set " + limitToPsId.get());
}
} else {
ps = cd.currentPatchSet();
if (ps == null) {
throw new OrmException(
"missing current patch set for change " + cd.getId());
}
}
src = Collections.singletonList(ps);
}
Map<String, RevisionInfo> res = Maps.newLinkedHashMap();
for (PatchSet in : src) {
if (ctl.isPatchVisible(in, db.get())) {

2
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
View File

@@ -215,7 +215,7 @@ public class ChangeField {
public Iterable<Integer> get(ChangeData input, FillArgs args)
throws OrmException {
Set<Integer> r = Sets.newHashSet();
for (PatchSetApproval a : input.allApprovals()) {
for (PatchSetApproval a : input.approvals()) {
r.add(a.getAccountId().get());
}
return r;

113
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -17,13 +17,11 @@ package com.google.gerrit.server.query.change;
import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
import com.google.common.base.Objects;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -65,7 +63,6 @@ import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
public class ChangeData {
public static void ensureChangeLoaded(Iterable<ChangeData> changes)
@@ -111,9 +108,6 @@ public class ChangeData {
for (PatchSet ps : db.patchSets().get(missing.keySet())) {
ChangeData cd = missing.get(ps.getId());
cd.currentPatchSet = ps;
if (cd.limitedIds == null) {
cd.patches = Lists.newArrayList(ps);
}
}
}
}
@@ -123,7 +117,7 @@ public class ChangeData {
List<ResultSet<PatchSetApproval>> pending = Lists.newArrayList();
for (ChangeData cd : changes) {
if (!cd.notesMigration.readPatchSetApprovals()) {
if (cd.currentApprovals == null && cd.limitedApprovals == null) {
if (cd.currentApprovals == null) {
pending.add(cd.db.patchSetApprovals()
.byPatchSet(cd.change().currentPatchSetId()));
}
@@ -134,7 +128,7 @@ public class ChangeData {
if (!pending.isEmpty()) {
int idx = 0;
for (ChangeData cd : changes) {
if (cd.currentApprovals == null && cd.limitedApprovals == null) {
if (cd.currentApprovals == null) {
cd.currentApprovals = sortApprovals(pending.get(idx++));
}
}
@@ -172,9 +166,7 @@ public class ChangeData {
private String commitMessage;
private List<FooterLine> commitFooters;
private PatchSet currentPatchSet;
private Set<PatchSet.Id> limitedIds;
private Collection<PatchSet> patches;
private ListMultimap<PatchSet.Id, PatchSetApproval> limitedApprovals;
private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals;
private List<PatchSetApproval> currentApprovals;
private List<String> currentFiles;
@@ -184,7 +176,6 @@ public class ChangeData {
private List<ChangeMessage> messages;
private List<SubmitRecord> submitRecords;
private ChangedLines changedLines;
private boolean patchesLoaded;
@AssistedInject
private ChangeData(
@@ -252,21 +243,6 @@ public class ChangeData {
returnedBySource = s;
}
public void limitToPatchSets(Collection<PatchSet.Id> ids) {
limitedIds = Sets.newLinkedHashSetWithExpectedSize(ids.size());
for (PatchSet.Id id : ids) {
if (!id.getParentKey().equals(legacyId)) {
throw new IllegalArgumentException(String.format(
"invalid patch set %s for change %s", id, legacyId));
}
limitedIds.add(id);
}
}
public Collection<PatchSet.Id> getLimitedPatchSets() {
return limitedIds;
}
public void setCurrentFilePaths(List<String> filePaths) {
currentFiles = ImmutableList.copyOf(filePaths);
}
@@ -395,9 +371,6 @@ public class ChangeData {
currentApprovals = Collections.emptyList();
} else if (allApprovals != null) {
return allApprovals.get(c.currentPatchSetId());
} else if (limitedApprovals != null &&
(limitedIds == null || limitedIds.contains(c.currentPatchSetId()))) {
return limitedApprovals.get(c.currentPatchSetId());
} else {
currentApprovals = approvalsUtil.byPatchSet(
db, notes(), c.currentPatchSetId());
@@ -451,32 +424,35 @@ public class ChangeData {
}
/**
* @return patches for the change. If {@link #limitToPatchSets(Collection)}
* was previously called, only contains patches with the specified IDs.
* @return patches for the change.
* @throws OrmException an error occurred reading the database.
*/
public Collection<PatchSet> patches()
throws OrmException {
if (patches == null || !patchesLoaded) {
if (limitedIds != null) {
patches = Lists.newArrayList();
for (PatchSet ps : db.patchSets().byChange(legacyId)) {
if (limitedIds.contains(ps.getId())) {
patches.add(ps);
}
}
} else {
patches = db.patchSets().byChange(legacyId).toList();
}
patchesLoaded = true;
if (patches == null) {
patches = db.patchSets().byChange(legacyId).toList();
}
return patches;
}
/**
* @return patch set approvals for the change in timestamp order. If
* {@link #limitToPatchSets(Collection)} was previously called, only contains
* approvals for the patches with the specified IDs.
* @return patch with the given ID, or null if it does not exist.
* @throws OrmException an error occurred reading the database.
*/
public PatchSet patch(PatchSet.Id psId) throws OrmException {
if (currentPatchSet != null && currentPatchSet.getId().equals(psId)) {
return currentPatchSet;
}
for (PatchSet ps : patches()) {
if (ps.getId().equals(psId)) {
return ps;
}
}
return null;
}
/**
* @return patch set approvals for the change in timestamp order.
* @throws OrmException an error occurred reading the database.
*/
public List<PatchSetApproval> approvals()
@@ -485,51 +461,12 @@ public class ChangeData {
}
/**
* @return patch set approvals for the change, keyed by ID, ordered by
* timestamp within each patch set. If
* {@link #limitToPatchSets(Collection)} was previously called, only
* contains approvals for the patches with the specified IDs.
* @return all patch set approvals for the change, keyed by ID, ordered by
* timestamp within each patch set.
* @throws OrmException an error occurred reading the database.
*/
public ListMultimap<PatchSet.Id, PatchSetApproval> approvalsMap()
throws OrmException {
if (limitedApprovals == null) {
limitedApprovals = ArrayListMultimap.create();
if (allApprovals != null) {
for (PatchSet.Id id : limitedIds) {
limitedApprovals.putAll(id, allApprovals.get(id));
}
} else {
for (PatchSetApproval psa
: approvalsUtil.byChange(db, notes()).values()) {
if (limitedIds == null || limitedIds.contains(legacyId)) {
limitedApprovals.put(psa.getPatchSetId(), psa);
}
}
}
}
return limitedApprovals;
}
/**
* @return all patch set approvals for the change in timestamp order
* (regardless of whether {@link #limitToPatchSets(Collection)} was
* previously called).
* @throws OrmException an error occurred reading the database.
*/
public List<PatchSetApproval> allApprovals()
throws OrmException {
return ImmutableList.copyOf(allApprovalsMap().values());
}
/**
* @return all patch set approvals for the change (regardless of whether
* {@link #limitToPatchSets(Collection)} was previously called), keyed by
* ID, ordered by timestamp within each patch set.
* @throws OrmException an error occurred reading the database.
*/
public ListMultimap<PatchSet.Id, PatchSetApproval> allApprovalsMap()
throws OrmException {
if (allApprovals == null) {
allApprovals = approvalsUtil.byChange(db, notes());
}
@@ -538,7 +475,7 @@ public class ChangeData {
public SetMultimap<ReviewerState, Account.Id> reviewers()
throws OrmException {
return ApprovalsUtil.getReviewers(allApprovals());
return ApprovalsUtil.getReviewers(approvals());
}
public Collection<PatchLineComment> comments()