Multi-parametric route params problem
ivan-tymoshenko opened this issue · 15 comments
I will just try to generalize this issue #235. Sorry, it seems to be too long, I just tried to explain it clearly.
Problem: It's not clear where the param ends in the multiparametric node.
Why parametric routes are good:
If it's just a parametric route - it is always placed between two slashes and if you have a slash inside your param it will not parse. It's ok because you shouldn't have slashes as a part of param. That's why we can just parse the data between two slashes and check it for regex if it's needed. That's it.
Example:
findMyWay.on('GET', '/foo/:param/bar', () => {})
t.same(findMyWay.find('GET', '/foo/hello/bar').params, { param: 'hello' }) // it's ok
t.same(findMyWay.find('GET', '/foo/h-e-l-l-o/bar').params, { param: 'h-e-l-l-o' }) // it's ok, why not
t.same(findMyWay.find('GET', '/foo/he/llo/bar').params, { param: 'he/llo' }) // it will fail, you can't use a "/" as a part of paramWhy multi-parametric routes are bad:
If we define plenty of params or just one param and a static part between two slashes, we need to know how to separate them. Now we use "." and "-" symbols for marking that parameter is over and we use any symbol to define where a regex parameter is over.
Example:
findMyWay.on('GET', '/foo/:param1-hello/bar', () => {})
findMyWay.on('GET', '/foo/:param1-:param2/bar', () => {})
findMyWay.on('GET', '/foo/:param1.:param2/bar', () => {})
findMyWay.on('GET', '/at/:hour(^\\d+)h:minute(^\\d+)m', () => {})The problem is that these characters ("-", ".") can be inside the parameter value. That's why we don't know where we should stop parameter parsing.
findMyWay.on('GET', '/foo/:param1-:param2/bar', () => {})
t.same(findMyWay.find('GET', '/foo/hello1-hello2-hello3/bar').params, { param1: '???', param2: '???' }) It might be { param1: 'hello1', param2: 'hello2-hello3' } or { param1: 'hello1-hello2', param2: 'hello3' } or { param1: 'hello1', param2: 'hello2' } and route will failed.
findMyWay.on('GET', '/foo/:param1-static/bar', () => {})
t.same(findMyWay.find('GET', '/foo/hello1-hello2-static/bar').params, { param1: '???' }) Should this work or fail? It's not clear.
Multi-parametrical regex routes
At first, it seems that it's much easier with a regex route. We just parse the param while it's matching for regex. But it also has the same problem.
findMyWay.on('GET', '/test/:id(^\\d+)42', () => {})
t.same(findMyWay.find('GET', '/test/1242').params, { id: '12' }) // this route just failsSome people might think that this should work, but it doesn't. If it were just a parametric route we would definitely know that we should stop on the "/" symbol whatever it is.
findMyWay.on('GET', '/test/:id(^\\d+$)-static', () => {})
t.same(findMyWay.find('GET', '/test/12-static').params, { id: '12' })It also will not work, because of the "$" symbol that marks the end of the string. But it works inside the non-multiparam route because we know where the param ends and can slice it first and then check for regex.
And there can be a lot to such examples.
Summary:
We can't just make it work. We can choose one way of how multi-params should be parsed and there still would be some obvious cases that will not work or we can deprecate multi-parametrical routes which seems impossible as I understand.
@delvedor @mcollina @Eomm @RafaelGSS any thoughts?
It might be { param1: 'hello1', param2: 'hello2-hello3' } or { param1: 'hello1-hello2', param2: 'hello3' } or { param1: 'hello1', param2: 'hello2' } and route will failed.
For me: { param1: 'hello1', param2: 'hello2-hello3' }
t.same(findMyWay.find('GET', '/foo/hello1-hello2-static/bar').params, { param1: '???' })
For me: { param: 'hello1-hello2' }
Actually, I feel that would be a bit hard to support those multi-parametric routes without any performance regressions.
@RafaelGSS Thank you for your comments.
Can you explain how your suggestion should work? From the first example, I understand that you decided to split params by the first match of the '-' symbol and then ignore all other '-' and '.' symbols. But in the second example, you decided to skip the first '-' symbol in order to match the whole URL.
I'm not talking about performance at all. Can you please describe how this algorithm should work to receive parameters as you described?
t.same(findMyWay.find('GET', '/foo/he/llo/bar').params, { param: 'he/llo' }) // it will fail, you can't use a "/" as a part of param
Parameters must be escaped, so it must be find('GET', '/foo/he%2Fllo/bar').
we can deprecate multi-parametrical routes which seems impossible as I understand.
Why?
I think here there is a lack of documentation that stands:
I would not deprecate this feature: it is useful to cover some (not so good) patterns used by legacy frameworks. Let's call it an advanced feature: every user has a delete button hidden somewhere.
I think this feature could be optimized a bit but the developer should think about the application and if the input has dots or hyphens the multi-parametric is a clear bad idea:
the developer should write a simple parameter or wildcard and then write some old and good string.indexOf().
So, I think that trying to manage all the valid use cases you mentioned is an endless work and I would invest more energy by explaining to devs how to make good choices.
@Eomm I agree with you on most points. Supporting all cases seems impossible in that situation. What I want to do is define "common sense". Just a simple rule on how we will work with such cases. It can't be perfect, but it will exist and will be written in the documentation. So when issues like this #235 appear, we could definitely say is a bug or not.
P.S. I'm trying to optimize some parts of the code and I can't do it without knowing how it should work.
Here is my rule of thumb: if it's covered by tests, that's behavior. If it's not covered by tests, it's either a new feature or a bug. We could codify some of this unsupported behavior with a few assertions and that'd be ok.
Here is my rule of thumb: if it's covered by tests, that's behavior. If it's not covered by tests, it's either a new feature or a bug. We could codify some of this unsupported behavior with a few assertions and that'd be ok.
I don't wanna break current behavior. Can you suggest how a new version with adjustments should work?
If the tests pass, you are not breaking current behavior.
Yeah, I know, but we don't have tests that cover cases that we discuss here. And the problem is that we can fix only some of them. So we need to decide what behavior should be. Let's start with this issue #235. Should it work how the author described it?
I think it should work as you would feel it's good to work. Once there is a PR to implement those edge cases we can evaluate it more closely. It's hard to know just by looking at the issues.
I'm working on that. It's PR #237 where it will be released. But since there can't be one right way to do it, I decided to create this issue and ask you about how would you want this to work. Am I doing this in the wrong way?
At this point I think you have more knowledge on the topic than possibly anyone on this team.
Neither @delvedor nor myself use multi-parametric params and
As a rule I would loosely follow whatever https://github.com/pillarjs/path-to-regexp does. We can differentiate but it's better we follow that standard because it's the most widespread.
This is what I was looking for. Thank you very much. I will look deeply into how it is implemented. In case it's working as we expected, can we use it as a dependency for multiparametric routes? Just convert all that we have in the multiparametric route into one regex.
Update: Seems it's a solution, that can fix all issues that we have here.
That module creates regexps that must be all tested (like express does). find-my-way creates a radix trie. I don't think we can just convert that straight away.
Thanks for bearing with us, it took quite a bit of time to understand and fix this!