RFC: Query Methods

[Zachary Voase]

At the moment it's very easy to add methods to individual models, just
by putting a method on the model class itself which accepts 'self'.
However, defining business logic on collections of records is
currently pretty complicated — you have to create at least a custom
Manager subclass, and if you want to chain those methods together
you'll need a QuerySet subclass. An example of the desired behaviour,
and the steps required for it, is shown in [1].

I originally created django-qmixin [2] to tackle this problem; you can
define methods which will be present on the manager and all querysets
produced therefrom, including on relations. However, the django-qmixin
approach of creating a mixin and then including that on the Manager
class object doesn't really gel with the rest of Django core. I've
worked out two approaches which are easier for novices to understand,
and match the idioms of the rest of Django. They both involve adding a
@models.querymethod decorator, which would be applied to methods which
operate on collections of records (i.e. querysets). It's an analog to
Python's @classmethod.

The first approach [3] involves adding these querymethods to the model
class itself; the second [4] adds them to a manager subclass (but
without the trouble of the QuerySet subclass). I prefer the former,
for simplicity, but you may believe otherwise.

I'm working on a proof-of-concept implementation, but I feel it's more
important to agree on the interface and rationale beforehand. Any
thoughts?

[1]: https://gist.github.com/1553675#file_old_queryset_method.py
[2]: https://github.com/zacharyvoase/django-qmixin
[3]: https://gist.github.com/1553675#file_model_querymethod.py
[4]: https://gist.github.com/1553675#file_manager_querymethod.py

[Russell Keith-Magee]

On Tue, Jan 3, 2012 at 1:55 PM, Zachary Voase <zachar...@gmail.com> wrote:
> At the moment it's very easy to add methods to individual models, just
> by putting a method on the model class itself which accepts 'self'.
> However, defining business logic on collections of records is
> currently pretty complicated — you have to create at least a custom
> Manager subclass, and if you want to chain those methods together
> you'll need a QuerySet subclass. An example of the desired behaviour,
> and the steps required for it, is shown in [1].
>
> I originally created django-qmixin [2] to tackle this problem; you can
> define methods which will be present on the manager and all querysets
> produced therefrom, including on relations. However, the django-qmixin
> approach of creating a mixin and then including that on the Manager
> class object doesn't really gel with the rest of Django core. I've
> worked out two approaches which are easier for novices to understand,
> and match the idioms of the rest of Django. They both involve adding a
> @models.querymethod decorator, which would be applied to methods which
> operate on collections of records (i.e. querysets). It's an analog to
> Python's @classmethod.
>
> The first approach [3] involves adding these querymethods to the model
> class itself; the second [4] adds them to a manager subclass (but
> without the trouble of the QuerySet subclass). I prefer the former,
> for simplicity, but you may believe otherwise.

I think I'm in the "otherwise" camp -- primarily for reasons of namespacing.

The QuerySet and the Manager are different classes, but they effective
share a namespace. It's good practice (and a practice that your
example demonstrates) for the Manager and the QuerySet to use the same
names for their methods; it makes a certain amount of sense to me that
there should be handy shortcut to automagically perform this
duplication.

However, the same isn't true of the Model and the QuerySet. The API
for a Model and the API for a QuerySet are quite different -- and
intentionally so. Sticking QuerySet methods on a Model in order to
avoid a couple of lines of manager definition and registration seems a
little odd to me, in an "explicit is better that implicit" kinda way.
For my money, it's better to be explicit that you're defining a custom
manager.

In practical terms, there are two ways that this could manifest.

Firstly, if there is a namespace collision -- i.e., if you want
MyObject.objects.foo() and myinstance.foo() do different things. I
can't think of an obvious practical example off the top of my head,
but anything where the verb and collective noun aren't distinguishable
would be a candidate for a collision.

Secondly, if you have more than one manager, you may want a method on
one manager, but not the other. For this case, we'd need to have the
Manager-based shortcut anyway, or tell people that if you have
multiple managers, you need to do the full-manual queryset definion.

> I'm working on a proof-of-concept implementation, but I feel it's more
> important to agree on the interface and rationale beforehand. Any
> thoughts?

As I mentioned to you in person at DC.eu -- details notwithstanding,
I'm +1 to this idea. QuerySet+Manager is a common pattern, and it
deserves to have a simplification in Django's core.

Yours,
Russ Magee %-)

[Zachary Voase]

On Jan 3, 7:01 am, Russell Keith-Magee <russ...@keith-magee.com>
wrote:

Here's an example which proves your point: an `as_json()` method which
serializes a model to a JSON-compatible dictionary. You'd probably
want that on both the model instance and the QuerySet. The model
instance one would just pluck out attributes, whereas the QuerySet one
might use `values()` instead.

I just created a ticket for this feature proposal:
https://code.djangoproject.com/ticket/17494

[Donald Stufft]

I replied to the ticket, but I'll mention it here as well. django-model-utils has an implementation of something

that achieves the same result. It was originally from http://paulm.us/post/3717466639/passthroughmanager-for-django

and has since been added to https://github.com/carljm/django-model-utils

--

You received this message because you are subscribed to the Google Groups "Django developers" group.

To post to this group, send email to django-d...@googlegroups.com.

To unsubscribe from this group, send email to django-develop...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Zachary,

On 01/02/2012 10:55 PM, Zachary Voase wrote:
> At the moment it's very easy to add methods to individual models, just
> by putting a method on the model class itself which accepts 'self'.
> However, defining business logic on collections of records is

> currently pretty complicated � you have to create at least a custom

> Manager subclass, and if you want to chain those methods together
> you'll need a QuerySet subclass. An example of the desired behaviour,
> and the steps required for it, is shown in [1].
>
> I originally created django-qmixin [2] to tackle this problem; you can
> define methods which will be present on the manager and all querysets
> produced therefrom, including on relations. However, the django-qmixin
> approach of creating a mixin and then including that on the Manager
> class object doesn't really gel with the rest of Django core. I've
> worked out two approaches which are easier for novices to understand,
> and match the idioms of the rest of Django. They both involve adding a
> @models.querymethod decorator, which would be applied to methods which
> operate on collections of records (i.e. querysets). It's an analog to
> Python's @classmethod.
>
> The first approach [3] involves adding these querymethods to the model
> class itself; the second [4] adds them to a manager subclass (but
> without the trouble of the QuerySet subclass). I prefer the former,
> for simplicity, but you may believe otherwise.
>
> I'm working on a proof-of-concept implementation, but I feel it's more
> important to agree on the interface and rationale beforehand. Any
> thoughts?

Thanks for working on this. I think the decorator API you've selected is
a pretty good one for this; I agree with Russell that the methods belong
on a custom Manager, not on the model itself.

There will be some tricky points in the implementation. In particular,
QuerySets must remain pickleable (so they can be cached), which is
problematic if you are dynamically creating the QuerySet subclass. As
Donald Stufft points out on the ticket, we previously had an approach in
django-model-utils (manager_from) that was based on dynamically-created
QuerySet subclasses, and we moved away from it because it broke pickling
of QuerySets. There may be another way around this; I didn't look at the
problem in depth.

I have to say, this all feels to me a bit like polishing a turd; adding
ever-more complex hacks with metaclasses, dynamic subclasses, etc., all
to paper over what is really a more fundamental design problem. To wit:
a Manager is no different from a QuerySet in practice - it's just a sort
of "starter" queryset accessible from the model. If in practice people
pretty much always want to pair a custom Manager with a custom QuerySet
that has the same extra methods (and that is my experience), I think
it's telling us that those shouldn't be two separate classes at all.

I can't help but wonder if it would be possible to solve the problem
more thoroughly and remove the need for magic decorators by eliminating
the Manager class entirely and making it possible to simply assign a
QuerySet to a model, perhaps with a helper method:

objects = manager(PersonQuerySet.filter(active=True))

I'm sure there are devils in the details here, too, but on cursory
inspection of the Manager code, it seems like it might be doable.

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8DPlYACgkQ8W4rlRKtE2e+TQCffZZmaqKRc4Hyn3reUzYj6Cga
ja8AoNyIutFq4GuUfTgn5RgBMQaW/rJk
=AIQa
-----END PGP SIGNATURE-----

[Alex Gaynor]

On Tue, Jan 3, 2012 at 11:43 AM, Carl Meyer <ca...@oddbird.net> wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Zachary,

On 01/02/2012 10:55 PM, Zachary Voase wrote:
> At the moment it's very easy to add methods to individual models, just
> by putting a method on the model class itself which accepts 'self'.
> However, defining business logic on collections of records is

> currently pretty complicated — you have to create at least a custom

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

I haven't analyzed your suggestion in detail Carl, but there is a good reason for managers to exist:

querysets cache internally on iteration, managers are not directly evaluable, but were they you could end up doing something like:

for instance in Model.objects:
    pass

and you'd cache those results in global state.

Alex

--
"I disapprove of what you say, but I will defend to the death your right to say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/03/2012 10:42 AM, Alex Gaynor wrote:
> I haven't analyzed your suggestion in detail Carl, but there is a good
> reason for managers to exist:

> querysets cache internally on iteration, managers are not directly
> evaluable, but were they you could end up doing something like:

> for instance in Model.objects:
> pass

> and you'd cache those results in global state.

Indeed, that's of those devils in the details, but it's not
insurmountable. Clearly, either the Model metaclass or the hypothetical
"manager" function would need to do something to the provided QuerySet
to make it not iterable directly. This could be a pretty simple flag on
QuerySet, it doesn't require a separate class.

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8DQEsACgkQFRcxmeyPUXI5DQCffoTuvSuJ84HlQq1F8SGv6yRX
SFMAniE3FhaGH5eVlrgtSy3se/g2/4Nj
=lqm7
-----END PGP SIGNATURE-----

[Tai Lee]

I have had difficulty in finding an easy way to add common methods to
querysets and managers for multiple models in Django.

I solved the problem in my projects by defining a custom `Manager`
class which defines `use_for_related_fields = True` and also overrides
`__getattr__()` to look for queryset and manager methods on an inner
`QuerySet` class on the model class, similar to the inner `Admin`
class.

{{{
from django.db import models
from django.db.models import query
from turbia import models as turbia_models

class Client(models.Model):
is_deleted = models.BooleanField(default=False)
objects = turbia_models.Manager()

class QuerySet:
def not_deleted(self):
return self.filter(is_deleted=False)

class Brand(models.Model):
client = models.ForeignKey(Client)
is_deleted = models.BooleanField(default=False)
objects = turbia_models.Manager()

class QuerySet:
def not_deleted(self):
return self.filter(client__is_deleted=False, is_deleted=False)
}}}

I use this custom manager on an abstract base model class that all my
models are derived from, so it becomes trivial to add manager and
queryset methods when defining a new model. I can also define an inner
`QuerySet` class that is a subclass of another models inner `QuerySet`
class, when there are common queryset methods.

I'm sure there are problems with my implementation and it may not be
flexible or general enough to be relevant here (especially regarding
multiple manager support), but I would love to see improved support
built into the core of Django that would allow methods to be added to
both the manager and queryset for a model, and shared between
different (but similar) models much more easily.

I agree with Russell and others that these methods should not be
defined directly on the model (with a decorator).

Cheers.
Tai.

[Aaron Merriam]

Tai, we use that same structure at my work and so far it has worked well.

https://github.com/fusionbox/django-fusionbox/blob/master/fusionbox/db/models.py

It also extends well.  You can define queryset methods elsewhere and then inherit from them on multiple models to reuse those methods easily.  So far it has been a really nice solution for us.

[Daniel Sokolowski]

Forgive me if I am missing something --- I use custom managers extensively and do not see what your code can do that the django custom managers can't ? Can you provide an example please ?

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

--

Daniel Sokolowski

Web Engineer

KL Insight
http://klinsight.com/
Tel: 613-344-2116 | Fax: 613.634.7029
993 Princess Street, Suite 212
Kingston, ON K7L 1H3, Canada


Notice of Confidentiality:
The information transmitted is intended only for the person or entity to which it is addressed and may contain confidential and/or privileged material. Any review re-transmission dissemination or other use of or taking of any action in reliance upon this information by persons or entities other than the intended recipient is prohibited. If you received this in error please contact the sender immediately by return electronic transmission and then immediately delete this transmission including all attachments without copying distributing or disclosing same.

[Russell Keith-Magee]

On Fri, Jan 6, 2012 at 12:02 AM, Daniel Sokolowski
<daniel.s...@klinsight.com> wrote:
> Forgive me if I am missing something --- I use custom managers extensively
> and do not see what your code can do that the django custom managers can't ?
> Can you provide an example please ?

Zachary provided a complete example in his original post. The crux of
it -- at present, in order to support the following two expressions:

MyModel.objects.foo()
MyModel.objects.filter(...).foo()

you currently have to provide 2 definitions of foo() -- one on a
custom Manager, and one on a custom QuerySet that is generated by the
custom manager. Zachary's proposal is to provide a decorator so that
you only have to provide the Manager definition.

Yours,
Russ Magee %-)

[Anssi Kääriäinen]

I haven't read all the comments in this thread, but I think I have an
idea for implementation that might work, and which I haven't seen in
the discussions before.

The implementation idea is to add __getattr__ to QuerySet. The
__getattr__ would go through managers associated with that queryset
(don't know how to do that association. An API issue), and if it
founds a chainable method in any associated manager, it will do the
following:
- create a dynamic subclass of the manager
- patch that subclass so that its get_query_set() will return
self._chain_to_qs
- instantiate that subclass as dynamic_manager
- set dynamic_manager._chain_to_qs = self
- return getattr(dynamic_manager, attr)

I have a very quickly crafted draft of the main idea at
https://github.com/akaariai/django/compare/dynamic_man

As you can see, the implementation is very simple. It is missing
manager autodiscovery, and the @querymethod decorator.

I have tested it with:

class PublishedManager(Manager):
def published(self):
return
self.get_query_set().filter(published_date__lte=datetime.now())

class SomeModel(models.Model):
published_date = models.DateTimeField()
objects = PublishedManager()

print SomeModel.objects.published().query
print
SomeModel.objects.order_by('id').add_managers([SomeModel.objects]).published().query

These simple test-cases seem to work. Note that @querymethod is not
implemented. Also, add_managers is not part of the proposed API. The
set of managers to look for chainable methods should somehow be
autodiscovered, but I have no good ideas about how to do that.

I also did some speedtesting, using
objects.order_by('id').filter(published_date__lte=now()) is around 5%
faster than using .order_by('id').published().

All in all, I think the above idea is worth exploring, as it is
simple, yet powerful enough to allow what is wanted by the original
poster.

- Anssi

[Anssi Kääriäinen]

On Jan 9, 10:51 pm, Anssi Kääriäinen <anssi.kaariai...@thl.fi> wrote:
> I have a very quickly crafted draft of the main idea athttps://github.com/akaariai/django/compare/dynamic_man

>
> As you can see, the implementation is very simple. It is missing
> manager autodiscovery, and the @querymethod decorator.
>
> I have tested it with:
>
> class PublishedManager(Manager):
>     def published(self):
>         return
> self.get_query_set().filter(published_date__lte=datetime.now())
>
> class SomeModel(models.Model):
>     published_date = models.DateTimeField()
>     objects = PublishedManager()
>
> print SomeModel.objects.published().query
> print
> SomeModel.objects.order_by('id').add_managers([SomeModel.objects]).published().query
>
> These simple test-cases seem to work.  Note that @querymethod is not
> implemented. Also, add_managers is not part of the proposed API. The
> set of managers to look for chainable methods should somehow be
> autodiscovered, but I have no good ideas about how to do that.

I did some more testing, and I think this will work. Now, I also have
a suggestion about the manager autodiscovery API:
- All managers defined for the model are added into the
queryset.managers list. That is, the method resolution order is the
same order in which the managers are defined. Current model's managers
first, then parent model managers.
- The managers list should be _meta option. Currently this is not
available in the above order, but it should be possible to
have ._meta.ordered_managers attribute. This could then be used in
__getattr__. Instead of doing for manager in self.managers do: for
manager in self.model._meta.ordered_managers.
- You can also access managers directly, if you had:

class M1(Manager):
some_constant = 'bar'
def get_foo_objs(self):
return self.get_query_set().filter(foo=self.some_constant)

class M2(Manager):
def get_foo_objs(self):
return self.get_query_set().filter(foo='baz')

class SomeModel(Model):
...
objects = M1()
other_objects = M2()

and you do SomeModel.objects.order_by('id').get_foo_objs(), you will
get M1 get_foo_objs(). However, you can also do
SomeModel.objects.order_by('id').other_objects.get_foo_objs(), that
is, you can get the dynamically created manager by using other_objects
on the queryset. The only nasty thing about this is that now _all_
methods of other_objects manager are available to you, not only those
which are actually chainable.

I think the base idea (dynamic manager) has some merit which can not
be easily achieved by adding the chainable methods to QuerySet
(dynamic) subclasses:
- You can easily upgrade your current managers. Verify that your
manager method is chainable (it is based on .get_query_set()
and .get_query_set() is not overridden). If so, add @chainable and
things should just work.
- You can call other methods of the manager, or access class
variables. This is not achievable in the QuerySet subclassing idea,
unless you transfer all the methods, and all the variables to the
QuerySet subclass. And in this case, @chainable decorator is
pointless. For example M1.get_foo_objs() would not work, because it
accesses self.some_constant, and it would be hard to make that
available in the queryset subclass.
- You can't access duplicate manager methods. In the above example
M2.get_foo_objs() would not be available through the queryset.

It could be possible to make the "dynamic manager" idea work even
without creating dynamic manager subclasses. Just define
Manager.get_query_set() as:
if self._chain_to_qs is not None:
return self._chain_to_qs
else:
# return new queryset as normally.

And the only thing you need to do in __getattr__ is instantiating the
manager, set its _chain_to_qs to self. And return the asked method by
getattr.

This would make even overridden get_query_set() based managers work,
as long as your overriding implementation calls
super().get_query_set().

I have updated the draft patch according to the above idea of
modifying manager.get_query_set(). The patch is even more simple than
before. The manager resolution order is not implemented (I use
chain(_meta.abstract_managers, _meta.concrete_managers). @chainable
is still not implemented. The patch is available from
https://github.com/akaariai/django/compare/dynamic_man

It would be pretty easy to return the "dynamic managers" from
__getattr__, but I am not sure that is a good idea, so I have left
that out.

- Anssi

[Zachary Voase]

It should be relatively simple to allow QuerySets to know which
manager they came from, and then override __getattr__ to use the
declared @querymethod from that manager (instead of the other way
around). This avoids the unpickleable QuerySet subclass problem.

> is still not implemented.  The patch is available fromhttps://github.com/akaariai/django/compare/dynamic_man

[Anssi Kääriäinen]

On Jan 10, 12:19 pm, Zachary Voase <zacharyvo...@gmail.com> wrote:
> It should be relatively simple to allow QuerySets to know which
> manager they came from, and then override __getattr__ to use the
> declared @querymethod from that manager (instead of the other way
> around). This avoids the unpickleable QuerySet subclass problem.

Passing knowledge of which manager the method came from is trivial.
Just set qs.manager = self.__class__ in manager.get_query_set() and
use qs.manager in __getattr__.

However, I would like it if you could mix methods from different
managers. Maybe add qs.set_manager(manager_name), and so you could do:
SomeModel.objects.order_by('id').some_manager_method().set_manager('other_manager').some_manager_method_in_other_manager().
set_manager would simply change the qs.manager. This way you could
change the manager which is looked for chainable methods, and all
chainable methods would be available from all managers.

I hoverer do like the idea that you have all the methods defined in
any manager directly available in the queryset, without the need to
use set_manager. This is easy to achieve.

Thoughts?

- Anssi

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Anssi,

On 01/10/2012 04:14 AM, Anssi K��ri�inen wrote:
> However, I would like it if you could mix methods from different
> managers. Maybe add qs.set_manager(manager_name), and so you could do:
> SomeModel.objects.order_by('id').some_manager_method().set_manager('other_manager').some_manager_method_in_other_manager().
> set_manager would simply change the qs.manager. This way you could
> change the manager which is looked for chainable methods, and all
> chainable methods would be available from all managers.
>
> I hoverer do like the idea that you have all the methods defined in
> any manager directly available in the queryset, without the need to
> use set_manager. This is easy to achieve.
>
> Thoughts?

Having access to any manager method from any queryset, regardless of
which manager that queryset came from, seems to me like a highly
surprising anti-feature, not a benefit. Can you outline a realistic use
case for it?

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8McRgACgkQ8W4rlRKtE2fAlwCgq6JcsmkEyUR4djK8bPr49Uf0
lWIAmwdn2oAVqrv7GXph/qePaCYCuVVp
=GL8K
-----END PGP SIGNATURE-----

[Anssi Kääriäinen]

On Jan 10, 7:10 pm, Carl Meyer <c...@oddbird.net> wrote:
> Having access to any manager method from any queryset, regardless of
> which manager that queryset came from, seems to me like a highly
> surprising anti-feature, not a benefit. Can you outline a realistic use
> case for it?

Basically any time when you have multiple managers (due to model
inheritance, for example) and need to mix operations from different
managers. For example you could have an abstract base class
Publishable which defines 'publishable_manager' with one method
"visible(to_user)". Then you might need to mix methods:
News.objects.filter_by_search_params(search_params).visible(request.user)

I do think that having some way to access methods of different
managers is needed. Otherwise if you want to use all the defined
chainable methods in a single queryset, you will need to have them in
single manager. This results in unnecessary manager multi-inheritance.

My next idea is .use_manager(), so that you can change the queryset's
base manager on the fly. This simplifies the implementation, and is
more intuitive to begin with. The above would then be:
News.objects.filter_by_search_params(search_params).use_manager('publishable_manager').visible(request.user)

My API design skills are pretty bad, so other opinions welcome :)

Updated patch at github (https://github.com/akaariai/django/compare/
dynamic_man). It now has use_manager(), and checks methods only from
the manager the QuerySet came from (or from manager set
by .use_manager). It still misses @querymethod. Error messages are
messy, and there are no tests or documentation.

I really have no time to push this forward. I hope the idea is useful.
The patch at github is in good enough state that you can try the API.
Usage is really simple. Clone the branch, and you should be able to
use any manager method which uses self.get_query_set() as its base
query. @querymethod is nothing more than a marker at this point.

- Anssi

[Zachary Voase]

I strongly feel that switching around managers like that would be an
example of overengineering.

The user story which drove this RFC was based on an issue which has
been encountered by lots of developers, but typically solved in a
single way. Django's conservative style of development is very
accommodating to this 'lowest common denominator' approach to adding
features. However, by adding this manager-switching feature, we're
second-guessing our users' intentions, and introducing unjustified
complexity to the API.

Here's the change I propose: https://github.com/zacharyvoase/django/compare/master...feature-querymethod

It's very light for now, I'm not sure if there are other corner cases
where it breaks, but it's the simplest thing that could possibly work,
and it's conceptually very easy to understand.

On Jan 10, 8:30 pm, Anssi Kääriäinen <anssi.kaariai...@thl.fi> wrote:
> On Jan 10, 7:10 pm, Carl Meyer <c...@oddbird.net> wrote:
>
> > Having access to any manager method from any queryset, regardless of
> > which manager that queryset came from, seems to me like a highly
> > surprising anti-feature, not a benefit. Can you outline a realistic use
> > case for it?
>
> Basically any time when you have multiple managers (due to model
> inheritance, for example) and need to mix operations from different
> managers. For example you could have an abstract base class
> Publishable which defines 'publishable_manager' with one method
> "visible(to_user)". Then you might need to mix methods:
> News.objects.filter_by_search_params(search_params).visible(request.user)
>
> I do think that having some way to access methods of different
> managers is needed. Otherwise if you want to use all the defined
> chainable methods in a single queryset, you will need to have them in
> single manager. This results in unnecessary manager multi-inheritance.
>
> My next idea is .use_manager(), so that you can change the queryset's
> base manager on the fly. This simplifies the implementation, and is
> more intuitive to begin with. The above would then be:

> News.objects.filter_by_search_params(search_params).use_manager('publishabl e_manager').visible(request.user)

[Etienne Robillard]

On 01/12/2012 07:24 AM, Zachary Voase wrote:
> I strongly feel that switching around managers like that would be an
> example of overengineering.
>
> The user story which drove this RFC was based on an issue which has
> been encountered by lots of developers, but typically solved in a
> single way. Django's conservative style of development is very
> accommodating to this 'lowest common denominator' approach to adding
> features. However, by adding this manager-switching feature, we're
> second-guessing our users' intentions, and introducing unjustified
> complexity to the API.
>

Agreed +1 it seems as well the whole model inheritance has the potential
to complexify code, not making it any more simpler than
it should by manually making SQL queries with the .extra method.

Cheers,
Etienne

[Anssi Kääriäinen]

On Jan 12, 2:24 pm, Zachary Voase <zacharyvo...@gmail.com> wrote:
> I strongly feel that switching around managers like that would be an
> example of overengineering.

I tend to over-engineer things. It is easy to add the manager
switching later on if needed. And it is easy to switch managers by
just changing qs.manager if really needed. So, I agree on leaving that
feature out.

> Here's the change I propose:https://github.com/zacharyvoase/django/compare/master...feature-query...

>
> It's very light for now, I'm not sure if there are other corner cases
> where it breaks, but it's the simplest thing that could possibly work,
> and it's conceptually very easy to understand.

And in the implementation I disagree. The implementation magically
changes what self references in querymethods. This will break any code
that wants to reference self in manager. For example:

def get_id_list_by_raw_sql(self, some_params):
return cursor.execute("select if from sometable where someparam =
%s", (some_param,))

@querymethod
def list_interesting_subjects(self, some_params):
self.filter(id__in=self.get_id_list_by_raw_sql(some_params))

Now, the above will work just fine if called through manager, but it
will not work if called through queryset, because self suddenly refers
to the qs, not to the manager. Depending how it is called. So, I would
suggest an approach like in my version, where self is always the
manager, and you get the queryset by self.get_query_set() (or maybe
with self.base_qs, which is a property returning either
self.get_query_set() or the queryset called from).

The main reason why I think it is a bad idea to "transfer" the method
to the qs is that if you do that, you will confuse people (what does
self refer to?), forbid some usecases, and gain exactly nothing by
doing that. And that can not be changed later on. Why not just let the
method be always bound to the manager instance? That is much cleaner.
This all assuming I actually understand what the patch does. Can't
test it currently...

Am I missing something that is gained by currying the method to
queryset instead of letting it be bound to the manager?

Still another idea is to somehow patch the parameters to the method in
querymethod, so that the queryset to chain to would be passed as a
parameter to the querymethod. It would either be self.get_query_set()
or the queryset called through.

- Anssi

[Michael Mior]

Got interested in this after a referral from the tricket tracker. Couldn't this specific case be solved with a simple change to __getattr__ on QuerySet?

 def __getattr__(self, attr):

     try:

         qmethod = getattr(self.manager, attr).im_func

         is_querymethod = qmethod.is_querymethod

     except AttributeError, exc:

         raise AttributeError(attr)

     if is_querymethod:

-        return curry(qmethod, self)

+        return curry(qmethod, self.manager)

     raise AttributeError(attr)

The only issue I see here is that it assumes self.manager is always defined so this would break if a QuerySet was instantiated outside of Manager.get_query_set(). But really, it doesn't make sense to add the @querymethod decorator on QuerySet functions anyway.

- Michael