opendev/gerrit
Code Issues Proposed changes

ApprovalsUtil: Throw RestApiExceptions from check* methods

Browse Source 
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
This commit is contained in:
Dave Borowitz
2017-02-21 10:03:06 -05:00
parent 8e247848ab
commit 4e6ffddaad
4 changed files with 20 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -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());