Opened 9 years ago

Closed 6 years ago

#6603 closed (fixed)

QuerySet in extra_context should be handled automatically

Reported by: Waldemar Kornewald Owned by: nobody
Component: Generic views Version: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

f I pass an 'extra_context' parameter to a generic view I can't use a
QuerySet because they'd only be evaluated once (at program start), so
the only option is to pass a callable. Unfortunately, this solution
increases code size and mental burden on the developer who can pass a
QuerySet to 'queryset', but not within 'extra_context', so it's even
inconsistent.

I'd like to suggest that if a QuerySet is passed to 'extra_context' it
should be handled exactly like the 'queryset' parameter: call
value._clone(). I've attached a patch that you might use as a starting
point. It even reduces the generic views code size by 34 lines, so I
hope that alone is convincing enough. ;) (though, I'm not sure if the
solution is OK)

Attachments (2)

extra_context.diff (8.2 KB) - added by Waldemar Kornewald 9 years ago.
patch for trunk
extra_context-r7720.diff (9.0 KB) - added by Adi J. Sieker 8 years ago.
updated the patch. changed docs to reflect that QuerySet are now cloned. current tests pas.

Download all attachments as: .zip

Change History (11)

Changed 9 years ago by Waldemar Kornewald

Attachment: extra_context.diff added

patch for trunk

comment:1 Changed 9 years ago by Jeff Forcier <jeff@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I'd like to throw my meager weight behind this ticket, despite it being documented for a good 2 years now (as of [2949]), it's a pretty irritating issue. Despite being a Django user since before aforementioned changeset, I still make this mistake (using QuerySets sans lambda within extra_context) quite often, and I'd like to think that's due more to the inconsistency aspect and less to my lack of mental conditioning.

The patch looks OK to me, although the raw_extra_context argument name should probably change to extra_context else the code as-is is a little broken :) I was also going to say that it might work better as an extension to RequestContext but then realized there's a few spots where we're operating on a normal dictionary and not a RC instance, so never mind.

comment:2 Changed 8 years ago by Waldemar Kornewald

Oh, apart from renaing raw_extra_context to extra_context in common.py, line 9 should also say isinstance(value, QuerySet) instead of isinstance(extra, QuerySet). Yeah, I did indeed not test the patch at that time, but with those two changes it works. So, is there anyone interested in applying the patch? :)

comment:3 Changed 8 years ago by Jeff Anderson

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

Changed 8 years ago by Adi J. Sieker

Attachment: extra_context-r7720.diff added

updated the patch. changed docs to reflect that QuerySet are now cloned. current tests pas.

comment:4 Changed 8 years ago by Adi J. Sieker

patch now works. The current generic view tests pass. Docs now mention that QuerySets are cloned so that the results are always fresh.

comment:5 Changed 8 years ago by Chris Beaven

Maybe I'm overlooking something, but can't you just pass the all method of the manager?

extra_context = {'news': News.objects.all}

comment:6 Changed 8 years ago by Jeff Forcier

SmileyChris -- that works if you want a top level manager result such as .all() (since you can grab the callable itself), but doesn't work if you want to do any sort of actual filtering, such as (totally arbitrary example):

extra_context = {'top_news': News.objects.filter(rating__gte=5)}

It's been a while since I looked at this issue but IIRC the deal is that the 'callable or not callable' test is too naive because QuerySets aren't callable (last I checked) and so the existing code does not re-evaluate them, despite QSs having the capacity to be re-evaluated via ._clone().

comment:7 Changed 8 years ago by Jeff Forcier

I should say, that would be another approach -- defining def __call__(self): return self._clone() on QuerySet -- but it seems to me that changing/adding something pretty integral like this is not as good a solution as updating the behavior of the generic views to more intelligently handle QuerySets.

comment:8 Changed 7 years ago by Luke Plant

I'm -0 on this.

To me, the problem is that once you special case this, there are so many other things that might need special casing. And the solution requires so little code:

extra_context = {'top_news': lambda: News.objects.filter(rating__gte=5)}

Perhaps special casing the queryset argument was also a mistake - we should just be educating people that objects stored at module level are persistent, and care needs to be taken with them.

comment:9 Changed 6 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

Function-based generic views were deprecated by the introduction of class-based views in [14254]. Class-based views should solve this problem.

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