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

[RFC] Allow overlap between static routes and wildcards #134

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

jodersky
Copy link
Member

@jodersky jodersky commented Jul 8, 2024

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("/:id")
def user(id: String) = s"user $id"

This is currently not allowed. With these changes, it would be allowed, and the static route settings would be preferred, with a fallback to the dynamic route user:

GET /settings => settings
GET /foo => user foo
GET /bar => user 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.

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.

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.
@jodersky jodersky requested a review from lihaoyi July 8, 2024 12:28
@lihaoyi
Copy link
Member

lihaoyi commented Jul 8, 2024

I think this looks reasonable. Let's give it a few days to see if anyone else has any views before merging it

@lihaoyi lihaoyi merged commit 080f2f3 into master Jul 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants