opendev/gerrit
Code Issues Proposed changes

Make RestApiException(String, Exception) constructor protected

Browse Source 
This constructor should only be used by subclasses and for wrapping
other exceptions, but not for signaling internal server errors.

Throwing RestApiExceptions to signal internal server errors is bad
since all RestApiExceptions are exempt from retries and auto-tracing.

To prevent that a RestApiException is accidentally thrown to signal an
internal server error we:

* make the RestApiException(String, Exception) constructor protected so
  that it can only be used by subclasses
* offer a static method for exception wrapping

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I3495cb63f66a669ac5849d5f28c3ce8cc4e5ae95
This commit is contained in:
Edwin Kempin 2019-12-23 15:00:42 +01:00
parent b5bee5779e
commit 7c97209f6d
4 changed files with 8 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/extensions/restapi/RestApiException.java
View File

@ -21,13 +21,17 @@ public class RestApiException extends Exception {
private static final long serialVersionUID = 1L;
private CacheControl caching = CacheControl.NONE;
public static RestApiException wrap(String msg, Exception e) {
return new RestApiException(msg, e);
}
protected RestApiException() {}
protected RestApiException(String msg) {
super(msg);
}
public RestApiException(String msg, Throwable cause) {
protected RestApiException(String msg, Throwable cause) {
super(msg, cause);
}

2
java/com/google/gerrit/server/api/ApiUtil.java
View File

@ -32,7 +32,7 @@ public class ApiUtil {
public static RestApiException asRestApiException(String msg, Exception e)
throws RuntimeException {
Throwables.throwIfUnchecked(e);
return e instanceof RestApiException ? (RestApiException) e : new RestApiException(msg, e);
return e instanceof RestApiException ? (RestApiException) e : RestApiException.wrap(msg, e);
}
private ApiUtil() {}

2
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@ -293,7 +293,7 @@ class ReceiveCommits {
} else if ((e instanceof ExecutionException) && (e.getCause() instanceof RestApiException)) {
return (RestApiException) e.getCause();
}
return new RestApiException("Error inserting change/patchset", e);
return RestApiException.wrap("Error inserting change/patchset", e);
}
// ReceiveCommits has a lot of fields, sorry. Here and in the constructor they are split up

2
plugins/plugin-manager

@ -1 +1 @@
Subproject commit 828d666bbb4aae1a2c348a12d7855ec5db3be46f
Subproject commit 2933add62ecf2cbfc28cfe2cff81ff0e0eecc913