From e0ef8d83da85b475a0a104c35176d077b62d6d0e Mon Sep 17 00:00:00 2001 From: Luis Lavena Date: Thu, 10 Mar 2016 09:55:10 -0300 Subject: [PATCH] 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 --- CHANGELOG.md | 6 ++++ spec/radix/tree_spec.cr | 77 +++++++++++++++++++++++++++++++++++++++++ src/radix/tree.cr | 53 ++++++++++++++++++++++++++-- 3 files changed, 134 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e54d64..aac25a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/spec/radix/tree_spec.cr b/spec/radix/tree_spec.cr index f889d20..44c7759 100644 --- a/spec/radix/tree_spec.cr +++ b/spec/radix/tree_spec.cr @@ -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 diff --git a/src/radix/tree.cr b/src/radix/tree.cr index 6a16a3c..7e88f5d 100644 --- a/src/radix/tree.cr +++ b/src/radix/tree.cr @@ -125,8 +125,29 @@ module Radix new_key = path_reader.string.byte_slice(path_reader.pos) node.children.each do |child| - # compare first character - next unless child.key[0]? == new_key[0]? + # 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