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

Allow colons in routes again #1657

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 27 additions & 9 deletions starlette/routing.py
Expand Up @@ -108,34 +108,47 @@ def replace_params(


def compile_path(
path: str,
pattern: str,
Copy link
Member

Choose a reason for hiding this comment

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

Question, isn't this still considered backwards-incompatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh. I guess technically you are correct. I didn't think about keyword argument usage.
We are also on a 0.x version. So we could do breaking changes in 0.21 I guess.
How about defining an alias for now. Then we can also get rid of the inprecise compile_path name internally.

E.g. rename into def compile_pattern(pattern: str) and def compile_path(path: str): return compile_pattern(path) for backwards compatibility.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We try to avoid breaking changes as much as possible. I also don't see much benefit on the renaming here. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Indeed adding a parameter is a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho the method should not have been used for compiling host patterns in the first place, because a hostname is not a path.
The least we can do now is apply proper naming and documentation.

The renaming would allow keeping backward-compatibility for the old interface, while implementing a better on. Personally I wouldn’t regard this function as part of the public starlette api currently, because it is not documented anywhere and I would keep it that way. So the new function name would be _compile_pattern I guess.

Having said all that, I don’t feel too strongly about any of this. Just thought it would be an improvement to the code base. If you don’t care about correct naming, I can simply revert that part.

Copy link
Member

Choose a reason for hiding this comment

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

yes I think this has come up a few times before, but the priority is to keep backwards-compatibility until there's a major release like from 1.x.x to 2.x.x

Copy link
Member

Choose a reason for hiding this comment

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

The only case when this would break is if people do compile_path(path=...).

It's possible to go for a deprecation, allowing this usage at runtime while keeping the new signature for type hints, with a decorator...

def _allow_deprecated_path_kwarg(func: Callable) -> Callable:
    @wraps(func)
    def wrapped(*args: Any, **kwargs: Any):
        if "path" in kwargs:
            ...  # Raise a deprecation warning
            return func(kwargs["path"])
        return func(*args, **kwargs)

    return wrapped

@_allow_deprecated_path_kwarg
def compile_path(pattern: str):
    ...

Edit: But not really convinced either this is something we should do, especially in this bugfix PR (see #1657 (comment)).

) -> typing.Tuple[typing.Pattern, str, typing.Dict[str, Convertor]]:
"""
Given a path string, like: "/{username:str}", return a three-tuple
Can compile hostname patterns as well as path patterns. When the patterns
starts with a slash it is regarded as a path, otherwise as a hostname.

When given a path pattern, like: "/{username:str}", return a three-tuple
of (regex, format, {param_name:convertor}).

regex: "/(?P<username>[^/]+)"
format: "/{username}"
convertors: {"username": StringConvertor()}

When given a hostname pattern, like: "{subdomain:str}.mydomain.tld:8080",
return a three-tuple of (regex, format, {param_name:convertor}).

regex: "(?P<subdomain>[^/]+).mydomain.tld"
format: "{subdomain}.mydomain.tld:8080"
convertors: {"subdomain": StringConvertor()}

The regex is used for parsing URIs, while the format is used to generate URIs.
"""
is_host: bool = not pattern.startswith("/")
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
is_host: bool = not pattern.startswith("/")
is_host = not pattern.startswith("/")

path_regex = "^"
path_format = ""
duplicated_params = set()

idx = 0
param_convertors = {}
for match in PARAM_REGEX.finditer(path):
for match in PARAM_REGEX.finditer(pattern):
param_name, convertor_type = match.groups("str")
convertor_type = convertor_type.lstrip(":")
assert (
convertor_type in CONVERTOR_TYPES
), f"Unknown path convertor '{convertor_type}'"
), f"Unknown param convertor '{convertor_type}'"
convertor = CONVERTOR_TYPES[convertor_type]

path_regex += re.escape(path[idx : match.start()])
path_regex += re.escape(pattern[idx : match.start()])
path_regex += f"(?P<{param_name}>{convertor.regex})"

path_format += path[idx : match.start()]
path_format += pattern[idx : match.start()]
path_format += "{%s}" % param_name

if param_name in param_convertors:
Expand All @@ -148,10 +161,14 @@ def compile_path(
if duplicated_params:
names = ", ".join(sorted(duplicated_params))
ending = "s" if len(duplicated_params) > 1 else ""
raise ValueError(f"Duplicated param name{ending} {names} at path {path}")
input_type = "host" if is_host else "path"
raise ValueError(
f"Duplicated param name{ending} {names} at {input_type} pattern {pattern}"
)

path_regex += re.escape(path[idx:].split(":")[0]) + "$"
path_format += path[idx:]
tail: str = pattern[idx:].split(":")[0] if is_host else pattern[idx:]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
tail: str = pattern[idx:].split(":")[0] if is_host else pattern[idx:]
tail = pattern[idx:].split(":")[0] if is_host else pattern[idx:]

path_regex += re.escape(tail) + "$"
path_format += pattern[idx:]

return re.compile(path_regex), path_format, param_convertors

Expand Down Expand Up @@ -429,6 +446,7 @@ class Host(BaseRoute):
def __init__(
self, host: str, app: ASGIApp, name: typing.Optional[str] = None
) -> None:
assert not host.startswith("/"), "Routed hosts must not start with '/'"
self.host = host
self.app = app
self.name = name
Expand Down
16 changes: 14 additions & 2 deletions tests/test_routing.py
Expand Up @@ -18,6 +18,11 @@ def users(request):
return Response("All users", media_type="text/plain")


def disable_user(request):
content = "User " + request.path_params["username"] + " disabled"
return Response(content, media_type="text/plain")


def user(request):
content = "User " + request.path_params["username"]
return Response(content, media_type="text/plain")
Expand Down Expand Up @@ -108,6 +113,7 @@ async def websocket_params(session: WebSocket):
routes=[
Route("/", endpoint=users),
Route("/me", endpoint=user_me),
Route("/{username}:disable", endpoint=disable_user, methods=["PUT"]),
Route("/{username}", endpoint=user),
Route("/nomatch", endpoint=user_no_match),
],
Expand Down Expand Up @@ -189,6 +195,11 @@ def test_router(client):
assert response.url == "http://testserver/users/tomchristie"
assert response.text == "User tomchristie"

response = client.put("/users/tomchristie:disable")
assert response.status_code == 200
assert response.url == "http://testserver/users/tomchristie:disable"
assert response.text == "User tomchristie disabled"

response = client.get("/users/nomatch")
assert response.status_code == 200
assert response.text == "User nomatch"
Expand Down Expand Up @@ -702,13 +713,14 @@ def test_partial_async_ws_endpoint(test_client_factory):
def test_duplicated_param_names():
with pytest.raises(
ValueError,
match="Duplicated param name id at path /{id}/{id}",
match="Duplicated param name id at path pattern /{id}/{id}",
):
Route("/{id}/{id}", user)

with pytest.raises(
ValueError,
match="Duplicated param names id, name at path /{id}/{name}/{id}/{name}",
match="Duplicated param names id, name"
" at path pattern /{id}/{name}/{id}/{name}",
):
Route("/{id}/{name}/{id}/{name}", user)

Expand Down