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

Improve typing of view decorators #2164

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions django-stubs/views/decorators/__init__.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from typing import Any, Protocol, TypeVar

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

# `*args: Any, **kwargs: Any` means any extra argument(s) can be provided, or none.
class _View(Protocol):
Copy link
Collaborator

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.

def __call__(self, request: HttpRequest, /, *args: Any, **kwargs: Any) -> HttpResponseBase: ...
Copy link
Collaborator

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.


class _AsyncView(Protocol):
async def __call__(self, request: HttpRequest, /, *args: Any, **kwargs: Any) -> HttpResponseBase: ...

_ViewFuncT = TypeVar("_ViewFuncT", bound=_View | _AsyncView) # noqa: PYI018
Copy link
Collaborator

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.

12 changes: 6 additions & 6 deletions django-stubs/views/decorators/cache.pyi
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
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
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
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
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
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
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: ...
46 changes: 46 additions & 0 deletions tests/assert_type/views/decorators/test_csrf.py
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,46 @@
from typing import Callable

from django.http.request import HttpRequest
from django.http.response import HttpResponse
from django.views.decorators.csrf import csrf_protect
from typing_extensions import assert_type


@csrf_protect
def good_view_positional(request: HttpRequest, /) -> HttpResponse:
return HttpResponse()


# `assert_type` can only be used when `request` is pos. only.
assert_type(good_view_positional, Callable[[HttpRequest], HttpResponse])


# The decorator works too if `request` is not explicitly pos. only.
@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

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

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.

Copy link
Member

@flaeppe flaeppe Jun 6, 2024

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[type-var] # pyright: ignore[reportArgumentType, reportUntypedFunctionDecorator]
def bad_view(request: int) -> str:
return ""


@csrf_protect # type: ignore[type-var] # pyright: ignore[reportArgumentType, reportUntypedFunctionDecorator]
def bad_view_no_arguments() -> HttpResponse:
return HttpResponse()
Copy link
Collaborator

@intgr intgr Jun 6, 2024

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