If-Modified-Since checked for exact match

[Maniac <Maniac@…>]

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.

[Adrian Holovaty]

Nice catch. Looks like the solution will require parsing the Last-Modified and HTTP_IF_MODIFIED_SINCE headers into datetime objects for comparison.

[Adrian Holovaty]

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?

[Maniac <Maniac@…>]

Patch

[Maniac <Maniac@…>]

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 :-).

[Maniac <Maniac@…>]

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

[Adrian Holovaty]

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.

[hugo]

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.

[Julian Bez]

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"

[Łukasz Rekucki]

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.

[Luke Plant]

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!

[Julian Bez]

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.

[Aymeric Augustin]

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 in django.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.

[Luke Plant]

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 from unittest.TestCase (specifically, django.utils.unittest.TestCase), and not django.test.TestCase.

[Aymeric Augustin]

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!

[Luke Plant]

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.

[Luke Plant]

In [15696]:

Fixed #717 - If-Modified-Since handling should compare dates according to RFC 2616

Thanks to Maniac for the report, julienb for the initial patch, and
especially to aaugustin for the final patch and tests.

[Luke Plant]

In [15697]:

[1.2.X] Fixed #717 - If-Modified-Since handling should compare dates according to RFC 2616

Thanks to Maniac for the report, julienb for the initial patch, and
especially to aaugustin for the final patch and tests.

Backport of [15696] from trunk.

[Luke Plant]

In [15699]:

Added file missing from [15696], sorry for breakage.

Thanks to Ramiro for letting me know. Refs #717.

[Luke Plant]

In [15700]:

[1.2.X] Added file missing from [15696], sorry for breakage.

Thanks to Ramiro for letting me know. Refs #717.

Backport of [15699] from trunk.