Re: Confusion around generic views queryset cloning.

[Hanne Moa]

On 22 July 2013 10:37, Loic Bistuer  wrote:

> I updated the PR to do away with the public `clone()` method.
>
> As long as we have a long-term goal for a `QuerySet.clone()` method, I'm
> happy.
>
> If we plan on changing the cloning behavior, now is effectively a bad time
> to introduce it.
>

No need to change existing ._clone()-behavior. Split what ._clone() does,
have ._clone() use all the new methods, .all() use the methods that it
needs, and .clone() only use the cloning bits. Or do the split and don't
add .clone() yet as it sems we're not quite in agreement. Anyway, plenty of
time to discuss before 1.7.


HM

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Loic Bistuer]

I updated the PR to do away with the public `clone()` method.

As long as we have a long-term goal for a `QuerySet.clone()` method, I'm happy.

If we plan on changing the cloning behavior, now is effectively a bad time to 
introduce it.

-- 
Loic

On Jul 22, 2013, at 2:45 PM, Anssi Kääriäinen  wrote:

> On Sunday, July 21, 2013 9:53:36 PM UTC+3, Loic Bistuer wrote:
> Explicit is better than implicit and `all()` is more a `Manager` concept than 
> a `QuerySet` one. 
> 
> IMO `QuerySet.all()` should only be used when API parity with `Manager` is 
> needed. 
> 
> Internally `QuerySet` uses `_clone()` rather than `all()` and with #20625 on 
> its way, I think it's useful to have a public `clone()` method. 
> 
> 
> The biggest problem with public .clone() method is that the private ._clone() 
> doesn't do just cloning. There are some cases where cloning also changes how 
> the QuerySet behaves. For example using QuerySet.query.add_q() multiple times 
> could behave differently depending on if there is a .clone() operation in 
> between. IIRC there are some other cases which behave differently when 
> chaining multiple operations. Of course, the same problem is there when using 
> .all() for cloning, too. Still, I don't think it is a good idea to have 
> public .clone() API if it does more than a pure clone.
> 
> Splitting ._clone() to two parts has been on my wishlist for a long time. The 
> first part would be pure cloning, the second could be .next_operation() or 
> something like that which does the needed changes to support queryset 
> chaining behaviour. Another benefit of doing this cleanup is that it would be 
> fairly straightforward to implement "in-place" querysets which would make 
> certain ORM operations cheaper.
> 
> So, for the time being having .all() as clone API seems good enough to me.
> 
>  - Anssi
> 
> -- 
> 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 django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> 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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Anssi Kääriäinen]

On Sunday, July 21, 2013 9:53:36 PM UTC+3, Loic Bistuer wrote:
>
> Explicit is better than implicit and `all()` is more a `Manager` concept 
> than a `QuerySet` one. 
>
> IMO `QuerySet.all()` should only be used when API parity with `Manager` is 
> needed. 
>
> Internally `QuerySet` uses `_clone()` rather than `all()` and with #20625 
> on its way, I think it's useful to have a public `clone()` method. 
>
>
The biggest problem with public .clone() method is that the private 
._clone() doesn't do just cloning. There are some cases where cloning also 
changes how the QuerySet behaves. For example using QuerySet.query.add_q() 
multiple times could behave differently depending on if there is a .clone() 
operation in between. IIRC there are some other cases which behave 
differently when chaining multiple operations. Of course, the same problem 
is there when using .all() for cloning, too. Still, I don't think it is a 
good idea to have public .clone() API if it does more than a pure clone.

Splitting ._clone() to two parts has been on my wishlist for a long time. 
The first part would be pure cloning, the second could be .next_operation() 
or something like that which does the needed changes to support queryset 
chaining behaviour. Another benefit of doing this cleanup is that it would 
be fairly straightforward to implement "in-place" querysets which would 
make certain ORM operations cheaper.

So, for the time being having .all() as clone API seems good enough to me.

 - Anssi

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Loic Bistuer]

Explicit is better than implicit and `all()` is more a `Manager` concept than a 
`QuerySet` one.

IMO `QuerySet.all()` should only be used when API parity with `Manager` is 
needed.

Internally `QuerySet` uses `_clone()` rather than `all()` and with #20625 on 
its way, I think it's useful to have a public `clone()` method.

-- 
Loic

On Jul 22, 2013, at 1:25 AM, Rafał Stożek  wrote:

> Can't we just use .all() in views instead of creating a new QuerySet method 
> which does exactly the same thing?
> 
> 
> On Sun, Jul 21, 2013 at 12:34 PM, Loic Bistuer  
> wrote:
> My attempt with a public `QuerySet.clone()` method and docs for 
> `*ObjectMixin.queryset`:
> 
> https://github.com/django/django/pull/1384
> 
> Made a PR to facilitate review and discussion, feel free to close if you 
> think it's the wrong approach.
> 
> --
> Loic
> 
> --
> 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 django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> 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 django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> 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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Rafał Stożek]

Can't we just use .all() in views instead of creating a new QuerySet method
which does exactly the same thing?


On Sun, Jul 21, 2013 at 12:34 PM, Loic Bistuer wrote:

> My attempt with a public `QuerySet.clone()` method and docs for
> `*ObjectMixin.queryset`:
>
> https://github.com/django/django/pull/1384
>
> Made a PR to facilitate review and discussion, feel free to close if you
> think it's the wrong approach.
>
> --
> Loic
>
> --
> 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 django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> 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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Loic Bistuer]

My attempt with a public `QuerySet.clone()` method and docs for 
`*ObjectMixin.queryset`:

https://github.com/django/django/pull/1384

Made a PR to facilitate review and discussion, feel free to close if you think 
it's the wrong approach.

-- 
Loic

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Tom Christie]

Thanks both, the responses are very much appreciated.

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

True, it's exactly the same issue, although it's more of a gotcha, in that 
appending to a list is explicitly a stateful operation, whereas it's more 
of an implicit side effect when iterating over a queryset.

Something as simple as the following is broken, although it might not be 
obvious on first sight.

class MyView(View):
queryset = Users.objects.all()
template_name = 'foobar'

def get(self, request):
return render(request, self.template_name, {'items': 
self.queryset})

I don't know that there's a huge amount worth doing about that.  We *could* 
add some documentation warning against modifying class attributes on CBV, 
but I'm not sure if it'd just be adding noise and be more confusing that 
it's worth for many users.

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

Generally, ensuring that the GCBVs are implemented using public API seems 
sensible. Anything else is indicative that there's something missing from 
the API that is in fact necessary to end-users. Having said that I really 
don't have strong views on the right approach here.

* `.clone` as public API seems okay, but I'd always prefer to avoid 
introducing new API if it can be avoided.
* Using `.all` also seems okay, but it feels a bit odd that 
`MyModel.objects.all()` would then have end up having a second `.all()` 
operation chained to it during `get_queryset()`.  I realize that's 
equivalent to what's happening now, but it looks odd and might be confusing.
* Continue using `._clone`, but adding a comment in the implementation 
might be sufficient.
* Maybe things are okay as they are right now.

Cheers,

  Tom

On Friday, 12 July 2013 09:43:10 UTC+1, Marc Tamlyn wrote:
>
> 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 > 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 (

Re: Confusion around generic views queryset cloning.

[Marc Tamlyn]

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

Re: Confusion around generic views queryset cloning.

[Carl Meyer]

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
> .
> 
> 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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Confusion around generic views queryset cloning.

[Tom Christie]

My apologies, this was intended to be posted to the Django users group.

On Thursday, 11 July 2013 21:54:25 UTC+1, Tom Christie wrote:
>
> I've noted that the generic view implementations use the private `_clone` 
> method when returning the queryset 
> attribute
> .
>
> 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'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.
>
> Many thanks,
>
>   Tom
>
>

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Confusion around generic views queryset cloning.

[Tom Christie]

I've noted that the generic view implementations use the private `_clone` 
method when returning the queryset 
attribute
.

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

Many thanks,

  Tom

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.