QuerySet in extra_context should be handled automatically

[Waldemar Kornewald]

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)

[Waldemar Kornewald]

patch for trunk

[Jeff Forcier <jeff@…>]

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.

[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? :)

[Adi J. Sieker]

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

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

[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}

[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().

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

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

[Russell Keith-Magee]

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