Propagate errors sensibly from proxied IS requests

[dbkr]

When we're proxying Matrix endpoints, parse out Matrix error
responses and turn them into SynapseErrors so they can be
propagated sensibly upstream.

This will allow clients to give sensible error messages if the ID server rejects their request, for example vector-im/riot-web#3542

[ara4n]

do we really want to raise an exception on 300s?

[dbkr]

Probably not, yeah

[ara4n]

I'm a bit confused - why can't we just pass the Matrix-style error response through to the client?

[dbkr]

Basically we just want to indicate that an error has occurred, and since post_json_get_json just returns the body, previously we were masking the error status code and behaving as if the endpoint returned 2xx which confused things. The main thing is that we raise an exception on error responses, and if we turn it into a SynapseException then it just works without having to handle it specifically in all the different places.

[ara4n]

looks plausible beyond the fact i think i'm entirely missing how this works :D might need slightly more of an explanation as to why turning a matrix error into a synapse error is actually the soln.

[erikjohnston]

What's the rationale here for not making the simple http client look for synapse style error messages?

[dbkr]

It seems pretty terrible for the HTTP client to look for any error responses that happen to have 'error' and 'errcode' keys in the response and interpret them without necessarily knowing what that means at the app protocol level.

[erikjohnston]

Having yet another HTTP client is going to be a bit confusing, I would have thought.

There are two choices for doing this in SimpleHttpClient, the first is to add a parameter to the functions to specify whether it should try and parse synapse style error messages. This would be something like is_remote_a_matrix_server or something.

The other option, that i think is probably nicer, is to add a subclass to CodeMessageException that is thrown when we managed to parse the error message as a synapse one. Thus callers can either ignore the new exception if don't care, or do something like the following if they do:

try:
    result = client.get_json(...)
    ...
except MatrixCodeMessageException as e:
    raise e.as_synapse_error()  # Converts the exception to SynapseError

[dbkr]

OK - I'd like to understand a bit better why more clients is going to be more confusing, but happy to change it back if you prefer.

[dbkr]

Done - ptal

[erikjohnston]

Generally we prefer to list the types of exceptions, as otherwise its too easy to catch a typo or something. Its probably fine for me.

[erikjohnston]

lgtm