Correctly split named parameters on tree insert

Our Radix implementation was literally considering every single
character as candidate for splitting, which caused keys that
contained named parameters markers (`:foo`) to be broken across
nodes:

    tree = Radix::Tree.new
    tree.add "/", :root
    tree.add "/:post", :post
    tree.add "/:category/:post", :category_post
    # /
    #  :
    #   post
    #   category/:post

This caused incorrect behavior when performing lookup (`Tree#find`)
and failing to detect and map the key name, even when the value
was properly captured:

    result = tree.find "/example"
    pp result.found? # => false

This change corrects the issue by identifying named parameter marker
and once detected, consider everything until a separator or the end
of the supplied string is reached to be a unique key:

    tree = Radix::Tree.new
    tree.add "/", :root
    tree.add "/:post", :post
    tree.add "/:category/:post", :category_post
    # /
    #  :category/:post
    #  :post

However, due how Radix tree is structured, two named parameters at the
same level might result in problems during lookup phase:

    tree = Radix::Tree.new
    tree.add "/", :root
    tree.add "/:post", :post
    tree.add "/:category/:post", :category_post
    # /
    #  :category/:post
    #  :post

    tree.root.sort!
    # /
    #  :post
    #  :category/:post

    result = tree.find "/example"
    pp result.found? # => false
    pp result.params # => {"post" => "example"}

    result = tree.find "/news/first-post"
    pp result.found? # => false
    pp result.params # => {"post" => "news"}

Causing lookup to fail and values be stored under incorrect keys
for the parameters.

Because of this, a deprecation warning will be shown to allow
users adjust and correct their code prior fully removing it and
raise error (you know, semantic versioning and all that jazz).

This fixes #5 and closes #4
This commit is contained in:
Luis Lavena 2016-03-10 09:55:10 -03:00
parent 689625acfe
commit e0ef8d83da
3 changed files with 134 additions and 2 deletions

View file

@ -5,6 +5,12 @@ This project aims to comply with [Semantic Versioning](http://semver.org/),
so please check *Changed* and *Removed* notes before upgrading.
## [Unreleased]
### Fixed
- No longer split named parameters that share same level (@alsm)
### Changed
- Attempt to use two named parameters at same level will display a
deprecation warning. Future versions will raise `Radix::Tree::SharedKeyError`
## [0.1.1] - 2016-02-29
### Fixed

View file

@ -1,5 +1,25 @@
require "../spec_helper"
# Silence deprecation warnings when running specs and allow
# capture them for inspection.
module Radix
class Tree
@show_deprecations = false
@stderr : MemoryIO?
def show_deprecations!
@show_deprecations = true
end
private def deprecation(message)
if @show_deprecations
@stderr ||= MemoryIO.new
@stderr.not_nil!.puts message
end
end
end
end
module Radix
describe Tree do
context "a new instance" do
@ -171,6 +191,63 @@ module Radix
tree.root.children[1].key.should eq("*filepath")
end
it "does not split named parameters across shared key" do
tree = Tree.new
tree.add "/", :root
tree.add "/:category", :category
tree.add "/:category/:subcategory", :subcategory
# / (:root)
# +-:category (:category)
# \-/:subcategory (:subcategory)
tree.root.children.size.should eq(1)
tree.root.children[0].key.should eq(":category")
# inner children
tree.root.children[0].children.size.should eq(1)
tree.root.children[0].children[0].key.should eq("/:subcategory")
end
it "does not split named parameter marker when only root is shared" do
tree = Tree.new
tree.add "/", :root
tree.add "/:post", :post
tree.add "/:category/:post", :category_post
# / (:root)
# +-:category/:post (:category_post)
# \-:post (:post)
tree.root.children.size.should eq(2)
tree.root.children[0].key.should eq(":category/:post")
tree.root.children[1].key.should eq(":post")
end
it "displays deprecation warning when two different named parameters share same level" do
tree = Tree.new
tree.show_deprecations!
tree.add "/", :root
tree.add "/:post", :post
tree.add "/:category/:post", :category_post
stderr = tree.@stderr.not_nil!
stderr.rewind
message = stderr.gets_to_end
message.should contain("DEPRECATION WARNING")
message.should contain("Tried to place key ':category/:post' at same level as ':post'")
end
pending "does not allow different named parameters sharing same level" do
tree = Tree.new
tree.add "/", :root
tree.add "/:post", :post
expect_raises Tree::SharedKeyError do
tree.add "/:category/:post", :category_post
end
end
end
end

View file

@ -125,8 +125,29 @@ module Radix
new_key = path_reader.string.byte_slice(path_reader.pos)
node.children.each do |child|
# compare first character
# if child's key starts with named parameter, compare key until
# separator (if present).
# Otherwise, compare just first character
if child.key[0]? == ':' && new_key[0]? == ':'
unless _same_key?(new_key, child.key)
message = <<-NOTICE
DEPRECATION WARNING: Usage of two different named parameters at same level
will result in lookup issues and misplaced values.
Tried to place key '%s' at same level as '%s'.
Future versions will raise `Radix::Tree::SharedKeyError`.
See Issue #5 for details:
https://github.com/luislavena/radix/issues/5
NOTICE
deprecation message % {new_key, child.key}
next
end
else
next unless child.key[0]? == new_key[0]?
end
# when found, add to this child
added = true
@ -358,5 +379,33 @@ module Radix
count
end
# :nodoc:
private def _same_key?(path, key)
path_reader = Char::Reader.new(path)
key_reader = Char::Reader.new(key)
different = false
while (path_reader.has_next? && path_reader.current_char != '/') &&
(key_reader.has_next? && key_reader.current_char != '/')
if path_reader.current_char != key_reader.current_char
different = true
break
end
path_reader.next_char
key_reader.next_char
end
(!key_reader.has_next? && !different) &&
(path_reader.current_char == '/' || !path_reader.has_next?)
end
# :nodoc:
private def deprecation(message)
STDERR.puts message
STDERR.flush
end
end
end