opendev/gerrit
Code Issues Proposed changes

Get LabelTypes from ChangeData, reducing ChangeControl usage

Browse Source 
With LabelTypes accessible on ChangeData some callers of getLabelTypes
can shift to using the ChangeData instead of the ChangeControl.

getLabelTypes is still delegated through the ChangeControl, but that
can be more easily cleaned up later as the number of ChangeControl
callers is futher reduced.

Change-Id: I83f6f6920cb019ca865ad4e6ff68ed11c8efbca4
This commit is contained in:
Shawn Pearce
2017-02-19 14:49:54 -08:00
committed by David Pursehouse
parent 1feb1d1914
commit cd84f3bb83
6 changed files with 39 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -524,10 +524,11 @@ public class ChangeJson {
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())) {
if (user.isIdentifiedUser()
&& (!limitToPsId.isPresent() || limitToPsId.get().equals(in.currentPatchSetId()))) {
out.permittedLabels =
cd.change().getStatus() != Change.Status.ABANDONED
? permittedLabels(perm, ctl, cd)
? permittedLabels(perm, cd)
: ImmutableMap.of();
}
@@ -621,17 +622,16 @@ public class ChangeJson {
return null;
}
LabelTypes labelTypes = ctl.getLabelTypes();
LabelTypes labelTypes = cd.getLabelTypes();
Map<String, LabelWithStatus> withStatus =
cd.change().getStatus().isOpen()
? labelsForOpenChange(perm, ctl, cd, labelTypes, standard, detailed)
: labelsForClosedChange(perm, ctl, cd, labelTypes, standard, detailed);
? labelsForOpenChange(perm, cd, labelTypes, standard, detailed)
: labelsForClosedChange(perm, cd, labelTypes, standard, detailed);
return ImmutableMap.copyOf(Maps.transformValues(withStatus, LabelWithStatus::label));
}
private Map<String, LabelWithStatus> labelsForOpenChange(
PermissionBackend.ForChange perm,
ChangeControl ctl,
ChangeData cd,
LabelTypes labelTypes,
boolean standard,
@@ -639,7 +639,7 @@ public class ChangeJson {
throws OrmException, PermissionBackendException {
Map<String, LabelWithStatus> labels = initLabels(cd, labelTypes, standard);
if (detailed) {
setAllApprovals(perm, ctl, cd, labels);
setAllApprovals(perm, cd, labels);
}
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
LabelType type = labelTypes.byLabel(e.getKey());
@@ -726,10 +726,7 @@ public class ChangeJson {
}
private void setAllApprovals(
PermissionBackend.ForChange basePerm,
ChangeControl baseCtrl,
ChangeData cd,
Map<String, LabelWithStatus> labels)
PermissionBackend.ForChange basePerm, ChangeData cd, Map<String, LabelWithStatus> labels)
throws OrmException, PermissionBackendException {
Change.Status status = cd.change().getStatus();
checkState(status.isOpen(), "should not call setAllApprovals on %s change", status);
@@ -744,17 +741,15 @@ public class ChangeJson {
}
Table<Account.Id, String, PatchSetApproval> current =
HashBasedTable.create(allUsers.size(), baseCtrl.getLabelTypes().getLabelTypes().size());
HashBasedTable.create(allUsers.size(), cd.getLabelTypes().getLabelTypes().size());
for (PatchSetApproval psa : cd.currentApprovals()) {
current.put(psa.getAccountId(), psa.getLabel(), psa);
}
LabelTypes labelTypes = baseCtrl.getLabelTypes();
LabelTypes labelTypes = cd.getLabelTypes();
for (Account.Id accountId : allUsers) {
IdentifiedUser user = userFactory.create(accountId);
PermissionBackend.ForChange perm = basePerm.user(user);
ChangeControl ctl = baseCtrl.forUser(user);
Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(perm, ctl, cd));
PermissionBackend.ForChange perm = basePerm.user(userFactory.create(accountId));
Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(perm, cd));
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
LabelType lt = labelTypes.byLabel(e.getKey());
if (lt == null) {
@@ -833,7 +828,6 @@ public class ChangeJson {
private Map<String, LabelWithStatus> labelsForClosedChange(
PermissionBackend.ForChange basePerm,
ChangeControl baseCtrl,
ChangeData cd,
LabelTypes labelTypes,
boolean standard,
@@ -906,10 +900,8 @@ public class ChangeJson {
Map<String, ApprovalInfo> byLabel = Maps.newHashMapWithExpectedSize(labels.size());
Map<String, VotingRangeInfo> pvr = Collections.emptyMap();
if (detailed) {
IdentifiedUser user = userFactory.create(accountId);
PermissionBackend.ForChange perm = basePerm.user(user);
ChangeControl ctl = baseCtrl.forUser(user);
pvr = getPermittedVotingRanges(permittedLabels(perm, ctl, cd));
PermissionBackend.ForChange perm = basePerm.user(userFactory.create(accountId));
pvr = getPermittedVotingRanges(permittedLabels(perm, cd));
for (Map.Entry<String, LabelWithStatus> entry : labels.entrySet()) {
ApprovalInfo ai = approvalInfo(accountId, 0, null, null, null);
byLabel.put(entry.getKey(), ai);
@@ -984,14 +976,10 @@ public class ChangeJson {
}
private Map<String, Collection<String>> permittedLabels(
PermissionBackend.ForChange perm, ChangeControl ctl, ChangeData cd)
PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException, PermissionBackendException {
if (ctl == null || !ctl.getUser().isIdentifiedUser()) {
return null;
}
boolean isMerged = cd.change().getStatus() == Change.Status.MERGED;
LabelTypes labelTypes = ctl.getLabelTypes();
LabelTypes labelTypes = cd.getLabelTypes();
Map<String, LabelType> toCheck = new HashMap<>();
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels != null) {
@@ -1021,7 +1009,7 @@ public class ChangeJson {
boolean ok = can.contains(new LabelPermission.WithValue(type, v));
if (isMerged) {
if (labels == null) {
labels = currentLabels(ctl);
labels = currentLabels(perm, cd);
}
short prev = labels.getOrDefault(type.getName(), (short) 0);
ok &= v.getValue() >= prev;
@@ -1045,11 +1033,14 @@ public class ChangeJson {
return permitted.asMap();
}
private Map<String, Short> currentLabels(ChangeControl ctl) throws OrmException {
private Map<String, Short> currentLabels(PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException {
IdentifiedUser user = perm.user().asIdentifiedUser();
ChangeControl ctl = cd.changeControl().forUser(user);
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa :
approvalsUtil.byPatchSetUser(
db.get(), ctl, ctl.getChange().currentPatchSetId(), ctl.getUser().getAccountId())) {
db.get(), ctl, cd.change().currentPatchSetId(), user.getAccountId())) {
result.put(psa.getLabel(), psa.getValue());
}
return result;

4
gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
View File

@@ -106,7 +106,8 @@ public class ReviewerJson {
ChangeControl ctl,
Iterable<PatchSetApproval> approvals)
throws OrmException, PermissionBackendException {
LabelTypes labelTypes = ctl.getLabelTypes();
ChangeData cd = changeDataFactory.create(db.get(), ctl);
LabelTypes labelTypes = cd.getLabelTypes();
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
out.approvals = new TreeMap<>(labelTypes.nameComparator());
@@ -123,7 +124,6 @@ public class ReviewerJson {
// Add dummy approvals for all permitted labels for the user even if they
// do not exist in the DB.
ChangeData cd = changeDataFactory.create(db.get(), ctl);
PatchSet ps = cd.currentPatchSet();
if (ps != null) {
for (SubmitRecord rec :

2
gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java
View File

@@ -41,7 +41,7 @@ public class MergedSender extends ReplyToChangeSender {
public MergedSender(EmailArguments ea, @Assisted Project.NameKey project, @Assisted Change.Id id)
throws OrmException {
super(ea, "merged", newChangeData(ea, project, id));
labelTypes = changeData.changeControl().getLabelTypes();
labelTypes = changeData.getLabelTypes();
}
@Override

5
gerrit-server/src/main/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java
View File

@@ -164,5 +164,10 @@ public class FailedPermissionBackend {
throws PermissionBackendException {
throw new PermissionBackendException(message, cause);
}
@Override
public CurrentUser user() {
throw new UnsupportedOperationException("FailedPermissionBackend is not scoped to user");
}
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
View File

@@ -176,6 +176,9 @@ public abstract class PermissionBackend {
/** PermissionBackend scoped to a user, project, reference and change. */
public abstract static class ForChange extends AcceptsReviewDb<ForChange> {
/** @return user this instance is scoped to. */
public abstract CurrentUser user();
/** @return new instance rescoped to same change, but different {@code user}. */
public abstract ForChange user(CurrentUser user);

7
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -527,9 +527,14 @@ public class ChangeControl {
return cd;
}
@Override
public CurrentUser user() {
return getUser();
}
@Override
public ForChange user(CurrentUser user) {
return getUser().equals(user) ? this : forUser(user).asForChange(cd, db);
return user().equals(user) ? this : forUser(user).asForChange(cd, db);
}
@Override