Code

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#5791 closed (fixed)

Conditional get decorator

Reported by: Maniac@… Owned by: nobody
Component: HTTP handling Version: master
Severity: Keywords:
Cc: simon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

A view decorator taking as a parameter a function calculating Last-Modified and/or ETag that takes care of all
the mechanic of checking request headers, returning 304 Not Modified and setting response headers.

More discussion: http://groups.google.com/group/django-developers/t/fa6c36efde4cf106

Attachments (5)

5791.diff (9.6 KB) - added by Maniac@… 7 years ago.
Patch
5791.2.diff (12.8 KB) - added by isagalaev 6 years ago.
Patch updated to current trunk + docs
5791.3.diff (12.8 KB) - added by isagalaev 6 years ago.
Patch updated to current trunk + new docs structure
5791.4.diff (12.9 KB) - added by isagalaev 6 years ago.
Patch updated to current trunk + Adrian's new docs structure
5791.5.diff (14.0 KB) - added by isagalaev 6 years ago.
Patch with proper etag parsing and quoting

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by Maniac@…

Patch

comment:1 Changed 7 years ago by Simon G <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 6 years ago by telenieko

  • Needs documentation set
  • Triage Stage changed from Ready for checkin to Accepted

Maybe I got it wrong, but should this add documentation?

comment:3 Changed 6 years ago by Ivan Sagalaev <Maniac@…>

Sure it's better to have docs than not to have :-). The main reason why I was lazy with it is that existing decorators (require_POST et al.) also don't have docs so I couldn't completely figure out where to put it. But I'll do it now if it helps to commit it :-)

Changed 6 years ago by isagalaev

Patch updated to current trunk + docs

comment:4 Changed 6 years ago by isagalaev

  • Needs documentation unset

I've updated the patch to apply to current trunk and wrote some docs. The patch is now a bit easier to review (I've thrown out some scary looking memoizations). Docs require proof-reading, of course, English is not my native tongue.

comment:5 Changed 6 years ago by julianb

  • milestone set to 1.0 maybe
  • Patch needs improvement set

Strangely, I don't see a patch when I follow the link. Does it include "returning 304 Not Modified and setting response headers"? Would be better if that's handled by ConditionalGetMiddleware (DRY!).

comment:6 follow-up: Changed 6 years ago by isagalaev

  • milestone 1.0 maybe deleted
  • Patch needs improvement unset

Strangely, I don't see a patch when I follow the link.

I gather this is a bug in Trac. You can still see the patch be following the "Original format" link there.

Does it include "returning 304 Not Modified and setting response headers"?

Sure, it's the whole point of conditional get :-)

Would be better if that's handled by ConditionalGetMiddleware (DRY!)

A middleware by definition can't handle it the way the patch does. It enables generation of last modified times and etags on a per-view basis using application specific semantics.

P.S. Please don't alter flags like "Patch needs improvement" unless you're a Django triager/comitter and especially even if you didn't read it.

comment:7 in reply to: ↑ 6 Changed 6 years ago by julianb

Replying to isagalaev:

A middleware by definition can't handle it the way the patch does. It enables generation of last modified times and etags on a per-view basis using application specific semantics.

I was talking about the evaluation of Last-Modified and etag headers and returning 304 if there was no change, so that is the task of the middleware and should not be included here again. The middleware handles that beautifully afaik.

comment:8 follow-up: Changed 6 years ago by isagalaev

Ah, I've got what you mean.
I don't think it should be delegated to middleware because it'll make it more complex to use: it requires two middlewares in settings apart from just importing a decorator and use it. It's also not very obvious. And, frankly, code reuse that we're talking about here is both small -- returning a HttpResponseNotModified with Last-Modified and ETag -- and hard -- inventing a way to tell middleware to do something without returning a response because not generating a response is the point of this decorator. So I wouldn't bother...

comment:9 in reply to: ↑ 8 Changed 6 years ago by julianb

Ah, now I've got what you mean ;)

Unless there's a way to invoke the ConditionalGetMiddleware in the middle of what you patch does, it's really better to not bother.

Changed 6 years ago by isagalaev

Patch updated to current trunk + new docs structure

Changed 6 years ago by isagalaev

Patch updated to current trunk + Adrian's new docs structure

comment:10 Changed 6 years ago by mmalone

There's a (rather trivial) bug in the latest patch. According to the HTTP spec, ETags are opaque strings. The only requirement is that they must be quoted. The current diff does a simple split on the comma character, but this breaks things if the ETag contains a comma. I'd submit a new diff and add a test, but I don't have a django dev environment setup at the moment. The fix should be simple enough, instead of doing:

if_none_match = [e.strip() for e in if_none_match.split(',')]

do this:

if_none_match = re.findall('(?:W/)?"[^"]*"', if_none_match)

Note that there's no check that the quoted strings are comma-separated. I think that's ok -- the spec says they have to be comma separated, and if they're not then the result is undefined. Relevant spec info is here:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.11

comment:11 Changed 6 years ago by isagalaev

Thanks for the pointer! Indeed, I've missed that the string must be quoted. They are also \-escaped so I've gone further and implemented proper parsing of etags from If-None-Match and If-Match, along with proper quoting of a user-supplied string for the response ETag header. Patch with updated code and tests (5791.5.diff) follows.

Changed 6 years ago by isagalaev

Patch with proper etag parsing and quoting

comment:12 Changed 5 years ago by sfllaw

  • Cc simon@… added

comment:13 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [10114]) Fixed #5791 -- Added early-bailout support for views (ETags and Last-modified).

This provides support for views that can have their ETag and/or Last-modified
values computed much more quickly than the view itself. Supports all HTTP
verbs (not just GET).

Documentation and tests need a little more fleshing out (I'm not happy with the
documentation at the moment, since it's a bit backwards), but the functionality
is correct.

comment:14 Changed 5 years ago by mtredinnick

Oh, sorry Ivan. I completely forgot to give you credit for all the heavy-lifting in the commit message. I'm getting too tired. Really sorry about that. :-(

comment:15 Changed 5 years ago by isagalaev

Malcolm, it's no a problem! I'll just blog about it and have my share of fame anyway :-)

A big thank you for doing it for 1.1!

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.