PostReview: Remove support for 'strictLabels' option Currently we support strict/non-strict label in PostReview, which is controlled by the 'strictLabels' option. For strict labels, we require all labels to be within the user's granted range. Attempting to vote a label value not granted will fail the entire operation. For non-strict labels, we squashes the proposed value to the nearest granted value and the operation will be executed. This change removes the support for non-strict labels. Change-Id: Icb9dfafea567e1e9c469811e830e3519b865700f
diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt index 8f40d6c..58ff25c 100644 --- a/Documentation/cmd-review.txt +++ b/Documentation/cmd-review.txt
@@ -134,12 +134,6 @@ 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'
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 79c7a1a..490bc4d 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -6754,13 +6754,6 @@ |`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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 4ce412c..bdf6885 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -1571,7 +1571,6 @@ // 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);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java index 05e5f99..a4f44e4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -147,7 +147,6 @@ ReviewInput in = new ReviewInput(); in.onBehalfOf = user.id.toString(); - in.strictLabels = true; in.label("Not-A-Label", 5); exception.expect(BadRequestException.class); @@ -156,23 +155,6 @@ } @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(); LabelType verified = Util.verified();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java index 113651b..f851d5e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
@@ -60,7 +60,6 @@ private native void init() /*-{ this.labels = {}; - this.strict_labels = true; }-*/; public final native void prePost() /*-{
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 8c4dd04..9d66bfb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -228,7 +228,7 @@ 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); @@ -438,12 +438,9 @@ 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()) { @@ -474,8 +471,7 @@ return new RevisionResource(changes.parse(ctl), 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(); @@ -483,12 +479,8 @@ 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) { @@ -498,23 +490,16 @@ } 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)); } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java index 1917d6f..785ae38 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -79,7 +79,7 @@ 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 @@ 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 @@ 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; }
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index c46f2ca..2f11a10 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -141,12 +141,6 @@ 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", usage = "applies a tag to the given review", @@ -309,7 +303,6 @@ 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) {