REVIEW NEEDED [WIP] global: add vary header to the response #273
Conversation
| @@ -30,6 +30,7 @@ def view(pid, record, code=200, headers=None, links_factory=None): | |||
| response.status_code = code | |||
| response.set_etag(str(record.revision_id)) | |||
| response.last_modified = record.updated | |||
| response.vary = 'Content-Type, Accept' | |||
There was a problem hiding this comment.
Not sure if anybody is relying on them, but I think it's better to use other Accept- headers too, especially Accept-Language
There was a problem hiding this comment.
I think since you provide Accept the Content-Type is redundant.
I would go with response.vary = 'Accept, Accept-Encoding, Accept-Language'
There was a problem hiding this comment.
once this PR gets approved please use the headers like @topless is suggesting
| @@ -75,3 +76,49 @@ def test_item_get_invalid_mimetype(app, test_records): | |||
| # Check that GET with non accepted format will return 406 | |||
| res = client.get(record_url(pid), headers=[('Accept', 'video/mp4')]) | |||
| assert res.status_code == 406 | |||
|
|
|||
|
|
|||
| @pytest.mark.skip | |||
There was a problem hiding this comment.
please add a comment why the test is skipped
equadon
force-pushed
the
vary-content
branch
from
17eab2a to
a7974cf
Compare
equadon
changed the title
kpsherva
changed the title
|
As far as I have seen, |
FOR THE REVIEWERS: this is a proposal of the solution, it will be applied not only for the get method but please first share your thoughts
( addresses #250)
Update:
The main issue seemed to be that the vary header had no effect if the
etagwas unchanged so the new WIP version has theetaginclude not only therecord.revision_idbut also the mimetype. It's still unclear which effect theVaryheader has but without it sometimes it doesn't recognize if a change is made in theAcceptheader so it seems to be needed.Requires inveniosoftware/invenio-rest#100
PAST NOTES ON THE ISSUE:
THIS IS not working fully - below my investigation
How to test:
3.1. on your clean instance create custom serializer of the record f.e. application/x-custom mimetype - make sure it returns also this content type as a response example: https://github.com/kprzerwa/test-headers-instance/blob/master/my_site/records/config.py
You can use firefox for debugging - it has neat way of editing headers in the dev tools
/records/1Vary: Content-Type, Acceptheader - check this PRapplication/x-customin the request and request again the endpoint. You will get the same json response, cached, while you should get 200 status code response with x-custom content type.What is probably the problem here: The cached response is not adding the Vary header so the browser does not know that it should check the Accept value every time, and relies only on the combination of
ETag(which is version of the record) passed toIf-None-Matchheader andIf-Modified-Sinceheader which won't change in this case.