opendev/gerrit
Code Issues Proposed changes

Remove ChangeContol#getLabelRanges()

Browse Source 
The only caller of this method is ReviewerJson#format(). The purpose of
the returned value is to remove dummy votes from the REST response.

The method has the following direct and indirect callers:
 - PostReviewers#461: ChangeData is initialized in l.468
 - PostReviewers#449: ChangeData is initialized in l.438
 - ReviewerJson#89: ChangeData is initialized in l.86

All of these callers initialize ChangeData through ChangeData.Factory
which does not initialize the changeControl member inside ChangeData.
When ChangeData#changeControl() is called, it instantiates a
ChangeControl with the change owner as user. This user is then used
across in the two dependent controls: RefControl and ProjectControl.

When ChangeContol#getLabelRanges() is invoked, it uses the change owner
and returns all labels the change owner is permitted to vote on. Given
the nature of the check in ReviewerJson, this makes no sense and can be
dropped.

Should there be a need to drop dummy votes from the REST response in
the future we can come up with a proper check.

Change-Id: Ia350c4f4493b8587f37e7f29e170b9589232c8df
This commit is contained in:
Patrick Hiesel
2017-09-05 13:35:24 +02:00
parent 27208b1367
commit 1d87f7a4f3
3 changed files with 3 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.reviewdb.client.Account;
@@ -126,13 +125,9 @@ public class ReviewerJson {
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
out.approvals = new TreeMap<>(labelTypes.nameComparator());
for (PatchSetApproval ca : approvals) {
for (PermissionRange pr : cd.changeControl().getLabelRanges()) {
if (!pr.isEmpty()) {
LabelType at = labelTypes.byLabel(ca.getLabelId());
if (at != null) {
out.approvals.put(at.getName(), formatValue(ca.getValue()));
}
}
LabelType at = labelTypes.byLabel(ca.getLabelId());
if (at != null) {
out.approvals.put(at.getName(), formatValue(ca.getValue()));
}
}

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

@@ -48,7 +48,6 @@ import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collection;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -269,11 +268,6 @@ public class ChangeControl {
return canAbandon(db) && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
}
/** All value ranges of any allowed label permission. */
public List<PermissionRange> getLabelRanges() {
return getRefControl().getLabelRanges(isOwner());
}
/** The range of permitted values associated with a label permission. */
public PermissionRange getRange(String permission) {
return getRefControl().getRange(permission, isOwner());

21
gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
View File

@@ -323,27 +323,6 @@ public class RefControl {
return canForcePerform(Permission.EDIT_TOPIC_NAME);
}
/** All value ranges of any allowed label permission. */
List<PermissionRange> getLabelRanges(boolean isChangeOwner) {
List<PermissionRange> r = new ArrayList<>();
for (Map.Entry<String, List<PermissionRule>> e : relevant.getDeclaredPermissions()) {
if (Permission.isLabel(e.getKey())) {
int min = 0;
int max = 0;
for (PermissionRule rule : e.getValue()) {
if (projectControl.match(rule, isChangeOwner)) {
min = Math.min(min, rule.getMin());
max = Math.max(max, rule.getMax());
}
}
if (min != 0 || max != 0) {
r.add(new PermissionRange(e.getKey(), min, max));
}
}
}
return r;
}
/** The range of permitted values associated with a label permission. */
PermissionRange getRange(String permission) {
return getRange(permission, false);