Opened 3 years ago

Last modified 2 years ago

#24863 new New feature

Make `django.db.models.Manager.from_queryset` copy over properties and not just methods

Reported by: coolRR Owned by:
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Loic Bistuer, Marc Tamlyn Triage Stage: Accepted
Has patch: no 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 (10)

comment:1 Changed 3 years ago by Tim Graham

Cc: Loic Bistuer added

Loic, what do you think about this?

comment:2 Changed 3 years ago by m_sha

Owner: changed from nobody to m_sha
Status: newassigned

comment:3 Changed 3 years ago by Marc Tamlyn

Cc: Marc Tamlyn added
Triage Stage: UnreviewedAccepted

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 Changed 3 years ago by Carl Meyer

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 Changed 3 years ago by coolRR

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 Changed 3 years ago by Markus Holtermann

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 Changed 3 years ago by coolRR

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 Changed 3 years ago by Marc Tamlyn

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 Changed 3 years ago by m_sha

Owner: m_sha deleted
Status: assignednew

comment:10 Changed 2 years ago by Tim Graham

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

Note: See TracTickets for help on using tickets.
Back to Top