From 4e6ffddaad83e8240eb8c4120d04f4f228375e02 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Feb 2017 10:03:06 -0500 Subject: [PATCH] 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 --- .../google/gerrit/server/ApprovalsUtil.java | 19 ++++++++++++------- .../gerrit/server/change/ChangeInserter.java | 4 +++- .../gerrit/server/git/ReceiveCommits.java | 3 ++- .../google/gerrit/server/git/ReplaceOp.java | 4 +++- 4 files changed, 20 insertions(+), 10 deletions(-) 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 87969ece62..910fbc294c 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.LabelType; 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 @@ public class ApprovalsUtil { * @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 addApprovalsForNewPatchSet( @@ -310,7 +314,7 @@ public class ApprovalsUtil { PatchSet ps, ChangeControl changeCtl, Map approvals) - throws OrmException { + throws RestApiException, OrmException { Account.Id accountId = changeCtl.getUser().getAccountId(); checkArgument( accountId.equals(ps.getUploader()), @@ -334,25 +338,26 @@ public class ApprovalsUtil { 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 approvals, ChangeControl changeCtl) { + private static void checkApprovals(Map approvals, ChangeControl changeCtl) + throws AuthException { for (Map.Entry 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 240a3eecdc..9983928320 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.common.data.LabelTypes; 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 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { } @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 a6c35ed9c6..572289740d 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; 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 @@ public class ReceiveCommits { 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 4d99dd09ff..3cd3c1edcc 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.Nullable; 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 @@ public class ReplaceOp extends BatchUpdate.Op { } @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;