ApprovalsUtil: Throw RestApiExceptions from check* methods This means callers don't have to catch the unchecked IllegalArgumentException, which it's hard to guarantee is from this block of code, and is easy to forget. In particular, ChangeInserter previously let IAE propagate, which turned into a generic error during ReceiveCommits if trying to set a disallowed label. (Actually it's still a fairly generic error, but it's now at least possible for future cleanup to ReceiveCommits to report the error properly should it so desire.) Use BadRequestException and AuthException to match the behavior of PostReview for these cases. Change-Id: I267e7168e254d6fc3fb54516feee089d69265d67
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java index 87969ec..910fbc2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -32,6 +32,9 @@ import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.LabelId; @@ -301,6 +304,7 @@ * @param ps patch set being approved. * @param changeCtl change control for user adding approvals. * @param approvals approvals to add. + * @throws RestApiException * @throws OrmException */ public Iterable<PatchSetApproval> addApprovalsForNewPatchSet( @@ -310,7 +314,7 @@ PatchSet ps, ChangeControl changeCtl, Map<String, Short> approvals) - throws OrmException { + throws RestApiException, OrmException { Account.Id accountId = changeCtl.getUser().getAccountId(); checkArgument( accountId.equals(ps.getUploader()), @@ -334,25 +338,26 @@ return cells; } - public static void checkLabel(LabelTypes labelTypes, String name, Short value) { + public static void checkLabel(LabelTypes labelTypes, String name, Short value) + throws BadRequestException { LabelType label = labelTypes.byLabel(name); if (label == null) { - throw new IllegalArgumentException( - String.format("label \"%s\" is not a configured label", name)); + throw new BadRequestException(String.format("label \"%s\" is not a configured label", name)); } if (label.getValue(value) == null) { - throw new IllegalArgumentException( + throw new BadRequestException( String.format("label \"%s\": %d is not a valid value", name, value)); } } - private static void checkApprovals(Map<String, Short> approvals, ChangeControl changeCtl) { + private static void checkApprovals(Map<String, Short> approvals, ChangeControl changeCtl) + throws AuthException { for (Map.Entry<String, Short> vote : approvals.entrySet()) { String name = vote.getKey(); Short value = vote.getValue(); PermissionRange range = changeCtl.getRange(Permission.forLabel(name)); if (range == null || !range.contains(value)) { - throw new IllegalArgumentException( + throw new AuthException( String.format("applying label \"%s\": %d is restricted", name, value)); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 240a3ee..9983928 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -28,6 +28,7 @@ import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; @@ -335,7 +336,8 @@ } @Override - public boolean updateChange(ChangeContext ctx) throws OrmException, IOException { + public boolean updateChange(ChangeContext ctx) + throws RestApiException, OrmException, IOException { change = ctx.getChange(); // Use defensive copy created by ChangeControl. ReviewDb db = ctx.getDb(); ChangeControl ctl = ctx.getControl();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index a6c35ed..5722897 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -63,6 +63,7 @@ import com.google.gerrit.extensions.registration.DynamicMap.Entry; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; @@ -1327,7 +1328,7 @@ try { LabelType.checkName(v.label()); ApprovalsUtil.checkLabel(labelTypes, v.label(), v.value()); - } catch (IllegalArgumentException e) { + } catch (BadRequestException e) { throw clp.reject(e.getMessage()); } labels.put(v.label(), v.value());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java index 4d99dd0..3cd3c1e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
@@ -24,6 +24,7 @@ import com.google.gerrit.common.data.LabelType; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ChangeKind; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; @@ -207,7 +208,8 @@ } @Override - public boolean updateChange(ChangeContext ctx) throws OrmException, IOException { + public boolean updateChange(ChangeContext ctx) + throws RestApiException, OrmException, IOException { change = ctx.getChange(); if (change == null || change.getStatus().isClosed()) { rejectMessage = CHANGE_IS_CLOSED;