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

Liskov substitution problems in new Response.render type signature #2242

Closed
salotz opened this issue Aug 7, 2023 · 4 comments · Fixed by #2264
Closed

Liskov substitution problems in new Response.render type signature #2242

salotz opened this issue Aug 7, 2023 · 4 comments · Fixed by #2264
Labels
typing Type annotations or mypy issues

Comments

@salotz
Copy link

salotz commented Aug 7, 2023

I recently updated to 0.31.0 and found I had a new type checking error (I run with mypy --strict).

myfile.py:28: error: Argument 1 of "render" is incompatible with supertype "Response"; supertype defines the argument type as "str | bytes | None"  [override]
        def render(self, model: Model) -> bytes:
                         ^~~~~~~~~~~~
src/ibeks/server/response.py:28: note: This violates the Liskov substitution principle
src/ibeks/server/response.py:28: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

This is due to the base class Response having the type signature for render being constrained to str | bytes | None. From this PR: #2183 @Viicos @Kludex

Here is my derived class using starlette 0.28.0:

class JSONSerializedResponse(Response):
    media_type = "application/json"
    charset = CHARSET

    def render(self, model: str | bytes | None) -> bytes:
        # unstructure the model to JSON using its registered converter, then
        # encode the payload
        return model.to_json().encode(self.charset)

Now I could put the encoding from my arbitrary Python object Model into the application code, but I quite like it being part of the response. In any case this is how it is advertised in the documentation and indeed the derived class JSONResponse in that same module re-types it to Any: https://github.com/encode/starlette/blob/master/starlette/responses.py#L180.

Whatever the final decision is I think this contradiction needs to be clarified.

Possible solutions would be (in order of preference):

  • create a new generic base class that that uses a type variable (see below for example)
  • revert Response.render type signature to accept Any (Any is typically the wrong solution for most issues and especially this, but it at least works with subtyping to more specific types)

from typing import Generic, TypeVar

AnyContent = TypeVar("AnyContent")
class AbstractResponse(Generic[AnyContent]):

    def render(self, content: AnyContent) -> bytes:
        ...


class Response(AbstractResponse[str | bytes | None]):
    def render(self, content: str | bytes | None) -> bytes:
        ...

And my class could be:

class JSONSerializedResponse(AbstractResponse[Model):
    media_type = "application/json"
    charset = CHARSET

    def render(self, model: Model) -> bytes:
        # unstructure the model to JSON using its registered converter, then
        # encode the payload
        return model.to_json().encode(self.charset)

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Viicos
Copy link
Contributor

Viicos commented Aug 8, 2023

Thanks @salotz, mypy didn't catch this as JSONResponse uses Any and not a specific type as you did with Model. As much as I like typing in Python, I always struggle finding workarounds with the Liskov principle 😅 but at the same time it forces you to find better design patterns to your code.
I like the idea of making this argument generic, I'll let the maintainers choose and I can make a PR if you'd like

@Kludex
Copy link
Member

Kludex commented Aug 8, 2023

I'm fine with reverting the Response.render to use content: typing.Any.

@Kludex Kludex added the typing Type annotations or mypy issues label Aug 8, 2023
@salotz
Copy link
Author

salotz commented Aug 8, 2023

As much as I like typing in Python, I always struggle finding workarounds with the Liskov principle sweat_smile but at the same time it forces you to find better design patterns to your code.

I agree I only started writing types this year. The promise of "gradual typing" seems nice but in practice I've found that you should either start a project with mypy --strict or not really bother. Once you bake in even a few Anys without a very good reason its hard to really follow what is typed properly or not.

@aminalaee
Copy link
Member

I also agree with @Kludex, I will do a simple PR to revert it, feel free to close it if we decide to implement the generic types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Type annotations or mypy issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants