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.

Sign up for GitHub

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

Added route value to scope #804

Closed
wants to merge 4 commits into from
Closed

Added route value to scope #804

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b7d2d5
Added route value to scope
victoraugustolls Jan 16, 2020
5dc3170
Added matched routes to scope
victoraugustolls Jan 29, 2020
61f0f72
Updating routing format
victoraugustolls Feb 24, 2020
fd359c1
Merge branch 'master' into feature/route_value
victoraugustolls Sep 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion starlette/routing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,10 @@ def matches(self, scope: Scope) -> typing.Tuple[Match, Scope]:
matched_params[key] = self.param_convertors[key].convert(value)
path_params = dict(scope.get("path_params", {}))
path_params.update(matched_params)
child_scope = {"endpoint": self.endpoint, "path_params": path_params}
child_scope = {
"endpoint": self.endpoint,
"path_params": path_params,
}
if self.methods and scope["method"] not in self.methods:
return Match.PARTIAL, child_scope
else:
Expand Down Expand Up @@ -557,17 +560,20 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:

partial = None

scope["routes"] = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, perhaps I'm misunderstanding the aim of this — assuming we want routes to contain all BaseRoute items (mounts, routes) that have matched while going through the routing system, it seems that currently it will only ever contain a single item, i.e. the first one that matched? In case of a route under a Mount-ed app, it will be the Mount (even if the mounted app is actually a Starlette app perhaps with its own router), and in case of a regular route then it will be that Route. Correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basing my judgement off #804 (comment) here - Not entirely sure that's something that's actually possible at all, given the ASGI-interoperable design aspects of Starlette.

For example, if you have…

async def other_non_starlette_app(scope, receive, send):
    ...

app = Starlette(routes=[Mount("/subapp", other_non_starlette_app)])

Then in this example context using scope["routes"][-1] as a way to get the "route that actually responds to the request" might be a bit confusing — it's only guaranteed to be the last route within this app that handles the request at least partly.

Don't know if we want to play on words here, or if for the use case you're describing (manipulating route information for telemetry purposes) having partial route info in some cases and marking it as a limitation would be enough?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that currently it will only ever contain a single item, i.e. the first one that matched?

Yes, you are right! I didn't proceed with the PR because wanted to discuss the best strategy with the maintainers before and wasn't sure this was it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure that's something that's actually possible at all, given the ASGI-interoperable design aspects of Starlette.

An example would be subapps on a FastAPI app? If so, I posted some code on a comment here that works by iterating the routes and searching for a match, but I don't think the problem you described would happen with FastAPI :/

for route in self.routes:
# Determine if any route matches the incoming scope,
# and hand over to the matching route if found.
match, child_scope = route.matches(scope)
if match == Match.FULL:
scope.update(child_scope)
scope["routes"].append(route)
await route.handle(scope, receive, send)
return
elif match == Match.PARTIAL and partial is None:
partial = route
partial_scope = child_scope
scope["routes"].append(route)

if partial is not None:
#  Handle partial matches. These are cases where an endpoint is
Expand Down