Merge pull request #19 from luislavena/improve-node-sorting

Makes node prioritization insertion-independent

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

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,28 +78,28 @@ 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)
 
         node = Node(Nil).new("abc")
         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)

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

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,19 +147,50 @@ 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)
 
       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