Skip to content

Commit

Permalink
Make all HTTP endpoints send return JSON-friendly error responses.
Browse files Browse the repository at this point in the history
When generating an error response (say, 400, 404, 500), honor the
`json' query string parameter, which indicates that the client wants
a JSON response.  This was only done by /q, but is now automagically
done for all endpoints.

In the UI, take advantage of the JSON encoded error responses to
show more meaningful error messages than just "Internal Server Error"
and such.  This allows the UI to show stack traces from the server side
with detailed error messages.

Change-Id: I2af1a02341850e176daf4fff966999fc652701b6
  • Loading branch information
tsuna committed Nov 8, 2010
1 parent 6c1ee67 commit 41cdce4
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 28 deletions.
32 changes: 9 additions & 23 deletions src/tsd/GraphHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -138,30 +138,16 @@ private void doGraph(final TSDB tsdb, final HttpQuery query)
}
Query[] tsdbqueries;
List<String> options;
try {
tsdbqueries = parseQuery(tsdb, query);
options = query.getQueryStringParams("o");
if (options == null) {
options = new ArrayList<String>(tsdbqueries.length);
for (int i = 0; i < tsdbqueries.length; i++) {
options.add("");
}
} else if (options.size() != tsdbqueries.length) {
throw new BadRequestException(options.size() + " `o' parameters, but "
+ tsdbqueries.length + " `m' parameters.");
}
} catch (BadRequestException e) {
if (query.hasQueryStringParam("json")) {
final String err = e.getMessage();
final StringBuilder buf = new StringBuilder(10 + err.length());
buf.append("{\"err\":\"");
HttpQuery.escapeJson(err, buf);
buf.append("\"}");
query.sendReply(buf);
return;
} else {
throw e;
tsdbqueries = parseQuery(tsdb, query);
options = query.getQueryStringParams("o");
if (options == null) {
options = new ArrayList<String>(tsdbqueries.length);
for (int i = 0; i < tsdbqueries.length; i++) {
options.add("");
}
} else if (options.size() != tsdbqueries.length) {
throw new BadRequestException(options.size() + " `o' parameters, but "
+ tsdbqueries.length + " `m' parameters.");
}
for (final Query tsdbquery : tsdbqueries) {
try {
Expand Down
24 changes: 21 additions & 3 deletions src/tsd/HttpQuery.java
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,14 @@ public void internalError(final Exception cause) {
tp.calculatePackagingData();
final String pretty_exc = ThrowableProxyUtil.asString(tp);
tp = null;
{
if (hasQueryStringParam("json")) {
// 32 = 10 + some extra space as exceptions always have \t's to escape.
final StringBuilder buf = new StringBuilder(32 + pretty_exc.length());
buf.append("{\"err\":\"");
HttpQuery.escapeJson(pretty_exc, buf);
buf.append("\"}");
sendReply(HttpResponseStatus.INTERNAL_SERVER_ERROR, buf);
} else {
sendReply(HttpResponseStatus.INTERNAL_SERVER_ERROR,
makePage("Internal Server Error", "Houston, we have a problem",
"<blockquote>"
Expand All @@ -214,7 +221,13 @@ public void internalError(final Exception cause) {
* @param explain The string describing why the request is bad.
*/
public void badRequest(final String explain) {
{
if (hasQueryStringParam("json")) {
final StringBuilder buf = new StringBuilder(10 + explain.length());
buf.append("{\"err\":\"");
HttpQuery.escapeJson(explain, buf);
buf.append("\"}");
sendReply(HttpResponseStatus.BAD_REQUEST, buf);
} else {
sendReply(HttpResponseStatus.BAD_REQUEST,
makePage("Bad Request", "Looks like it's your fault this time",
"<blockquote>"
Expand All @@ -231,7 +244,12 @@ public void badRequest(final String explain) {
/** Sends a 404 error page to the client. */
public void notFound() {
logWarn("Not Found: " + request.getUri());
sendReply(HttpResponseStatus.NOT_FOUND, PAGE_NOT_FOUND);
if (hasQueryStringParam("json")) {
sendReply(HttpResponseStatus.NOT_FOUND,
new StringBuilder("{\"err\":\"Page Not Found\"}"));
} else {
sendReply(HttpResponseStatus.NOT_FOUND, PAGE_NOT_FOUND);
}
}

/** An empty JSON array ready to be sent. */
Expand Down
34 changes: 32 additions & 2 deletions src/tsd/client/QueryUi.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import com.google.gwt.user.client.ui.CheckBox;
import com.google.gwt.user.client.ui.DecoratedTabPanel;
import com.google.gwt.user.client.ui.DecoratorPanel;
import com.google.gwt.user.client.ui.DisclosurePanel;
import com.google.gwt.user.client.ui.FlexTable;
import com.google.gwt.user.client.ui.HTML;
import com.google.gwt.user.client.ui.HorizontalPanel;
Expand Down Expand Up @@ -591,8 +592,37 @@ public void onResponseReceived(final Request request,
callback.got(JSONParser.parse(response.getText()));
return;
} else if (code >= Response.SC_BAD_REQUEST) { // 400+ => Oops.
displayError("Request failed while getting " + url + ": "
+ response.getStatusText());
String err = response.getText();
// If the response looks like a JSON object, it probably contains
// an error message.
if (!err.isEmpty() && err.charAt(0) == '{') {
final JSONValue json = JSONParser.parse(err);
final JSONObject result = json == null ? null : json.isObject();
final JSONValue jerr = result == null ? null : result.get("err");
final JSONString serr = jerr == null ? null : jerr.isString();
err = serr.stringValue();
// If the error message has multiple lines (which is common if
// it contains a stack trace), show only the first line and
// hide the rest in a panel users can expand.
final int newline = err.indexOf('\n', 1);
final String msg = "Request failed: " + response.getStatusText();
if (newline < 0) {
displayError(msg + ": " + err);
} else {
displayError(msg);
final DisclosurePanel dp =
new DisclosurePanel(err.substring(0, newline));
RootPanel.get("queryuimain").add(dp); // Attach the widget.
final InlineLabel content =
new InlineLabel(err.substring(newline, err.length()));
content.addStyleName("fwf"); // For readable stack traces.
dp.setContent(content);
current_error.getElement().appendChild(dp.getElement());
}
} else {
displayError("Request failed while getting " + url + ": "
+ response.getStatusText());
}
graphstatus.setText("");
}
}
Expand Down

0 comments on commit 41cdce4

Please sign in to comment.