search

luislavena / radix

Radix Tree implementation for Crystal
MIT License
102 stars 14 forks source link

Inconsistency with lookups #24

Closed jwoertink closed 6 years ago

jwoertink commented 6 years ago

I feel like I posted this already, but maybe not since I can't find a closed issue on this.

Take this example

icr(0.24.1) > tree = Radix::Tree(Symbol).new
 => #<Radix::Tree(Symbol):0x101345f00 @root=#<Radix::Node(Symbol):0x101350f50 @payload=nil, @priority=0, @children=[], @kind=Normal, @key="", @placeholder=true>>
icr(0.24.1) > tree.add("/teams", :teams)
 => #<Radix::Node(Symbol):0x107f84f00 @payload=:teams, @priority=6, @children=[], @kind=Normal, @key="/teams", @placeholder=false>
icr(0.24.1) > tree.add("/teams/:team_id", :team)
 => [#<Radix::Node(Symbol):0x10a71feb0 @payload=:team, @priority=1, @children=[], @kind=Named, @key="/:team_id", @placeholder=false>]
icr(0.24.1) > tree.find("/teams").found?
 => true
icr(0.24.1) > tree.find("/teams/1").found?
 => true
icr(0.24.1) > tree.find("/teams.json").found?
 => false  # Why is this false, if the next one is true?
icr(0.24.1) > tree.find("/teams/1.json").found?
 => true   # Why is this true, if the last one was false?

It seems that when the route ends in a variable, you can add arbitrary stuff to the end, and it will still match. It makes sense because how would you know when to end it; however this could be potentially be dangerous tree.find("/teams/1%20select 1 + 1").found? # => true.

Though, maybe this is wanted for this low level. If so, then I guess higher level like Kemal would need to do a bit to adapt for web usage.

Thanks!

jwoertink commented 6 years ago

Ok, so after some investigation, this issue may just need to be closed. I took a look at a few other frameworks, and noticed the same behavior. For example, Sinatra works this same way. However, with sinatra, you can do an optional parameter which fixes the issue.

I guess the question becomes, what are your thoughts on optional params? Maybe I could just open up a new issue on that.

luislavena commented 3 years ago

Hello @jwoertink, just recently noticed you opened and closed this issue. Apologies for the delay on getting back on this subject. Right now, in your example, 1.json gets captured as :team_id.

I think the optional parameters (like Rails with /path(.:format) could be tricky to be added to Radix, as position characters will need to account for the displacement of () and also the . (dot) when performing the lookup.

Glob (*) and named options (:name) were a bit complicated already.

For the scenarios I'm using, I consider Radix to be pretty much feature-complete. Definitely is not perfect for handing all web routing, but perhaps that can be performed by a total different routing library.

Cheers.