Skip to content

create and re-use TypeAliases and TypeVars for "user" and "any user" #2384

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

Merged
merged 2 commits into from
Apr 17, 2025

Conversation

terencehonles
Copy link
Contributor

@terencehonles terencehonles commented Sep 27, 2024

I have made things!

This change also adds a few user type vars and alises to cover the common use cases of User, User | AnonymousUser, and their TypeVar forms for using in generic contexts.

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.

Generally - looks great! Just one note

@sobolevn sobolevn requested a review from flaeppe September 27, 2024 06:00
Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

I don't think we should change the base class to be AbstractUser. This is a section of what the "Specifying a custom user model" says in the docs:

[...] The easiest way to construct a compliant custom user model is to inherit from AbstractBaseUser. AbstractBaseUser provides the core implementation of a user model, including hashed passwords and tokenized password resets. [...]

https://docs.djangoproject.com/en/5.1/topics/auth/customizing/#specifying-a-custom-user-model

@sobolevn
Copy link
Member

I don't think we should change the base class to be AbstractUser.

As far as I understand, this is not the base class, this is the default type, unless AUTH_USER_MODEL is specified.

@terencehonles
Copy link
Contributor Author

That's interesting that they suggest that... The issue is that AbstractBaseUser does not include the PermissionsMixin and if we shouldn't be requiring AbstractUser we may want to create a type checking only protocol (not sure if that's easy to do as a subtype and I can play around with that).

It does look like there's probably some complications if we move the base class up (subclasses of AbstractBaseUser will not work as subclasses of AbstractUser), so I'll look into this more, but since the plugin is patching the type based on AUTH_USER_MODEL it may only be a problem if you aren't using mypy or haven't configured the plugin.

@terencehonles
Copy link
Contributor Author

I don't think we should change the base class to be AbstractUser. This is a section of what the "Specifying a custom user model" says in the docs:

I rolled back the change since it does affect a user who subclasses AbstractBaseUser based on the documentation. I can look into the permissions issue a little if I have time, but I had already fixed our custom plugin not pulling the right model so the replacement is working for us and we're not hit by AbstractBaseUser not implementing permissions.

This change still adds some helpful aliases so it can get merged in and the follow-up can be done independently.

@terencehonles terencehonles changed the title update default AUTH_USER_MODEL to be AbstractUser and reuse aliases create and re-use TypeAliases and TypeVars for User and AnyUser Oct 1, 2024
@terencehonles terencehonles changed the title create and re-use TypeAliases and TypeVars for User and AnyUser create and re-use TypeAliases and TypeVars for "user" and "any user" Oct 1, 2024
@terencehonles terencehonles force-pushed the update-user-aliases branch 2 times, most recently from bf15c5b to 2697324 Compare April 17, 2025 07:04
This change also adds a few user type vars and alises to cover the
common use cases of ``User``, ``User | AnonymousUser``, and their
``TypeVar`` forms for using in generic contexts.
@terencehonles
Copy link
Contributor Author

@sobolevn any chance I can get a re-review? I updated this to fix the conflicts, but it was already updated to remove the concern about shifting the base user. It now only provides a few useful type aliases that can be shared across the codebase.

High level changes:

  • _UserModel renamed to _User since it's an instance of the model not the model (type) itself
  • _UserType shared TypeVar that represents a _User that should be used in generic context to capture the exact user type being passed into a function/class.
  • _AnyUser alias of _User | AnonymousUser and its shared TypeVar _AnyUserType for the same reason as ^^^

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.

Thanks for the ping! Mostly looks good.

def get_user_model() -> type[_UserModel]: ...
def get_user(request: HttpRequest | Client) -> _UserModel | AnonymousUser: ...
async def aget_user(request: HttpRequest | Client) -> _UserModel | AnonymousUser: ...
def get_user_model() -> type[_User]: ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_user_model() -> type[_User]: ...
def get_user_model() -> 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.

I'll look to see if there are any type[_User] that can be replaced with the UserModel alias

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 think there was just one other that you didn't mention

Copy link
Contributor Author

@terencehonles terencehonles left a comment

Choose a reason for hiding this comment

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

The suggestion seems reasonable, but a few comments and I just want to understand what you'd prefer before continuing.

def get_user_model() -> type[_UserModel]: ...
def get_user(request: HttpRequest | Client) -> _UserModel | AnonymousUser: ...
async def aget_user(request: HttpRequest | Client) -> _UserModel | AnonymousUser: ...
def get_user_model() -> type[_User]: ...
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'll look to see if there are any type[_User] that can be replaced with the UserModel alias

@sobolevn
Copy link
Member

Yes, I prefer re-exporting over re-definition, because definition can change at some point :)

@sobolevn sobolevn closed this Apr 17, 2025
@sobolevn sobolevn reopened this Apr 17, 2025
@sobolevn
Copy link
Member

(missclick, sorry)

@terencehonles
Copy link
Contributor Author

Ruff really doesn't like re-exporting with a rename, but I think I finally got it to stop deleting the re-exports 😒

@terencehonles
Copy link
Contributor Author

updated

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.

Thanks!

@sobolevn sobolevn merged commit 8249ccd into typeddjango:master Apr 17, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants