connexion icon indicating copy to clipboard operation
connexion copied to clipboard

Connexion 3.0.0a2 connextion.request does not expose the starlette's properties in a type checker friendly manner.

Open mmohaveri opened this issue 1 year ago • 10 comments

Description

I'm using Connextion 3 ASGI app. There, connextion.request in a wrapper around starlette's request and I can access any starlette's request info I want using it, e.g: connextion.request.headers. The way that it has been implemented is by overriding __getattr__ in the following way:

    def __getattr__(self, item):
        return getattr(self._starlette_request, item)

It works in runtime, but type checkers like mypy or pyright are not able to infer the connextion.request's properties correctly, so I'm getting an error in my type checker whenever I access a starlette's request property.

Expected behaviour

I expect connextion to define ASGIRequest in a way that allows type checker to know about starlette's reqeust properties.

I think re-defining the properties explicitly in ASGIRequest is the easies way, but in for any reason starlette decides to change its request object, connextion's ASGIRequest has to change as well.

Additional info:

Output of the commands:

  • python --version: 3.11.2
  • pip show connexion | grep "^Version\:": 3.0.0a2

mmohaveri avatar Mar 10 '23 15:03 mmohaveri

Hi @mmohaveri

mypy has supported __getattr__ for a long time though. Eg. the following code should not raise any errors:

class Foo:
    def __getattr__(self, item):
        return item

foo = Foo()
foo.bar

Of course it cannot infer the type of foo.bar either, so you'll just get an Any type instead.

Is this not the behavior you're seeing?

RobbeSneyders avatar Mar 10 '23 16:03 RobbeSneyders

Yeah, upon further investigation the source of the problem is werkzeug.local.LocalProxy, which wraps ASGIRequest. In connexion.context the request is being defined as:

request = LocalProxy(lambda: ASGIRequest(scope, receive), unbound_message=UNBOUND_MESSAGE)

In this case, the type checker is not able to understand that in our context the type of the request is ASGIRequest. By changing it to

request: ASGIRequest = LocalProxy(lambda: ASGIRequest(scope, receive), unbound_message=UNBOUND_MESSAGE)

I was able to fix the error. I'm not sure why this happens as LocalProxy has been defined as a

class LocalProxy(t.Generic[T]):
        def __init__(
        self,
        local: t.Union[ContextVar[T], Local, LocalStack[T], t.Callable[[], T]],
        name: t.Optional[str] = None,
        *,
        unbound_message: t.Optional[str] = None,
    ) -> None: ...

So the type checker should be able to infer the type of LocalProxy(ASGIRequest) is a subtype of ASGIProxy and has all of its properties, but it's not. I'm not sure if specifying the type here is a good idea, but it was the only way for me to get rid of the error in a sensible way.

But still I think connexion should pass starlette's request properties in a more type friendly manner. Because by using __getattr__ tell type checker to ignore any property access and assume it to be Any, which can cause runtime problems.

Also by using it we're throwing the type information stored in starlette's request away. For example request.header get recognized as Header not Any.

mmohaveri avatar Mar 10 '23 18:03 mmohaveri

Adding types for all the LocalProxys in connexion.context would indeed be a nice improvement. If you want, you can submit a PR for this, otherwise I'll add it soon.

I understand that using __getattr__ is a trade off here which loses some typing information, but I don't think the additional overhead to solve this is worth it. There's also a workaround for users, as they can cast the attribute in their code. Building on my previous example:

class Foo:
    def __getattr__(self, item):
        return "bar"


foo = Foo()
bar: str = foo.bar
bar.strip()

RobbeSneyders avatar Mar 10 '23 19:03 RobbeSneyders

Adding types for LocalProxy is a quick fix and solves the end user's problem. But this introduce type errors in the connexion codebase and requires maintenance, i.e. in case of a change in the content of a LocalProxy variable, we need to remember to change its type hint as well.

If these two problems are ok with you, I can submit a PR for this.

mmohaveri avatar Mar 10 '23 19:03 mmohaveri

I just tested this a bit further, and mypy is able to infer the correct type of the LocalPoxy from the current definition.

from connexion.context import context

context.get("foo")  # not an error
context.add("foo")  #  error: "Dict[Any, Any]" has no attribute "add"

Which typechecker are you using and which version?

RobbeSneyders avatar Mar 10 '23 20:03 RobbeSneyders

I'm using pyright 1.1.293.

When I run the following code:

from typing import reveal_type
from connexion import request

reveal_type(request)
reveal_type(request._starlette_request)

by running pyright --lib main.py I get the following output:

main.py:4:13 - information: Type of "request" is "LocalProxy[ASGIRequest]"
main.py:5:21 - error: Cannot access member "_starlette_request" for type "LocalProxy[ASGIRequest]"  Member "_starlette_request" is unknown
main.py:5:13 - information: Type of "request._starlette_request" is "Unknown"

As you can see, pyright is not able to infer the _starlette_request's type as it does not understand that request should act as a subtype of ASGIRequest.

P.S:

I was not able to run the code using mypy as I don't use it normally and don't know how can I tell it to use library's code to infer type (like --lib flag in pyright). When I run this code using mypy main.py I get the following output:

main.py:2: error: Skipping analyzing "connexion": module is installed, but missing library stubs or py.typed marker  [import]
main.py:4: note: Revealed type is "Any"
main.py:5: note: Revealed type is "Any"

Which is expected because connextion does nto define py.typed marker. pyright allows me "use library code to infer types when stubs are missing" by using --lib flag, but I don't know the equivalent solution for mypy.

mmohaveri avatar Mar 12 '23 17:03 mmohaveri

You're right, we should add a py.typed marker to fix it for mypy. Feel free to submit a PR.

I have no idea why it doesn't work for pyright. It does recognize request as LocalProxy[ASGIRequest], so the issue seems to be with LocalProxy. I don't think this is something we can fix in Connexion. Can you check if you are using the latest werkzeug version?

RobbeSneyders avatar Mar 12 '23 20:03 RobbeSneyders

Just chipping in to say I have run into the same issue, I'm using Pycharm 2022.3.3. Properties happily appear in the debugger but whatever linter/type hinter pycharm uses for editing is clueless about things like operation, context etc. Bit annoying, looking forward to a fix.

SpudInNZ avatar Mar 12 '23 21:03 SpudInNZ

The Pycharm linter does work correctly when run on connexion locally, just like mypy. So I assume adding a py.typed will solve this as well.

RobbeSneyders avatar Mar 12 '23 22:03 RobbeSneyders

As far as I understand, connexion 3 is not a typed project, by which I mean many of its functions do not have correct (or complete) type annotations. I can see that some parts of the project are typed, but I don't think all of it is typed.

For that reason I'm not sure if adding a py.typed is a good idea. Specially because adding it may disable other type checker's ability to inspect the code for inferred types (like what pyright does with its --lib flag.

@RobbeSneyders what do you thinks?

P.S: By complete I mean a type annotation that all of its template types are specified. e.g:

x: dict = {}  # incomplete
x: dict[str, Any] = {} # complete

The difference is in type checkers like pylance (and I think even mypy), the first one is detected as dict[Unknown, Unknown].

mmohaveri avatar Apr 13 '23 10:04 mmohaveri