From 372855ef96d23ccfed670b7717f3aa1f0564c8cd Mon Sep 17 00:00:00 2001 From: Changcheng Xiao Date: Mon, 17 Jul 2017 15:00:34 +0200 Subject: [PATCH] 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 --- Documentation/cmd-review.txt | 6 ---- Documentation/rest-api-changes.txt | 7 ---- .../acceptance/api/change/ChangeIT.java | 1 - .../rest/account/ImpersonationIT.java | 18 ---------- .../gerrit/client/changes/ReviewInput.java | 1 - .../gerrit/server/change/PostReview.java | 33 +++++-------------- .../query/change/EqualsLabelPredicate.java | 8 ++--- .../gerrit/sshd/commands/ReviewCommand.java | 7 ---- 8 files changed, 13 insertions(+), 68 deletions(-) diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt index 8f40d6cd76..58ff25ce1e 100644 --- a/Documentation/cmd-review.txt +++ b/Documentation/cmd-review.txt @@ -134,12 +134,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' diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 79c7a1a7ca..490bc4da58 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -6754,13 +6754,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 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 4ce412c94c..bdf6885160 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 @@ 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); 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 05e5f99927..a4f44e408f 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 @@ 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); @@ -155,23 +154,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(); 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 113651beca..f851d5e72e 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 @@ public class ReviewInput extends JavaScriptObject { 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 8c4dd0435c..9d66bfbde2 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 @@ 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); @@ -438,12 +438,9 @@ public class PostReview while (itr.hasNext()) { Map.Entry 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 @@ public class PostReview return new RevisionResource(changes.parse(ctl), rev.getPatchSet()); } - private void checkLabels( - RevisionResource rsrc, LabelTypes labelTypes, boolean strict, Map labels) + private void checkLabels(RevisionResource rsrc, LabelTypes labelTypes, Map labels) throws BadRequestException, AuthException, PermissionBackendException { PermissionBackend.ForChange perm = rsrc.permissions(); Iterator> itr = labels.entrySet().iterator(); @@ -483,12 +479,8 @@ public class PostReview Map.Entry 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 @@ 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)); } } } 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 1917d6f7e4..785ae38a58 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 @@ 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; } 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 c46f2cadbc..2f11a105c5 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 @@ -140,12 +140,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", @@ -309,7 +303,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) {