[RFC] Allow overlap between static routes and wildcards#134
Merged
Conversation
This changes the DispatchTrie to allow overlapping wildcard segments
in paths with static ones, with a preference for the latter.
For example, consider the following routes:
```
@cask.get("/settings")
def settings() = "settings"
@cask.get("/:user")
def user(id: String) = id
```
This is currently not allowed. With these changes, it would be
allowed, and the static route `settings` would be preffered, with a
fallback to the dynamic route `user`:
```
GET /settings => settings
GET /foo => foo
GET /bar => bar
```
---
The reason I'm proposing this change is mostly for use in HTML
applications (i.e. not RPC-style JSON APIs). In this scenario, short
URLs are useful, since users may type them directly and associate
meaning to them.
Consider for example the way GitHub structures URLs. If github were
written with cask's current routing logic, it would not be possible to
have URLs such as `/settings` and `/com-lihaoyi`, and instead some
namespacing would need to be introduced (e.g. /orgs/com-lihaoyi/) to
separate these, which might not actually be relevant for users.
Member
|
I think this looks reasonable. Let's give it a few days to see if anyone else has any views before merging it |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This changes the DispatchTrie to allow overlapping wildcard segments in paths with static ones, with a preference for the latter.
For example, consider the following routes:
This is currently not allowed. With these changes, it would be allowed, and the static route
settingswould be preferred, with a fallback to the dynamic routeuser:The reason I'm proposing this change is mostly for use in HTML applications (i.e. not RPC-style JSON APIs). In this scenario, short URLs are useful, since users may type them directly and associate meaning to them.
Consider for example the way GitHub structures URLs. If github were written with cask's current routing logic, it would not be possible to have URLs such as
/settingsand/com-lihaoyi, and instead some namespacing would need to be introduced (e.g./orgs/com-lihaoyi) to separate these, which might not actually be relevant for users.Of course, with these changes we will no longer catch developer errors that accidentally define wildcard-overlapping routes with non-wildcard ones. It will also be up to the application developer to make sure that there aren't any accidental overlaps between valid values of wildcards and static routes (e.g. in the example above, the application developer would somehow need to make sure that there isn't a user called "settings" in their system).
Given these drawbacks, I'd like to hear your thoughts on this in general. Personally I think that it's useful to enforce non-overlaps for API purposes, because this forces you to design a more robust url scheme. However, for HTML application purposes, I think that allowing shorter URLs is useful and probably outweighs the current limitations. Maybe we could also come up with a way to make this configurable.