#24863 closed New feature (wontfix)
Make `django.db.models.Manager.from_queryset` copy over properties and not just methods
Reported by: | Ram Rachum | Owned by: | Furkan Karataş |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Loic Bistuer, Marc Tamlyn | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When I define a QuerySet
subclass, I like defining not only methods but properties on it.
The function django.db.models.Manager.from_queryset
was introduced so we could succinctly create a Manager
subclass that gets the same functionality that we defined on our QuerySet
subclass. It copies over all the methods defind in the QuerySet
class to the Manager
class. But it doesn't copy properties, so they remain inaccessible from the Manager
class.
I want django.db.models.Manager.from_queryset
to copy over the properties as well.
Change History (13)
comment:1 by , 9 years ago
Cc: | added |
---|
comment:2 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 9 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
Queryset at the moment has only two properties, both of which make no sense on the manager (ordered
and db
).
I'm tentatively accepting this ticket, but I would like someone to explain a concrete use case.
comment:4 by , 9 years ago
I assume the OP is referring to dynamic properties (e.g. using @property
or @cached_property
- so basically methods but called implicitly), as that's the only use case for this I can imagine. Copying over static attributes doesn't make much sense to me.
I'm not strongly opposed to this, but if the implementation turns out to be complex or difficult, I'd be perfectly happy to close it wontfix and say "just use methods instead."
comment:5 by , 9 years ago
Carl: Yes, that's my intention.
mjtamlyn: Okay, here's a concrete use case. I have a model CreditCard
and a corresponding queryset CreditCardQuerySet
. I want to have a property expired
on CreditCardQuerySet
so I could filter to expired credit cards like CreditCard.objects.expired
. (Assuming there isn't a straightforward query to do that.) I could do it as a method but I always prefer using a property
when there aren't any arguments.
comment:6 by , 9 years ago
I have no idea how we'd be able to only take those properties and no attributes or other properties, except for requiring an attribute on the property. But I think just using a function without any arguments is just fine.
comment:7 by , 9 years ago
I'm not really understanding what the implementation problem is. If you're going over all the members of the QuerySet
class, you can see which are properties and then copy them. So what's the problem?
comment:8 by , 9 years ago
Stylistically, I'd say a property is wrong for that use case, I don't like properties which do round trips to the database as when reading the code it looks like an attribute. That said...
The implementation is straightforwards, we can use https://docs.python.org/3/library/inspect.html#inspect.isdatadescriptor
comment:9 by , 9 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:10 by , 9 years ago
Would this solve #25201? That is, use_for_related_fields
could be set on the QuerySet
and then copied to the manager when using as_manager()
? Would it be better to have something like .as_manager(use_for_related_fields=True)
?
comment:11 by , 17 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:12 by , 17 months ago
Has patch: | set |
---|
PR is opened: https://github.com/django/django/pull/16919
comment:13 by , 17 months ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
Triage Stage: | Accepted → Unreviewed |
I agree with Lily, Marc and Carl weren't convinced either. Using properties in Queryset
to perform queries is confusing, it gets even more puzzling when we realize that some of them (e.g. ordered
, query
) have to be omitted in a potential implementation. IMO, it's not worth the additional complexity.
Loic, what do you think about this?