Handle internal server errors in the registration form

[seanh]

This fixes #1755: no error is shown on the client side when an attempt
to register an account receives a 500 Server Error response.

The failure() method in AuthController gets called and tries to do
{errors, reason} = response.data, but this raises a TypeError because
response.data is undefined (because the response didn't have a valid
JSON body), so the client crashes as well.

The fix is to catch this exception on the client side, and show a
generic error to the user.

[seanh]

@csillag What do you think? Is this a fair solution to the problem? When debugging the failure() method in AuthController, the response object did not seem to contain any useful information that could be used to provide a more informative error message, so I just fell back on a completely generic one.

If we want to provide some kind of more informative error that actually comes from the server response, we'd have to catch the error somewhere else. The h server actually sends:

500 Server Error, We're very sorry, our application wasn't able to load this page. The team has been
notified and we'll fix it shortly (and a sad face image).

But that's in the body of an HTML response that contains CSS, HTML tags, I don't think there's any reasonable way to parse a plain string error out of that for us to use.

I feel like we should at least be able to get the 500 Server Error response header from somewhere though, so maybe this is not the right place to catch this.

I am still getting a JSON parsing error in the JavaScript console as well, even with this fix in place. Some code in session.min.js tries to parse the response body as JSON and spits out an error. This actually looks like something Angular does automatically,

[seanh]

A test should be added for this

[tilgovi]

We could also add an error renderer in h.views for accept=application/json and return JSON errors.

[tilgovi]

That won't catch issues that happen outside the application, in the WSGI stack somewhere, but it would catch errors within the application.

[csillag]

@csillag What do you think? Is this a fair solution to the problem?

Well, it's certainly an improvement. I would accept any user-visible indication of an error as a valid resolution of this issue.

[seanh]

@tilgovi

We could also add an error renderer in h.views for accept=application/json and return JSON errors.

The problem is in this case we don't know what type of exception we're looking for (the issue is just to handle any type of "server error", I've been testing by hacking Horus to raise AssertionError and RuntimeError etc on user registration), so would you do:

@view_config(context=Exception, ...

(This works, we can make the callable return a 500 with a JSON body in the format the client expects for errors.)

[seanh]

Added a JSON error view, so the frontend now shows the error string from the server in case of an exception on the server. If the server has an exception that isn't caught by this error view, then the frontend falls back on its own generic error message.

This does override Accept:*/* requests and return the error in JSON rather than HTML. But as long as the request has Accept:text/html (and it comes before any application/json in the Accept header) then it'll get the HTML error page. Browsers still get the HTML.

[BigBlueHat]

👍 for the JSON Error response.

What do you all think of sending something more "informative"? Here's some options:
http://www.hydra-cg.com/spec/latest/core/#description-of-http-status-codes-and-errors
and
https://github.com/blongden/vnd.error (based on HAL style _link constructs).

Since we'd have to be "all in" on HAL, I'd vote for using the Hydra option (if we'd even be open to considering it at this point.

I'll try and craft more Hypermedia API thoughts separately, but the thought was that sending a JSON-LD-based Hydra response would at least put us closer to an eventual Open Annotation Data Model API...plus it provides URLs to meanings for things...which is pretty rad. 😉

If this is too crazy an 💡 at this point...just ignore me. 🎩

[seanh]

Not sure. I'm all for giving specific error messages for specific types of error that we handle, but this page handles unexpected exceptions (e.g. due to some unknown bug), the only way to try to get a more specific error would be to call str() or repr() on the exception object, which is just gonna reveal Python exceptions to the user. I've followed the same approach we took for our existing HTML error page and just sent a generic error.

[BigBlueHat]

@seanh yeah. The idea is less about specific types of errors (though that is the Hydra example I linked too... 😛) and more about having "Things" instead of "Strings."

By providing a JSON-LD @context that references the Hydra spec, devs and client code can "understand" what title and description are meant to represent. Without an @context the reason key (or title, description, whatever) could all just as well be written @#$%#...as long as we documented it for a human being to hard code into their client. 😉

It's a shift in thinking away from "magic strings" that exist in only human readable clients, toward "explained things" that have some semantic definitions that machines can (at their option) "understand."

Might be too futuristic for this PR, but it's where things are headed. 🚀 🔬

[nickstenning]

Rebased this for you.

[tilgovi]

I think you want to say reason = because formRespond sets responseErrorMessage along with marking the form as invalid.

[seanh]

Done

[seanh]

I wanted to add a Python test that the json_error() gets used and does what's expected, if the view callable raises an uncaught exception. But I can't find a way to make a Pyramid view callable raise an exception for testing purposes (you can't patch the view callable functions).

@tilgovi @csillag @nickstenning If one of you could review this, once it gets the go-ahead I'll rebase it

[tilgovi]

LGTM

[nickstenning]

Eek. Can we be a bit more careful about merging histories like this? The diff looks good, sure, but the history includes a merge of a branch into itself. This should really have had a rebase before going into master.

[tilgovi]

I used to care about these things but I stopped. Merge commits FTW, everything else in swept under the rug.

But I don't mind if we want to insist on some standards there.

[nickstenning]

VCS history is incredibly important when debugging. Having a history that makes it clear what happened when and why makes it orders of magnitude easier to understand a codebase as it grows.

I'm in favour of us striving to write better commit messages and rebasing pull requests to have a clear, atomic history. And certainly no second-order merge commits.