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)
Change History (32)
by , 17 years ago
Attachment: | patch.diff added |
---|
comment:1 by , 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 , 17 years ago
Resolution: | → worksforme |
---|---|
Status: | new → 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:
- 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.
- 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 , 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 , 17 years ago
Resolution: | worksforme |
---|---|
Status: | closed → 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 by , 17 years ago
It also seems to happend when using @cache_page and @vary_on_cookie decorators.
comment:6 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:7 by , 16 years ago
Cc: | 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 , 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 , 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 , 16 years ago
Attachment: | 0001_fix_test_client.diff added |
---|
Fixes SimpleCookie problem in django.test.client.Client.
comment:10 by , 16 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:11 by , 16 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → 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:
- 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 likeif not cookie: continue
(but on two lines). - 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 , 16 years ago
Cc: | added |
---|
comment:13 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | 0002_generate_cache_key.diff added |
---|
Some basic test code added, simply check that cookies work the they are supposed to work.
follow-up: 15 comment:14 by , 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.
comment:15 by , 16 years ago
Cc: | added |
---|---|
Patch needs improvement: | unset |
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.
comment:16 by , 16 years ago
Cc: | added |
---|
comment:17 by , 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 , 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 , 16 years ago
Cc: | added |
---|---|
Keywords: | testclient added |
comment:20 by , 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:22 by , 15 years ago
Cc: | added; removed |
---|
by , 15 years ago
Attachment: | 0001-Fixed-5176-TestClient-now-can-accept-SimpleCookies-w.patch added |
---|
comment:24 by , 15 years ago
Cc: | added |
---|
comment:25 by , 15 years ago
Cc: | added |
---|
comment:26 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
patch to django.utils.cache