Resolve all queryset methods on managers as attributes

[ljodal]

This changes the logic for resolving queryset methods on managers from copying the methods over to adding them as attributes and then resolving the type of those attributes in the get_attribute_hook callback method, similar to how it is done for custom manager methods.

This might have introduced some new errors, but I'm not sure. The test suite passes, but I'm seeing some new errors in our project. Those might be from changes made since 1.11 that we're currently using though.

Related issues

• Closes: Alias argument to QuerySet.using() typed as _SpecialForm #958
• Closes: error: Name "Sequence" is not defined #1022

[flaeppe]

Perhaps a micro-optimisation and out of scope. But manager_method_names could be a set, right?

manager_method_names = set()
for manager_mro_info in new_manager_info.mro:
    if manager_mro_info.fullname == fullnames.BASE_MANAGER_CLASS_FULLNAME:
        for name in manager_mro_info.names:
             manager_method_names.add(name)
}

One could also make a comprehension out of it. Though I'm not sure if that's more, or less, readable..

manager_method_names = {
    *manager_mro_info.names
    for manager_mro_info in new_manager_info.mro
    if manager_mro_info.fullname == fullnames.BASE_MANAGER_CLASS_FULLNAME
}

[ljodal]

Haha, I just did basically the same, but decided that it was less readable, so I changed it back.

I'm wondering if it would be better to have a simple predefined list of methods here though. It's fairly easy to define that list and it might be a better approach than trying to detect which methods return querysets. What do you think of that? Then we can use that same list to update the return types in resolve_manager_method and avoid potentially affecting custom queryset methods

[flaeppe]

What do you think of that?

I'd agree with that (I've had similar thoughts too). It's more difficult to understand what that "_QS" is there for, in addition to this list..

[flaeppe]

I mean, as long as there's a/some test cases testing that whole list. That should suffice for coverage and future changes.

I added a case close to that some time ago

django-stubs/tests/typecheck/managers/querysets/test_from_queryset.yml

Lines 350 to 386 in fe2d228

	- case: from_queryset_includes_methods_returning_queryset
	main: |
	from myapp.models import MyModel
	reveal_type(MyModel.objects.none) # N: Revealed type is "def () -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.all) # N: Revealed type is "def () -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.filter) # N: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.exclude) # N: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.complex_filter) # N: Revealed type is "def (filter_obj: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.union) # N: Revealed type is "def (*other_qs: Any, *, all: builtins.bool =) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.intersection) # N: Revealed type is "def (*other_qs: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.difference) # N: Revealed type is "def (*other_qs: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.select_for_update) # N: Revealed type is "def (nowait: builtins.bool =, skip_locked: builtins.bool =, of: typing.Sequence[builtins.str] =, no_key: builtins.bool =) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.select_related) # N: Revealed type is "def (*fields: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.prefetch_related) # N: Revealed type is "def (*lookups: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.annotate) # N: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.alias) # N: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.order_by) # N: Revealed type is "def (*field_names: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.distinct) # N: Revealed type is "def (*field_names: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.reverse) # N: Revealed type is "def () -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.defer) # N: Revealed type is "def (*fields: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.only) # N: Revealed type is "def (*fields: Any) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	reveal_type(MyModel.objects.using) # N: Revealed type is "def (alias: Union[builtins.str, None]) -> myapp.models.MyQuerySet[myapp.models.MyModel]"
	installed_apps:
	- myapp
	files:
	- path: myapp/__init__.py
	- path: myapp/models.py
	content: |
	from django.db import models
	from django.db.models.manager import BaseManager
	class MyQuerySet(models.QuerySet["MyModel"]):
	...
	MyManager = BaseManager.from_queryset(MyQuerySet)
	class MyModel(models.Model):
	objects = MyManager()

Asserting on all django manager builtins.

[ljodal]

I decided to implement this and think the code is much more readable and understandable now. It's just 20 methods that we need to modify. I updated (and sorted) the from_queryset_includes_methods_returning_queryset test as well, it was missing .extra() :)

[flaeppe]

Agreed, it looks way way better. Django's queryset API, thus also stubs, won't change that often that there's a need to figure them out programmatically..

[flaeppe]

The test suite passes, but I'm seeing some new errors in our project.

For some additional reference, I just tried running this on one of our projects without issues.

[sobolevn]

Thank you! Looks promising

[sobolevn]

Hm, good idea. Let's try this. You can create a new PR with just this change. It will be helpful!

[ljodal]

Yeah, I removed it as it's no longer needed to reproduce the problem after adding disable_cache: true to the test case. If we're going to add this I think it makes sense to rename the files as well, but I stole this from #1017 so I think I'll leave it up to the author there to decide if he wants to create a PR for that.

[sobolevn]

Let's add a comment why do we need this.
And a link to all the discussions somewhere above this test case.

[ljodal]

Done ✅

[flaeppe]

LGTM 👍

[sobolevn]

Thank you, great work!

[christianbundy]

Thanks a lot! I'm excited to try this when a new version is released.