Ratings and Reviews server

Merge lp://staging/~fgallina/rnr-server/fix-vary-headers-for-api-endpoints into lp://staging/rnr-server

fix-vary-headers-for-api-endpoints
Merge into trunk

Proposed by Fabián Ezequiel Gallina on 2014-07-03

Status:	Merged
Approved by:	Fabián Ezequiel Gallina on 2014-07-03
Approved revision:	252
Merged at revision:	251
Proposed branch:	lp://staging/~fgallina/rnr-server/fix-vary-headers-for-api-endpoints
Merge into:	lp://staging/rnr-server
Diff against target:	435 lines (+252/-27) 7 files modified src/clickreviews/api/urls.py (+3/-2) src/clickreviews/tests/test_handlers.py (+7/-0) src/reviewsapp/api/decorators.py (+42/-0) src/reviewsapp/api/urls.py (+20/-7) src/reviewsapp/tests/test_decorators.py (+116/-1) src/reviewsapp/tests/test_handlers.py (+51/-0) src/reviewsapp/tests/test_rnrclient.py (+13/-17)
To merge this branch:	bzr merge lp://staging/~fgallina/rnr-server/fix-vary-headers-for-api-endpoints
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Ricardo Kirkner (community)		2014-07-03	Approve on 2014-07-03
Review via email: mp+225535@code.staging.launchpad.net

Commit message

Fix vary headers for unauthenticated endpoints

Previously, Vary headers were completely removed for these without
taking in consideration the Accept header (nor the Accept-Encoding in
the case for review-stats) sent by the client, therefore incorrect
content was returned for clients expecting different encodings or
serialization than what was cached.

Revision history for this message

Ricardo Kirkner (ricardokirkner) wrote on 2014-07-03:

LGTM overall.

General question: this service is base completely on anonymous users, right? Otherwise, why don't we vary on Cookie or Authentication too?

Also, why do we vary on Accept for some things and on Accept, Accept-Encoding on others?

Revision history for this message

Fabián Ezequiel Gallina (fgallina) wrote on 2014-07-03:

We have both, authenticated and unauthenticated views, for unauthenticated views there was an explicit desire of not vary at all so cache hits would be increased. This just adds necessary headers in order to not mess with clients by serving wrong cached content. For authenticated endpoints we just happen to allow POST only, so no caching is done there but the Authentication and other vary headers are allowed to be added if necessary (piston takes care of that), so there are no need to explicitly set Vary on them.

On the second question: in the places we vary for Accept, Accept-Encoding is where we use the gzip_content decorator, which would have added such header automatically, but since the vary_only_on_headers kills previously set Vary values we need to explicitly set it back again for review-stats related views.

lp://staging/~fgallina/rnr-server/fix-vary-headers-for-api-endpoints updated on 2014-07-03

252. By Fabián Ezequiel Gallina on 2014-07-03: Fix typo

Revision history for this message

Ricardo Kirkner (ricardokirkner) wrote on 2014-07-03:

Thanks for the answers. I'm not particularly fond of losing work already done (eg, by the gzip_content decorator) but then I supposed that's a drawback that has already been considered, plus being explicit about vary headers is not bad either.

review: Approve