luislavena/radix

Mismatched route

crisward opened this issue · 9 comments

I'm using kemal, and I've discovered a bug which seems to come from radix.

The below should output three, but is not found. However if I change contact to anything which doesn't being with a c or a o it works as expected. Any ideas?

require "radix"

tree = Radix::Tree(Symbol).new
tree.add "/get/", :one
tree.add "/head/", :two
tree.add "/get/*", :three
tree.add "/head/*", :four
tree.add "/get/open", :five
tree.add "/head/open", :six
tree.add "/get/open/*", :seven
tree.add "/head/open/*", :eight
tree.add "/get/close", :nine
tree.add "/head/close", :ten
tree.add "/get/close/*", :eleven
tree.add "/head/close/*", :twelve

result = tree.find "/get/hello"

if result.found?
  puts result.payload 
else
  puts "not found"
end

This works for me using Radix 0.3.4

icr(0.19.4) > result.found?
 => true
icr(0.19.4) > result.payload 
 => :three
icr(0.19.4) > tree.root.key
 => "/"
icr(0.19.4) > tree.root.children.size
 => 2
icr(0.19.4) > tree.root.children[0].key
 => "head/"
icr(0.19.4) > tree.root.children[1].key
 => "get/"
icr(0.19.4) > tree.root.children[1].children.size
 => 3
icr(0.19.4) > tree.root.children[1].children[0].key
 => "close"
icr(0.19.4) > tree.root.children[1].children[1].key
 => "open"
icr(0.19.4) > tree.root.children[1].children[2].key
 => "*"

Make sure you pull from Kemal on master in your shards.yml. It has the latest radix in it.

Hello @crisward, thank you for your report.

I was unable to reproduce this with latest version 0.3.4, released a few days ago:

p Radix::VERSION # => 0.3.4

result = tree.find "/get/hello"

if result.found?
  puts result.payload # => three
  puts result.params  # => {"" => "hello"}
else
  puts "not found"
end

Please check if possible on your end to update your Kemal dependency so latest Radix is available.

Cheers.

Thanks for you help on this.

I just read through my explanation, possibly wasn't very good.
I'm using sdogruyol/kemal master - radix 0.3.4. The below test I ran on radix 0.3.4 - Crystal 0.19.4 (2016-10-21). Mac Sierra, 10.12.1. I also tried deleting radix out of my deps, the doing another crystal deps update to make sure I had the correct version. I also double checked the source code in the lib folder incase this is an issue with crystal shards.

# radix-test.cr
require "radix"

tree = Radix::Tree(Symbol).new
tree.add "/get/", :one
tree.add "/head/", :two
tree.add "/get/*", :three
tree.add "/head/*", :four
tree.add "/get/open", :five
tree.add "/head/open", :six
tree.add "/get/open/*", :seven
tree.add "/head/open/*", :eight
tree.add "/get/close", :nine
tree.add "/head/close", :ten
tree.add "/get/close/*", :eleven
tree.add "/head/close/*", :twelve

result = tree.find "/get/ola" # this doesn't work, I get not found

if result.found?
  puts result.payload
else
  puts "not found"
end

result = tree.find "/get/hello" # this works fine, I get 'three'
if result.found?
  puts result.payload
else
  puts "not found"
end

Then I ran, and got the below output

$ crystal radix-test.cr 
not found
three

If you still can't recreate this, let me know and I'll fork and add a failing test, I could even try to get it to pass if I'm feeling brave...

Ah, that makes more sense @crisward. Sorry didn't understood your problem from the initial description 😄

Will take a look later today and get back to you.

Thank you for your patience.

It's ok, I wrote it last night at 11:30pm, been up since 6am, so my descriptive powers had deserted me. Thanks again for your help.

No worries.

Got a spec that reproduces the issue:

it "does prefer catch all over specific key with partially shared key" do
  tree = Tree(Symbol).new
  tree.add "/orders/*anything", :orders_catch_all
  tree.add "/orders/closed", :closed_orders

  result = tree.find("/orders/cancelled")
  result.found?.should be_true
  result.key.should eq("/orders/*anything")
end

Need a bit more of investigation since this might impact other lookup paths.

Cheers.

Hello @crisward, thank you for your report and your patience.

I've released a new version that corrects this issue.

Cheers!

Cool, thanks for you work on this. I'll check if kemal has updated and perhaps nudge / do a pull request for the latest version.

He's already done it... that was quick!