django-stubs icon indicating copy to clipboard operation
django-stubs copied to clipboard

TemplateView and AuthenticatedHttpRequest producing Liskov substitution error

Open sshishov opened this issue 2 years ago • 2 comments

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

sshishov avatar Jun 27 '22 11:06 sshishov

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!

sterliakov avatar Sep 10 '22 07:09 sterliakov

I believe @sshishov is referring to the suggestion in the README (the same reason I am here). Please consider mentioning the assert in there.

BranislavBajuzik avatar Sep 16 '22 20:09 BranislavBajuzik

@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?

sshishov avatar Oct 18 '22 08:10 sshishov

@sterliakov thank you very much! You helped me.

thanksyouall avatar Mar 06 '23 19:03 thanksyouall

Thanks @sterliakov , we have started using mentioned assert everywhere, no problems since then 👍

sshishov avatar May 27 '23 11:05 sshishov