Skip to content

TemplateView and AuthenticatedHttpRequest producing Liskov substitution error #1024

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

Closed
sshishov opened this issue Jun 27, 2022 · 5 comments
Closed
Labels
bug Something isn't working

Comments

@sshishov
Copy link
Contributor

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
@sterliakov
Copy link
Contributor

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!

@BranislavBajuzik
Copy link

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

@sshishov
Copy link
Contributor Author

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

@thanksyouall
Copy link

@sterliakov thank you very much! You helped me.

@sshishov
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

4 participants