django-stubs
django-stubs copied to clipboard
TemplateView and AuthenticatedHttpRequest producing Liskov substitution error
Bug report
Trying to apply the suggestion, how to assign authenticated user to the request:
from typing import Any
from django.contrib.auth.decorators import login_required
from django.contrib.auth.models import User
from django.http.request import HttpRequest
from django.http.response import HttpResponse
from django.utils.decorators import method_decorator
from django.views.generic import TemplateView
class AuthenticatedHttpRequest(HttpRequest):
user: User
@method_decorator(login_required, name='dispatch')
class UserProfileView(TemplateView):
def get(self, request: AuthenticatedHttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:
return HttpResponse(content=request.user.first_name)
What's wrong
MyPy returns the following error:
test_app/view.py:17: error: Argument 1 of "get" is incompatible with supertype "TemplateView"; supertype defines the argument type as "HttpRequest" [override]
test_app/view.py:17: note: This violates the Liskov substitution principle
test_app/view.py:17: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
How is that should be
No errors should be reported
System information
- OS: MacOS
-
python
version: 3.9.12 -
django
version: 3.2.13 -
mypy
version: 0.950 -
django-stubs
version: 1.12.0 -
django-stubs-ext
version: 0.5.0
Are you sure that this is a bug? Looks more like a feature request for me. Currently there is no way to say mypy "this class decorator allows LSP violation, when HttpRequest is replaced by some user-defined AuthenticatedHttpRequest (with this name? Or how should we determine it?)". The stubs are saying basically "get
method accepts HttpRequest in first place" - and without @method_decorator
this LSP violation would have been reported correctly. So, the workaround (and type-safe way to go) is:
@method_decorator(login_required, name='dispatch')
class UserProfileView(TemplateView):
def get(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:
assert isinstance(request, AuthenticatedHttpRequest)
return HttpResponse(content=request.user.first_name)
1-line assert is not worth complexity required to support this automatically, IMO. Please share your suggestions on other ways to handle this, if you have any ideas - PR or clean explanation is welcome!
I believe @sshishov is referring to the suggestion in the README (the same reason I am here). Please consider mentioning the assert
in there.
@sterliakov yes I was referencing exactly the example from README.
As it looks very clear as a solution and we tried to apply it everywhere.
Sometimes it is working, but sometimes it produces violates the Liskov substitution principle
Could you please mention in README that the provided case will work only for @login_required
decorator case then?
@sterliakov thank you very much! You helped me.
Thanks @sterliakov , we have started using mentioned assert
everywhere, no problems since then 👍