feat: add wildcard path route - #586
Conversation
github-actions
Bot
added
A-docs
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ElijahAhianyo
left a comment
ElijahAhianyo
left a comment
There was a problem hiding this comment.
@dharshan-0 Thanks a lot for your contribution. This is a great start! Let's address the comments, and it's good to merge.
|
@ElijahAhianyo Thanks for your comments, I will look into it. |
…nto wildcard-feature pulled remote changes
Co-authored-by: EBADF <elijahahianyo@gmail.com>
ElijahAhianyo
left a comment
ElijahAhianyo
left a comment
There was a problem hiding this comment.
@dharshan-0 Thanks for your contribution once again!
seqre
left a comment
seqre
left a comment
There was a problem hiding this comment.
Overall, it's a great job, thank you for your contribution!
I see one possible clash with overlapping routes, eg. /foo/{*path} and /foo/bar. To be honest, I'm not sure which route the framework would use atm. I think that it should first use the qualified path if it exists, and if it doesn't, then match the wildcard one.
I'd like for you to add a test(s) checking that functionality (also with different ordering of route definition in Router::with_urls) and if it does not work like that, to adapt the code. Also, mention that in the documentation, please.
Once that's done, I'll be happy to approve!
|
Thats a valid point, I will look into that |
| pub(crate) fn compare_weights(a: &[u8], b: &[u8]) -> std::cmp::Ordering { | ||
| let max_len = std::cmp::max(a.len(), b.len()); | ||
| let mut score: isize = 0; | ||
| let mut lexical_order_a = String::with_capacity(a.len()); | ||
| let mut lexical_order_b = String::with_capacity(b.len()); | ||
|
|
||
| for i in 0..max_len { | ||
| let (wa, wb) = match (a.get(i), b.get(i)) { | ||
| (None, _) => return std::cmp::Ordering::Less, | ||
| (_, None) => return std::cmp::Ordering::Greater, | ||
| (Some(&w), Some(&v)) => (w, v), | ||
| }; | ||
| lexical_order_a.push(char::from_digit(u32::from(wa), 10).unwrap()); | ||
| lexical_order_b.push(char::from_digit(u32::from(wb), 10).unwrap()); | ||
|
|
||
| if wa == 2 && wb == 2 { | ||
| return lexical_order_a.cmp(&lexical_order_b); | ||
| } | ||
| if wa == 2 { | ||
| return std::cmp::Ordering::Greater; | ||
| } | ||
| if wb == 2 { | ||
| return std::cmp::Ordering::Less; | ||
| } | ||
| if wa != wb { | ||
| if wa < wb { | ||
| score += 1; | ||
| } else { | ||
| score -= 1; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if score != 0 { | ||
| return if score > 0 { | ||
| std::cmp::Ordering::Less | ||
| } else { | ||
| std::cmp::Ordering::Greater | ||
| }; | ||
| } | ||
|
|
||
| lexical_order_a.cmp(&lexical_order_b) |
There was a problem hiding this comment.
Why this whole logic is here? Why can't you just compare the weights vectors?
There was a problem hiding this comment.
A direct comparison of the weight vectors is purely lexicographic it only considers the first segment where the routes differ and ignores everything after that.
For example:
/1 / 2 / {*rest}
/1 /{id}/ 3 / 4 / 5
The first difference is 2 (static) vs {id} (parameter), so a lexicographic comparison immediately ranks the first route ahead of the second. However, the second route is actually more specific overall.
Let me know, if Iam wrong @seqre .
There was a problem hiding this comment.
Hey @dharshan-0, sorry for the slow turnaround on this.
Taking another look, I think we've hit the limits of storing routes in a Vec. It makes structural validation and conflict detection (like params vs. wildcards) too brittle to handle well, and that's really a limitation of the underlying data structure rather than your approach to it. I'm working on moving the router to a radix trie, which should let us catch these conflicts properly at a structural level instead of relying on priority ordering.
Given that, I think your wildcard implementation is solid up to the point before you started handling the case where params take precedence over wildcards. That part's good enough to merge as-is. I don't think we need to solve that precedence case in this PR (or possibly at all, once the trie is in place).
Could you revert the commits where you handled that precedence logic? Happy to review and merge right after.
Thanks for your patience on this one, and sorry again for sitting on it so long.
Co-authored-by: Marek Grzelak <git@seqre.dev>
3 participants
Related issue or discussion
Description
It closes #545 by adding wildcard routing feature.
It uses this
{*param_name}to define wildcard.Type of change
Checklist
just test-all)just clippy)cargo fmt)