Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Treat get_user_model as a dynamic class factory #1730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e9529fa
Treat get_user_model as a dynamic class factory
ljodal Sep 21, 2023
6d493d8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 21, 2023
990fb64
Fix CI failures
ljodal Sep 21, 2023
05c09f5
Fix test case for old python versions
ljodal Sep 21, 2023
42c8eee
Fix some of the stubtest failures
ljodal Sep 21, 2023
f5ed69d
Maybe fix the remaining stubtest errors?
ljodal Sep 21, 2023
10d3528
Change dependencies and fall back to a variable
ljodal Sep 25, 2023
8f72a8b
Handle django.contrib.auth not installed
ljodal Sep 25, 2023
fdea347
Reference django.contrib.auth._UserModel
ljodal Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions django-stubs/contrib/auth/backends.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor Author

@ljodal ljodal Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

_AnyUser: TypeAlias = UserModel | AnonymousUser
_UserModel = get_user_model()
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this:

from django.contrib.auth import _UserModel as _UserModel

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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 sure

Copy link
Member

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:

         # 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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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:

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

Copy link
Member

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

Copy link
Contributor Author

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 🤔

_AnyUser: TypeAlias = _UserModel | AnonymousUser

class BaseBackend:
def authenticate(self, request: HttpRequest | None, **kwargs: Any) -> UserModel | None: ...
def get_user(self, user_id: int) -> UserModel | None: ...
def authenticate(self, request: HttpRequest | None, **kwargs: Any) -> _UserModel | None: ...
def get_user(self, user_id: int) -> _UserModel | None: ...
def get_user_permissions(self, user_obj: _AnyUser, obj: Model | None = ...) -> set[str]: ...
def get_group_permissions(self, user_obj: _AnyUser, obj: Model | None = ...) -> set[str]: ...
def get_all_permissions(self, user_obj: _AnyUser, obj: Model | None = ...) -> set[str]: ...
Expand All @@ -21,7 +21,7 @@ class BaseBackend:
class ModelBackend(BaseBackend):
def authenticate(
self, request: HttpRequest | None, username: str | None = ..., password: str | None = ..., **kwargs: Any
) -> UserModel | None: ...
) -> _UserModel | None: ...
def has_module_perms(self, user_obj: _AnyUser, app_label: str) -> bool: ...
def user_can_authenticate(self, user: _AnyUser | None) -> bool: ...
def with_perm(
Expand All @@ -30,11 +30,11 @@ class ModelBackend(BaseBackend):
is_active: bool = ...,
include_superusers: bool = ...,
obj: Model | None = ...,
) -> QuerySet[UserModel]: ...
) -> QuerySet[_UserModel]: ...

class AllowAllUsersModelBackend(ModelBackend): ...

_U = TypeVar("_U", bound=UserModel)
_U = TypeVar("_U", bound=_UserModel)

class RemoteUserBackend(ModelBackend):
create_unknown_user: bool
Expand Down
4 changes: 2 additions & 2 deletions django-stubs/contrib/auth/views.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_UserModel = get_user_model()

class RedirectURLMixin:
next_page: str | None
Expand Down Expand Up @@ -65,7 +65,7 @@ class PasswordResetConfirmView(PasswordContextMixin, FormView):
token_generator: Any
validlink: bool
user: Any
def get_user(self, uidb64: str) -> UserModel | None: ...
def get_user(self, uidb64: str) -> _UserModel | None: ...

class PasswordResetCompleteView(PasswordContextMixin, TemplateView):
title: Any
Expand Down
4 changes: 2 additions & 2 deletions django-stubs/http/request.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ from django.urls import ResolverMatch
from django.utils.datastructures import CaseInsensitiveMapping, ImmutableList, MultiValueDict
from typing_extensions import Self, TypeAlias

UserModel = get_user_model()
_UserModel = get_user_model()

RAISE_ERROR: object
host_validation_re: Pattern[str]
Expand Down Expand Up @@ -47,7 +47,7 @@ class HttpRequest(BytesIO):
# django.contrib.admin views:
current_app: str
# django.contrib.auth.middleware.AuthenticationMiddleware:
user: UserModel | AnonymousUser
user: _UserModel | AnonymousUser
# django.middleware.locale.LocaleMiddleware:
LANGUAGE_CODE: str
# django.contrib.sites.middleware.CurrentSiteMiddleware
Expand Down
4 changes: 2 additions & 2 deletions django-stubs/test/client.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ MULTIPART_CONTENT: str
CONTENT_TYPE_RE: Pattern
JSON_CONTENT_TYPE_RE: Pattern

UserModel = get_user_model()
_UserModel = get_user_model()

class RedirectCycleError(Exception):
last_response: HttpResponseBase
Expand Down Expand Up @@ -182,7 +182,7 @@ class ClientMixin:
@property
def session(self) -> SessionBase: ...
def login(self, **credentials: Any) -> bool: ...
def force_login(self, user: UserModel, backend: str | None = ...) -> None: ...
def force_login(self, user: _UserModel, backend: str | None = ...) -> None: ...
def logout(self) -> None: ...

class Client(ClientMixin, _RequestFactory[_MonkeyPatchedWSGIResponse]):
Expand Down