Opened 8 years ago

Closed 5 years ago

#5176 closed (fixed)

Bug in django.utils.cache._generate_cache_key(request, headerlist, key_prefix)

Reported by: Eratothene Owned by: nobody
Component: Core (Cache system) Version: master
Severity: Keywords: cache session testclient
Cc: ryszard.szopa+django@…, sciyoshi@…, karld@…, lcordier@…, charpentier.johan@…, me@…, andy@…, paulswartz@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Checked on latest SVN

If the !Order of middlewares goes like this:
'django.middleware.cache.CacheMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',

And I want the order to be exactly like this, because SessionMiddleware adds Vary on Cookie where appropriate.
Django dies on every request, which actually varyies on cookies. Generate_cache_key tries to get cookie into the cache key, what
is exactly what I want, but fails as it expects string not a SimpleCookie. Path is trivial though..

  File "/usr/local/lib/python2.5/site-packages/django/middleware/cache.py", line 82, in process_response
    cache_key = learn_cache_key(request, response, self.cache_timeout, self.key_prefix)
  File "/usr/local/lib/python2.5/site-packages/django/utils/cache.py", line 163, in learn_cache_key
    return _generate_cache_key(request, headerlist, key_prefix)
  File "/usr/local/lib/python2.5/site-packages/django/utils/cache.py", line 120, in _generate_cache_key
    ctx.update(value)
TypeError: update() argument 1 must be string or read-only buffer, not SimpleCookie


Attachments (6)

patch.diff (769 bytes) - added by Eratothene 8 years ago.
patch to django.utils.cache
patch.2.diff (489 bytes) - added by Eratothene 8 years ago.
The same patc
0001_fix_test_client.diff (1.8 KB) - added by lcordier 7 years ago.
Fixes SimpleCookie problem in django.test.client.Client.
0001_generate_cache_key.diff (1.2 KB) - added by lcordier 7 years ago.
A much more elegant solution.
0002_generate_cache_key.diff (3.6 KB) - added by lcordier 7 years ago.
Some basic test code added, simply check that cookies work the they are supposed to work.
0001-Fixed-5176-TestClient-now-can-accept-SimpleCookies-w.patch (4.3 KB) - added by tclineks 6 years ago.

Download all attachments as: .zip

Change History (32)

Changed 8 years ago by Eratothene

patch to django.utils.cache

Changed 8 years ago by Eratothene

The same patc

comment:1 Changed 8 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Bug in django.utils.cache_generate_cache_key(request, headerlist, key_prefix) to Bug in django.utils.cache._generate_cache_key(request, headerlist, key_prefix)

Please make review, it is a serious bug!

comment:2 Changed 8 years ago by semenov

  • Resolution set to worksforme
  • Status changed from new to closed

I spent half an hour trying to reproduce the bug, unsuccessfully. Can you please provide more details? What are the caching settings, what is the code of your (cached?) view?

In current Django trunk, SimpleCookie is used in only 2 (two) places:

  1. When parsing request HTTP_COOKIES at django.http.parse_cookie. A SimpleCookie object is used only temporary, a simple string-to-string dict is returned.
  2. When creating a HTTP response, for setting new cookies.

Thus, I can't see any way how a SimpleCookie object can appear in request.META (being iterated in django.utils.cache._generate_cache_key), unless you manually hack with the request object.

comment:3 Changed 7 years ago by andrewbadr

After turning on caching, the django test client gets this error. Only other installed middleware are Common, Session, and Authentication.

comment:4 Changed 7 years ago by Atul Varma <varmaa@…>

  • Resolution worksforme deleted
  • Status changed from closed to reopened

I'm running into this problem, and it's being reproduced reliably in my project's unit test suite (I'm using r7434 of Django SVN). If I apply the following diff to my settings.py, the problem disappears:

 MIDDLEWARE_CLASSES = (
-    'django.middleware.cache.CacheMiddleware',
     'django.middleware.common.CommonMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',
+    'django.middleware.cache.CacheMiddleware',
     'django.contrib.auth.middleware.AuthenticationMiddleware',
     'django.middleware.doc.XViewMiddleware',
 )

However, according to what the documentation says on the order of middleware classes, committing this patch means that my cache will no longer be varying on session information.

I can attach a tarball of my Django project--it's about 140k in size--if it helps diagnose this error. Alternatively, I may have misinterpreted the documentation on middleware or I may be missing something else here; any help would be appreciated.

comment:5 Changed 7 years ago by Timothée Peignier

It also seems to happend when using @cache_page and @vary_on_cookie decorators.

comment:6 Changed 7 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:7 Changed 7 years ago by sciyoshi

  • Cc sciyoshi@… added

Note that the same bug is present even when the order of the caching middleware is the new recommended one:

MIDDLEWARE_CLASSES = (
	'django.middleware.cache.UpdateCacheMiddleware',
	'django.middleware.common.CommonMiddleware',
	'django.contrib.sessions.middleware.SessionMiddleware',
	'django.middleware.locale.LocaleMiddleware',
	'django.contrib.auth.middleware.AuthenticationMiddleware',
	'django.middleware.http.ConditionalGetMiddleware',
	'django.middleware.gzip.GZipMiddleware',
	'django.contrib.flatpages.middleware.FlatpageFallbackMiddleware',
	'django.middleware.cache.FetchFromCacheMiddleware',
)

Gets triggered from the auth tests when I run manage.py test.

comment:8 Changed 7 years ago by jcharpentier

When i launch my project's test suite i have several errors TypeError: update() argument 1 must be string or read-only buffer, not SimpleCookie

All of theses appear because of same backtrace on description :

File "/var/www/caverne.bearstech.com/caverne/../django/middleware/cache.py", line 92, in process_response
    cache_key = learn_cache_key(request, response, timeout, self.key_prefix)
  File "/var/www/caverne.bearstech.com/caverne/../django/utils/cache.py", line 192, in learn_cache_key
    return _generate_cache_key(request, headerlist, key_prefix)
  File "/var/www/caverne.bearstech.com/caverne/../django/utils/cache.py", line 145, in _generate_cache_key
    ctx.update(value)

4 Django's tests fails (tests launched in the begining of an ./manage.py test):

ERROR: test_confirm_complete (django.contrib.auth.tests.views.PasswordResetTest)
ERROR: test_confirm_invalid (django.contrib.auth.tests.views.PasswordResetTest)
ERROR: test_confirm_valid (django.contrib.auth.tests.views.PasswordResetTest)
ERROR: Error is raised if the provided email address isn't currently registered

And somes of my project tests fails too : the views who fails use request.user and request.user.is_staff, the other views won't fail.
If I deactivate the cache, test suite is OK.
I have tried my testsuite with and without client.login .

I have tried the middleware cache order on documentation and the new recommended here.
SVN rev is 8911

Hope that can help to fix this ticket before 1.0

comment:9 Changed 7 years ago by lcordier

It seems this ticket needs to be renamed, currently the problem is only related to django.test.client. What is happening is that it uses a SimpleCookie as an cookie jar.
It is trying to send that object in the HTTP request. I have added a function to turn this cookie jar into a proper HTTP_COOKIE string. It solves the problem in terms of using SimpleCookie as the cookie jar. However, I would like the cookie jar upgraded (maybe a new ticket?), so that it deals better with cookies. At the creation of the
request environment, the request path and request time should be passed to the cookie jar and only the cookies that actually match those parameters should be returned and
used in the HTTP request.

Changed 7 years ago by lcordier

Fixes SimpleCookie problem in django.test.client.Client.

comment:10 Changed 7 years ago by lcordier

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 7 years ago by mtredinnick

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

Please don't mark your own patches as "ready for checkin", since they always need a second review. In this case, there a few things I'd like to see addressed:

  1. The extra loop to remove empty strings from cookie_strings is unnecessary. When you're looping over that list the second time, just do something like if not cookie: continue (but on two lines).
  2. This sort of change needs a test. It's failing now and should be working afterwards, so prove it with a test so that we don't accidentally reintroduce the problem at a later date.

A test will probably also describe the problem a bit better. I'm a little uncomfortable with this solution because I don't really understand how it is triggered. So a test that shows how a cookie is used in the normal course of using TestClient will provide us a way to confirm that the fix is the simplest and most robust thing we can do.

comment:12 Changed 7 years ago by karld

  • Cc karld@… added

Changed 7 years ago by lcordier

A much more elegant solution.

comment:13 Changed 7 years ago by lcordier

  • Cc lcordier@… added

Changed 7 years ago by lcordier

Some basic test code added, simply check that cookies work the they are supposed to work.

comment:14 follow-up: Changed 7 years ago by lcordier

NOTE: The patch http://code.djangoproject.com/attachment/ticket/5176/0002_generate_cache_key.diff 0002_generate_cache_key.diff are correct even though it doesn't display in the Trac HTML previewer. It is due to the file in trunk not ending in a newline,
which causes a diff comment which breaks the Trac HTML preveiewer. If one removes the diff comment it breaks patching.
Simply download it in http://code.djangoproject.com/attachment/ticket/5176/0002_generate_cache_key.diff?format=raw raw format.

comment:15 in reply to: ↑ 14 Changed 7 years ago by jcharpentier

  • Cc charpentier.johan@… added
  • Patch needs improvement unset

comment:16 Changed 7 years ago by clint

  • Cc me@… added

comment:17 Changed 7 years ago by jcharpentier

  • Patch needs improvement set

This patch on svn (rev 9787) resolves TypeError: update() argument 1 must be string or read-only buffer, not SimpleCookie but send new AssertionError :

======================================================================
FAIL: test_confirm_valid (django.contrib.auth.tests.views.PasswordResetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "<mydjangopath>/../django/contrib/auth/tests/views.py", line 46, in test_confirm_valid
    self.assert_("Please enter your new password" in response.content)
AssertionError

----------------------------------------------------------------------

comment:18 Changed 6 years ago by jki

Situation that "01/23/09 07:20:19 changed by jcharpentier" describes may happen when default cache interval (CACHE_MIDDLEWARE_SECONDS) in settings.py > 0:

What happens:

1) "test_confirm_invalid" is performed first- response (for invalid password reset link) is generated and cached for django.contrib.auth.views.password_reset_confirm

2) "test_confirm_valid" runs after "test_confirm_invalid" and gets the cached response for django.contrib.auth.views.password_reset_confirm, so test fails

comment:19 Changed 6 years ago by anonymous

  • Cc andy@… added
  • Keywords testclient added

comment:20 Changed 6 years ago by forest

Hi,

Personally, I would prefer that this:

if isinstance(value, SimpleCookie): 
    value = value.output() 

Be written more like this:

if not isinstance(value, (str, unicode)):
    value = unicode(value)

comment:21 Changed 6 years ago by lcordier

Just tested it, seems to produce the same results. Make a patch.

comment:22 Changed 6 years ago by anonymous

  • Cc andy@… added; andy@… removed

comment:23 Changed 6 years ago by tclineks

  • Needs tests unset

attached updated patch (against r11475)

comment:24 Changed 6 years ago by ryszard

  • Cc ryszard.szopa+django@… added

comment:25 Changed 6 years ago by anonymous

  • Cc paulswartz@… added

comment:26 Changed 5 years ago by SmileyChris

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in r12343 (and r12344 for 1.1.x)

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