falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 944 forks source link

Improve typing of custom req/resp type #2372

Open vytas7 opened 1 week ago

vytas7 commented 1 week ago

Discussed in https://github.com/falconry/falcon/discussions/2370

Originally posted by **jkmnt** October 14, 2024 Testing f4 beta with the our falcon-powered app. No runtime issues so far, but a few errors from the typechecker. The `app.add_error_handler` and `app.add_error_handler` expects the callables with signature of [falcon.Request, falcon.Response, ...etc]. Our app is using the custom Request class: ```python falcon.App(request_type=MyRequest) def my_error_handler(req: MyRequest, ...): ... ``` Maybe the request/response types should be typed as generics bound to the base type ? Something like ``` TReq = TypeVar('TReq', bound=Request) class App(Generic[TReq, TResp]): def __init__( self, request_type: Optional[type[TReq]] = None, ...): ... def add_error_handler(self, exception, handler: Callable[[TReq, TResp, TCtx, ...], None]): ... ``` The same issue is with `before`/`after` hook adders . Yes, it's way harder to type since they know nothing about the app class.
jkmnt commented 1 week ago

Hi. My POC on the issue here: https://github.com/jkmnt/falcon/tree/generics_poc Diff: https://github.com/jkmnt/falcon/commit/a868ff58ac54821a70405e4b50a6d72d56332c99

I typed wsgi App class with generics. The concrete types are seems to be correctly inferred. The only problem was with the standard App: Request/Response are Unknown in this case and it's bad! I solved it by introducing the SimpleApp type alias declaring standard App[Request, Response]. (It should be done the opposite way of course: App and CustomApp)

I agree it's should be deferred for 4.1+. Too much changes for 4.0 )

BTW, there was some (missed by mypy) errors reported by pyright. I think I'll try to fix some and submit a separate PR.

CaselIT commented 1 week ago

BTW, there was some (missed by mypy) errors reported by pyright. I think I'll try to fix some and submit a separate PR.

that would be great. Feel free to also open an issue when you do that. also if you know tox and github action we could add a gate with it too, so that it stays tested

vytas7 commented 1 week ago

I have just created an issue to track the Pyright gate: #2373.

And echoing @CaselIT, re PR, your help is really appreciated, because we are no huge experts of typing (OK, @CaselIT maybe is, but I am definitely not :sweat_smile:).

Edit: and as to typing extensions, I don't want Falcon to depend on any 3rd party runtime packages for the majority of use cases. We can add it as a conditional requirement for Python versions older than 3.X where they total less than Y% of PyPI downloads. Y being, say, 30% or so.

jkmnt commented 5 days ago

Another thing related to the typing: I noticed the middleware type is just the object. It's the user-facing interface so imho typing should be better.

I suggest something like

class MwWithReqHandler(Protocol):
    def process_request(self, req: Request, resp: Response) -> None: ...

class MwWithRespHandler(Protocol):
    def process_response(self, req: Request, resp: Response, resource: object, req_succeeded: bool) -> None: ...

class MwWithProcessHandler(Protocol):
    def process_resource(self, req: Request, resp: Response, resource: object, params: dict[str, Any]) -> None: ...

Middleware = Union[MwWithReqHandler, MwWithRespHandler, MwWithProcessHandler]

This is not perfect but better than nothing. The bad thing is that it's enough to have one conforming (with others nonconformin) methods to pass the type check. The good thing: the user may inherit it's middleware class from these protocols (kind of abc but w/o runtime overhead) and get full typesafety.

class MyMiddleware(MwWithReqHandler, MwWithRespHandler):
    def process_req(...)
    def process_resp(...)
CaselIT commented 4 days ago

I thought about it while doing that part, but if we type it as Middleware = Union[MwWithReqHandler, MwWithRespHandler, MwWithProcessHandler] won't that require that all 3 method are provided?

If not then that's for sure an option!

jkmnt commented 4 days ago

No afaik its enough to satisfy just one protocol of the union.

CaselIT commented 4 days ago

Seems like a good option then!

CaselIT commented 4 days ago

it probably makes sense to open a new issue with it. If you can work on a PR it would be great!

vytas7 commented 4 days ago

Yes, let's divide and conquer these typing improvements. Like improvement of the middleware protocols could be one issue/PR :slightly_smiling_face:

jkmnt commented 1 day ago

it probably makes sense to open a new issue with it. If you can work on a PR it would be great!

Ok, I'll draft the PR then.