Skip to content
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

Resolve configured AUTH_USER_MODEL with a get_type_analyze_hook #2335

Merged
merged 8 commits into from
Aug 12, 2024

Conversation

flaeppe
Copy link
Member

@flaeppe flaeppe commented Aug 10, 2024

It's nothing special really. We declare a TypeAlias in the pyi, thus getting a fullname, somewhere under django.contrib.auth. We then build a type analyze hook for that fullname. And the hook simulates what django.contrib.auth.get_user_model does

The alias is set up to point to AbstractBaseUser, so for a type checker other than mypy nothing should've changed

The implementation idea directly taken from: #1058 (comment). Thanks for the suggestion.

Related issues

@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

Hm, we should do the same thing for aauthenticate

@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

Not sure why this fails. It passes locally for me

@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

Right, I think I figured it out. The test needs to invalidate cache or something, because if django.contrib.auth has already been analyzed by another test we don't end up in get_additional_deps and then myapp.models doesn't end up in TypeChecker.modules and we can't find the configured model.

Comment on lines 328 to 331
extra_data = {}
if ctx.id == "django.contrib.auth":
extra_data["AUTH_USER_MODEL"] = self.django_context.settings.AUTH_USER_MODEL
return self.plugin_config.to_json(extra_data)
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we get "implicit" cache tainting on django.contrib.auth, and thus authenticate and friends if AUTH_USER_MODEL changes.

There might be other modules we want to add some extra config data to. Not sure though

@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

Actually, I want to try out a better idea. A bit bigger but if it works it would be nice. This: #1058 (comment)

It's nothing special really. We declare a `TypeAlias` in the pyi, thus
getting a fullname, somewhere under `django.contrib.auth`. We then build
a hook for that fullname. And the hook simulates what
`django.contrib.auth.get_user_model` does

The alias is set up to point to `AbstractBaseUser`, so for a type
checker other than mypy nothing should've changed
@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

I think I got it working. It should now improve and resolve multiple other stuff as well.

@flaeppe flaeppe changed the title Return configured auth user in django.contrib.auth.authenticate Resolve configured AUTH_USER_MODEL with a get_type_analyze_hook Aug 10, 2024
@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

CC @ljodal, this supersedes #1730. Hoping it'll solve any issues that we had there, almost a year ago

@ljodal
Copy link
Contributor

ljodal commented Aug 10, 2024

CC @ljodal, this supersedes #1730. Hoping it'll solve any issues that we had there, almost a year ago

I'm currently on garden leave from my previous job, but I still have my work laptop and access to the codebase so I can try to give this a spin. It's been a while since I touched that project though, so I'm not sure what state it is in with regards to mypy updates etc. Otherwise I no longer have any large Django projects that I'm working on.

@flaeppe
Copy link
Member Author

flaeppe commented Aug 10, 2024

Thank you, would be awesome if you had the possibility to run against something we've tried previously.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

This is an amazing stuff! Thank you!

@flaeppe flaeppe merged commit 81ecdcf into typeddjango:master Aug 12, 2024
36 checks passed
@flaeppe flaeppe deleted the fix/contrib-auth-authenticate branch August 12, 2024 07:29
@flaeppe flaeppe added the release notes reminder User impact should be explicitly documented in release notes. label Aug 12, 2024
@ljodal
Copy link
Contributor

ljodal commented Aug 16, 2024

Thank you, would be awesome if you had the possibility to run against something we've tried previously.

I just gave it a go and I'm not seeing any crashes at least. There's a ton of new errors though. Randomly looking at them there seems to be a lot of errors resolving return types of querysets. That might be from something else between 5.0.2 and laster master though.

@flaeppe
Copy link
Member Author

flaeppe commented Aug 16, 2024

Cool, thank you for trying it out and reporting back!

@flaeppe flaeppe removed the release notes reminder User impact should be explicitly documented in release notes. label Sep 23, 2024
# This is our "placeholder" type the mypy plugin refines to configured 'AUTH_USER_MODEL'
# wherever it is used as a type. The most recognised example of this is (probably)
# `HttpRequest.user`
_UserModel: TypeAlias = AbstractBaseUser # noqa: PYI047
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just a placeholder, and was chosen to not change things, but should this not be AbstractUser not AbstractBaseUser?

Looking at Django the difference between the two would be:

https://github.com/django/django/blob/39de2e97a06d0317973b280bc159ca6f89fc64e3/django/contrib/auth/models.py#L335-L341

For me I'm seeing that is_superuser is not defined and that's because it's checking against AbstractBaseUser.

Anyways, just asking since I'm assuming you know the hierarchy a little better than I do, and I will continue trying to figure out why my customized plugin might not be resolving the user model.

intellij-monorepo-bot pushed a commit to JetBrains/intellij-community that referenced this pull request Dec 17, 2024
`PyTypeHintProvider` allows providing type hints for resolved elements before the main logic of `PyTypingTypeProvider`.
See an example of `django-stubs`: typeddjango/django-stubs#2335.
`_UserModel` type might be replaced with a custom user model, provided in `settings.py` (`AUTH_USER_MODEL`).

GitOrigin-RevId: 986fb91c800be3ccfbc002c73c673896efec8a1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants