Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

query parameters oddness #293

Closed
rdp opened this issue Jan 17, 2017 · 12 comments
Closed

query parameters oddness #293

rdp opened this issue Jan 17, 2017 · 12 comments
Labels

Comments

@rdp
Copy link

rdp commented Jan 17, 2017

Hello. I noticed with this snippet:


require "kemal"

get "/new_tag_edit_list/:id" do |env|
  "hello1"
end

get "/new_tag_edit_list2" do |env|
  "hello2"
end

Kemal.run

it returns a 404 for this path:

http://127.0.0.1:3000/new_tag_edit_list2

FWIW. Seems to also affect POST's etc. as well.

Making them not "match" in prefix and everything works again...

@sdogruyol sdogruyol added the bug label Jan 17, 2017
@sdogruyol
Copy link
Member

This seems to be a bug in the router.

WDYT @luislavena ?

@airinfection
Copy link

Can you try for

get "/new_tag_edit_list2/" do |env|
"hello2"
end

Add / before " do

@luislavena
Copy link
Contributor

@sdogruyol looks like a bug in Radix:

tree = Radix::Tree(Symbol).new
tree.add "/new_tag_edit_list/:id", :one
tree.add "/new_tag_edit_list2", :two

result = tree.find "/new_tag_edit_list2"

# this should be `true`
p result.found? # => false

As a workaround for now I recommend changing the order on how the routes are defined.

Will try to look at this in detail this week.

Cheers.

@jasonl99
Copy link

@luislavena it's interesting. If you add p.result it indeed prints out a radix node.

tree = Radix::Tree(Symbol).new
tree.add "/new_tag_edit_list/:id", :one
tree.add "/new_tag_edit_list2", :two

result = tree.find "/new_tag_edit_list2"

# this should be `true`
p result.found? # => false
p result # #<Radix::Result(Symbol):0x55ccc1f68c80..,>

The radix repo shows an earlier problem in November that seems related: luislavena/radix#14

@luislavena
Copy link
Contributor

If you add p.result it indeed prints out a radix node.

Yes, because the nodes that were found are stacked up, but what is important is the value of found? which is determined by Radix found an associated payload or not.

The radix repo shows an earlier problem in November that seems related: luislavena/radix#14

The actual issue is Node#priority, both /:id and 2 child nodes have for the same priority (1) which place them at the same level, causing the lookup to fail.

Cheers.

luislavena added a commit to luislavena/radix that referenced this issue Jan 17, 2017
Given two similar keys, one short and one with named parameter, lookup
was incorretly picking up the named parameter version instead of the
specific one:

    tree = Radix::Tree(Symbol).new
    tree.add "/tag-edit/:id", :edit_tag
    tree.add "/tag-edit2", :alternate_edit_tag

    result = tree.find("/tag-edit2")
    result.found? # => false

The order of insertion (named before specific) was causing the
lookup mechanism to validate it before checking the other options.

With the changes present here, short or long keys will be affected
anymore by named or globbed keys present when sharing part of the
key.

Fixes kemalcr/kemal#293
luislavena added a commit to luislavena/radix that referenced this issue Jan 17, 2017
Given two similar keys, one short and one with named parameter, lookup
was incorrectly picking up the named parameter version instead of the
specific one:

    tree = Radix::Tree(Symbol).new
    tree.add "/tag-edit/:id", :edit_tag
    tree.add "/tag-edit2", :alternate_edit_tag

    result = tree.find("/tag-edit2")
    result.found? # => false

The order of insertion (named before specific) was causing the
lookup mechanism to validate it before checking the other options.

With the changes present here, short or long keys will be affected
anymore by named or globbed keys present when sharing part of the
key.

Fixes kemalcr/kemal#293
@luislavena
Copy link
Contributor

@rdp @sdogruyol just released v0.3.6 of Radix with the fix for this (see associated pull request for details).

Please try it and let me know if you encounter any issue.

Cheers.

sdogruyol added a commit that referenced this issue Jan 18, 2017
@sdogruyol
Copy link
Member

@luislavena @rdp fixed on master 👍

@rdp
Copy link
Author

rdp commented Jan 20, 2017

OK that issue was fixed thanks!

I do have one other small question, perhaps I should file a new issue do you think?

With the following snippet (unrelated) I seem unable to access http://localhost:3000/edit_tag_edit_list/3 (404) anybody know if this is expected?

get "/edit_tag/:tag_id" do |env|
end

get "/edit_tag_edit_list/:tag_id" do |env|
  "the bad one"
end

Thanks!

@luislavena
Copy link
Contributor

luislavena commented Jan 20, 2017

@rdp same issue, caused by insertion order, change the order (define longer path first) and should work:

Radix::VERSION # => "0.3.6"
/edit_tag (9)
         /:tag_id (-1)
         _edit_list/:tag_id (-1)

result.found? # => false

vs:

Radix::VERSION # => "0.3.6"
/edit_tag (9)
         _edit_list/:tag_id (-1)
         /:tag_id (-1)

result.found? # => true

@sdogruyol please open an issue on Radix (opened issue luislavena/radix#18), will try to work on that over the weekend (and also correct the Radix::VERSION mismatch 😞)

@luislavena
Copy link
Contributor

Hello @rdp @sdogruyol, I've pushed a new version of Radix (v0.3.7) that corrects the insertion issue reported here (entirely, finally).

If interested, you can read all the details in luislavena/radix#18 and solution at luislavena/radix#19.

Thank you for your report. ❤️ ❤️ ❤️

@sdogruyol
Copy link
Member

Thanks @luislavena 🙏

@rdp
Copy link
Author

rdp commented Feb 6, 2017

Awesome, thanks @luislavena and everybody, seems fixed now, perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants