-
-
Notifications
You must be signed in to change notification settings - Fork 499
Treat get_user_model as a dynamic class factory #1730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
e9529fa
6d493d8
990fb64
05c09f5
42c8eee
f5ed69d
10d3528
8f72a8b
fdea347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,7 +4,7 @@ | |||||||||
from typing import Any, Callable, Dict, List, Optional, Tuple, Type | ||||||||||
|
||||||||||
from mypy.modulefinder import mypy_path | ||||||||||
from mypy.nodes import MypyFile, TypeInfo | ||||||||||
from mypy.nodes import ImportFrom, MypyFile, TypeInfo | ||||||||||
from mypy.options import Options | ||||||||||
from mypy.plugin import ( | ||||||||||
AnalyzeTypeContext, | ||||||||||
|
@@ -23,7 +23,7 @@ | |||||||||
from mypy_django_plugin.django.context import DjangoContext | ||||||||||
from mypy_django_plugin.exceptions import UnregisteredModelError | ||||||||||
from mypy_django_plugin.lib import fullnames, helpers | ||||||||||
from mypy_django_plugin.transformers import fields, forms, init_create, meta, querysets, request, settings | ||||||||||
from mypy_django_plugin.transformers import fields, forms, init_create, meta, querysets, settings | ||||||||||
from mypy_django_plugin.transformers.functional import resolve_str_promise_attribute | ||||||||||
from mypy_django_plugin.transformers.managers import ( | ||||||||||
create_new_manager_class_from_as_manager_method, | ||||||||||
|
@@ -111,15 +111,18 @@ def get_additional_deps(self, file: MypyFile) -> List[Tuple[int, str, int]]: | |||||||||
return [self._new_dependency("typing"), self._new_dependency("django_stubs_ext")] | ||||||||||
|
||||||||||
# for `get_user_model()` | ||||||||||
if self.django_context.settings: | ||||||||||
if file.fullname == "django.contrib.auth" or file.fullname in {"django.http", "django.http.request"}: | ||||||||||
auth_user_model_name = self.django_context.settings.AUTH_USER_MODEL | ||||||||||
try: | ||||||||||
auth_user_module = self.django_context.apps_registry.get_model(auth_user_model_name).__module__ | ||||||||||
except LookupError: | ||||||||||
# get_user_model() model app is not installed | ||||||||||
return [] | ||||||||||
return [self._new_dependency(auth_user_module), self._new_dependency("django_stubs_ext")] | ||||||||||
if self.django_context.settings and any( | ||||||||||
i.id == "django.contrib.auth" and any(name == "get_user_model" for name, _ in i.names) | ||||||||||
for i in file.imports | ||||||||||
if isinstance(i, ImportFrom) | ||||||||||
): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this a regular import dependency, on At least I was thinking this hook is for "invisible" dependencies There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds a dependency on the configured user model, which might not be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that reads exactly how I imagine this works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason, doing it like that causes the same issue as above where mypy gets stuck in an infinite loop Edit: I see now that my issue is that |
||||||||||
auth_user_model_name = self.django_context.settings.AUTH_USER_MODEL | ||||||||||
try: | ||||||||||
auth_user_module = self.django_context.apps_registry.get_model(auth_user_model_name).__module__ | ||||||||||
except LookupError: | ||||||||||
# get_user_model() model app is not installed | ||||||||||
return [] | ||||||||||
return [self._new_dependency(auth_user_module), self._new_dependency("django_stubs_ext")] | ||||||||||
|
||||||||||
# ensure that all mentioned to='someapp.SomeModel' are loaded with corresponding related Fields | ||||||||||
defined_model_classes = self.django_context.model_modules.get(file.fullname) | ||||||||||
|
@@ -273,10 +276,6 @@ def get_attribute_hook(self, fullname: str) -> Optional[Callable[[AttributeConte | |||||||||
if info and info.has_base(fullnames.PERMISSION_MIXIN_CLASS_FULLNAME) and attr_name == "is_superuser": | ||||||||||
return partial(set_auth_user_model_boolean_fields, django_context=self.django_context) | ||||||||||
|
||||||||||
# Lookup of the 'request.user' attribute | ||||||||||
if info and info.has_base(fullnames.HTTPREQUEST_CLASS_FULLNAME) and attr_name == "user": | ||||||||||
return partial(request.set_auth_user_model_as_type_for_request_user, django_context=self.django_context) | ||||||||||
|
||||||||||
# Lookup of the 'user.is_staff' or 'user.is_active' attribute | ||||||||||
if info and info.has_base(fullnames.ABSTRACT_USER_MODEL_FULLNAME) and attr_name in ("is_staff", "is_active"): | ||||||||||
return partial(set_auth_user_model_boolean_fields, django_context=self.django_context) | ||||||||||
|
@@ -306,6 +305,9 @@ def get_type_analyze_hook(self, fullname: str) -> Optional[Callable[[AnalyzeType | |||||||||
return None | ||||||||||
|
||||||||||
def get_dynamic_class_hook(self, fullname: str) -> Optional[Callable[[DynamicClassDefContext], None]]: | ||||||||||
if fullname == fullnames.GET_USER_MODEL_FULLNAME: | ||||||||||
return partial(settings.transform_get_user_model_hook, django_context=self.django_context) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add branching on a helper that checks if
Suggested change
That would help in the dependency hook as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was intending to fix this in a separate PR. I think it's better to change the return type to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, that's totally fair, we can definitely do it later on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Doing this would also assist in resolving #1380. We could include a test to resolve, looking like below, which currently crashes the plugin: - case: test_xyz
main: |
from django.contrib.auth import get_user_model
User = get_user_model()
installed_apps:
- myapp
files:
- path: myapp/__init__.py
- path: myapp/models.py |
||||||||||
|
||||||||||
# Create a new manager class definition when a manager's '.from_queryset' classmethod is called | ||||||||||
class_name, _, method_name = fullname.rpartition(".") | ||||||||||
if method_name == "from_queryset": | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,17 +1,24 @@ | ||||||||||||||||||||||
from mypy.nodes import MemberExpr | ||||||||||||||||||||||
from mypy.plugin import AttributeContext, FunctionContext | ||||||||||||||||||||||
from typing import Union | ||||||||||||||||||||||
|
||||||||||||||||||||||
from mypy.nodes import GDEF, MemberExpr, PlaceholderNode, SymbolTableNode, TypeInfo | ||||||||||||||||||||||
from mypy.plugin import AttributeContext, DynamicClassDefContext, FunctionContext | ||||||||||||||||||||||
from mypy.types import AnyType, Instance, TypeOfAny, TypeType | ||||||||||||||||||||||
from mypy.types import Type as MypyType | ||||||||||||||||||||||
|
||||||||||||||||||||||
from mypy_django_plugin.config import DjangoPluginConfig | ||||||||||||||||||||||
from mypy_django_plugin.django.context import DjangoContext | ||||||||||||||||||||||
from mypy_django_plugin.lib import helpers | ||||||||||||||||||||||
from mypy_django_plugin.lib import fullnames, helpers | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
def get_user_model_hook(ctx: FunctionContext, django_context: DjangoContext) -> MypyType: | ||||||||||||||||||||||
auth_user_model = django_context.settings.AUTH_USER_MODEL | ||||||||||||||||||||||
model_cls = django_context.apps_registry.get_model(auth_user_model) | ||||||||||||||||||||||
model_cls_fullname = helpers.get_class_fullname(model_cls) | ||||||||||||||||||||||
|
||||||||||||||||||||||
model_info = None | ||||||||||||||||||||||
try: | ||||||||||||||||||||||
model_cls = django_context.apps_registry.get_model(auth_user_model) | ||||||||||||||||||||||
model_cls_fullname = helpers.get_class_fullname(model_cls) | ||||||||||||||||||||||
except LookupError: | ||||||||||||||||||||||
model_cls_fullname = fullnames.ABSTRACT_BASE_USER_MODEL_FULLNAME | ||||||||||||||||||||||
|
||||||||||||||||||||||
model_info = helpers.lookup_fully_qualified_typeinfo(helpers.get_typechecker_api(ctx), model_cls_fullname) | ||||||||||||||||||||||
if model_info is None: | ||||||||||||||||||||||
|
@@ -20,6 +27,28 @@ def get_user_model_hook(ctx: FunctionContext, django_context: DjangoContext) -> | |||||||||||||||||||||
return TypeType(Instance(model_info, [])) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
def transform_get_user_model_hook(ctx: DynamicClassDefContext, django_context: DjangoContext) -> None: | ||||||||||||||||||||||
auth_user_model = django_context.settings.AUTH_USER_MODEL | ||||||||||||||||||||||
try: | ||||||||||||||||||||||
model_cls = django_context.apps_registry.get_model(auth_user_model) | ||||||||||||||||||||||
model_cls_fullname = helpers.get_class_fullname(model_cls) | ||||||||||||||||||||||
except LookupError: | ||||||||||||||||||||||
model_cls_fullname = fullnames.ABSTRACT_BASE_USER_MODEL_FULLNAME | ||||||||||||||||||||||
Comment on lines
+32
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have my lazy reference resolver in #1719 we wouldn't need to use the runtime model here:
Suggested change
We might want to use the fallback only on last iteration then |
||||||||||||||||||||||
|
||||||||||||||||||||||
model_info: Union[TypeInfo, PlaceholderNode, None] | ||||||||||||||||||||||
model_info = helpers.lookup_fully_qualified_typeinfo(helpers.get_semanal_api(ctx), model_cls_fullname) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
if model_info is None: | ||||||||||||||||||||||
if not ctx.api.final_iteration: | ||||||||||||||||||||||
ctx.api.defer() | ||||||||||||||||||||||
# Temporarily replace the node with a PlaceholderNode. This suppresses | ||||||||||||||||||||||
# 'Variable "..." is not valid as a type' errors from mypy | ||||||||||||||||||||||
model_info = PlaceholderNode(model_cls_fullname, ctx.call, ctx.call.line) | ||||||||||||||||||||||
else: | ||||||||||||||||||||||
return | ||||||||||||||||||||||
|
||||||||||||||||||||||
ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, model_info)) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
def get_type_of_settings_attribute( | ||||||||||||||||||||||
ctx: AttributeContext, django_context: DjangoContext, plugin_config: DjangoPluginConfig | ||||||||||||||||||||||
) -> MypyType: | ||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we declare
_UserModel
in 1 file and import it in the others? Or does_UserModel
exist runtime?Then we should probably keep it in
contrib.auth
next to definition ofget_user_model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_UserModel
does not exist at runtime, but in some modulesUserModel
does exist, for example in django/contrib/auth/backens.py. The subtest complains about these though, saying the typing doesn't match runtime for some attributesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also think we should import from one place it and not define for each module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this approach a go now, but for some reason it ends up causing an endless defer look, which crashes mypy. Can we leave it like this for now and maybe revisit? It does kinda seem like a bug in mypy, cause we never get a call with
final_iteration=True
before it gives up, but I'm not entirely sureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run the whole suite just yet, but I tried this diff:
Together with these changes:
And an additional test case that depends on
django.contrib.auth
to declare a custom user, while also testing that the custom user is resolved viaget_user_model
by itself.I get both your test and the one above passing with these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen things crash a bit randomly, but I'll try without the requests one. I though mypy was supposed to handle cyclic references though, but maybe not at this level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try this test:
It's one of the ones triggering the cyclic dependencies problem here. If fails even if I remove all references to
auth
inrequests.pyi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't hit any errors when trying it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, so my issue is that I'm checking if
django.contrib.auth
is installed, and if it's not I don't add the dependencies. Guess I'll have to properly figure out what do when that isn't installed. I believe it'll raise an error at runtime 🤔