Opened 17 years ago

Closed 15 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: dev
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: no UI/UX: no

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 17 years ago.
patch to django.utils.cache
patch.2.diff (489 bytes ) - added by Eratothene 17 years ago.
The same patc
0001_fix_test_client.diff (1.8 KB ) - added by lcordier 16 years ago.
Fixes SimpleCookie problem in django.test.client.Client.
0001_generate_cache_key.diff (1.2 KB ) - added by lcordier 16 years ago.
A much more elegant solution.
0002_generate_cache_key.diff (3.6 KB ) - added by lcordier 16 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 Travis Cline 15 years ago.

Download all attachments as: .zip

Change History (32)

by Eratothene, 17 years ago

Attachment: patch.diff added

patch to django.utils.cache

by Eratothene, 17 years ago

Attachment: patch.2.diff added

The same patc

comment:1 by anonymous, 17 years ago

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

Please make review, it is a serious bug!

comment:2 by Ilya Semenov, 17 years ago

Resolution: worksforme
Status: newclosed

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 by Andrew Badr, 17 years ago

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

comment:4 by Atul Varma <varmaa@…>, 17 years ago

Resolution: worksforme
Status: closedreopened

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 by Timothée Peignier, 17 years ago

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

comment:6 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:7 by Samuel Cormier-Iijima, 16 years ago

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 by Johan Charpentier, 16 years ago

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 by lcordier, 16 years ago

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.

by lcordier, 16 years ago

Attachment: 0001_fix_test_client.diff added

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

comment:10 by lcordier, 16 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Malcolm Tredinnick, 16 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 by karld, 16 years ago

Cc: karld@… added

by lcordier, 16 years ago

A much more elegant solution.

comment:13 by lcordier, 16 years ago

Cc: lcordier@… added

by lcordier, 16 years ago

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

comment:14 by lcordier, 16 years ago

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.

in reply to:  14 comment:15 by Johan Charpentier, 16 years ago

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

comment:16 by clint, 16 years ago

Cc: me@… added

comment:17 by Johan Charpentier, 16 years ago

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 by jki, 16 years ago

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 by anonymous, 16 years ago

Cc: andy@… added
Keywords: testclient added

comment:20 by Forest Bond, 15 years ago

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 by lcordier, 15 years ago

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

comment:22 by anonymous, 15 years ago

Cc: andy@… added; andy@… removed

comment:23 by Travis Cline, 15 years ago

Needs tests: unset

attached updated patch (against r11475)

comment:24 by ryszard, 15 years ago

Cc: ryszard.szopa+django@… added

comment:25 by anonymous, 15 years ago

Cc: paulswartz@… added

comment:26 by Chris Beaven, 15 years ago

Resolution: fixed
Status: reopenedclosed

Fixed in r12343 (and r12344 for 1.1.x)

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