[Django] #20625: Custom Chainable QuerySets
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Keywords: QuerySet,
Severity: Normal | models.Manager, chainable
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Provide a mechanism for defining chainable custom QuerySet directly
through a subclassed `models.Manager` definition instead of the current
approach that requires a custom `models.Manager` + `QuerySet` pair. This
ticket potentially replaces/complements
https://code.djangoproject.com/ticket/16748.
I suggest that we allow defining chainable querysets by allowing the
django programmer to add a `chainable == True` attribute to the custom
queryset method. The proposed syntax would look as shown below and a
working github pull request will be submitted shortly; thoughts and
feedback welcomed.
{{{
class OfferManager(models.Manager):
""" Example of a chainable custom query set """
...
QUERYSET_PUBLIC_KWARGS = {'status__gte': STATUS_ENABLED}
QUERYSET_ACTIVE_KWARGS = {'status': STATUS_ENABLED}
...
def public(self):
""" Returns all entries accessible through front end site"""
return self.all().filter(...)
public.chainable = True # instructs to dynamically tranplat this
method onto
# returned QuerySet as
<queryset>.public(...)
#
effectively providing chainable custom QuerySets
def active(self):
""" Returns offers that are open to negotiation """
return self.public().filter(**OfferManager.QUERYSET_ACTIVE_KWARGS)
#
an example of how to reffer to OfferManager
#
constants as 'self' context changes
active.chainable = True
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Database layer | Version: master
Severity: Normal | Triage Stage:
Keywords: QuerySet, | Unreviewed
models.Manager, chainable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by danols):
* status: new => assigned
* needs_better_patch: => 0
* owner: nobody => danols
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:1>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage:
Keywords: QuerySet, | Unreviewed
models.Manager, chainable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by danols):
Initial pull request created: https://github.com/django/django/pull/1284
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:2>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage:
Keywords: QuerySet, | Unreviewed
models.Manager, chainable | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Properly formatted usage code example:
...
...
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:3>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by lukeplant):
* stage: Unreviewed => Accepted
Comment:
I'm accepting the ticket in principle - we need a solution to this
problem, evidenced by the number of solutions/complaints out there:
https://github.com/zacharyvoase/django-qmethod
http://stackoverflow.com/questions/4576622/in-django-can-you-add-a-method-
to-querysets
http://djangosnippets.org/snippets/562/
http://djangosnippets.org/snippets/734/
http://djangosnippets.org/snippets/2114/
http://djangosnippets.org/snippets/2117/
And I don't think having multiple solutions to this problem helps people -
it just adds more-than-one-way-to-do-it.
However, I'm not at the moment accepting the approach given in the ticket
(neither am I rejecting it). We need to consider the pros/cons of the
different methods in the existing solutions above.
For the solution in the PR above, I'd say the major disadvantage is that
you have methods on the Manager which are not being used as Manager
methods, but callables that could be attached to either Managers or
!QuerySets. This means surprises w.r.t to `self` (as documented in the
example), and it would be easy to think it is working (because it works
without chaining), or to have nasty surprises if you call methods that
exist on !QuerySet but have been overridden on the Manager subclass.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:4>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: loic@… (added)
Comment:
Overlapping ticket with related discussion: #16748
I see two main patterns in these attempts:
1. Those that make it easier to provide a custom `QuerySet`:
https://github.com/jtillman/django/compare/CustomQuerySet
https://github.com/django/django/pull/317
http://djangosnippets.org/snippets/734/
2. Those that pass custom `Manager` methods onto the `QuerySet`:
https://github.com/django/django/pull/1284/
https://github.com/zacharyvoase/django-qmethod
http://djangosnippets.org/snippets/562/
I personnaly favor the second option because it results in an API where
both `Model.objects.custom_method()` and
`Model.objects.all().custom_method()` work, which is what you've come to
expect with the methods from the stock `QuerySet`.
Before we worry about the implementation we'll have to settle on the
general idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:5>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I believe one problem with the proposed way is that chainable manager
methods are lost when the QuerySet switches its class. The switching
happens as side effect of certain calls (for example .values(),
.values_list()). Also, creating dynamic classes as done in the patch is
likely somewhat expensive, and will also likely break pickling (as the
dynamic class can't be found from module level). Both of the latter
problems are solvable, first one by caching the dynamic class, second one
by altering QuerySet.reduce().
Other solutions to this problem are to make Manager itself a QuerySet (or
subclass of it). I tried this some time ago and it was evident this path
has a lot of problems ahead. Yet another solution is to call the chainable
manager methods from the manager, but explicitly pass the queryset into
the method, something like
{{{
class MyManager:
@chainable
def published(self, chain_to, other_params...):
return chain_to.filter(...)
}}}
with some meta-magic (or just plain old-style wrapper function) the
chain_to could be automatically created as get_queryset() if called from
manager, but if called from qs, then chained would be the qs instance.
However it will be a bit confusing to call this method with
objects.published(datetime.now())... Alternative is to not magically
provide the chain_to qs when called from manager - instead the user should
create the chain_to if it is None.
A big +1 to the idea, but there needs to be a bit more discussion about
what way to solve this.
My opinion is that if it is possible to get rid of the Manager class
completely, or make it QuerySet subclass, that would be optimal path.
Unfortunately this seems hard to do.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:6>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
akaariai's idea of having `Manager` inherit from `QuerySet` made me think
of another option.
We can copy `QuerySet` methods onto the `Manager`.
That's similar to what we manually do to proxy all the current `QuerySet`
methods, we'd just have to do it dynamically.
The end user's code would look like this:
{{{
class MyQueryset(QuerySet):
def is_published(self):
pass
class MyModel(Model):
objects = Manager(queryset_class=MyQueryset)
>>> hasattr(MyModel.objects, 'is_published')
True
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:7>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Proof of concept: https://github.com/loic/django/compare/ticket20625
I wanted to leverage the Django test suite, so I tested my concept on the
actual `Manager` class, the complete suite is passing.
We might not be able to do such refactor in the wild (removing all the
proxy methods) since calls to `super` wouldn't work anymore, but the idea
is there.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:8>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
FWIW here's [https://gist.github.com/charettes/5822558 what I've been
using for the past years].
It doesn't account for QuerySet class changes thought (`values_list`) and
should be adapted to make sure `Model.objects.delete()` can't be call
directly.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:9>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by danols):
From the semantic/syntax angle I don't believe we should pursue the idea
of combining QuerySet and models.Manager into one as to me even though
they do overlap in functionality they make sense to remain distinct, and
the more I think about creating custom QuerySets on the models.Manager
level the more I realize it is not the the right approach; it is just what
we are familiar with and thought through the Django documentation.
I think the `objects=Manager(queryset_class=...) is the correct way , I
would say all or almost all of the custom managers I have written should
actually have been custom QuerySets. So I endorse loic84 approach (but
would like to see if we could omit the need for the '.manager' attribute).
So Djangonauts - speak up and let's get a consensus going :)
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:10>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
@charettes' proposal is pretty much identical to mine (as opposed to the 2
patterns described in comment:5), except for the factory which if I
understand correctly wouldn't be needed anymore if this made it into core.
I picked an opt-in strategie with `QuerySet.method.manager=True` but we
could just as well pick the following opt-out strategie:
- Accept by default if `QuerySet.method.manager` is not present and the
method name doesn't start with an underscore (thus protecting private and
magic methods)
- `QuerySet.method.manager=False` for public methods that don't make sense
at the `Manager` level (i.e. `QuerySet.delete()`)
- `QuerySet.method.manager=True` to opt in for private methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:11>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
@loic84 patch is looking good!
The only issue left is the `QuerySet` class changes, e.g. `values()` which
returns a `ValuesQuerySet` instance that wouldn't inherit for the custom
`QuerySet` subclass.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:12>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
@charettes: It's not a trivial issue! I pushed an extra commit to the
branch where I play with dynamic types, everything but pickling works. Any
suggestion?
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:13>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Added another commit which fixes pickling, I believe this POC is as good
as it can be, we now have to decide whether it's the approach we want to
pursue.
If we decide to go for it, we need to settle on the scope (do we try to
deprecate all legacy proxy methods or not) and I'll turn it into a proper
patch with tests and docs.
For the record, I did try having `Manager` inherit from `QuerySet`, it's
quite hard because of all the magic methods. I also believe it's
undesirable in term of API as there are plenty of `QuerySet` methods - the
private ones in particular - that we don't want on the manager.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:14>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by loic84):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* needs_docs: 0 => 1
Comment:
I've tried to come up with a strategy to deprecate all the legacy proxy
methods.
The issue at hand is custom managers in the wild that call `super` in
overridden methods.
@charettes mentioned on IRC that `__getattribute__` would enable calls to
`super` but that we wouldn’t be able to differentiate a normal access
(which shouldn't go through the legacy method) and calls to `super` which
should trigger the deprecation warning.
We could have a metaclass, similar to `RenameMethodsBase` which adds back
legacy methods for every method with the same name in the MRO: if our
method is ever called (by `super`) we trigger the deprecation warning.
I ran into a problem when trying to deprecate `Manager.create()` and
`Manager.get_or_create()` with `RelatedManager` (and similar):
https://gist.github.com/loic/5861660
https://github.com/django/django/blob/master/tests/multiple_database/tests.py#L907-L921
If `RelatedManager` doesn't call ` super`, we'll never reach custom
managers' `create()` and `get_or_create()`. I don't see a solution to that
other than making a backward incompatible change. Or we could elect a few
proxy methods that shouldn’t be deprecated (like `Manager.all()`, see
comment in the patch).
Any suggestions?
If we can't reach consensus on the deprecation plan I suggest we move on
with this ticket and open a new one regarding the deprecation itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:15>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
@loic84 I agree that deprecation/removal of `Manager` proxy methods
shouldn't block this ticket and discussion should be moved to another
ticket once we manage to come up with a solution for this one.
We should play safe and avoid messing with `Manager`'s MRO too much in the
1.6 release.
Lets see if #15363 behaves correctly first to avoid flooding users with
deprecation warnings all over the place.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:16>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: danols => loic84
Comment:
To summarize a bit:
- The latest patch should cover all concerns raised so far: it handles
specialized querysets such as `ValuesQuerySet`, doesn't break pickling,
cruft-free methods definitions, sane defaults (no private or magic
methods), a mechanism to opt-out or force-in certain methods and it's
robust enough to sustain the complete Django test suite while replacing
almost 90% of `Manager`'s public API.
- We settled that deprecating legacy proxy methods is out of scope for
this ticket while acknowledging that it's desirable in the long run. We'll
open a dedicated ticket once we land this feature.
- Since we are '''not''' deprecating the legacy methods, we need a
dedicated test suite.
- 1.6b1 has landed, so we can resume work targeting master.
Where are we in term of consensus? I'll turn this into an RFC patch as
soon as I get a green flag on the implementation.
Latest implementation can be found here:
https://github.com/loic/django/compare/ticket20625_optout.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:17>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I have tried various versions of making Manager a subclass of QuerySet.
The most promising approach was to use a temporary class for hiding
QuerySet only functionality from the manager, and drop the temp class on
first clone. So:
1. On creation of manager, create a dynamic class and add
HideQuerySetMethods to the `__bases__` of the dynamic class. Effect is
that for example .delete() calls HideQuerySetMethods.delete(), not
QuerySet.delete().
2. On first clone, switch the class to the original class, that is drop
the HideQuerySetMethods class from `__bases__`.
I got the solution to almost working state, but it required too many
kluges. I ended up deleting the branch as there was just too many little
problems to make it work cleanly.
I have seen the idea of this ticket discussed multiple times. Every time
some solutions are offered, but the solutions always have some problematic
cases that end up stalling the development effort. I think the solution in
this ticket is clean, and makes it easy enough to add queryset methods.
So, my opinion is that we should go forward with it.
I wouldn't actually remove the proxy methods at all. Removing them doesn't
seem worth the effort, the maintenance burden of the methods is very
small. If they are going to be removed, then all must be removed in one
go. It is just weird if some of the proxy methods are still there because
Django needs the super() method to be present, but other are removed.
It seems #17270 is a duplicate, I guess that one could be closed as this
ticket seems to have more momentum at the moment.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:18>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
@akaariai: I agree with the all or nothing stance regarding the method
deprecation, and I also agree that the maintenance burden is rather small.
Just a clarification, Django itself doesn't need the calls to `super()`,
the problem is backward compatibility.
I'll tackle docs and tests tomorrow, hopefully we'll have an RFC patch
pretty soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:19>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Sorry for the Django super() requirement confusion.
While writing my previous post I got an idea. I did some quick testing and
it seems the idea needs to be presented. There are two key things in the
idea:
- Managers are created by CustomQuerySet.manager_cls() method - this is
a factory constructing a new manager class.
- The new manager class has dynamically created proxy methods for those
methods in the CustomQuerySet that should be present in the manager.
So, you could do something like this:
{{{
class MyQuerySet(models.QuerySet):
def bar(self):
return ...
bar.manager = True
class MyModel(Model):
objects = MyQuerySet.as_manager()
}}}
Now, MyModel.objects is a custom Manager subclass which has a proxy method
for bar(). And it really *has the method*, so super() works correctly.
The API might not be optimal, but the idea that proxy methods are
automatically created on dynamically created Manager classes should be
considered carefully. In my opinion it offers better semantics than the
`__getattr__` based approach.
I wrote a very quick proof-of-concept patch. It is available from:
https://github.com/akaariai/django/tree/manager_factory. The only
interesting part of the patch is the method at
https://github.com/akaariai/django/commit/283af213d1873cefd642b6d9aeb97f285ef227c4#L2R44
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:20>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
@akaariai: I'm glad you presented this idea; I find `as_manager()` very
elegant as it removes all the boilerplate from `Manager` while remaining
backward compatible, it's probably more efficient as well.
Its semantic is already used throughout Django (`View.as_view()`,
`Form.as_p()`, etc.) so people should feel right at home.
Regarding the copy logic, I see you've used the "opt in" strategy from my
initial patch, as per @danols' suggestion I like better the "opt out with
smart defaults" one.
I had a stab at it:
https://github.com/loic/django/compare/loic:ticket20625_copy
{{{
class MyQuerySet(models.QuerySet):
# Private methods are ignored by default.
def _foo(self):
return ...
# Public methods are copied by default.
def foo(self):
return ...
# Opt in.
def _bar(self):
return ...
_bar.manager = True
# Opt out.
def bar(self):
return ...
bar.manager = False
class MyModel(Model):
objects = MyQuerySet.as_manager()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:21>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Yes, I like the smart defaults idea more than opt-in.
The only ugly case in the as_manager() approach seems to be that if you
want manager-only methods you will need to create a Manager class, and
then use MyQuerySet.as_manager(base_cls=MyManager) (or alternatively
subclass MyQuerySet.manager_cls()). This isn't as-pretty-as-possible. But,
in general having methods on the qs even if they only make sense as
manager methods isn't too bad. Django has multiple examples of this,
qs.create() doesn't actually make sense, but the create on qs doesn't hurt
anybody either.
I am sure that if one defines the same method on both the qs and the
manager in complex enough ways you can break super(). This seems to be a
case of "don't do that".
I think it is now the time to complete the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:22>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
@akaariai: wouldn't that address the issue?
{{{
class MyManager(Manager):
def manager_only():
pass
class MyQuerySet(QuerySet):
manager_class = MyManager
def manager_and_queryset_method():
pass
class MyModel(Model):
objects = MyQuerySet.as_manager()
}}}
So basically rename `manager_cls()` to `get_manager_class()` and have it
use a `cls.manager_class` (or `cls.base_manager_class`) attribute.
If we go for that we should probably remove `Manager.queryset_class` since
it would become an antipattern.
Are you planning to complete the patch or should I do it?
Since we are replacing all the legacy methods with this technique there is
not much left to do in term of testing: basically just the copying logic.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:23>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Implementation of the idea above with accompanying (quick and dirty) test
at
https://github.com/loic/django/commit/2e129ae8aba862054373a3c5789493bc456d1ed7.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:24>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Seems good to me.
If you want a custom manager, but not queryset you can use subclass
Manager approach. If you want custom queryset with methods added to the
manager, you can use custom queryset + as_manager(). If you want both, you
will need to have both Manager and QuerySet subclass, and use
QuerySet.manager_class to tie them together. This feels clean and correct.
I think one area in need of testing is what happens when you define a
custom method on both Manager and QuerySet class. I am sure that complex
scenarios will break, but that is OK. This is mostly knowing what will
fail, and if possible having informative error messages for the failing
cases.
I am currently on holiday so I am not willing to spend too much time to
work on Django, feel free to go for the final patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 1
models.Manager, chainable | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
The only issue I can think of is that we would override the `Manager`
method when its is defined on both.
I've just addressed that in
https://github.com/loic/django/commit/b1d6d2a0cbdb5ad7decc2cf6fc8b8dde960061cf.
I previously shielded `Manager.all()` from this issue with `manager=False`
so it's no longer necessary. I believe `Manager.all()` and
`RelatedManager.create()|get_or_create()` are in the situation you've
described and it seems to work well.
I'll start working on an RFC patch, hopefully I'll get pretty close by
tonight.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by loic84):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
Comment:
PR https://github.com/django/django/pull/1328
Docs probably need more love but that gives us something to work with.
I had to change the implementation of `get_manager_class()` because it
relied on `__dict__` which doesn't work with inheritance.
I tried using `dir()` and the previous `callable()` test and while it
worked for most purposes it also produced a few false positive; for
example `base_manager_class` was picked up as a callable and made it into
`new_methods`.
In the end I use `inspect.getmembers()` with a `ismethod` predicate and
the result has been very robust.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
For the record, we lost support for the specialized querysets with the
last implementation; I've added a new commit to the PR to address that.
There is also a couple of minor/cosmetic pending questions in the comments
on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: mike@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
A quick skim of the patch didn't reveal anything surprising. I don't see
anything that needs to be addressed before commit. The minor issues
mentioned in the PR reviews seem minor enough to be decided at commit
time.
If somebody can review the docs part that would help a lot. I am not too
good with documentation changes myself.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
Docs look good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Squashed down to a single commit that applies on top of the latest master.
Hopefully this is RFC.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Rebased to remove the new proxy method introduced by #20429.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Anyone would volunteer for a merge? I rebase the branch to master daily
but it doesn't seem merging is on anyone's todo list.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I'll be back to work next week. I will try to merge this one then unless
somebody beats me to it.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"31fadc120213284da76801cc7bc56e9f32d7281b"]:
{{{
#!CommitTicketReference repository=""
revision="31fadc120213284da76801cc7bc56e9f32d7281b"
Fixed #20625 -- Chainable Manager/QuerySet methods.
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"36f260341a1720d7a38e482234c84ff8e73eab85"]:
{{{
#!CommitTicketReference repository=""
revision="36f260341a1720d7a38e482234c84ff8e73eab85"
Extended the release notes for chainable Manager/QuerySet methods.
Refs #20625.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:37>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: QuerySet, | Needs documentation: 0
models.Manager, chainable | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"c9a53035d61588bc7c76eb38db74b774d64b5c1c"]:
{{{
#!CommitTicketReference repository=""
revision="c9a53035d61588bc7c76eb38db74b774d64b5c1c"
Generate GeoManager from GeoQuerySet. Refs #20625.
This cleanup lays the groundwork for #23533 and also addresses
the issue that GeoManager.get_queryset() failed to pass the
database router hints to the QuerySet constructor.
Thanks Anssi Kääriäinen for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:38>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: QuerySet, | Triage Stage: Accepted
models.Manager, chainable |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by acoady):
+* The custom ``QuerySet`` and its custom methods were lost after the
first
+ call to ``values()`` or ``values_list()``.
This isn't fully fixed. Now the custom `QuerySet` and its custom methods
are lost after the ''second'' call to `values()` for `values_list()`.
This affects 1.8, but not master, as #24211 removed the ValuesQuerySets
anyway.
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:39>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: QuerySet, | Triage Stage: Accepted
models.Manager, chainable |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"7d471fe6626de2b36b97ac04086d8ea307be8288" 7d471fe]:
{{{
#!CommitTicketReference repository=""
revision="7d471fe6626de2b36b97ac04086d8ea307be8288"
[1.8.x] Refs #20625 -- Fixed custom queryset chaining with values() and
values_list().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:40>
Django
-------------------------------------+-------------------------------------
Reporter: danols | Owner: loic84
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: QuerySet, | Triage Stage: Accepted
models.Manager, chainable |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"f33b3ebd5302fe5821dd796156b0be66c5aebec9" f33b3ebd]:
{{{
#!CommitTicketReference repository=""
revision="f33b3ebd5302fe5821dd796156b0be66c5aebec9"
Refs #20625 -- Forwardported tests and release notes for queryset chaining
fix.
The issue was fixed on master due to the removal of ValuesQuerySet.
Partial forwardport of 7d471fe6626de2b36b97ac04086d8ea307be8288 from
stable/1.8.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20625#comment:41>