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

Content Range unit for RFC 7233 compliance #2199

Closed
wants to merge 1 commit into from

Conversation

Oquile
Copy link

@Oquile Oquile commented Mar 23, 2022

The Content Range response HTTP header is missing a unit declaration. It should be in the form :
Content-Range: <unit> <range-start>-<range-end>/<size>

Supplementary docs-
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Range
https://httpwg.org/specs/rfc7233.html#header.content-range

This change also makes PostgREST compatible with Varnish HTTP Cache (https://varnish-cache.org/).

The Content Range response HTTP header is missing a unit declaration. It should be in the form :
'Content-Range: <unit> <range-start>-<range-end>/<size>'

Supplementary docs-
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Range
https://httpwg.org/specs/rfc7233.html#header.content-range

This change also makes PostgREST compatible with Varnish HTTP Cache (https://varnish-cache.org/).
@Oquile
Copy link
Author

Oquile commented Mar 23, 2022

This breaks build and tests. Will close and re-roll another

@Oquile Oquile closed this Mar 23, 2022
@steve-chavez
Copy link
Member

This change also makes PostgREST compatible with Varnish HTTP Cache (https://varnish-cache.org/).

@Oquile Definitely something we want, the lack of range-unit was also reported in #1089.

@steve-chavez
Copy link
Member

@Oquile You still interested in this one? Since it's a breaking change I'd be better to merge it now, I'll be doing a new release later today.

@steve-chavez
Copy link
Member

Seeing #1089 (comment), I think we should go with items instead of bytes for our <range-unit>.

There's also a Content-Range: rows.. I found in the wild but I don't think the name matters as much, let's go with the most common one(which seems to be items).

@Oquile
Copy link
Author

Oquile commented Mar 25, 2022

@steve-chavez - It is (currently!) beyond my ablity to do a succesfully patch for this quickly.

@wolfgangwalther
Copy link
Member

Since it's a breaking change I'd be better to merge it now, I'll be doing a new release later today.

I think it would be better to be able to discuss breaking changes - and not have a workflow of doing them and then releasing immediately.

If you want to make a release soon, this should not go in.

@steve-chavez
Copy link
Member

It is (currently!) beyond my ablity to do a succesfully patch for this quickly.

No problem! Seemed like a simple fix so I gave it a try on #2204.. I bet it breaks a lot of tests though.

If you want to make a release soon, this should not go in.

Bummer, this seems like something in need of a fix for quite a while now.

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

Successfully merging this pull request may close these issues.

None yet

3 participants