Error payload

[stefanoborini]

Fixes #175

Missing tests. Will be added.

[codecov-io]

Current coverage is 88.69% (diff: 90.42%)

Merging #186 into master will increase coverage by 0.85%

@@             master       #186   diff @@
==========================================
  Files            36         37     +1   
  Lines          1275       1336    +61   
  Methods           0          0          
  Messages          0          0          
  Branches        121        129     +8   
==========================================
+ Hits           1120       1185    +65   
+ Misses          116        112     -4   
  Partials         39         39          

Powered by Codecov. Last update adbc9aa...869230a

[stefanoborini]

Note that it's open to discussion if we should deliver a payloaded HTTPError for any exception that may occur. Right now, only REST exceptions produce an error that is potentially payloaded. Any other exception will produce a regular internal server error with no payload.

[kitchoi]

These changes on the doc do not seem relevant to this PR, can you create an issue separately please?

[kitchoi]

Alternative to catching RESTException and raising a custom PayloadedHTTPError, RESTBaseHandler.write_error can recognise that the Exception is a RESTException and compose the payload accordingly.

[stefanoborini]

@kitchoi I already thought about that and I don't like it, because in write_error you have no knowledge of which methods was actually invoked.

[kitchoi]

Class method for PayloadedHTTPError?

[stefanoborini]

No. In the future, the conversion will be done by a serializer that may produce xml instead of json. This serializer will be a instance var.

[kitchoi]

Why is this a problem?

[stefanoborini]

Why would anyone say "application/json" and then provide no payload?

[kitchoi]

It is not what you would expect, but does it actually cause problem? If not, seem to be an unnecessary potential failure here.

[stefanoborini]

It is not unnecessary. The class constraints are clear. if you specify the payload, you can or cannot specify the content type. If you don't, it's by default text/plain. If you don't specify a payload, there's no reason for specifying the content type, and if you do, you are doing something very wrong, because it means that your payload is None by accident.

[kitchoi]

Is it wrong to set the header content-type to something but provide an empty content?
(by the way you could default payload to an empty string, just an idea)

[stefanoborini]

There's a difference between empty payload an no payload. some http responses don't want payload, and it's invalid if they do provide one (e.g. 204 No content, or 404 Not Found)

[stefanoborini]

At least in the context of REST, I mean. 404 can have a payload in non-rest context

[kitchoi]

No payload but has content-type header - is it just because it is wrong in terms of definition or does it cause error?

[stefanoborini]

it causes an error.

[kitchoi]

Would you explain how please? I just wanted to understand.

[kitchoi]

Just a comment, instead of raising an Error, one could also call send_error and pass keyword arguments to it, the keyword arguments can then be passed to write_error. These two methods are designed to be extendable. So for example, one could instead of raising PayloadedHTTPError, call send_error(status_code, ..., payload='...')

[itziakos]

👍