Transform user_passes_test signature #1229
Conversation
Add a transformer that updates the signature of user_passes_test to reflect settings.AUTH_USER_MODEL. Fixes typeddjango#1058
| from users.models import User | ||
| def check_user(user: Union[User, AnonymousUser]) -> bool: ... | ||
| @user_passes_test(check_user) | ||
| def view_func(request: HttpRequest) -> HttpResponse: ... |
There was a problem hiding this comment.
You need to assert types here, current case is not enough.
What do you expect to happen?
There was a problem hiding this comment.
Without the changes in the PR mypy will produce a type error on line 46 (because check_user doesn't match Callable[[AbstratBaseUser|AnonymousUser], bool]), so the test fails. With this PR the signature of user_passes_test is changed so there's no type errors and thus the test passes.
| from django.contrib.auth.models import AnonymousUser | ||
| from django.http import HttpRequest, HttpResponse | ||
| from users.models import User | ||
| def check_user(user: Union[User, AnonymousUser]) -> bool: ... |
There was a problem hiding this comment.
What about def check_user2(user: User) -> bool: ... ?
There was a problem hiding this comment.
And def check_user(user: Union[ AnonymousUser, User]) -> bool: ...
There was a problem hiding this comment.
The former would be an error, because it doesn't accept AnonymousUser, while the latter is perfectly valid. Note that this change only changes the signature of user_passes_test, all the type checking is done by mypy. So basically the signature is changed from:
def user_passes_test(test_func: Callable[[AbstractBaseUser, AnonymousUser], bool]], ...):to:
def user_passes_test(test_func: Callable[[settings.AUTH_USER_MODEL, AnonymousUser], bool]], ...):So anything valid matching the first signature would still match the second one, but the second one allows writing more specific types (using your configured user model).
I added a few more tests cases to demonstrate this.
|
This seems like significant complexity to dynamically update the signature of just one function. There are lots more places in Django that depend on
|
This is already done for Same with I'm not sure if there's an easy way to do this generically (I'm also unsure how many places this is needed). I've just fixed the only place where this was causing new type issues in our codebase |
|
I think instead we can make |
|
I'm not exactly sure if I'm following how you'd envision that working? Altering the type of I'm happy to do some cleanup here and add a helper to extract the 'current' user type (to reduce duplication), but I don't see how this could be done in another way. |
|
I think we can at least experiment to do something like this:
So, ideally we can change all (most) def user_passes_test(func: Callable[[AUTH_USER_MODEL | AnonymousUser], bool]) -> ...: ...`However, I don't say that:
|
|
One immediate problem I see with that is that User = get_user_model() |
|
It can work because of Ok, we can try to populate |
|
Hmm, wouldn't that break usage of the stubs without the plugin? For this to work properly I'd think it has to be defined somewhere in the stubs at least? |
| if not test_func_type.arg_types or not isinstance(test_func_type.arg_types[0], UnionType): | ||
| return ctx.default_signature | ||
| union = test_func_type.arg_types[0] |
There was a problem hiding this comment.
It's not obvious why we're extracting a Union here. We should add a comment displaying why (context of us expecting AbstractBaseUser | AnonymousUser isn't anywhere near here)
| if not test_func_type.arg_types or not isinstance(test_func_type.arg_types[0], UnionType): | |
| return ctx.default_signature | |
| union = test_func_type.arg_types[0] | |
| # Extract the Union argument for user out of the test function: | |
| # user: AbstractBaseUser | AnonymousUser | |
| if not test_func_type.arg_types or not isinstance(test_func_type.arg_types[0], UnionType): | |
| return ctx.default_signature | |
| union = test_func_type.arg_types[0] |
|
|
||
| new_union = UnionType([Instance(user_model_info, [])] + union.items[1:]) |
There was a problem hiding this comment.
| new_union = UnionType([Instance(user_model_info, [])] + union.items[1:]) | |
| # Replace AbstractBaseUser with the model type AUTH_USER_MODEL points to | |
| # user: <AUTH_USER_MODEL> | AnonymousUser | |
| new_union = UnionType([Instance(user_model_info, [])] + union.items[1:]) |
|
|
||
| new_test_func_type = test_func_type.copy_modified( | ||
| arg_types=[new_union] + test_func_type.arg_types[1:], | ||
| ) |
There was a problem hiding this comment.
| new_test_func_type = test_func_type.copy_modified( | |
| arg_types=[new_union] + test_func_type.arg_types[1:], | |
| ) | |
| # Update the test function with the adjusted user annotation | |
| new_test_func_type = test_func_type.copy_modified( | |
| arg_types=[new_union] + test_func_type.arg_types[1:], | |
| ) |
|
|
||
| return ctx.default_signature.copy_modified( | ||
| arg_types=[new_test_func_type] + ctx.default_signature.arg_types[1:], | ||
| ) |
There was a problem hiding this comment.
| return ctx.default_signature.copy_modified( | |
| arg_types=[new_test_func_type] + ctx.default_signature.arg_types[1:], | |
| ) | |
| # Update user_passes_test function signature to expect the new test function signature as first argument | |
| return ctx.default_signature.copy_modified( | |
| arg_types=[new_test_func_type] + ctx.default_signature.arg_types[1:], | |
| ) |
| from mypy_django_plugin.lib import helpers | ||
|
|
||
|
|
||
| def transform_user_passes_test(ctx: FunctionSigContext, django_context: DjangoContext) -> FunctionLike: |
There was a problem hiding this comment.
Can't we name this function a bit more specifically?
e.g.
| def transform_user_passes_test(ctx: FunctionSigContext, django_context: DjangoContext) -> FunctionLike: | |
| def attach_AUTH_USER_MODEL_to_user_passes_test(ctx: FunctionSigContext, django_context: DjangoContext) -> FunctionLike: |
I got a spin on this idea:
So, what if the stubs add some variable: Then the stubs replaces all places where we expect - -> Type[Model]:
+ -> _AUTH_USER_MODEL:Which (hopefully) would align all types. |
|
I like this idea (I think that adding a hook is a must, however - otherwise, we will have too many errors in users' code). |
|
Hmm, interesting. Sounds like it's worth a try! Maybe best to try in a separate PR though? |
|
Yeah a separate PR would be best |
|
Are we sure we can't just use # stubs.pyi
User = get_auth_user()
class HttpRequest:
user: User | AnonymousUser |
|
I think we should, because it is a perfect use-case for it. |
|
I don't think that works. Isn't it only TypeAlias that can be annotated with? And that function call can't return a type alias, I've tried make the plugin spoof that kind of aliasing as well, in #1699 |
|
Mypy makes |
|
But can't we change that using |
Probably, I've tried that in #1699 but it isn't trivial |
|
I think I managed to make it work: #1730 Main trick was replacing the symbol with a placeholder when deferring, as otherwise the variable error popped up (that seems like a bug in mypy) |
Add a transformer that updates the signature of
user_passes_testto reflectsettings.AUTH_USER_MODEL.Related issues
Fixes #1058