RestApiServlet: Give more useful errors to users
REST API handlers actually try pretty hard to give readable error
messages in their Gerrit-specific REST exception instances, even in
generic cases like ResourceNotFoundException. RestApiServlet was doing
the end user no favors by swallowing those, so stop.
For non-Gerrit-specific exception classes, which may be generated in
code not under our control, we continue to assume that the exception
message should not be shown to the user.
Change-Id: Ice3484e21c76019846874a8c39820672ad9bd2f0
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
index 10fbed0..2442b595 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
@@ -57,7 +57,7 @@
assertEquals(triplet, c.id);
assertEquals(ChangeStatus.DRAFT, c.status);
RestResponse r = deletePatchSet(changeId, ps, userSession);
- assertEquals("Not found", r.getEntityContent());
+ assertEquals("Not found: " + changeId, r.getEntityContent());
assertEquals(404, r.getStatusCode());
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
index 76942e6..611812a 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
@@ -22,19 +22,17 @@
public ResourceNotFoundException() {
}
- /** @param id portion of the resource URI that does not exist. */
- public ResourceNotFoundException(String id) {
- super(id);
+ public ResourceNotFoundException(String msg) {
+ super(msg);
}
- /** @param id portion of the resource URI that does not exist. */
- public ResourceNotFoundException(String id, Throwable cause) {
- super(id, cause);
+ public ResourceNotFoundException(String msg, Throwable cause) {
+ super(msg, cause);
}
/** @param id portion of the resource URI that does not exist. */
public ResourceNotFoundException(IdString id) {
- super(id.get());
+ super("Not found: " + id.get());
}
@SuppressWarnings("unchecked")
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index bf29c60..634e3e9 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -32,7 +32,6 @@
import com.google.common.base.Function;
import com.google.common.base.Joiner;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMultimap;
@@ -345,27 +344,34 @@
}
}
} catch (AuthException e) {
- replyError(req, res, status = SC_FORBIDDEN, e.getMessage(), e.caching(), e);
+ replyError(req, res, status = SC_FORBIDDEN, messageOr(e, "Forbidden"),
+ e.caching(), e);
} catch (BadRequestException e) {
- replyError(req, res, status = SC_BAD_REQUEST, e.getMessage(), e.caching(), e);
+ replyError(req, res, status = SC_BAD_REQUEST, messageOr(e, "Bad request"),
+ e.caching(), e);
} catch (MethodNotAllowedException e) {
- replyError(req, res, status = SC_METHOD_NOT_ALLOWED, "Method not allowed", e.caching(), e);
+ replyError(req, res, status = SC_METHOD_NOT_ALLOWED,
+ messageOr(e, "Method not allowed"), e.caching(), e);
} catch (ResourceConflictException e) {
- replyError(req, res, status = SC_CONFLICT, e.getMessage(), e.caching(), e);
+ replyError(req, res, status = SC_CONFLICT, messageOr(e, "Conflict"),
+ e.caching(), e);
} catch (PreconditionFailedException e) {
replyError(req, res, status = SC_PRECONDITION_FAILED,
- MoreObjects.firstNonNull(e.getMessage(), "Precondition failed"), e.caching(), e);
+ messageOr(e, "Precondition failed"), e.caching(), e);
} catch (ResourceNotFoundException e) {
- replyError(req, res, status = SC_NOT_FOUND, "Not found", e.caching(), e);
+ replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Not found"),
+ e.caching(), e);
} catch (UnprocessableEntityException e) {
- replyError(req, res, status = 422,
- MoreObjects.firstNonNull(e.getMessage(), "Unprocessable Entity"), e.caching(), e);
+ replyError(req, res, status = 422, messageOr(e, "Unprocessable Entity"),
+ e.caching(), e);
} catch (AmbiguousViewException e) {
- replyError(req, res, status = SC_NOT_FOUND, e.getMessage(), e);
+ replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e);
} catch (MalformedJsonException e) {
- replyError(req, res, status = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e);
+ replyError(req, res, status = SC_BAD_REQUEST,
+ "Invalid " + JSON_TYPE + " in request", e);
} catch (JsonParseException e) {
- replyError(req, res, status = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e);
+ replyError(req, res, status = SC_BAD_REQUEST,
+ "Invalid " + JSON_TYPE + " in request", e);
} catch (Exception e) {
status = SC_INTERNAL_SERVER_ERROR;
handleException(e, req, res);
@@ -377,6 +383,13 @@
}
}
+ private static String messageOr(Throwable t, String defaultMessage) {
+ if (!Strings.isNullOrEmpty(t.getMessage())) {
+ return t.getMessage();
+ }
+ return defaultMessage;
+ }
+
private static boolean notModified(HttpServletRequest req, RestResource rsrc) {
if (!isGetOrHead(req)) {
return false;
