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
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
019857b
Allow colons in routes
bodograumann 5aef9ab
Make compile_path behaviour implicit but document it
bodograumann 25122e1
Merge branch 'master' into fix-allow-colon-in-route
Kludex 77975ca
Merge branch 'master' into fix-allow-colon-in-route
adriangb File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -108,34 +108,47 @@ def replace_params( | |||||
|
||||||
|
||||||
def compile_path( | ||||||
path: str, | ||||||
pattern: str, | ||||||
) -> 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("/") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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: | ||||||
|
@@ -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:] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
path_regex += re.escape(tail) + "$" | ||||||
path_format += pattern[idx:] | ||||||
|
||||||
return re.compile(path_regex), path_format, param_convertors | ||||||
|
||||||
|
@@ -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 | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
anddef compile_path(path: str): return compile_pattern(path)
for backwards compatibility.There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
Edit: But not really convinced either this is something we should do, especially in this bugfix PR (see #1657 (comment)).