Decode path parameters and encode URLRouter paths. #35
Conversation
There are two changes in this MR that work together: * Remove logic to abort routing if a path segment contains a path separator, e.g. '%2F'. * URL path parameters and decoded before being passed to route handlers. * To still permit matching URL routes that are not URL encoded, e.g. '/foo bar', encode these paths before adding them to route matching. See: vibe-d#34
|
Does this still work for matching When going this route, as opposed to the replacement character approach, AFAICS we'd have to insert a kind of encoding normalization step for |
Just for clarification, inserting routes is a 1-time operation, thus any memory allocation done during the normalization of a route would only happen a single time but not during request handling, which happens in much higher volume throughout the lifetime of the program. As I dig in further, should avoiding memory allocation when adding a route be considered a priority or is it a 1-time small cost that can be safely ignored? |
|
I added some changes that, without memory allocation, decode individual characters that are encoded and refer to a non-reserved character. Essentially both routing paths and request paths are converted into the same form, URL-encoded with only reserved-characters being encoded. |
|
This is a nice approach and the allocations during route adding can indeed be ignored in terms of performance (rebuilding the match graph will be the far bigger issue, in case somebody actually dynamically adds routes while the server is running). Now the only issues appears to be to get the semantics fully nailed down:
|
To clarify, An
The current code in match uses the code above, namely |
You are right, I skimmed over the code a bit too quickly and mistook the list of special characters to be the reserved characters instead of the unreserved ones. It should be noted that since
I meant the case of |
I got a bit of time today to take another look. Indeed, if the interface allows The current code assumes Posix paths and has this problem. The original unmodified code for adding a path is It can be made more consistent by changing the interface to always assume that mach will be given an Inet path string, but this will break compatibility with anyone who was doing things like having spaces in the path given to |
|
I added changes to change the router path to be
Now, the code can route and match paths with URL-escaped characters, and include path parameters that have such characters as well. While looking through the code, I had the following observation:
The code I have here goes down the pathway of matching URL-encoded paths, thus this requires making sure URL-encoding is applied consistently, e.g. only URL-encoding reserved characters. I believe this path achieves the end-goal with as few changes as possible. The alternative pathway would be to only accept InetPath format paths in the router, and decode them when adding them to the match-graph. But this would require decoding the request paths as well. Because a decoded path may contain additional |
|
Doing some local testing, another issue was discovered. There mere existence of routes added by calling A snippet of the relevant stack trace: |
Seems to be a red-herring. In my local setup, the |
This is indeed the case, but requests will fall-through to the next matching route if no a response is written and no exception is thrown.
We can't afford to make any changes to behavior that was previously accepted, as that would mean a high risk of breaking production code. But I think we are actually fine here, except for the That would URL decode each segment of the incoming |
vnayar
force-pushed
the
vibe-http-34_decode-path-params-encode-router-paths
branch
from
37f8fbd
to
886fffa
Compare
Clever solution, I was worried that things had worked themselves into a corner. The solution worked with only minor modifications. The |
There are two changes in this MR that work together:
See: vibe-d/vibe-core#396
See: #34