Fixed #20625 -- Chainable Manager/QuerySet methods. #1328

loic · 2013-07-03T18:41:11Z

No description provided.

mjtamlyn · 2013-07-04T07:09:56Z

docs/topics/db/managers.txt

+        sex = models.CharField(max_length=1, choices=(('M', 'Male'), ('F', 'Female')))
+        people = PersonManager()
+
+This example allows you to call both ``men()`` and ``women()`` directly from the manager ``Person.people``.


Text wrapping

mjtamlyn · 2013-07-04T07:22:58Z

Overall I love this change!

Just to check cos I'm not that clear on how inspect is working - are classmethods copied over here? I feel perhaps they should not be, but maybe we should be checking that. Similarly what happens to properties - I'm looking at ordered and db in particular.

loic · 2013-07-04T09:45:57Z

@mjtamlyn for classmethods it depends. In PY3 these are not included, in PY2 they are if they are public and their manager attribute is not False. A minor issue is that to set the manager attribute we need to use old style decorator (as_manager = classmethod(as_manager) instead of @classmethod). properties are not included.

I've added a new commit with a test that ensures that the stock Manager gets exactly the same proxy methods as before. I'll leave it in a seperate commit for now, I'll squash the commit when we are close from completion.

Btw thanks for the review. I think I've already made most of the changes you've suggested.

loic · 2013-07-05T06:31:22Z

django/db/models/query.py

+        manager_cls._queryset_class = cls
+        return manager_cls
+
+    def as_manager(cls, base_class=None):


Should we keep the base_class argument for this method?

As documented it's already possible to set the attribute QuerySet.base_manager_class, so it effectively introduces more than one way to do it.

@akaariai: I leave it up to you since it was part of your original proposal.

I removed it, I think it makes for a cleaner API. It wasn't documented by an example anyway because I didn't want to show 2 different ways of doing the same thing.

In any case, it would be a lot easier to add it back if people feel a need for it than it would be to deprecate it down the road.

loic · 2013-07-09T16:07:20Z

@timgraham: I made most of the suggested changes.

The comment regarding the gender issue has been collapsed because of the updated diff but I replied to it.

There is also a pending remark regarding the use of "automatically".

mjtamlyn · 2013-07-09T21:36:13Z

@loic FYI this is no longer merging cleanly. We can probably fix it when we merge it though

loic · 2013-07-09T21:56:07Z

@mjtamlyn thanks for the heads up; I've rebased to the latest master.

timgraham · 2013-07-10T14:28:28Z

docs/topics/db/managers.txt

+This example allows you to call both ``men()`` and ``women()`` directly from
+the manager ``Person.people``.
+
+.. _create-manager-with-queryset-methods:


I'd add a section heading here "Creating Manager instances with QuerySet methods" (you can then remove the custom text from the :ref: links as well) and make it more clear that this approach is intended to replace the above: "In lieu of the above approach which requires duplicating methods on the both the QuerySet and the Manager, ...."

Done even though GH doesn't collapse this outdated diff.

aaugustin · 2013-07-22T08:40:54Z

django/db/models/query.py

+            return manager_method
+
+        new_methods = {}
+        predicate = inspect.isfunction


This would be more readable as a one-liner:
predicate = inspect.isfunction if six.PY3 else inspect.ismethod

No worries, will update promptly.

aaugustin · 2013-07-22T09:14:47Z

django/db/models/query.py

+                continue
+            # Only copy public methods or methods with the attribute `manager=True`.
+            should_copy = getattr(method, 'manager', None)
+            if should_copy is False or should_copy is None and name.startswith('_'):


Please add parentheses around the and block, precedence isn't obvious.

loic · 2013-07-22T15:01:19Z

I tested the __new__ idea from @akaariai on the ML, the conclusion is that it'll break inheritance, just like __getattr__ and __getattribute__ did.

The only way to support inheritance is by generating real classes ahead of time, a class factory can do that, the Manager(CustomQueryset) API can't.

Between CustomQuerySet.as_manager() and Manager.from_queryset(CustomQuerySet), I prefer the former by a big margin. I've been writing custom Manager classes extensively and with this patch in core, I don't plan on ever writing a custom Manager. Manager.from_queryset(CustomQuerySet) is more verbose and requires importing a Manager class that we are not going use.

Also I'd like to point out that both base_manager_class and the manager method attribute are there to support advanced usage, most people will never have to worry about them.

loic · 2013-07-22T17:34:32Z

Added two commits which hopefully address the concerns of making QuerySet overly dependent on the Manager class.

I still like better the end user API of CustomQuerySet.as_manager(), but Manager.from_queryset(CustomQuerySet) does have the advantage of removing the need for the base_manager_class attribute. To be completely fair to the as_manager() factory, CustomQueryset.as_manager(CustomManager) could have had the same effect.

I've wanted this general feature pretty much since I've started using Django, so if there is consensus on from_queryset(), count me in.

Edit: One more reason I'm not so keen on the Manager(CustomQuerySet) idea is that we add the burden of handling the queryset_class argument onto anyone with a custom Manager.__init__(), especially considering it's a kwarg that will generally be used as an arg.

akaariai · 2013-07-22T18:40:04Z

Both as_manager() and from_queryset() work and have some advantages each. as_manager() is slightly shorter and requires one less import in the common case, while from_queryset() is better if you need both custom queryset and manager classes, and doesn't have the circular dependency problem.

My preference is as_manager() because it makes the common case shortest. But, I can go with from_queryset(), too. Most of all I don't want to stall the patch on this issue. Seems like overall there is a slight preference for from_queryset(), so if no further ideas or opinions are presented lets just go with from_queryset().

loic · 2013-07-23T08:49:12Z

Maybe there is a middle ground:

Make Manager.from_queryset() return a class, therefore replacing Manager.create_from_manager().
Make QuerySet.as_manager() a shortcut for the common case, therefore removing support for custom base Manager which eliminates the need of handling __init__() arguments.

So effectivelly you could do:

class BaseManager(Manager):
    def __init__(self, *args, **kwargs):
        pass

CustomManager = BaseManager.from_queryset(CustomQuerySet)

# For the common case where you only need a custom `QuerySet`:
manager = CustomQuerySet.as_manager()

# When you need a custom `Manager`:
manager = CustomManager(*args, **kwargs)
# or
manager = BaseManager.from_queryset(CustomQuerySet)(*args, **kwargs)

Edit: One thing I like about this approach is that we make it elegant to create a CustomManager class, which will become useful later on when tickets like #14891 are fixed.

Edit 2: d8d3a60 implements this. I'm now nearly convinced this is the way to go.

Edit 3: Touched up the docs which should be mostly accurate again, we may need to update the release notes to mention from_queryset() depending on how much we want to promote the advanced usage.

carljm · 2013-07-23T18:43:49Z

What is the argument for QuerySet.as_manager() (vs just using Manager.from_queryset in all cases)? Simply to save the user one import? It doesn't even do that; in your models file you almost certainly have from django.db import models already, so it's just models.Manager.from_queryset(MyCustomQuerySet)() with no additional import needed. Is it to save the additional parentheses?

Personally I don't see sufficient justification there for adding manager-specific API to queryset and making the dependency between those classes bidirectional.

loic · 2013-07-23T19:29:42Z

My argument is that this feature will pretty much deprecate the concept of manager as we know it. And when most of the time all we need is MyQueryset.as_manager(), we will be wondering why the API is as clunky as models.Manager.from_queryset(MyQueryset)().

This new feature opens up a lot of new possibilities, QuerySets that generate disposable Managers are much more flexible than custom Managers ever were. Look at how many projects provide custom Managers, yet they are inflexible and can't work with each other.

With this new feature we can do:

class CompositeQuerySet(RatedQuerySet, PublishedQuerySet, SiteRelatedQuerySet):
    pass
objects = CompositeQuerySet.as_manager()

Which replaces:

class RatingManager(Manager):
    ...
class PublishingManager(Manager):
    ...
class SiteManager(Manager):
    ...
objects = ? # Pick one only.

Combining multiple QuerySets, even if they are provided by different apps, makes them work together; you can effectively chain the filtering provided by each of them and start the chain all the way from the model manager instance. I don't see why we should put the legacy class in the middle of this API.

Yet this patch ensures that anyone who really want to create a custom Manager type from a custom QuerySet can do so in an elegant way, but we have to realize that it's going to be an exception, not the norm.

Edit: Reworked the example and changed wording to some extent.

Edit 2: For the record, I do not suggest we deprecate the Manager class, nor do I think it'll become obsolete, quite the contrary. If anything I want to see more places where we can customize them. What I believe will become obsolete is the way we've been defining them and using them.

shaib · 2013-07-24T12:43:56Z

django/db/models/manager.py

+            '_queryset_class': queryset_class,
+        }
+        class_dict.update(cls._get_queryset_methods(queryset_class))
+        return type(cls.__name__, (cls,), class_dict)


I'd try to build a more informative name, using also the queryset_class's name; ("%sFrom%s" % (cls.__name__,queryset_class.__name__)) seems reasonable. True, normally you won't see the class name in code, but it will be helpful in tracebacks and Django debug screens where locals are printed.

I agree with this suggestion; it's a small change but it could really help debugging. Without it, all manager classes created from querysets will just be called "Manager", which could be quite confusing if you're working with more than one of them in a PDB session or a traceback.

Yep, I'm on it already, running the test suite.

Done. Please review the implementation.

Additionally this patch solves the orthogonal problem that specialized `QuerySet` like `ValuesQuerySet` didn't inherit from the current `QuerySet` type. This wasn't an issue until now because we didn't officially support custom `QuerySet` but it became necessary with the introduction of this new feature. Thanks aaugustin, akaariai, carljm, charettes, mjtamlyn, shaib and timgraham for the reviews.

timgraham · 2013-07-29T10:49:43Z

merged in 31fadc1

mjtamlyn reviewed Jul 4, 2013
View reviewed changes

loic reviewed Jul 5, 2013
View reviewed changes

timgraham reviewed Jul 10, 2013
View reviewed changes

This was referenced Jul 11, 2013

For ticket #20625 to allow defining chainable QuerySet on custom model managers. #1284

Closed

Custom QuerySets without creating custom Manager #317

Closed

aaugustin reviewed Jul 22, 2013
View reviewed changes

shaib reviewed Jul 24, 2013
View reviewed changes

timgraham closed this Jul 29, 2013

Uh oh!

Fixed #20625 -- Chainable Manager/QuerySet methods. #1328

Fixed #20625 -- Chainable Manager/QuerySet methods. #1328

Uh oh!

Conversation

loic commented Jul 3, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjtamlyn commented Jul 4, 2013

Uh oh!

loic commented Jul 4, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic commented Jul 9, 2013

Uh oh!

mjtamlyn commented Jul 9, 2013

Uh oh!

loic commented Jul 9, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic commented Jul 22, 2013

Uh oh!

loic commented Jul 22, 2013

Uh oh!

akaariai commented Jul 22, 2013

Uh oh!

loic commented Jul 23, 2013

Uh oh!

carljm commented Jul 23, 2013

Uh oh!

loic commented Jul 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Jul 29, 2013

Uh oh!

Uh oh!