Opened 13 years ago
Closed 12 years ago
#20346 closed Bug (fixed)
cache middleware should vary on full URL
| Reported by: | Jamey Sharp | Owned by: | |
|---|---|---|---|
| Component: | Core (Cache system) | Version: | dev |
| 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 by , 12 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 12 years ago
PR is here: https://github.com/django/django/pull/1341 Feel free to code review. Thanks.
comment:3 by , 12 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:4 by , 12 years ago
| Has patch: | set |
|---|---|
| Needs tests: | set |
comment:5 by , 12 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:6 by , 12 years ago
| Cc: | added |
|---|
comment:8 by , 12 years ago
| Needs documentation: | set |
|---|---|
| Needs tests: | unset |
| Patch needs improvement: | set |
| Version: | 1.3 → master |
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.
comment:9 by , 12 years ago
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 by , 12 years ago
| Needs documentation: | unset |
|---|
Docs still need some minor tweaks. Please uncheck "Patch needs improvement" when you have a chance to update it.
comment:11 by , 12 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
This seems like a reasonable idea. Can't find any reasons why we shouldn't do this.