aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python
https://docs.aiohttp.org
Other
15.12k stars 2.02k forks source link

How to type check an Application? #5864

Closed Dreamsorcerer closed 1 year ago

Dreamsorcerer commented 3 years ago

I think this is something we should discuss and make any changes necessary before v4 is released.

Our examples and demos tend to use the app object in a way that is incompatible with static typing:

def handler(request: Request):
    db = request.app["db"]  # Any

app = Application()
app["db"] = ...

This tends to leave large amounts of code without static typing (e.g. anything that interacts with DB or interacts with the values returned from the DB).

How can we improve this to have better typing support?

I don't think it's possible to inherit from TypedDict, so I don't think there is any way to support the current dict-like interface.

If we moved the dict to a separate attribute, then maybe it could be possible to do something along these lines:

class ConfigDict(TypedDict):
    db: ...

class MyApp(web.Application):
    config: ConfigDict

class MyRequest(web.Request):
    app: MyApp
    config_dict: ConfigDict  # ??

def handler(request: MyRequest):
    db = request.app.config["db"]

app = MyApp()
app.dict["db"] = ...
Dreamsorcerer commented 3 years ago

Any other ideas or thoughts? Maybe it would be better if the app was passed directly to the handler, rather than an attribute of the request?

Another detail: I think the above example would need careful consideration about setting covariant in Handler signatures to type check correctly.

Hanaasagi commented 3 years ago

Emm, TypedDict is not work in this case. It needs to be declared frist. At that time, aiohttp doesn't know what fields and types int the Application. And Application has a typing.final decorator. User could not inherit this class like

from aiohttp import web

class MyApplication(web.Application, TypedDict):
    db: DB
    ...

In actual use, I used to do this to avoid this kind of problem:

from typing import cast

def handler(request: Request):
    db = cast(DB, request.app["db"])

Add redundant code for Mypy check.

Dreamsorcerer commented 3 years ago

Emm, TypedDict is not work in this case. It needs to be declared frist.

Not sure what you mean. This works for me at runtime and almost type checks successfully:

from typing import Any, TypedDict
from aiohttp import web

class _AiohttpApplication(web.Application):
    """This would be in web.Application, not part of the user code."""
    config: Any = {}

class ConfigDict(TypedDict):
    db: str

class MyApp(_AiohttpApplication):
    config: ConfigDict

class MyRequest(web.Request):
    app: MyApp
    #config_dict: ConfigDict  # ??

def handler(request: MyRequest):
    db = request.app.config["db"]
    # reveal_type(db)  # Revealed type is "builtins.str"

app = MyApp()
app.config["db"] = "MYDB"
app.add_routes((web.get("/", handler),))

web.run_app(app)

And Application has a typing.final decorator.

Curiously, I don't get an error when subclassing in the above example. But, this is really a discussion about what we could do in v4, so if we need to remove @final, then we should remove it.

class MyApplication(web.Application, TypedDict):

The real issue is that we can't subclass TypedDict. There is no way around that. Which is why I think it's impossible to support the current dict-like approach (app["db"]), and hence my idea to move the dict to an attribute.

    db = cast(DB, request.app["db"])

Yep, but it's rather annoying to require users to add casts or extra annotations within every handler. That's a workaround rather than a solution. I think we can find something better.

Hanaasagi commented 3 years ago

Thanks for your correction!

I think the reason that the example could pass the check is that you tell the Mypy the type request param in handler function is MyRequest instead of a web.Request(the true type in runtime). So it knows the app is a MyApp. If using the web.Request, Mypy could only find it's super class Application.

https://github.com/aio-libs/aiohttp/blob/3d73221072c350d973b2fb0c54c78ad30863d433/aiohttp/web_request.py#L887-L892


Curiously, I don't get an error when subclassing in the above example.

I run mypy --strict, and get following error. (mypy-0.910)

Cannot inherit from final class "Application"

But, this is really a discussion about what we could do in v4, so if we need to remove @final, then we should remove it.

I agree with this. I browsed the history of aiohttp to figure out why there is a protection for inheritance. It related to https://github.com/aio-libs/aiohttp/issues/2691.

Dreamsorcerer commented 3 years ago

I think the reason that the example could pass the check is that you tell the Mypy the type request param in handler function is MyRequest instead of a web.Request(the true type in runtime).

Yes, the only other way I've thought of, is with the app passed in as a second argument (which would change the signature of handlers, resulting in many more changes): def handler(request: web.Request, app: MyApp):

I agree with this. I browsed the history of aiohttp to figure out why there is a protection for inheritance. It related to #2691.

Yes, maybe there's a way to do this without inheritance, or restrict inheritance in some way that you can only define that one attribute... The subclass doesn't even change anything at runtime, as it's just an annotation.

AustinScola commented 3 years ago

Some first thoughts:

I'm not sure that inheritance should be the default way to use the Application.

Can we do something with generics? Very roughly something like this:

from typing import TypeVar
from typing_extensions import TypedDict

class Database:
    pass

class Context(TypedDict):
    database: Database

T = TypeVar('T')
class Application:
    def __init__(self, context: T):
        self.context: T = context

database = Database()
context: Context = Context({'database': database})
app: Application = Application(context)

assert app.context['database'] is database

This wouldn't address the Request type though.

Dreamsorcerer commented 3 years ago

That looks like a great idea. I was able to edit the Application class like this (we do have to ignore the assignment error):

class Application(MutableMapping[str, Any], Generic[_T]):
    def __init__(...) -> None:
        self.config: _T = {}  # type: ignore

I can then get this user code to type check (except the add_routes() call as before):

from typing import TypedDict
from aiohttp import web

class ConfigDict(TypedDict):
    db: str

class MyRequest(web.Request):
    app: web.Application[ConfigDict]
    #config_dict: ConfigDict  # ??

async def handler(request: MyRequest):
    db = request.app.config["db"]
    # reveal_type(db)  # Revealed type is "builtins.str"

app: web.Application[ConfigDict] = web.Application()
app.config["db"] = "MYDB"
app.add_routes((web.get("/", handler),))

web.run_app(app)

Though I can't seem to put an upper bound on the TypeVar, it seems that it won't allow TypedDict as a bound and that a TypedDict is not a subtype of Dict...

AustinScola commented 3 years ago

What do you think about making web.Request generic in a similar way? On the one hand it would make the style match a bit more, but on the other hand generics could potentially deter novices?

Dreamsorcerer commented 3 years ago

OK, tweaking Request like so:

class Request(BaseRequest, Generic[_T]):
    ...
    def app(self) -> "Application[_T]":

Allows me to write this user code:

from typing import TypedDict
from aiohttp import web

class ConfigDict(TypedDict):
    db: str

async def handler(request: web.Request[ConfigDict]):
    db = request.app.config["db"]
    # reveal_type(db)  # Revealed type is "builtins.str"

app: web.Application[ConfigDict] = web.Application()
app.config["db"] = "MYDB"
app.add_routes((web.get("/", handler),))

web.run_app(app)

I think that could be the solution then. I think there are probably some errors internal to aiohttp that will need to be looked at, but I'll take a look at putting together a PR later.

Dreamsorcerer commented 3 years ago

OK, I think #5893 is ready, if anyone wants to take a look over the code. I still need to consider if extra tests can be added for this, and update the documentation.

AustinScola commented 3 years ago

Cool, I'll check it out!

socketpair commented 1 year ago

6498 is the new attempt

Dreamsorcerer commented 1 year ago

Yes, can probably close this now.