Skip to content

only allow redirect_chain attribute in response when client sets the follow flag #1257

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

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

only allow redirect_chain attribute in response when client sets the follow flag #1257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8296c1c
only allow redirect_chain attribute in response when client sets the …
hoefling Nov 14, 2022
187c354
narrow ignores to override error category
hoefling Nov 16, 2022
1e62e20
address review comments
hoefling Nov 16, 2022
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
130 changes: 122 additions & 8 deletions django-stubs/test/client.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ from io import BytesIO
from json import JSONEncoder
from re import Pattern
from types import TracebackType
from typing import Any, Generic, NoReturn, TypeVar
from typing import Any, Generic, NoReturn, TypeVar, overload

from django.contrib.auth.base_user import AbstractBaseUser
from django.contrib.sessions.backends.base import SessionBase
Expand All @@ -16,6 +16,7 @@ from django.http.response import HttpResponseBase
from django.template.base import Template
from django.test.utils import ContextList
from django.urls import ResolverMatch
from typing_extensions import Literal

BOUNDARY: str
MULTIPART_CONTENT: str
Expand Down Expand Up @@ -98,6 +99,8 @@ class _MonkeyPatchedWSGIResponse(_WSGIResponse):
context: ContextList | dict[str, Any]
content: bytes
resolver_match: ResolverMatch

class _MonkeyPatchedWSGIResponseRedirect(_MonkeyPatchedWSGIResponse):
redirect_chain: list[tuple[str, int]]

class _MonkeyPatchedASGIResponse(_ASGIResponse):
Expand Down Expand Up @@ -128,25 +131,136 @@ class Client(ClientMixin, _RequestFactory[_MonkeyPatchedWSGIResponse]):
) -> None: ...
# Silence type warnings, since this class overrides arguments and return types in an unsafe manner.
def request(self, **request: Any) -> _MonkeyPatchedWSGIResponse: ...
def get( # type: ignore
@overload # type: ignore[misc,override]
def get(
Copy link
Member

@sobolevn sobolevn Nov 14, 2022

Choose a reason for hiding this comment

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

I think we can remove this overload, it is the same as bool

Copy link
Contributor Author

@hoefling hoefling Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@sobolevn this will cause a false positive tho for the default value, e.g. Client().get('foo').response_chain will be inferred as existing since the follow: Literal[True] case matches and I can't put it behind the broader follow: bool case. The client_follow_flag test I added should also fail when the first override is deleted.

Copy link
Collaborator

@intgr intgr Mar 6, 2023

Choose a reason for hiding this comment

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

Hmm. Ordinarily you would remove the default value (= ...) from the argument in one of the overloads, like

@overload
def get(self, path: str, data: Any = ..., follow: Literal[True], secure: bool = ..., **extra: Any) -> _MonkeyPatchedWSGIResponseRedirect: ...
#                                         ^^^^^^^^^^^^^^^^^^^^^
@overload
def get(self, path: str, data: Any = ..., follow: bool = ..., secure: bool = ..., **extra: Any) -> _MonkeyPatchedWSGIResponse: ...

But here it's not allowed because it follows another argument with default data.

I'm not aware of any tricks to make it work. May indeed have to use 3 overloads.

self, path: str, data: Any = ..., follow: Literal[False] = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def get(
self, path: str, data: Any = ..., follow: Literal[True] = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def get(
self, path: str, data: Any = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
def post( # type: ignore
@overload # type: ignore[misc,override]
def post(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[False] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def post(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[True] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def post(
self, path: str, data: Any = ..., content_type: str = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
def head( # type: ignore
@overload # type: ignore[misc,override]
def head(
self, path: str, data: Any = ..., follow: Literal[False] = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def head(
self, path: str, data: Any = ..., follow: Literal[True] = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def head(
self, path: str, data: Any = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
def trace( # type: ignore
@overload # type: ignore[misc,override]
def trace(
self, path: str, data: Any = ..., follow: Literal[False] = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def trace(
self, path: str, data: Any = ..., follow: Literal[True] = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def trace(
self, path: str, data: Any = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
def put( # type: ignore
@overload # type: ignore[misc,override]
def put(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[False] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def put(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[True] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def put(
self, path: str, data: Any = ..., content_type: str = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
def patch( # type: ignore
@overload # type: ignore[misc,override]
def patch(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[False] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def patch(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[True] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def patch(
self, path: str, data: Any = ..., content_type: str = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
def delete( # type: ignore
@overload # type: ignore[misc,override]
def delete(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[False] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponse: ...
@overload
def delete(
self,
path: str,
data: Any = ...,
content_type: str = ...,
follow: Literal[True] = ...,
secure: bool = ...,
**extra: Any
) -> _MonkeyPatchedWSGIResponseRedirect: ...
@overload
def delete(
self, path: str, data: Any = ..., content_type: str = ..., follow: bool = ..., secure: bool = ..., **extra: Any
) -> _MonkeyPatchedWSGIResponse: ...

Expand Down
74 changes: 73 additions & 1 deletion tests/typecheck/test/test_client.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
reveal_type(response.client) # N: Revealed type is "django.test.client.Client"
reveal_type(response.context) # N: Revealed type is "Union[django.test.utils.ContextList, builtins.dict[builtins.str, Any]]"
reveal_type(response.content) # N: Revealed type is "builtins.bytes"
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
response.json()
- case: async_client_methods
main: |
Expand All @@ -35,3 +34,76 @@
async_factory = AsyncRequestFactory()
async_request = async_factory.get('foo')
reveal_type(async_request) # N: Revealed type is "django.core.handlers.asgi.ASGIRequest"
- case: client_follow_flag
main: |
from django.test.client import Client
client = Client()
response = client.get('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.get('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.get('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.get('foo', follow=x)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.post('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.post('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.post('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.post('foo', follow=x)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.head('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.head('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.head('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.head('foo', follow=x)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.trace('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.trace('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.trace('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.trace('foo', follow=x)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.put('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.put('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.put('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.put('foo', follow=x)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.patch('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.patch('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.patch('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.patch('foo', follow=x)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.delete('foo')
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.delete('foo', follow=False)
response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"
response = client.delete('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.delete('foo', follow=x)
Copy link
Member

@sobolevn sobolevn Nov 16, 2022

Choose a reason for hiding this comment

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

Maybe we should allow bool? I think the main usecase for this is in unit-tests. They will be checked in all cases, but we can just make things harder for users by producing errors (and possibly false-positives).

Copy link
Contributor Author

@hoefling hoefling Nov 17, 2022

Choose a reason for hiding this comment

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

@sobolevn sorry, I cannot follow you. Or do you mean "not allow bool" instead? Only boolean literals?

Copy link
Member

@sobolevn sobolevn Nov 18, 2022

Choose a reason for hiding this comment

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

Sorry :(

I mean that using bool should not produce this error. Because users will get a unittest failure if it is not True anyway. And I don't want to have false-positives in test code.

Is it better now? 😊

response.redirect_chain # E: "_MonkeyPatchedWSGIResponse" has no attribute "redirect_chain"