Skip to content

Commit

Permalink
Lowers named and glob node priority to fix lookup issue
Browse files Browse the repository at this point in the history
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
  • Loading branch information
luislavena committed Jan 17, 2017
1 parent 40b8e26 commit 5a6811e
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 15 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ This project aims to comply with [Semantic Versioning](http://semver.org/),
so please check *Changed* and *Removed* notes before upgrading.

## [Unreleased]
### Fixed
- Correct lookup issue caused by similar priority between named paramter and
shared partial key [kemalcr/kemal#293](https://github.com/kemalcr/kemal/issues/293)

## [0.3.5] - 2016-11-24
### Fixed
Expand Down
12 changes: 6 additions & 6 deletions spec/radix/node_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,18 @@ module Radix

it "returns zero for catch all (globbed) key" do
node = Node(Nil).new("*filepath")
node.priority.should eq(0)
node.priority.should eq(-2)

node = Node(Nil).new("/src/*filepath")
node.priority.should eq(0)
node.priority.should eq(-2)
end

it "returns one for keys with named parameters" do
node = Node(Nil).new(":query")
node.priority.should eq(1)
node.priority.should eq(-1)

node = Node(Nil).new("/search/:query")
node.priority.should eq(1)
node.priority.should eq(-1)
end

it "changes when key changes" do
Expand All @@ -68,10 +68,10 @@ module Radix
node.priority.should eq(3)

node.key = "*filepath"
node.priority.should eq(0)
node.priority.should eq(-2)

node.key = ":query"
node.priority.should eq(1)
node.priority.should eq(-1)
end
end

Expand Down
22 changes: 22 additions & 0 deletions spec/radix/tree_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,28 @@ module Radix
result.params.has_key?("name").should be_true
result.params["name"].should eq("日本語")
end

it "does prefer specific path over named parameters one if both are present" do
tree = Tree(Symbol).new
tree.add "/tag-edit/:tag", :edit_tag
tree.add "/tag-edit2", :alternate_tag_edit

result = tree.find("/tag-edit2")
result.found?.should be_true
result.key.should eq("/tag-edit2")
end

it "does prefer named parameter over specific key with partially shared key" do
tree = Tree(Symbol).new
tree.add "/orders/:id", :specific_order
tree.add "/orders/closed", :closed_orders

result = tree.find("/orders/10")
result.found?.should be_true
result.key.should eq("/orders/:id")
result.params.has_key?("id").should be_true
result.params["id"].should eq("10")
end
end

context "dealing with multiple named parameters" do
Expand Down
18 changes: 9 additions & 9 deletions src/radix/node.cr
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ module Radix
# This value will be directly associated to the key size or special
# elements of it.
#
# * A catch all (globbed) key will receive lowest priority (`0`)
# * A named parameter key will receive priority above catch all (`1`)
# * A catch all (globbed) key will receive lowest priority (`-2`)
# * A named parameter key will receive priority above catch all (`-1`)
# * Any other type of key will receive priority based on its size.
#
# ```
Expand All @@ -49,10 +49,10 @@ module Radix
# # => 3
#
# Radix::Node(Nil).new("*filepath").priority
# # => 0
# # => -2
#
# Radix::Node(Nil).new(":query").priority
# # => 1
# # => -1
# ```
getter priority : Int32

Expand Down Expand Up @@ -113,9 +113,9 @@ module Radix
while reader.has_next?
case reader.current_char
when '*'
return 0
return -2
when ':'
return 1
return -1
else
reader.next_char
end
Expand All @@ -130,14 +130,14 @@ module Radix
#
# ```
# root = Radix::Node(Nil).new("/")
# root.children << Radix::Node(Nil).new("*filepath") # node.priority => 0
# root.children << Radix::Node(Nil).new(":query") # node.priority => 1
# root.children << Radix::Node(Nil).new("*filepath") # node.priority => -2
# root.children << Radix::Node(Nil).new(":query") # node.priority => -1
# root.children << Radix::Node(Nil).new("a") # node.priority => 1
# root.children << Radix::Node(Nil).new("bc") # node.priority => 2
# root.sort!
#
# root.children.map &.priority
# # => [2, 1, 1, 0]
# # => [2, 1, -1, -2]
# ```
def sort!
@children.sort_by! { |node| -node.priority }
Expand Down

0 comments on commit 5a6811e

Please sign in to comment.