Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shortcomings in HTTP compliance #1089

Open
vfaronov opened this issue Apr 1, 2018 · 2 comments
Open

Shortcomings in HTTP compliance #1089

vfaronov opened this issue Apr 1, 2018 · 2 comments
Labels
http http compliance

Comments

@vfaronov
Copy link

vfaronov commented Apr 1, 2018

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
Copy link

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http http compliance
Development

No branches or pull requests

3 participants