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
This commit is contained in:
parent
f87dca186b
commit
9ff64a60c5
|
@ -120,15 +120,10 @@ branch.
|
|||
Votes that are not permitted for the user are silently ignored.
|
||||
|
||||
--label::
|
||||
Set a label by name to the value 'N'. Invalid votes (invalid label
|
||||
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.
|
||||
Set a label by name to the value 'N'. The ability to vote on all specified
|
||||
labels is required. 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::
|
||||
|
|
|
@ -6695,13 +6695,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
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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();
|
||||
|
|
|
@ -60,7 +60,6 @@ public class ReviewInput extends JavaScriptObject {
|
|||
|
||||
private native void init() /*-{
|
||||
this.labels = {};
|
||||
this.strict_labels = true;
|
||||
}-*/;
|
||||
|
||||
public final native void prePost() /*-{
|
||||
|
|
|
@ -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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -134,12 +134,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",
|
||||
|
@ -279,7 +273,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) {
|
||||
|
|
Loading…
Reference in New Issue