I don't think this should need documenting explicitly. It's the same issue as using mutable default arguments or attributes to a class. If someone is writing their own generic CBVs without understanding I don't see why the queryset case is any more problematic than having a class with an empty list defined on it and appending things to it from instances. I think it's quite well documented that the a queryset caches its results.
Also, in many cases in the CBVs this clone is not strictly necessary as the qs is being further filtered (or called with `get` or `count` or some such) in such a way that the original qs is not being used directly. What we could do is add an explanatory comment to the code as to why we are doing that and/or change it to use `all()` instead and/or promote `clone()` to be a real part of the API. Hope that makes some sense. On 11 July 2013 22:26, Carl Meyer <[email protected]> wrote: > Hi Tom, > > (You said in a follow-up this was intended for django-users, but > personally I think it's on-topic for -developers, so I'm replying here.) > > On 07/11/2013 02:54 PM, Tom Christie wrote: > > I've noted that the generic view implementations use the private > > `_clone` method when returning the queryset attribute > > < > https://github.com/django/django/blob/master/django/views/generic/detail.py#L72 > >. > > > > Presumably the need for that is something to do with the potential for > > querysets to be cached or otherwise incorrectly stateful if this cloning > > isn't performed. What's confusing me is that it's not at all obvious > > *exactly* what the implications of making (or failing to make) this call > > are. > > > > Furthermore if it *is* something that's strictly required in this > > circumstance then do we need to be documenting whatever behavior is > > being triggered, so that developers writing their own class based views > > don't make the (mistake?) of simply returning/using a queryset attribute > > without having first cloned it? > > > > For example, is the following incorrect? > > > > class SomeBaseGenericView(View): > > queryset = None > > > > def get_queryset(self): > > """ > > Simply return `.queryset` by default. Subclasses may > > override this behavior. > > """ > > return self.queryset > > > > If so, under what set of conditions can it fail, and is it possible to > > unit test for the failing behavior? > > I don't use CBVs much, so I haven't run into this personally or tested > it, but I believe that that code is indeed wrong. Here's the scenario: > > 1. A subclass defines the "queryset" class attr with an actual QuerySet. > > 2. This queryset is then evaluated (i.e. iterated over) by some caller > of ``get_queryset``. This populates its result cache. > > 3. Any future request iterating over that queryset will now get the > cached result instead of executing a new query. Since the queryset is a > class attribute, and the class is a persistent object at module level, > the queryset object is persistent from request to request, and the > result data for that queryset will never change in that server process. > > Cloning the queryset on every request before anyone uses it fixes this > problem, because only the per-request clones will get their > result-caches populated, never the original class-attribute queryset. > (Also if somehow the original queryset were evaluated, cloning a > queryset doesn't copy the result-cache anyway). > > (In your above example, if it so happened that every caller of > get_queryset did further filtering on the queryset before evaluating it, > that filtering would implicitly clone the queryset, as every call to > .filter() does, thus also preventing this problem.) > > It's a special case of the same problem that can occur anytime you > define a queryset at module-import-time scope. Generally my rule of > thumb for avoiding this problem is "don't ever do that", but since CBVs > encourage breaking that rule with the "queryset" class attribute, they > have to take care to clone that queryset in get_queryset. > > > I've dug into the source, but the `_clone` method isn't documented, nor > > can I find anything terribly helpful related to queryset cloning after > > googling around for a while. > > I think you may be right that this should be documented for authors of > their own CBVs who take the generic ones as an example, though I'll > leave a final decision on that to someone more involved in the CBV code > and docs. > > That would imply making qs._clone() a publicly documented method. The > issue would be that we'd probably want to make qs.clone() an alias for > it to avoid documenting an underscore-prefixed method as public API, and > that is potentially backwards-incompatible for anyone who has defined > their own .clone() method on a custom QuerySet subclass. > > The alternative would be to recommend the use of qs.all(), which is in > fact already an alias for qs._clone(). In that case perhaps the CBVs > should be changed to use .all() as well, so as to avoid using private API. > > Carl > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > Visit this group at http://groups.google.com/group/django-developers. > For more options, visit https://groups.google.com/groups/opt_out. > > > -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. For more options, visit https://groups.google.com/groups/opt_out.
