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

[Eratothene]

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

[Eratothene]

patch to django.utils.cache

[Eratothene]

The same patc

[anonymous]

Please make review, it is a serious bug!

[Ilya Semenov]

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.

[Andrew Badr]

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

[Atul Varma <varmaa@…>]

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.

[Timothée Peignier]

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

[Samuel Cormier-Iijima]

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.

[Johan Charpentier]

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

[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.

[lcordier]

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

[Malcolm Tredinnick]

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.

[lcordier]

A much more elegant solution.

[lcordier]

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

[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.

[Johan Charpentier]

After patch file utils/cache.py with ​http://code.djangoproject.com/attachment/ticket/5176/0002_generate_cache_key.diff?format=raw this patch it works fine here on 1.0.

[Johan Charpentier]

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

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

[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

[Forest Bond]

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)

[lcordier]

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

[Travis Cline]

attached updated patch (against r11475)

[Chris Beaven]

Fixed in r12343 (and r12344 for 1.1.x)