Opened 3 years ago

Closed 3 years ago

#20346 closed Bug (fixed)

cache middleware should vary on full URL

Reported by: jamey Owned by: Tim Graham <timograham@…>
Component: Core (Cache system) Version: master
Severity: Normal Keywords:
Cc: susan.tan.fleckerl@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Standard HTTP cache behavior treats different URLs as different resources, but Django's cache middleware only generates cache keys based on request.get_full_path().

I believe that django.utils.cache._generate_cache_[header_]key should use request.build_absolute_uri() instead of request.get_full_path() so different hosts and schemes will get different cache entries.

I'm using Django 1.3 but as far as I can tell this hasn't changed as of today's git master.

Change History (11)

comment:1 Changed 3 years ago by ludw

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

This seems like a reasonable idea. Can't find any reasons why we shouldn't do this.

comment:2 Changed 3 years ago by Susan Tan

PR is here: https://github.com/django/django/pull/1341 Feel free to code review. Thanks.

comment:3 Changed 3 years ago by Susan Tan

Owner: changed from nobody to Susan Tan
Status: newassigned

comment:4 Changed 3 years ago by anonymous

Has patch: set
Needs tests: set

comment:5 Changed 3 years ago by Susan Tan

Owner: Susan Tan deleted
Status: assignednew

comment:6 Changed 3 years ago by Susan Tan

Cc: susan.tan.fleckerl@… added

comment:7 Changed 3 years ago by Tim Graham

This needs a test to demonstrate the change in behavior.

comment:8 Changed 3 years ago by Tim Graham

Needs documentation: set
Needs tests: unset
Patch needs improvement: set
Version: 1.3master

Here's an updated PR with tests. I left some minor comments for improvement.

One concern is that upgrading Django will change cache keys for all the existing pages now that the domain name is part of the key. At a minimum, this needs to be documented in the release notes. Ideas for how we might be able to do this in a backwards compatible fashion would also be welcome.

Last edited 3 years ago by Tim Graham (previous) (diff)

comment:9 Changed 3 years ago by ijl

I've documented the changes in the 1.7 release notes. I'm at a loss for backwards-compatible changes using the keys, given that existing cache keys use a hash.

comment:10 Changed 3 years ago by Tim Graham

Needs documentation: unset

Docs still need some minor tweaks. Please uncheck "Patch needs improvement" when you have a chance to update it.

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 71a03e01aa19cbde08e915d156abf39b67d904ef:

Fixed #20346 -- Made cache middleware vary on the full URL.

Previously, only the URL path was included in the cache key.

Thanks jamey for the suggestion.

Note: See TracTickets for help on using tickets.
Back to Top