Treat get_user_model as a dynamic class factory #1730

ljodal · 2023-09-21T22:41:31Z

This treats the get_user_model function as a dynamic class factory in the Django plugin, which means that we can use it for type stubs etc and get the actually configured user model as a type instead of using AbstractBaseUser

Note that I couldn't update the last two modules in django.contrib.auth as that caused some kind of circular reference that mypy couldn't deal with.

Related issues

Closes user_passes_test should take in AUTH_USER_MODEL #1058

This treats the get_user_model function as a dynamic class factory in the Django plugin, which means that we can use it for type stubs etc and get the actually configured user model as a type instead of using AbstractBaseUser Note that I couldn't update the last two modules in django.contrib.auth as that caused some kind of circular reference that mypy couldn't deal with.

ljodal · 2023-09-21T23:10:40Z

django-stubs/contrib/auth/backends.pyi

@@ -7,12 +7,12 @@ from django.db.models.base import Model
 from django.http.request import HttpRequest
 from typing_extensions import TypeAlias

-UserModel = get_user_model()


I get stubtest errors if I leave this as it. Not entirely sure why. See the CI failures here: https://github.com/typeddjango/django-stubs/actions/runs/6267951533/job/17022010362

This type does exist at runtime though, but I'm not sure if we should expose it or not? Pretty sure it's not intended to be public API 🤔

If it exist at runtime we can start out by exposing it, regardless of public API or not I'd say

It exists at runtime, but the types seem to be different, so subtest complains. Is there a way to ignore things from stubtest? Is that what the allowlist is for?

Yes precisely. Update the allowlist, that's what it's for

But before we do, can you inspect what the runtime type is?

This is what subtest says at least:

error: django.contrib.auth.views.UserModel.username variable differs from runtime type django.db.models.query_utils.DeferredAttribute Stub: in file /home/runner/work/django-stubs/django-stubs/django-stubs/contrib/auth/views.pyi:76 django.db.models.fields.CharField[Union[builtins.str, builtins.int, django.db.models.expressions.Combinable], builtins.str] Runtime: <django.db.models.query_utils.DeferredAttribute object at 0x7f3b7d36c850>

Seems to be the same for all the attributes

Aha, interesting, that's a descriptor: https://github.com/django/django/blob/4de31ec680df062e5964b630f1b881ead5004e15/django/db/models/query_utils.py#L178

Should be fine to type as "what it really is"

ljodal · 2023-09-21T23:12:25Z

django-stubs/contrib/auth/views.pyi

@@ -7,7 +7,7 @@ from django.http.response import HttpResponse, HttpResponseRedirect
 from django.views.generic.base import TemplateView
 from django.views.generic.edit import FormView

-UserModel = get_user_model()


Same as the comment above, this exists at runtime and causes issues with stubtest

flaeppe

Nice work! Had a couple of thoughts

flaeppe · 2023-09-22T04:06:25Z

django-stubs/contrib/auth/backends.pyi

-_AnyUser: TypeAlias = AbstractBaseUser | AnonymousUser
-
-UserModel: Any
+_UserModel = get_user_model()


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 of get_user_model

Like this:

from django.contrib.auth import _UserModel as _UserModel

_UserModel does not exist at runtime, but in some modules UserModel 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 attributes

Yes, I also think we should import from one place it and not define for each module.

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 sure

I haven't run the whole suite just yet, but I tried this diff:

# for `get_user_model()` - 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) - ): + if file.fullname == "django.contrib.auth" and self.django_context.settings: auth_user_model_name = self.django_context.settings.AUTH_USER_MODEL ...

Together with these changes:

-from django.contrib.auth import get_user_model +from django.contrib.auth import _UserModel ... -_UserModel = get_user_model()

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 via get_user_model by itself.

- case: test_user_model_resolved_via_get_user_model main: | from typing import Union from django.contrib.auth import get_user_model from django.contrib.auth.models import AnonymousUser from django.http import HttpRequest, HttpResponse from django.contrib.auth.decorators import user_passes_test User = get_user_model() reveal_type(User) def check(user: Union[User, AnonymousUser]) -> bool: return True @user_passes_test(check) def view(request: HttpRequest) -> HttpResponse: reveal_type(request.user) return HttpResponse() out: | main:7: note: Revealed type is "def (*args: Any, **kwargs: Any) -> myapp.models.MyUser" main:11: note: Revealed type is "Union[myapp.models.MyUser, django.contrib.auth.models.AnonymousUser]" custom_settings: | INSTALLED_APPS = ("django.contrib.contenttypes", "django.contrib.auth", "myapp") AUTH_USER_MODEL = "myapp.MyUser" files: - path: myapp/__init__.py - path: myapp/models.py content: | from django.contrib.auth.models import AbstractUser, PermissionsMixin class MyUser(AbstractUser, PermissionsMixin): ...

I get both your test and the one above passing with these changes

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?

Can you try this test:

pytest tests/typecheck/test_shortcuts.yml::get_user_model_returns_proper_class

It's one of the ones triggering the cyclic dependencies problem here. If fails even if I remove all references to auth in requests.pyi

Hm, I don't hit any errors when trying it

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 🤔

flaeppe · 2023-09-22T04:13:14Z

mypy_django_plugin/main.py

+        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)
+        ):


Isn't this a regular import dependency, on django.contrib.auth? Which mypy should establish by itself

At least I was thinking this hook is for "invisible" dependencies

This adds a dependency on the configured user model, which might not be in django.contrib.auth. I guess one option we could do is to add the user model as a dependency of django.contrib.auth, that might be cleaner?

[...] I guess one option we could do is to add the user model as a dependency of django.contrib.auth, that might be cleaner?

Yes, that reads exactly how I imagine this works

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 django.contrib.auth is not registered as installed in the tests for some reason, so the dependency hook doesn't run

flaeppe · 2023-09-22T04:16:48Z

mypy_django_plugin/main.py

+        if fullname == fullnames.GET_USER_MODEL_FULLNAME:
+            return partial(settings.transform_get_user_model_hook, django_context=self.django_context)


Can we add branching on a helper that checks if django.contrib.auth is installed?

Suggested change

if fullname == fullnames.GET_USER_MODEL_FULLNAME:

return partial(settings.transform_get_user_model_hook, django_context=self.django_context)

if fullname == fullnames.GET_USER_MODEL_FULLNAME and self.django_context.is_contrib_auth_installed:

return partial(settings.transform_get_user_model_hook, django_context=self.django_context)

That would help in the dependency hook as well

I was intending to fix this in a separate PR. I think it's better to change the return type to NoReturn when auth is not configured. That's give a somewhat sensible error message, rather than Variable "User" is not valid as a type

Alright, that's totally fair, we can definitely do it later on

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

flaeppe · 2023-09-22T04:23:03Z

mypy_django_plugin/transformers/settings.py

+    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


If we have my lazy reference resolver in #1719 we wouldn't need to use the runtime model here:

Suggested change

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

auth_user_model = django_context.settings.AUTH_USER_MODEL

model_cls_fullname = helpers.resolve_lazy_reference(auth_user_model, ...)

if model_cls_fullname is None:

model_cls_fullname = fullnames.ABSTRACT_BASE_USER_MODEL_FULLNAME

We might want to use the fallback only on last iteration then

flaeppe · 2023-09-22T04:27:21Z

django-stubs/contrib/auth/backends.pyi

@@ -7,12 +7,12 @@ from django.db.models.base import Model
 from django.http.request import HttpRequest
 from typing_extensions import TypeAlias

-UserModel = get_user_model()


If it exist at runtime we can start out by exposing it, regardless of public API or not I'd say

flaeppe · 2023-09-22T06:05:52Z

tests/typecheck/contrib/auth/test_decorators.yml

@@ -41,6 +41,28 @@
        from django.contrib.auth.decorators import user_passes_test
        @user_passes_test  # E: Argument 1 to "user_passes_test" has incompatible type "Callable[[HttpRequest], HttpResponse]"; expected "Callable[[Union[AbstractBaseUser, AnonymousUser]], bool]"
        def view_func(request: HttpRequest) -> HttpResponse: ...
+-   case: user_passes_test_has_user_of_type_auth_user_model
+    disable_cache: true


Why would we need to disable cache?

Oops, this is just a copy-past mistake 😇

flaeppe · 2023-09-22T06:16:10Z

tests/typecheck/contrib/auth/test_decorators.yml

+        from typing import Union
+        from django.http import HttpRequest, HttpResponse
+        from django.contrib.auth.models import AnonymousUser
+        from django.contrib.auth.decorators import user_passes_test
+        from myapp.models import MyUser
+
+        def check_user(user: Union[MyUser, AnonymousUser]) -> bool: return True
+        @user_passes_test(check_user)
+        def view_func(request: HttpRequest) -> HttpResponse: ...


Can we check a couple of the often used and now plugin changed signatures from django.contrib.auth, e.g. authenticate and login

Suggested change

from typing import Union

from django.http import HttpRequest, HttpResponse

from django.contrib.auth.models import AnonymousUser

from django.contrib.auth.decorators import user_passes_test

from myapp.models import MyUser

def check_user(user: Union[MyUser, AnonymousUser]) -> bool: return True

@user_passes_test(check_user)

def view_func(request: HttpRequest) -> HttpResponse: ...

from typing import Union

from django.http import HttpRequest, HttpResponse

from django.contrib.auth import authenticate, login, get_user

from django.contrib.auth.models import AnonymousUser

from django.contrib.auth.decorators import user_passes_test

from myapp.models import MyUser

reveal_type(authenticate) # N: Revealed type is "def (...) -> myapp.models.MyUser | None"

reveal_type(login) # N: Revealed type is "..."

reveal_type(get_user) # N: Revealed type is "..."

def check_user(user: Union[MyUser, AnonymousUser]) -> bool: return True

@user_passes_test(check_user)

def view_func(request: HttpRequest) -> HttpResponse: ...

We could perhaps change the name of the test to something like test_auth_user_model in that case? Also, its "sibling" test is in models/test_contrib_models.yml but I'm totally fine to keep this test under contrib/auth/

flaeppe · 2023-09-22T06:17:22Z

mypy_django_plugin/transformers/settings.py

+    model_info: Union[TypeInfo, PlaceholderNode, None]
+    model_info = helpers.lookup_fully_qualified_typeinfo(helpers.get_semanal_api(ctx), model_cls_fullname)


Suggested change

model_info: Union[TypeInfo, PlaceholderNode, None]

model_info = helpers.lookup_fully_qualified_typeinfo(helpers.get_semanal_api(ctx), model_cls_fullname)

model_info: Union[TypeInfo, PlaceholderNode, None] = helpers.lookup_fully_qualified_typeinfo(helpers.get_semanal_api(ctx), model_cls_fullname)

ljodal · 2023-09-25T15:54:31Z

I'm not entirely sure why, but I seem to be threading a very thin line between this working and getting infinite recursion due to deferring. Not entirely sure what, if anything, I can do about that though @flaeppe @sobolevn?

flaeppe · 2023-09-25T18:41:46Z

I'm not entirely sure why, but I seem to be threading a very thin line between this working and getting infinite recursion due to deferring. Not entirely sure what, if anything, I can do about that though @flaeppe @sobolevn?

No stress, but could you try out what I mentioned here: #1730 (comment) whenever you get the chance?

ljodal · 2023-09-25T22:19:31Z

Hmm, damnit, this ends up with INTERNAL ERROR: maximum semantic analysis iteration count reached when running it on our codebase. So even though all is green, it doesn't seem like it actually works in a real-life project. Not sure what triggers it, but it does seem to be caused by adding the user model app as a dependency of django.contrib.auth, not any of the other changes

flaeppe · 2023-09-25T22:25:47Z

Hmm, damnit, this ends up with INTERNAL ERROR: maximum semantic analysis iteration count reached when running it on our codebase. So even though all is green, it doesn't seem like it actually works in a real-life project. Not sure what triggers it, but it does seem to be caused by adding the user model app as a dependency of django.contrib.auth, not any of the other changes

Could it be the introduced default dependency:

auth_user_module = "django.contrib.auth.models"

Can't we skip adding a dependency and try to fallback to Any if auth isn't installed? The runtime raises anyways:

https://github.com/django/django/blob/main/django/contrib/auth/__init__.py#L183-L197

ljodal · 2023-09-25T22:44:47Z

It's not that, it seems to be something in the transform_get_user_model_hook callback. If I replace that entire function with just return it works fine. I can't for the life of me figure out what is going wrong though

ljodal · 2023-09-25T23:17:56Z

Im really confused here. It seems like the issue is a forms file where we do something along these lines:

from django.contrib.auth import get_user_model

User = get_user_model()

class MyForm(forms.ModelForm):
    class Meta:
        model = User

It works fine if I replace get_user_model with a direct import. But it's also not the hook directly triggering the deferring. It's called many times and resolves the user model TypeInfo just fine every time 🤔

flaeppe · 2023-09-26T04:28:13Z

Hm, is it the PlaceholderNode returned there?

Does it stop deferring if you replace that? I would guess it might stop quickly since there's nothing else telling it to defer

ljodal · 2023-09-26T07:26:59Z

No, I've tried to remove the placeholder node, and that made no difference. I put in a defer call when we cannot resolve the correct type, but when I get the problem it seems like it resolves the correct type long before it crashes 🤔

flaeppe · 2023-09-26T07:29:27Z

Hm, perhaps we could start out with being a bit more defensive and avoid inserting a node every time?

Only do it if we need to

ljodal · 2023-09-26T07:51:50Z

I've tried that too! What I'm currently testing is master + this hook:

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:
        return

    node: Union[TypeInfo, PlaceholderNode, Var, None]
    node = helpers.lookup_fully_qualified_typeinfo(helpers.get_semanal_api(ctx), model_cls_fullname)
    if node is None:
        if not ctx.api.final_iteration:
            # Temporarily replace the node with a PlaceholderNode. This suppresses
            # 'Variable "..." is not valid as a type' errors from mypy
            ctx.api.defer()

        return

    module = ctx.api.modules[ctx.api.cur_mod_id]
    if node == module.names.get(ctx.name):
        return

    ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, node))

Edit: It seems like the problem is triggered as soon as I insert the symbol table node, even if I never create a placeholder nor defer. It also works if I defer, but don't insert the symbol table node. Really don't understand what's going on here to be honest.

flaeppe · 2023-09-28T19:51:32Z

Just realised one thing we've missed here. Does passing plugin_generated=True change anything?

-    ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, node))
+    ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, node, plugin_generated=True))

ljodal · 2023-09-29T07:51:11Z

That did not seem to make any difference. It works on our real-life project if I only insert the symbol table node on the final iteration, but then some tests start failing. I've been meaning to open a mypy issue, but I haven't gotten around to it yet

flaeppe · 2023-09-29T07:58:03Z

Gotcha, I've seen issues as well in some sort of close by area in #1699, so I agree with what you said earlier; that we're probably threading a thin line here..

ljodal and others added 2 commits September 22, 2023 00:02

[pre-commit.ci] auto fixes from pre-commit.com hooks

6d493d8

ljodal mentioned this pull request Sep 21, 2023

Transform user_passes_test signature #1229

Closed

ljodal added 4 commits September 22, 2023 00:47

Fix CI failures

990fb64

Fix test case for old python versions

05c09f5

Fix some of the stubtest failures

42c8eee

Maybe fix the remaining stubtest errors?

f5ed69d

ljodal commented Sep 21, 2023

View reviewed changes

flaeppe reviewed Sep 22, 2023

View reviewed changes

Change dependencies and fall back to a variable

10d3528

ljodal added 2 commits September 25, 2023 23:42

Handle django.contrib.auth not installed

8f72a8b

Reference django.contrib.auth._UserModel

fdea347

flaeppe mentioned this pull request Aug 10, 2024

Resolve configured AUTH_USER_MODEL with a get_type_analyze_hook #2335

Merged

flaeppe closed this in #2335 Aug 12, 2024

		if fullname == fullnames.GET_USER_MODEL_FULLNAME:
		return partial(settings.transform_get_user_model_hook, django_context=self.django_context)

		model_info: Union[TypeInfo, PlaceholderNode, None]
		model_info = helpers.lookup_fully_qualified_typeinfo(helpers.get_semanal_api(ctx), model_cls_fullname)

Treat get_user_model as a dynamic class factory #1730

Treat get_user_model as a dynamic class factory #1730

Conversation

ljodal commented Sep 21, 2023

Related issues

ljodal Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

ljodal Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljodal commented Sep 25, 2023

flaeppe commented Sep 25, 2023

ljodal commented Sep 25, 2023

flaeppe commented Sep 25, 2023

ljodal commented Sep 25, 2023

ljodal commented Sep 25, 2023

flaeppe commented Sep 26, 2023

ljodal commented Sep 26, 2023

flaeppe commented Sep 26, 2023

ljodal commented Sep 26, 2023 • edited Loading

flaeppe commented Sep 28, 2023

ljodal commented Sep 29, 2023

flaeppe commented Sep 29, 2023

ljodal Sep 21, 2023 •

edited

Loading

flaeppe Sep 22, 2023 •

edited

Loading

ljodal Sep 25, 2023 •

edited

Loading

ljodal commented Sep 26, 2023 •

edited

Loading