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

@@ -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