Opened 19 years ago

Closed 14 years ago

Last modified 14 years ago

#717 closed defect (fixed)

If-Modified-Since checked for exact match

Reported by: Maniac <Maniac@…> 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)

717.diff (1.1 KB ) - added by Maniac <Maniac@…> 19 years ago.
Patch
ifmodifiedsince.diff (6.2 KB ) - added by Julian Bez 14 years ago.
717.2.diff (17.8 KB ) - added by Aymeric Augustin 14 years ago.
717.3.diff (17.6 KB ) - added by Aymeric Augustin 14 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 by Adrian Holovaty, 19 years ago

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

comment:2 by Adrian Holovaty, 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?

by Maniac <Maniac@…>, 19 years ago

Attachment: 717.diff added

Patch

comment:3 by Maniac <Maniac@…>, 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 Maniac <Maniac@…>, 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 Adrian Holovaty, 19 years ago

Resolution: wontfix
Status: newclosed

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 hugo, 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 Julian Bez, 14 years ago

Component: django.contrib.adminHTTP handling
Has patch: set
Keywords: http middleware conditional get if-modified-since added
Needs tests: set
Resolution: wontfix
Status: closedreopened

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 Łukasz Rekucki, 14 years ago

Resolution: wontfix
Status: reopenedclosed

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 Luke Plant, 14 years ago

Patch needs improvement: set
Resolution: wontfix
Status: closedreopened
Triage Stage: UnreviewedAccepted

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 Julian Bez, 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 Julian Bez, 14 years ago

Attachment: ifmodifiedsince.diff added

comment:11 by Aymeric Augustin, 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 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.

by Aymeric Augustin, 14 years ago

Attachment: 717.2.diff added

comment:12 by Aymeric Augustin, 14 years ago

Needs tests: unset
Patch needs improvement: unset

comment:13 by Aymeric Augustin, 14 years ago

Cc: Aymeric Augustin added

comment:14 by Luke Plant, 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 from unittest.TestCase (specifically, django.utils.unittest.TestCase), and not django.test.TestCase.

comment:15 by Aymeric Augustin, 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 Aymeric Augustin, 14 years ago

Attachment: 717.3.diff added

comment:16 by Luke Plant, 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.

comment:17 by Luke Plant, 14 years ago

Resolution: fixed
Status: reopenedclosed

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.

comment:18 by Luke Plant, 14 years ago

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.

comment:19 by Luke Plant, 14 years ago

In [15699]:

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

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

comment:20 by Luke Plant, 14 years ago

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.

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