opendev/gerrit
Code Issues Proposed changes

Do not throw generic RestApiException to signal an internal server error

Browse Source 
Just throw the PermissionBackendException which will be mapped to
internal server error too. This removes the specific exception message,
but the same information is available from the stacktrace which will be
logged. Since this was an internal server error, the message wasn't
returned to clients anyway.

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

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I8b4a481d87bbed2fb102a4f203472b94b00fd521
This commit is contained in:
Edwin Kempin
2019-12-23 14:30:40 +01:00
parent 9f715382ab
commit b5bee5779e
2 changed files with 8 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/api/plugins/PluginApiImpl.java
View File

@@ -69,7 +69,11 @@ public class PluginApiImpl implements PluginApi {
@Override
public void disable() throws RestApiException {
try {
disable.apply(resource, new Input());
} catch (Exception e) {
throw asRestApiException("Cannot disable plugin", e);
}
}
@Override

7
java/com/google/gerrit/server/plugins/DisablePlugin.java
View File

@@ -45,12 +45,9 @@ public class DisablePlugin implements RestModifyView<PluginResource, Input> {
}
@Override
public Response<PluginInfo> apply(PluginResource resource, Input input) throws RestApiException {
try {
public Response<PluginInfo> apply(PluginResource resource, Input input)
throws RestApiException, PermissionBackendException {
permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (PermissionBackendException e) {
throw new RestApiException("Could not check permission", e);
}
loader.checkRemoteAdminEnabled();
String name = resource.getName();
if (mandatoryPluginsCollection.contains(name)) {