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.

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

Improve typing of view decorators #2164

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Improve typing of view decorators #2164

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d7a572
Improve typing of view decorators
Viicos May 15, 2024
a7827f9
Use `str` for `key_prefix`
Viicos May 15, 2024
40e0a36
Add test
Viicos May 15, 2024
a65d50d
Feedback
Viicos May 28, 2024
7b47826
Use callback protocols, apply feedback
Viicos May 30, 2024
74e07c0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 30, 2024
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
15 changes: 15 additions & 0 deletions django-stubs/views/decorators/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from collections.abc import Awaitable, Callable
from typing import TypeVar

from django.http.request import HttpRequest
from django.http.response import HttpResponseBase
from typing_extensions import Concatenate

# Examples:
# def (request: HttpRequest, path_param: str) -> HttpResponseBase
# async def (request: HttpRequest) -> HttpResponseBase
_ViewFuncT = TypeVar( # noqa: PYI018
"_ViewFuncT",
bound=Callable[Concatenate[HttpRequest, ...], HttpResponseBase]
| Callable[Concatenate[HttpRequest, ...], Awaitable[HttpResponseBase]],
Copy link
Collaborator

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)

Copy link
Member

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)

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

@flaeppe flaeppe May 28, 2024 •
edited
Loading

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

Copy link
Contributor Author

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:

)
12 changes: 6 additions & 6 deletions django-stubs/views/decorators/cache.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
from collections.abc import Callable
from typing import Any, TypeVar
from typing import Any

_F = TypeVar("_F", bound=Callable[..., Any])
from . import _ViewFuncT

def cache_page(
timeout: float | None, *, cache: Any | None = ..., key_prefix: Any | None = ...
) -> Callable[[_F], _F]: ...
def cache_control(**kwargs: Any) -> Callable[[_F], _F]: ...
def never_cache(view_func: _F) -> _F: ...
timeout: float | None, *, cache: Any | None = ..., key_prefix: str | None = ...
) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
def cache_control(**kwargs: Any) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
def never_cache(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
11 changes: 4 additions & 7 deletions django-stubs/views/decorators/clickjacking.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
from collections.abc import Callable
from typing import Any, TypeVar
from . import _ViewFuncT

_F = TypeVar("_F", bound=Callable[..., Any])

def xframe_options_deny(view_func: _F) -> _F: ...
def xframe_options_sameorigin(view_func: _F) -> _F: ...
def xframe_options_exempt(view_func: _F) -> _F: ...
def xframe_options_deny(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
def xframe_options_sameorigin(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
def xframe_options_exempt(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
7 changes: 2 additions & 5 deletions django-stubs/views/decorators/common.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
from collections.abc import Callable
from typing import Any, TypeVar
from . import _ViewFuncT

_C = TypeVar("_C", bound=Callable[..., Any])

def no_append_slash(view_func: _C) -> _C: ...
def no_append_slash(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
16 changes: 6 additions & 10 deletions django-stubs/views/decorators/csrf.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
from collections.abc import Callable
from typing import Any, TypeVar

from django.middleware.csrf import CsrfViewMiddleware

csrf_protect: Callable[[_F], _F]
from . import _ViewFuncT

def csrf_protect(view_func: _ViewFuncT, /) -> _ViewFuncT: ...

class _EnsureCsrfToken(CsrfViewMiddleware): ...

requires_csrf_token: Callable[[_F], _F]
def requires_csrf_token(view_func: _ViewFuncT, /) -> _ViewFuncT: ...

class _EnsureCsrfCookie(CsrfViewMiddleware): ...

ensure_csrf_cookie: Callable[[_F], _F]

_F = TypeVar("_F", bound=Callable[..., Any])

def csrf_exempt(view_func: _F) -> _F: ...
def ensure_csrf_cookie(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
def csrf_exempt(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
7 changes: 2 additions & 5 deletions django-stubs/views/decorators/gzip.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
from collections.abc import Callable
from typing import Any, TypeVar
from . import _ViewFuncT

_C = TypeVar("_C", bound=Callable[..., Any])

gzip_page: Callable[[_C], _C]
def gzip_page(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
22 changes: 9 additions & 13 deletions django-stubs/views/decorators/http.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,15 @@
from collections.abc import Callable, Container
from datetime import datetime
from typing import Any, TypeVar

_F = TypeVar("_F", bound=Callable[..., Any])

conditional_page: Callable[[_F], _F]

def require_http_methods(request_method_list: Container[str]) -> Callable[[_F], _F]: ...

require_GET: Callable[[_F], _F]
require_POST: Callable[[_F], _F]
require_safe: Callable[[_F], _F]
from . import _ViewFuncT

def conditional_page(view_func: _ViewFuncT, /) -> _ViewFuncT: ...
def require_http_methods(request_method_list: Container[str]) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
def require_GET(func: _ViewFuncT, /) -> _ViewFuncT: ...
def require_POST(func: _ViewFuncT, /) -> _ViewFuncT: ...
def require_safe(func: _ViewFuncT, /) -> _ViewFuncT: ...
def condition(
etag_func: Callable[..., str | None] | None = ..., last_modified_func: Callable[..., datetime | None] | None = ...
) -> Callable[[_F], _F]: ...
def etag(etag_func: Callable[..., str | None]) -> Callable[[_F], _F]: ...
def last_modified(last_modified_func: Callable[..., datetime | None]) -> Callable[[_F], _F]: ...
) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
def etag(etag_func: Callable[..., str | None]) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
def last_modified(last_modified_func: Callable[..., datetime | None]) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
7 changes: 3 additions & 4 deletions django-stubs/views/decorators/vary.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from collections.abc import Callable
from typing import Any, TypeVar

_F = TypeVar("_F", bound=Callable[..., Any])
from . import _ViewFuncT

def vary_on_headers(*headers: str) -> Callable[[_F], _F]: ...
def vary_on_cookie(func: _F) -> _F: ...
def vary_on_headers(*headers: str) -> Callable[[_ViewFuncT], _ViewFuncT]: ...
def vary_on_cookie(func: _ViewFuncT, /) -> _ViewFuncT: ...
33 changes: 33 additions & 0 deletions tests/assert_type/views/decorators/test_csrf.py
View file
Open in desktop
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 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

Copy link
Member

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from django.http.request import HttpRequest
from django.http.response import HttpResponse
from django.views.decorators.csrf import csrf_protect


@csrf_protect
def good_view(request: HttpRequest) -> HttpResponse:
Comment on lines +19 to +20
Copy link
Collaborator

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]

Copy link
Collaborator

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.

Copy link
Member

@flaeppe flaeppe Jun 6, 2024 •
edited
Loading

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

Copy link
Collaborator

@intgr intgr Jun 6, 2024 •
edited
Loading

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.

Sign in to view

Copy link
Member

@flaeppe flaeppe Jun 6, 2024 •
edited
Loading

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

return HttpResponse()


@csrf_protect
async def good_async_view(request: HttpRequest) -> HttpResponse:
return HttpResponse()


@csrf_protect
def good_view_with_arguments(request: HttpRequest, other: int, args: str) -> HttpResponse:
return HttpResponse()


@csrf_protect
async def good_async_view_with_arguments(request: HttpRequest, other: int, args: str) -> HttpResponse:
return HttpResponse()


@csrf_protect # type: ignore # pyright: ignore
def bad_view(request: int) -> str:
return ""


@csrf_protect # type: ignore # pyright: ignore
def bad_view_no_arguments() -> HttpResponse:
return HttpResponse()
Copy link
Collaborator

@intgr intgr Jun 6, 2024 •
edited
Loading

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

Loading