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

feat: run dependencies even when no route matches #66

Open
adriangb opened this issue Feb 14, 2022 · 3 comments
Open

feat: run dependencies even when no route matches #66

adriangb opened this issue Feb 14, 2022 · 3 comments

Comments

@adriangb
Copy link
Owner

adriangb commented Feb 14, 2022

Currently the following test fails:

def test_app_dependency_no_route_match() -> None:
    log: List[str] = []

    async def dep() -> None:
        log.append("called")
    
    app = App(routes=[], dependencies=[Depends(dep)])

    client = TestClient(app=app)

    resp = client.get("/")
    assert resp.status_code == 404, resp.content
    assert log == ["called"]

The reason for this is that the dependency DAG is not executed until a route is found.
The same applies to FastAPI.

This is a bit confusing, and prevents implementing logging, tracing, etc. as a dependency (usually you'd want to log 404s).

I think it should be possible to change this: we'd run each App's and Rotuer's dependencies as the request traverses through them.
It'd be roughly the same as what we are doing in Operation:

xpresso_scope: asgi_scope_extension.XpressoASGIExtension = scope["extensions"][
"xpresso"
]
async with xpresso_scope["container"].enter_scope("endpoint") as container:
endpoint_return = await container.execute_async(
self.dependant,
values=values,
executor=self.executor,
)

Ideally this change would mean that anything you do with ASGI middleware you can do with dependencies (there's still a good reason to support both: there is generic ASGI middleware but not generic Xpresso dependencies).

The main con is that I expect this to have a relatively large impact on performance and make the implementation a lot messier.
These are two very good arguments for not supporting this usage.

@adriangb
Copy link
Owner Author

On a related note, this could maybe address the issues in https://github.com/tiangolo/fastapi/releases/tag/0.74.0 if we change our execution model somehow. I'll have to think about it a bit.

@adriangb
Copy link
Owner Author

adriangb commented Mar 27, 2022

I'm thinking what might work is for Router and Path to build their own dependency graph (all of the dependencies of the routing nodes traversed to get to them). Then if Router doesn't match any routes it can run its own dependency graph, if Path doesn't match any methods (i.e. if it returns a PartialMatch and then handle gets called) it can run its dependency graph and otherwise Operations dependency graph will be called. But this feels like "special casing", which I don't particularly like. But maybe those are the only cases, so it's okay. The other routing primitives that Starlette provides (Mount and Host) don't raise exceptions, they either match or they don't. And I haven't seen any 3rd party routing primitivities that might be incompatible.

@adriangb
Copy link
Owner Author

adriangb commented Apr 2, 2022

So after a bit of testing, it looks like #66 (comment) will work if we want it to, but there is a big con: we need to dive into the routing and HTTP method matching, which are currently things that Starlette just handles, and provides no real hooks into. So this would mean a lot more code in Xpresso 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant