opendev/gerrit
Code Issues Proposed changes

Merge "PostReview: Remove support for 'strictLabels' option"

Browse Source
This commit is contained in:
Changcheng Xiao
2017-10-12 12:50:47 +00:00
committed by Gerrit Code Review
parent 389ac3ab01 372855ef96
commit 433f2cd56a
8 changed files with 13 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
Documentation/cmd-review.txt
View File

@@ -124,12 +124,6 @@ branch.
or invalid value) and votes that are not permitted for the user are
silently ignored.
--strict-labels::
Require ability to vote on all specified labels before reviewing change.
If the vote is invalid (invalid label or invalid name), the vote is not
permitted for the user, or the vote is on an outdated or closed patch set,
return an error instead of silently discarding the vote.
--tag::
-t::
Apply a 'TAG' to the change message, votes, and inline comments. The 'TAG'

7
Documentation/rest-api-changes.txt
View File

@@ -6693,13 +6693,6 @@ list of link:#comment-input[CommentInput] entities.
|`robot_comments` |optional|
The robot comments that should be added as a map that maps a file path
to a list of link:#robot-comment-input[RobotCommentInput] entities.
|`strict_labels` |`true` if not set|
Whether all labels are required to be within the user's permitted ranges
based on access controls. +
If `true`, attempting to use a label not granted to the user will fail
the entire modify operation early. +
If `false`, the operation will execute anyway, but the proposed labels
will be modified to be the "best" value allowed by the access controls.
|`drafts` |optional|
Draft handling that defines how draft comments are handled that are
already in the database but that were not also described in this

1
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -1582,7 +1582,6 @@ public class ChangeIT extends AbstractDaemonTest {
// Exact request format made by GWT UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8.
ReviewInput in = new ReviewInput();
in.labels = ImmutableMap.of("Code-Review", (short) 0);
in.strictLabels = true;
in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS;
in.message = "comment";
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);

18
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
View File

@@ -146,7 +146,6 @@ public class ImpersonationIT extends AbstractDaemonTest {
ReviewInput in = new ReviewInput();
in.onBehalfOf = user.id.toString();
in.strictLabels = true;
in.label("Not-A-Label", 5);
exception.expect(BadRequestException.class);
@@ -154,23 +153,6 @@ public class ImpersonationIT extends AbstractDaemonTest {
revision.review(in);
}
@Test
public void voteOnBehalfOfInvalidLabelIgnoredWithoutStrictLabels() throws Exception {
allowCodeReviewOnBehalfOf();
PushOneCommit.Result r = createChange();
RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
ReviewInput in = new ReviewInput();
in.onBehalfOf = user.id.toString();
in.strictLabels = false;
in.label("Code-Review", 1);
in.label("Not-A-Label", 5);
revision.review(in);
assertThat(gApi.changes().id(r.getChangeId()).get().labels).doesNotContainKey("Not-A-Label");
}
@Test
public void voteOnBehalfOfLabelNotPermitted() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
View File

@@ -60,7 +60,6 @@ public class ReviewInput extends JavaScriptObject {
private native void init() /*-{
this.labels = {};
this.strict_labels = true;
}-*/;
public final native void prePost() /*-{

33
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -233,7 +233,7 @@ public class PostReview
input.drafts = DraftHandling.DELETE;
}
if (input.labels != null) {
checkLabels(revision, labelTypes, input.strictLabels, input.labels);
checkLabels(revision, labelTypes, input.labels);
}
if (input.comments != null) {
cleanUpComments(input.comments);
@@ -443,12 +443,9 @@ public class PostReview
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
LabelType type = labelTypes.byLabel(ent.getKey());
if (type == null && in.strictLabels) {
if (type == null) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
} else if (type == null) {
itr.remove();
continue;
}
if (!caller.isInternalUser()) {
@@ -479,8 +476,7 @@ public class PostReview
changeResourceFactory.create(rev.getNotes(), reviewer), rev.getPatchSet());
}
private void checkLabels(
RevisionResource rsrc, LabelTypes labelTypes, boolean strict, Map<String, Short> labels)
private void checkLabels(RevisionResource rsrc, LabelTypes labelTypes, Map<String, Short> labels)
throws BadRequestException, AuthException, PermissionBackendException {
PermissionBackend.ForChange perm = rsrc.permissions();
Iterator<Map.Entry<String, Short>> itr = labels.entrySet().iterator();
@@ -488,12 +484,8 @@ public class PostReview
Map.Entry<String, Short> ent = itr.next();
LabelType lt = labelTypes.byLabel(ent.getKey());
if (lt == null) {
if (strict) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
}
itr.remove();
continue;
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
}
if (ent.getValue() == null || ent.getValue() == 0) {
@@ -503,23 +495,16 @@ public class PostReview
}
if (lt.getValue(ent.getValue()) == null) {
if (strict) {
throw new BadRequestException(
String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
}
itr.remove();
continue;
throw new BadRequestException(
String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
}
short val = ent.getValue();
try {
perm.check(new LabelPermission.WithValue(lt, val));
} catch (AuthException e) {
if (strict) {
throw new AuthException(
String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
}
ent.setValue(perm.squashThenCheck(lt, val));
throw new AuthException(
String.format("Applying label \"%s\": %d is restricted", lt.getName(), val));
}
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
View File

@@ -79,7 +79,7 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate {
for (PatchSetApproval p : object.currentApprovals()) {
if (labelType.matches(p)) {
hasVote = true;
if (match(object, p.getValue(), p.getAccountId(), labelType)) {
if (match(object, p.getValue(), p.getAccountId())) {
return true;
}
}
@@ -105,7 +105,7 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate {
return null;
}
protected boolean match(ChangeData cd, short value, Account.Id approver, LabelType type) {
protected boolean match(ChangeData cd, short value, Account.Id approver) {
if (value != expVal) {
return false;
}
@@ -119,11 +119,11 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate {
return false;
}
// Double check the value is still permitted for the user.
// Check the user has 'READ' permission.
try {
PermissionBackend.ForChange perm =
permissionBackend.user(reviewer).database(dbProvider).change(cd);
return perm.test(ChangePermission.READ) && expVal == perm.squashByTest(type, value);
return perm.test(ChangePermission.READ);
} catch (PermissionBackendException e) {
return false;
}

7
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
View File

@@ -133,12 +133,6 @@ public class ReviewCommand extends SshCommand {
@Option(name = "--json", aliases = "-j", usage = "read review input json from stdin")
private boolean json;
@Option(
name = "--strict-labels",
usage = "Strictly check if the labels specified can be applied to the given patch set(s)"
)
private boolean strictLabels;
@Option(
name = "--tag",
aliases = "-t",
@@ -273,7 +267,6 @@ public class ReviewCommand extends SshCommand {
review.notify = notify;
review.labels = new TreeMap<>();
review.drafts = ReviewInput.DraftHandling.PUBLISH;
review.strictLabels = strictLabels;
for (ApproveOption ao : optionList) {
Short v = ao.value();
if (v != null) {