get_route_path uses regex to remove the prefix

[sk-]

The logic to get the route path is using regex to remove the prefix. This had two potential issues. On one hand, this could produce unexpected results in case of using regex special characters like .,. On the other hand regexes are slow and could be replaced by removeprefix in Python 3.9+, and with a simple slicing in Python 3.8.

starlette/starlette/_utils.py

Lines 96 to 99 in 554f368

	def get_route_path(scope: Scope) -> str:
	root_path = scope.get("root_path", "")
	route_path = re.sub(r"^" + root_path, "", scope["path"])
	return route_path

I checked PR #2400 which introduced this function, and also https://fastapi.tiangolo.com/advanced/behind-a-proxy/#setting-the-root_path-in-the-fastapi-app, but nowhere it's mentioned that the root_path could be a regex.

I have already tested the change and there is some small improvement (~1-2%) in the number of RPS.

[shalabhc]

I'm guessing the regex nature of the mount path is accidental, not intentional. I think the root_path should be escaped as follows before being used in a regex match:

     root_path = re.escape(scope.get("root_path", ""))

The Mount("/path_here", ...) should follow the same format as a regular Route("/path_here", ...) and not be regex based. Currently this can be used with a user provided path and this completely breaks in case a URL with special regex chars is supplied. For instance if you define Mount("/{user_name:str}", ... and someone types in http://example.org/***/ in the browser.

I have a fix and I can send a PR based on the outcome of this discussion: master...shalabhc:starlette:shalabhc/fix-root-path-matching

[Kludex]

Can you send this? I'll check in some days.

[sk-]

Don't think that is the right fix as it's still using regexes. See 8fbb9f6#r145325740

[sk-]

Would be nice to hear from maintainers confirming what the logic should be so that PRs could be sent out for review.

[Kludex]

It looks like what you are saying makes sense.

There were so many wrongly issues about root path, that my brain just ignored when I see the words, sorry.

[adriangb]

Yep +1

[Kludex]

I'm going to merge this: #2600.

Can we still use removeprefix?

[sk-]

@Kludex I don't see how that PR solves the OP nor the issue raised by @shalabhc.

I think we should stop using regexes to strip the root prefix.

[shalabhc]

I agree regex substitution is not needed. I don't think perf will be a big issue but the simplest fix would be to just use slicing to remove the prefix as in the 3.8 solution here: 8fbb9f6#r145325740
It will work in all Python 3.8+. If 3.8 support is not needed then we can just do removeprefix().

[Kludex]

@Kludex I don't see how that PR solves the OP nor the issue raised by @shalabhc.

I think we should stop using regexes to strip the root prefix.

It doesn't... But that PR solves another issue.

[trim21]

should we close this？

[Kludex]

Closed. Thanks.