PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
23.51k stars 1.03k forks source link

Shortcomings in HTTP compliance #1089

Open vfaronov opened 6 years ago

vfaronov commented 6 years ago

Thank you for making PostgREST. It clearly attempts to comply with the HTTP protocol, but it falls short in a few places. I’m reporting these all in one issue, because I don’t know if you’re interested in fixing them. These problems are somewhat theoretical: they might trip up generic HTTP components (like caches), but it’s likely that none of your current users really suffer from them. If you prefer, I can split this issue into smaller ones.

I found all this playing with PostgREST 0.4.4.0 (f9e770b).

1. PostgREST uses the Range and Content-Range headers defined in RFC 7233, but it doesn’t implement RFC 7233 properly. Firstly, PostgREST sends Content-Range on all kinds of responses, including 200 (OK) to GET, 201 (Created) to POST, and 204 (No Content) to DELETE. But in RFC 7233 Content-Range is only a feature of special partial responses, which are indicated by status code 206 (Partial Content), which in turn can’t happen unless the request has a Range. RFC 7233 § 4.2:

The Content-Range header field has no meaning for status codes that do not explicitly describe its semantic.

Secondly, the Range and Content-Range headers as used by PostgREST are invalid: they don’t include the range-unit.

This can confuse software like HTTP caches, which might not see how one 200 response to GET /tasks differs from another. (Conversely, if you fix it, a theoretical smart cache might even be able to combine different ranges on the fly.) One way to fix this might be to switch into “standards mode” upon receiving something like Prefer: rfc7233.

2. PostgREST accepts PATCH requests with Content-Type: application/json. As explained in RFC 5789 errata, such patches are bad. A better patch format would be application/merge-patch+json, which has the semantics used by PostgREST, but only in the case of a single object (it doesn’t apply to arrays). PostgREST currently rejects patches with Content-Type: application/merge-patch+json. It might be a good idea to allow them, at least for single objects.

By the way, RFC 5789 § 3.1 also recommends sending an Accept-Patch header in responses to OPTIONS (and then perhaps also Accept-Post?).

3. PostgREST responds with 404 (Not Found) to requests with unsupported methods, such as HEAD. This is wrong, because the resource does exist, it just doesn’t support the method. A better response would be 405 (Method Not Allowed) with an Allow header like in response to OPTIONS.

4. PostgREST responds to OPTIONS with an Allow header that does not contain OPTIONS itself.

5. PostgREST responds to GET + Accept: application/x-something with 415 (Unsupported Media Type). This is wrong, because 415 concerns the Content-Type of the request. The correct status code here would be 406 (Not Acceptable).

Also a couple things that are not protocol violations, but I found them strange:

6. PostgREST responds to GET on multiple objects + Accept: application/vnd.pgrst.object+json with an error, but that error is itself sent with Content-Type: application/vnd.pgrst.object+json.

7. Conversely, PostgREST rejects POST + Content-Type: application/vnd.pgrst.object+json.

royfielding commented 5 years ago

I was about to open a similar issue regarding use of ranges and extended range units, but I will just confirm this one instead. Partial responses need to be correctly described in HTTP because they have an impact on caching, which itself is one of the main reasons to use HTTP. Range units must be provided in Range and Content-Range, not as a separate Range-Unit header field.

I am currently working in the HTTP specification to clarify the role and parsing of range unit extension. They should look something like

GET /people HTTP/1.1
Range: items=0-19

HTTP/1.1 206 Partial Content
Content-Range: items 0-19/*
steve-chavez commented 5 years ago

Sorry for letting this issue go unnoticed.

We definitely need to refocus on HTTP compliance and have an automated way to test such an important aspect as caching.

Thanks a lot for bringing this to attention @vfaronov @royfielding.