Opened 7 years ago

Closed 4 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 7 years ago.
patch for trunk
extra_context-r7720.diff (9.0 KB) - added by ajs 7 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 7 years ago by Waldemar Kornewald

patch for trunk

comment:1 Changed 7 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 7 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 7 years ago by programmerq

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

Changed 7 years ago by ajs

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

comment:4 Changed 7 years ago by ajs

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 6 years ago by SmileyChris

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 6 years ago by bitprophet

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 6 years ago by bitprophet

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 5 years ago by lukeplant

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 4 years ago by russellm

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

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