opendev/gerrit
Code Issues Proposed changes

RestApiServlet: Give more useful errors to users

Browse Source 
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
This commit is contained in:
Dave Borowitz
2014-12-05 12:46:29 -08:00
parent 8ba2da9074
commit a2fa864082
3 changed files with 31 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
View File

@@ -57,7 +57,7 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
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());
}

12
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/ResourceNotFoundException.java
View File

@@ -22,19 +22,17 @@ public class ResourceNotFoundException extends RestApiException {
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")

37
gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
View File

@@ -32,7 +32,6 @@ import static javax.servlet.http.HttpServletResponse.SC_PRECONDITION_FAILED;
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 @@ public class RestApiServlet extends HttpServlet {
}
}
} 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 @@ public class RestApiServlet extends HttpServlet {
}
}
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;