From 1764332123ca284648125ae07ff595d8cb8f2778 Mon Sep 17 00:00:00 2001 From: Luis Lavena Date: Sat, 4 Feb 2017 12:10:51 -0300 Subject: [PATCH] Makes node prioritization insertion-independent The order in which nodes were inserted into a tree might produce failures in lookup mechanism. tree = Radix::Tree(Symbol).new tree.add "/one/:id", :one tree.add "/one-longer/:id", :two result = tree.find "/one-longer/10" # expected `true` result.found? # => false In above example, reversing the order of insertion solved the issue, but exposed the naive sorting/prioritization mechanism used. This change improves that by properly identifying the kind of node being evaluated and compared against others of the same kind. It is now possible to know in advance if a node contains an special condition (named parameter or globbing) or is a normal one: node = Radix::Node(Nil).new("a") node.normal? # => true node = Radix::Node(Nil).new(":query") node.named? # => true node = Radix::Node(Nil).new("*filepath") node.glob? # => true Which helps out with prioritization of nodes: - A normal node ranks higher than a named one - A named node ranks higher than a glob one - On two of same kind, ranks are based on priority With this change in place, above example works as expected: tree = Radix::Tree(Symbol).new tree.add "/one/:id", :one tree.add "/one-longer/:id", :two result = tree.find "/one-longer/10" result.found? # => true Fixes #18 --- CHANGELOG.md | 3 + spec/radix/node_spec.cr | 76 ++++++++++++++----- spec/radix/tree_spec.cr | 13 ++++ src/radix/node.cr | 163 +++++++++++++++++++++++++++------------- 4 files changed, 187 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38aa01d..15c56ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 prioritization of node's children using combination of kind and + priority, allowing partial shared keys to coexist and resolve lookup. ## [0.3.6] - 2017-01-18 ### Fixed diff --git a/spec/radix/node_spec.cr b/spec/radix/node_spec.cr index 4be39c9..e43ffaa 100644 --- a/spec/radix/node_spec.cr +++ b/spec/radix/node_spec.cr @@ -2,6 +2,16 @@ require "../spec_helper" module Radix describe Node do + describe "#glob?" do + it "returns true when key contains a glob parameter (catch all)" do + node = Node(Nil).new("a") + node.glob?.should be_false + + node = Node(Nil).new("*filepath") + node.glob?.should be_true + end + end + describe "#key=" do it "accepts change of key after initialization" do node = Node(Nil).new("abc") @@ -10,6 +20,38 @@ module Radix node.key = "xyz" node.key.should eq("xyz") end + + it "also changes kind when modified" do + node = Node(Nil).new("abc") + node.normal?.should be_true + + node.key = ":query" + node.normal?.should be_false + node.named?.should be_true + end + end + + describe "#named?" do + it "returns true when key contains a named parameter" do + node = Node(Nil).new("a") + node.named?.should be_false + + node = Node(Nil).new(":query") + node.named?.should be_true + end + end + + describe "#normal?" do + it "returns true when key does not contain named or glob parameters" do + node = Node(Nil).new("a") + node.normal?.should be_true + + node = Node(Nil).new(":query") + node.normal?.should be_false + + node = Node(Nil).new("*filepath") + node.normal?.should be_false + end end describe "#payload" do @@ -36,7 +78,7 @@ module Radix end describe "#priority" do - it "calculates it based on key size" do + it "calculates it based on key length" do node = Node(Nil).new("a") node.priority.should eq(1) @@ -44,20 +86,20 @@ module Radix node.priority.should eq(3) end - it "returns zero for catch all (globbed) key" do - node = Node(Nil).new("*filepath") - node.priority.should eq(-2) + it "considers key length up until named parameter presence" do + node = Node(Nil).new("/posts/:id") + node.priority.should eq(7) - node = Node(Nil).new("/src/*filepath") - node.priority.should eq(-2) + node = Node(Nil).new("/u/:username") + node.priority.should eq(3) end - it "returns one for keys with named parameters" do - node = Node(Nil).new(":query") - node.priority.should eq(-1) + it "considers key length up until glob parameter presence" do + node = Node(Nil).new("/search/*query") + node.priority.should eq(8) - node = Node(Nil).new("/search/:query") - node.priority.should eq(-1) + node = Node(Nil).new("/*anything") + node.priority.should eq(1) end it "changes when key changes" do @@ -67,16 +109,16 @@ module Radix node.key = "abc" node.priority.should eq(3) - node.key = "*filepath" - node.priority.should eq(-2) + node.key = "/src/*filepath" + node.priority.should eq(5) - node.key = ":query" - node.priority.should eq(-1) + node.key = "/search/:query" + node.priority.should eq(8) end end describe "#sort!" do - it "orders children by priority" do + it "orders children" do root = Node(Int32).new("/") node1 = Node(Int32).new("a", 1) node2 = Node(Int32).new("bc", 2) @@ -90,7 +132,7 @@ module Radix root.children[2].should eq(node1) end - it "orders catch all and named parameters lower than others" do + it "orders catch all and named parameters lower than normal nodes" do root = Node(Int32).new("/") node1 = Node(Int32).new("*filepath", 1) node2 = Node(Int32).new("abc", 2) diff --git a/spec/radix/tree_spec.cr b/spec/radix/tree_spec.cr index a59132e..4c8ce04 100644 --- a/spec/radix/tree_spec.cr +++ b/spec/radix/tree_spec.cr @@ -544,6 +544,19 @@ module Radix result.payload.should eq(:featured) end end + + context "dealing with named parameters and shared key" do + it "finds matching path" do + tree = Tree(Symbol).new + tree.add "/one/:id", :one + tree.add "/one-longer/:id", :two + + result = tree.find "/one-longer/10" + result.found?.should be_true + result.key.should eq("/one-longer/:id") + result.params["id"].should eq("10") + end + end end end end diff --git a/src/radix/node.cr b/src/radix/node.cr index 985c323..a87d39e 100644 --- a/src/radix/node.cr +++ b/src/radix/node.cr @@ -4,42 +4,34 @@ module Radix # Carries a *payload* and might also contain references to other nodes # down in the organization inside *children*. # - # Each node also carries a *priority* number, which might indicate the - # weight of this node depending on characteristics like catch all - # (globbing), named parameters or simply the size of the Node's *key*. - # - # Referenced nodes inside *children* can be sorted by *priority*. + # Each node also carries identification in relation to the kind of key it + # contains, which helps with characteristics of the node like named + # parameters or catch all kind (globbing). # # Is not expected direct usage of a node but instead manipulation via # methods within `Tree`. - # - # ``` - # node = Radix::Node.new("/", :root) - # node.children << Radix::Node.new("a", :a) - # node.children << Radix::Node.new("bc", :bc) - # node.children << Radix::Node.new("def", :def) - # node.sort! - # - # node.priority - # # => 1 - # - # node.children.map &.priority - # # => [3, 2, 1] - # ``` class Node(T) + include Comparable(self) + + # :nodoc: + enum Kind : UInt8 + Normal + Named + Glob + end + getter key getter? placeholder + property children = [] of Node(T) property! payload : T | Nil - property children + + # :nodoc: + protected getter kind = Kind::Normal # Returns the priority of the Node based on it's *key* # - # This value will be directly associated to the key size or special - # elements of it. - # - # * 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. + # This value will be directly associated to the key size up until a + # special elements is found. # # ``` # Radix::Node(Nil).new("a").priority @@ -48,11 +40,11 @@ module Radix # Radix::Node(Nil).new("abc").priority # # => 3 # - # Radix::Node(Nil).new("*filepath").priority - # # => -2 + # Radix::Node(Nil).new("/src/*filepath").priority + # # => 5 # - # Radix::Node(Nil).new(":query").priority - # # => -1 + # Radix::Node(Nil).new("/search/:query").priority + # # => 8 # ``` getter priority : Int32 @@ -75,10 +67,62 @@ module Radix # node = Radix::Node.new("/") # ``` def initialize(@key : String, @payload : T? = nil, @placeholder = false) - @children = [] of Node(T) @priority = compute_priority end + # Compares this node against *other*, returning `-1`, `0` or `1` depending + # on whether this node differentiates from *other*. + # + # Comparison is done combining node's `kind` and `priority`. Nodes of + # same kind are compared by priority. Nodes of different kind are + # ranked. + # + # ### Normal nodes + # + # ``` + # node1 = Radix::Node(Nil).new("a") # normal + # node2 = Radix::Node(Nil).new("bc") # normal + # node1 <=> node2 # => 1 + # ``` + # + # ### Normal vs named or glob nodes + # + # ``` + # node1 = Radix::Node(Nil).new("a") # normal + # node2 = Radix::Node(Nil).new(":query") # named + # node3 = Radix::Node(Nil).new("*filepath") # glob + # node1 <=> node2 # => -1 + # node1 <=> node3 # => -1 + # ``` + # + # ### Named vs glob nodes + # + # ``` + # node1 = Radix::Node(Nil).new(":query") # named + # node2 = Radix::Node(Nil).new("*filepath") # glob + # node1 <=> node2 # => -1 + # ``` + def <=>(other : self) + result = kind <=> other.kind + return result if result != 0 + + other.priority <=> priority + end + + # Returns `true` if the node key contains a glob parameter in it + # (catch all) + # + # ``` + # node = Radix::Node(Nil).new("*filepath") + # node.glob? # => true + # + # node = Radix::Node(Nil).new("abc") + # node.glob? # => false + # ``` + def glob? + kind.glob? + end + # Changes current *key* # # ``` @@ -91,7 +135,7 @@ module Radix # # => "b" # ``` # - # This will also result in a new priority for the node. + # This will also result in change of node's `priority` # # ``` # node = Radix::Node(Nil).new("a") @@ -103,9 +147,38 @@ module Radix # # => 6 # ``` def key=(@key) + # reset kind on change of key + @kind = Kind::Normal @priority = compute_priority end + # Returns `true` if the node key contains a named parameter in it + # + # ``` + # node = Radix::Node(Nil).new(":query") + # node.named? # => true + # + # node = Radix::Node(Nil).new("abc") + # node.named? # => false + # ``` + def named? + kind.named? + end + + # Returns `true` if the node key does not contain an special parameter + # (named or glob) + # + # ``` + # node = Radix::Node(Nil).new("a") + # node.normal? # => true + # + # node = Radix::Node(Nil).new(":query") + # node.normal? # => false + # ``` + def normal? + kind.normal? + end + # :nodoc: private def compute_priority reader = Char::Reader.new(@key) @@ -113,9 +186,11 @@ module Radix while reader.has_next? case reader.current_char when '*' - return -2 + @kind = Kind::Glob + break when ':' - return -1 + @kind = Kind::Named + break else reader.next_char end @@ -124,23 +199,9 @@ module Radix reader.pos end - # Changes the order of Node's children based on each node priority. - # - # This ensures highest priority nodes are listed before others. - # - # ``` - # root = Radix::Node(Nil).new("/") - # 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, -2] - # ``` - def sort! - @children.sort_by! { |node| -node.priority } + # :nodoc: + protected def sort! + @children.sort! end end end