Use Matchit to Resolve Per-File Settings #11111
Conversation
|
| { | ||
| Ok(_) => {} | ||
| Err(InsertError::Conflict { .. }) => {} | ||
| Err(_) => unreachable!("file paths are escaped before being inserted in the router"), |
There was a problem hiding this comment.
I was curious about this, and I think I convinced myself that this is correct because all other possible errors (aside from Conflict) can only occur as a result of characters that the router treats as special. And the escaping done above means that there should be precisely zero special characters in the path.
(It might make sense to add an escape routine to matchit. Similar to regex::escape. That way, you can provide more of a stronger guarantee here.)
There was a problem hiding this comment.
Yeah after escaping we guarantee that we're inserting a static route, so none of the other errors are possible (barring a bug in matchit). An escape function in matchit is a good idea. I was also considering optimizing the double String::replace but decided it wasn't worth it for us.
| self.settings.push(settings); | ||
|
|
||
| // normalize the path to use `/` separators and escape the '{' and '}' characters, | ||
| // which matchit uses for routing parameters |
There was a problem hiding this comment.
Are there any other characters that we need to watch out for here? In my previous PR I mentioned colons, which seems irrelevant, but e.g. {*foo} is a special routing parameter in matchit, right? I assume that's also irrelevant as long as we're escaping the { and } characters.
There was a problem hiding this comment.
* is only considered after a non-escaped {, so that shouldn't be a problem.
Summary
Continuation of #9444.
matchitsupports escaping routing characters so this change should now be fully compatible.Test Plan
On my machine I'm seeing a ~3% improvement with this change.