Opened 2 years ago

Closed 18 months 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 2 years ago by ludw

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 2 years ago by susan

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

comment:3 Changed 2 years ago by susan

  • Owner changed from nobody to susan
  • Status changed from new to assigned

comment:4 Changed 2 years ago by anonymous

  • Has patch set
  • Needs tests set

comment:5 Changed 2 years ago by susan

  • Owner susan deleted
  • Status changed from assigned to new

comment:6 Changed 2 years ago by susan

  • Cc susan.tan.fleckerl@… added

comment:7 Changed 2 years ago by timo

This needs a test to demonstrate the change in behavior.

comment:8 Changed 21 months ago by timo

  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement set
  • Version changed from 1.3 to 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.

Last edited 21 months ago by timo (previous) (diff)

comment:9 Changed 21 months 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 20 months ago by timo

  • 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 18 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

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