luislavena/radix

Unexpected wildcard match

hmans opened this issue · 6 comments

hmans commented

This may be related to #23, but who knows... I am seeing some unexpected behavior when using wildcards (Radix 0.3.8 on Crystal 0.24.2):

require "radix"

tree = Radix::Tree(Symbol).new
tree.add "/app/*", :good

no_slash = tree.find "/app"
slash = tree.find "/app/"
subpath = tree.find "/app/foo"

# This one is the interesting one!
innocent = tree.find "/app.js"

# Just to make sure I'm using this correctly...
totally_innocent = tree.find "/foo"

puts "/app: #{no_slash.found? ? "found" : "not found"}"
puts "/app/: #{slash.found? ? "found" : "not found"}"
puts "/app/foo: #{subpath.found? ? "found" : "not found"}"
puts "/app.js: #{innocent.found? ? "found" : "not found"}"
puts "/foo: #{totally_innocent.found? ? "found" : "not found"}"

I'm seeing this output:

/app: found
/app/: found
/app/foo: found
/app.js: found
/foo: not found

Note that /app.js is reported as found, which I would not have expected, since the routing instruction uses a slash before the wildcard.

hmans commented

For a bit of added context: I'm building a small app with Kemal and I'm seeing some strange behavior around wildcard routes, and I'm currently trying to pin these down somewhat. The issue described here is not the one I'm seeing in my app, but it may be related.

I don't think it's necessarily just wildcards. I think it's more of Radix not knowing about a . character. I ran in to the same issue on #24. Though, I guess the question is should it know about the. character as some sort of stop character?

What I've found is that if the tree isn't complete then there is some routing oddities.
The first time I encountered this was with tbrand/router.cr#21 (includes reproducible examples)

Then I encountered a similar issue in another project: acaprojects/meraki-scanner@c769f57

The second issue was caused by having the following routes:

/GET/meraki
/GET/meraki/:id
/POST/meraki

I was getting 404s on /GET/meraki/:id (oddly my tests passed however it failed in production, possibly due to the lack of or inclusion of a trailing slash?)
I solved the issue by adding a root route. /GET/

So the tree now looks like this and things work as expected

/GET/
/GET/:id
/GET/meraki
/GET/meraki/:id
/POST/meraki

Hopefully the above can help shed some light on the issue.

Hello @hmans, @stakach and @jwoertink , sorry for the delayed response on this, been extremely busy (moving countries and all related stuff, so much fun) 🚀✈️ 🎉

Thanks to @silasb contribution, I believe recent v0.3.9 release fixed the issue reported here, using your same example script:

/app: found
/app/: found
/app/foo: found
/app.js: not found
/foo: not found

Please confirm if so and if not, then I can take a deeper look into the issue in the upcoming weeks.

Thank you for your patience and interest in Radix, and once again, apologies for the delayed response!
❤️ ❤️ ❤️

I just checked using master and got the same result. This doesn't really fix my original issue, but I think that's a whole separate deal, but it does fix this issue!

Thanks @luislavena and @silasb 🎉

Hello @hmans, thanks for your patience.

I recently corrected #23, so when run against master, it reports the following:

$ crystal run issue-25.cr
/app: found
/app/: found
/app/foo: found
/app.js: not found
/foo: not found

The reason /app and /app/ are found is that trailing slashes are ignored.

@jwoertink, re: #24, will respond there.

Thank you all for your patience, I'm closing this based on changes done in #30.

Feel free to comment and let me know otherwise.

Cheers.