Swap out julienschmidt/httprouter #605
Conversation
runtime/router/trie.go
Outdated
| for k < n && path[k] != '/' { | ||
| k++ | ||
| } | ||
| params = append(params, Param{t.key[oj:j], path[ok:k]}) |
There was a problem hiding this comment.
nit: use continue rather than more blocks
runtime/router/trie.go
Outdated
| if j < m { | ||
| // path matches up to node key's second to last character, | ||
| // the last char of node key is "*" and path is no shorter than longest matched prefix | ||
| if t.key[j:] == "*" && k < n { |
There was a problem hiding this comment.
Does registering /a* match /ab/c?
There was a problem hiding this comment.
zanzibar/runtime/router/trie.go
Lines 85 to 91 in 133a174
/a*
runtime/router/trie.go
Outdated
| // conflicts caused by ":" is only possible when j == m | ||
| if j == m { | ||
| for _, c := range t.children { | ||
| if c.key[0] == path[k] || c.key[0] == ':' || path[k] == ':' { |
There was a problem hiding this comment.
Shouldn't this logic be covered by c.get already?
There was a problem hiding this comment.
It is, mostly thinking of minimizing the call stack. Removed this check with the updated code.
runtime/router/trie.go
Outdated
| for _, c := range t.children { | ||
| if c.key[0] == path[k] || c.key[0] == ':' || path[k] == ':' { | ||
| if _, _, err := c.get(path[k:]); err == nil { | ||
| return errExist |
There was a problem hiding this comment.
You will probably want to print out the match that conflicted for diagnostic purposes.
runtime/router/router.go
Outdated
| // It implements a similar interface to the one in github.com/julienschmidt/httprouter, | ||
| // the main differences are: | ||
| // 1. this router does not treat "/a/:b" and "/a/b/c" as conflicts (https://github.com/julienschmidt/httprouter/issues/175) | ||
| // 2. this router does not treat "/a/:b" and "/a/:c" as different routes (https://github.com/julienschmidt/httprouter/issues/6) |
There was a problem hiding this comment.
nit: this router does not allow /a/:b and /a/:c to be registered at all.
| // and HTTP status code 405. | ||
| // If no other Method is allowed, the request is delegated to the NotFound | ||
| // handler. | ||
| HandleMethodNotAllowed bool |
There was a problem hiding this comment.
Do we need to support this? When this flag is false, then we return 404. When this flag is true, we return 405. What's wrong with always returning 405 and getting rid of this flag?
There was a problem hiding this comment.
The difference is that when both this flag and MethodNotAllowed is present, the request is handled by the MethodNotAllowed handler. We can totally get rid of this and always return 405, I don't feel strongly about it. How do you think?
| i++ | ||
| } | ||
|
|
||
| // Find the first character that differs between "path" and this node's key, if it exists. |
There was a problem hiding this comment.
This code is quite confusing. I am guessing you are trying to have not just O(n) on the length of key but that you only process each character of key and path once and allocate no additional memory other than the indices.
I think it may be much more readable if you split the string into path components and processed the equality of path components in sequence. Splitting the string into path components will roughly double the total memory used to register routes, but it's probably a better trade-off because routes are only registered once within the application.
There was a problem hiding this comment.
The trie does not necessarily split node on path segments, so splitting the path into segments and comparing equality doesn't work.
Register /ab/c
trie: [/ab/c]
Register /a
trie:
[/a]
|
[b/c]
|
Benchmark between zanzibar router and http router: |
This PR swaps out julienschmidt/httprouter with an inhouse router implementation that has similar interface to httprouter. The rationale is to overcome the limitations of julienschmidt/httprouter#6 and julienschmidt/httprouter#175.