-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
Improve typing of view decorators #2164
base: master
Are you sure you want to change the base?
Conversation
f709670
to
726654a
Compare
|
||
@csrf_protect | ||
def good_view(request: HttpRequest) -> HttpResponse: | ||
return HttpResponse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls, add reveal_type(good__view)
What about path parameters? |
It's probably fine to keep the first positional argument a request, but it needs to allow an arbitrary amount of args and kwargs after that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I like creating a common shared definition for _ViewFuncT
.
But some changes are needed.
|
||
from django.utils.deprecation import _AsyncGetResponseCallable, _GetResponseCallable | ||
|
||
_ViewFuncT = TypeVar("_ViewFuncT", bound=_AsyncGetResponseCallable | _GetResponseCallable) # noqa: PYI018 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would make sense to keep _AsyncGetResponseCallable
, _GetResponseCallable
definitions together in the same file with _ViewFuncT
.
Maybe move those 2 into this file? Or put all of them in django-stubs/views/__init__.pyi
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waitaminute. Views aren't necessarily compatible with _GetResponseCallable
.
Views can have extra arguments from URL parameters, e.g. def good_view2(request: HttpRequest, url_param: str) -> HttpResponse
This causes error
error: Value of type variable "_ViewFuncT" of "csrf_protect" cannot be "Callable[[HttpRequest, str], HttpResponse]" [type-var]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep as suggested by flaeppe
. I got mislead because some of these decorators are defined using decorator_from_middleware
, which will call get_response
(i.e. the view function) only by passing the request
. So probably we need another view func type for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok my bad, decorator_from_middleware
also supports extra arguments specified in a view function. So in the end I only need to update the definition of _ViewFuncT
.
I believe it would break the hack that our FAQ suggests to constrain I never liked this hack to begin with (#2046 (comment)), but I think lots of users are relying on it, we shouldn't break it, unless there's strong consensus to do so. |
A new type variable is defined, matching (async) callables taking a request and returning a response. Some decorators are defined as views to comply with pyright
c88356b
to
126664a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for the assert types. Unfortunately I can't do:
assert_type(good_view, Callable[[HttpRequest], HttpResponse])
As Callable
does not have information about the name of the arguments, while good_view
is inferred as "(request: HttpRequest) -> HttpResponse"
(which is a good thing).
Considering the decorators are defined with the type vars in a straightforward way I don't think it is that much of an issue to not be able to test this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say that we should prefer the type over the name of an argument and thus add an assert_type
to ensure the return type of the decorator.
You should be able to use assert_type(good_view, Callable[[HttpRequest], HttpResponse])
if your good_view
forces the request argument to be positional e.g.
@csrf_protect
-def good_view(request: HttpRequest) -> HttpResponse:
+def good_view(request: HttpRequest, /) -> HttpResponse:
return HttpResponse()
+assert_type(good_view, Callable[[HttpRequest], HttpResponse])
bound=Callable[Concatenate[HttpRequest, ...], HttpResponseBase] | ||
| Callable[Concatenate[HttpRequest, ...], Awaitable[HttpResponseBase]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this still breaks the AuthenticatedHttpRequest
hack, #2164 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we tried/do you think we can use callable protocols here? IIRC Concatenate
only works in function scope(?)
What if we add a callable protocol for each type? e.g. View
and AsyncView
class View(Protocol):
def __call__(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
class AsyncView(Protocol):
async def __call__(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
_ViewFuncT = TypeVar("_ViewFuncT", bound=View | AsyncView)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can we incorporate the AuthenticatedHttpRequest
hack in the test suite? Unless we haven't done so already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__call__(self, request: HttpRequest, *args: Any, **kwargs: Any)
will require every view function to have *args, **kwargs
int their signature. Callable[Concatenate[HttpRequest, ...], HttpResponseBase]
is currently the only way to express what we want here iirc.
I'll take a look at the AuthenticatedHttpRequest
hack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__call__(self, request: HttpRequest, *args: Any, **kwargs: Any)
will require every view function to have*args, **kwargs
int their signature.Callable[Concatenate[HttpRequest, ...], HttpResponseBase]
is currently the only way to express what we want here iirc.
From experimenting with mypy playground, I don't think that *args
and **kwargs
need to be included in the signature. Check this playground link: https://mypy-play.net/?mypy=latest&python=3.12&flags=strict&gist=e5d6cc2baf2095c4eab2863ea645faf3
Let me know if I missed something
Edit: Here's a different playground link that includes a TypeVar
and decorator, much similar to what we have in this PR; https://mypy-play.net/?mypy=latest&python=3.12&flags=strict&gist=277b229672c75316b1b9473c07d4712a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this seems to have changed really recently in the typing spec:
- spec: Expanded "Callables" chapter to include additional details. python/typing#1723
- mypy: This behavior was implemented from the beginning apparently.
- pyright:
(*args: Any, **kwargs: Any)
should be treated as...
in function definition microsoft/pyright#7848.
The |
@csrf_protect | ||
def good_view(request: HttpRequest) -> HttpResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring AuthenticatedHttpRequest
for the moment, the added test_csrf.py
is failing for me with mypy... Do you know what's up with that?
tests/assert_type/views/decorators/test_csrf.py:19: error: Value of type variable "_ViewFuncT" of "csrf_protect" cannot be "Callable[[HttpRequest], HttpResponse]" [type-var]
tests/assert_type/views/decorators/test_csrf.py:24: error: Value of type variable "_ViewFuncT" of "csrf_protect" cannot be "Callable[[HttpRequest], Coroutine[Any, Any, HttpResponse]]" [type-var]
tests/assert_type/views/decorators/test_csrf.py:29: error: Value of type variable "_ViewFuncT" of "csrf_protect" cannot be "Callable[[HttpRequest, int, str], HttpResponse]" [type-var]
tests/assert_type/views/decorators/test_csrf.py:34: error: Value of type variable "_ViewFuncT" of "csrf_protect" cannot be "Callable[[HttpRequest, int, str], Coroutine[Any, Any, HttpResponse]]" [type-var]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to look into what could be done about AuthenticatedHttpRequest
, but this is blocking that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is probably the forced positional of request
in the protocols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Removing the /,
from _View
signature fixes that, but then makes line 9 fail.
We can fix this by duplicating both View protocols for both positional/non-positional cases. I think that would be fine. Unless there are better solutions?
class _View(Protocol):
def __call__(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
class _ViewPositionalRequest(Protocol):
def __call__(self, request: HttpRequest, /, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
class _AsyncView(Protocol):
async def __call__(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
class _AsyncViewPositionalRequest(Protocol):
async def __call__(self, request: HttpRequest, /, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
_ViewFuncT = TypeVar(
"_ViewFuncT", bound=_View | _ViewPositionalRequest | _AsyncView | _AsyncViewPositionalRequest
) # noqa: PYI018
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what is best to do here really.. Django runs all this with a first, "forced" positional, HttpRequest
argument. But it's annoying that everyone then have to add the forced positional marker(/
) to avoid violating the protocol.
An additional thing to keep in mind with the protocols here is that if the request
argument isn't a forced positional it dictates the name of the argument to be "request".
i.e. trying something like below will yield an error:
@csrf_protect
def someview(req: HttpRequest) -> HttpResponse: ...
An additional approach here could be to dig deeper into what attributes the decorator expects. For instance, I think, @csrf_protect
"just" requires an object of something has the attributes session
and COOKIES
(went looking again and found there was a bunch of more attributes/methods, but you get the point) instead of the HttpRequest
type. But it requires it as the first argument.
I'm not sure how much or if that can help, I think we still would have to decide on forced positional or not, but just wanted to mention it as an alternate approach to the HttpRequest
type
class _AsyncView(Protocol): | ||
async def __call__(self, request: HttpRequest, /, *args: Any, **kwargs: Any) -> HttpResponseBase: ... | ||
|
||
_ViewFuncT = TypeVar("_ViewFuncT", bound=_View | _AsyncView) # noqa: PYI018 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw django.db.transaction.non_atomic_requests
decorator expects views as well, and could re-use this.
|
||
# `*args: Any, **kwargs: Any` means any extra argument(s) can be provided, or none. | ||
class _View(Protocol): | ||
def __call__(self, request: HttpRequest, /, *args: Any, **kwargs: Any) -> HttpResponseBase: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like defining a Protocol for request
wouldn't work either.
I'm open to just hinting as request: Any
to solve the AuthenticatedHttpRequest
hack. That's basically the approach we had until now.
|
||
@csrf_protect # type: ignore[type-var] # pyright: ignore[reportArgumentType, reportUntypedFunctionDecorator] | ||
def bad_view_no_arguments() -> HttpResponse: | ||
return HttpResponse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test for the hack as well:
from django.contrib.auth.models import User
class AuthenticatedHttpRequest(HttpRequest):
user: User
@csrf_protect
def view_hack_authenticated_request(request: AuthenticatedHttpRequest) -> HttpResponse:
return HttpResponse()
from django.http.response import HttpResponseBase | ||
|
||
# `*args: Any, **kwargs: Any` means any extra argument(s) can be provided, or none. | ||
class _View(Protocol): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @type_check_only
to all protocols.
A new type variable is defined, matching (async) callables taking a request and returning a response.
Some decorators are defined as views to comply with pyright (see microsoft/pyright#5821 (comment) for an explanation).
Note that some decorators allow
*args, **kwargs
to exist on the decorated views, e.g.never_cache
:However, I think it's good to actually enforce views to take a single
request
argument. What do you think?