#717 closed defect (fixed)
If-Modified-Since checked for exact match
Reported by: | Owned by: | Adrian Holovaty | |
---|---|---|---|
Component: | HTTP handling | Version: | |
Severity: | normal | Keywords: | http middleware conditional get if-modified-since |
Cc: | Aymeric Augustin | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In ConditionalGetMiddleWare Last-Modified date is checked for exact match with If-Modified-Since and only then 'Not Modified' response returned. There should be check if Last-Modified >= If-Modiifed-Since.
Attachments (4)
Change History (24)
comment:1 by , 19 years ago
comment:2 by , 19 years ago
From http://fishbowl.pastiche.org/2002/10/21/http_conditional_get_for_rss_hackers :
"""
Technically, what you should do with an If-Modified-Since header is convert it to a date, and compare it with your stored date. However, 90% of the time you can get away with just doing a straight match, so it's probably not worth the effort.
"""
Thoughts?
comment:3 by , 19 years ago
I've made a simple patch with such parsing. It's very nice that HTTP requires everything in one timezone :-)
However I just thought that in practice exact checking would work anyway since in most cases Last-Modified changes only to bigger values and browsers ask If-Modified-Since only from the last Last-Modified given :-).
comment:4 by , 19 years ago
Ah... I just missed your second comment and made it in my own :-)
In fact I don't know... Comparing dates seem somewhat cleaner. But this is more error-prone. If someone gives 'Last-Modified' in other two allowed formats in HTTP there'll be an exception (but again nobody uses those formats :-) ).
Indeed may be we shouldn't fix it if it's not broken...
comment:5 by , 19 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Yeah, let's keep it the way it is for now. Thanks for taking the time to submit a patch; it'll be here if we ever decide to change our thinking.
comment:6 by , 19 years ago
Just for the records: there is a use-case for really doing date comparisons, that's when people don't store the actual last-modified header but store the content in the cache and use the cache-storage-date for later if-modified-since checking.
comment:7 by , 14 years ago
Component: | django.contrib.admin → HTTP handling |
---|---|
Has patch: | set |
Keywords: | http middleware conditional get if-modified-since added |
Needs tests: | set |
Resolution: | wontfix |
Status: | closed → reopened |
After 5 years, I propose we tackle this. Currently it's not in line with the RFC (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html).
I have observed Google to often check with a newer Last-Modified date, and Django replying with "Oh, sure it got modified, but long time before what you ask, here's the resource anyways: 200 OK"
comment:8 by , 14 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
Django's policy is that you shouldn't reopen tickets resolved as "won't fix" without prior discussion on django-developers mailing list. Please post there describing why this ticket should be accepted. It's also a good idea to describe problem with current behavior in more detail.
comment:9 by , 14 years ago
Patch needs improvement: | set |
---|---|
Resolution: | wontfix |
Status: | closed → reopened |
Triage Stage: | Unreviewed → Accepted |
Accepted after discussion on mailing list, and a valid use case based on headers that Google sends.
The patch needs tests, including what happens with valid dates before and after, and what happens with invalid dates. Please can we also have it tested with someone who knows exactly what Google is sending.
Thanks!
comment:10 by , 14 years ago
I improved the patch for the middleware by using parsedate from email.utils which might parse more dates.
The HTTP decorater also has the same issue and I managed to add some tests for it. Tests for the ConditionalGetMiddleware are still needed, as I am not really into tests.
by , 14 years ago
Attachment: | ifmodifiedsince.diff added |
---|
comment:11 by , 14 years ago
New patch:
- builds upon julianb's last patch, but does much less changes to django/views/decorators/http.py,
- adds a
parse_http_date
function indjango.utils.http
that implements a RFC-compliant parsing of HTTP dates, and uses it wherever ad-hoc parsing was implemented; that function may be simplified with email.Utils.parsedate if RFC compliance is not required; - adds tests for ConditionalGetMiddleware (fairly similar to the tests in conditional_processing, but those test the decorators and not the middleware),
- fixes a related test that used UTC instead of GMT.
by , 14 years ago
Attachment: | 717.2.diff added |
---|
comment:12 by , 14 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:13 by , 14 years ago
Cc: | added |
---|
comment:14 by , 14 years ago
Patch needs improvement: | set |
---|
This looks like a very good patch in general, but with some problems:
- Why have tests for
ConditionalGetMiddleware
been added to tests/regressiontests/middleware/tests.py, instead of to tests/regressiontests/conditional_processing/models.py with the others? And why is there such overlap between the new tests (e.g. in 'ETag' processing) and the existing ones? This duplication is confusing and unhelpful. - The
HttpDateProcessing
class should inherit fromunittest.TestCase
(specifically,django.utils.unittest.TestCase
), and notdjango.test.TestCase
.
comment:15 by , 14 years ago
Patch needs improvement: | unset |
---|
Existing tests in tests/regressiontests/conditional_processing/models.py
are for the view decorators condition
, last_modified
and etag
. The tests I added are for ConditionnalGetMiddleware
. These are two different mechanisms to achieve the same goal, the former per-view, the latter globally. Obviously tests are similar, but they test different modules.
Currently there are no tests for ConditionnalGetMiddleware
. It seems to me that tests/regressiontests/middleware/tests.py
is the proper place to test middleware, so I put the tests there.
I have updated the patch to cope with the refactoring of django.views.static
and django.contrib.staticfiles
.
I'm now using django.utils.unittest.TestCase
when django.test.TestCase
is not needed.
New patch attached. Thanks for your review, lukeplant!
by , 14 years ago
Attachment: | 717.3.diff added |
---|
comment:16 by , 14 years ago
Ah, you're right - I think I was wrongly assuming that the decorators 'condition
' and others were created using decorator_from_middleware
, in which case they would be implicitly testing ConditionalGetMiddleware
. In fact, only 'conditional_page
' is created that way, which isn't tested in tests/regressiontests/conditional_processing/models.py
. Thanks for the explanation and updates.
Nice catch. Looks like the solution will require parsing the Last-Modified and HTTP_IF_MODIFIED_SINCE headers into datetime objects for comparison.