Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with key length, insertion order, named and glob parameters during lookup #18

Closed
luislavena opened this issue Jan 22, 2017 · 0 comments
Assignees
Labels

Comments

@luislavena
Copy link
Owner

Originally reported under kemalcr/kemal#293, the usage of named parameters seems to be affected by the order in which new keys are inserted in the tree when part of it is shared across entries.

require "./src/radix"

class Radix::Node
  # override for testing
  def to_s(io, nesting = 0)
    # format: # (priority) (indent) key
    io << ("# (%2d) " % priority) << (" " * nesting) << key << '\n'
    children.each do |child|
      child.to_s(io, nesting + key.size)
    end
  end
end

tree = Radix::Tree(Symbol).new
tree.add "/one/:id", :one
tree.add "/one-longer/:id", :two

# show tree
puts tree.root.to_s(STDOUT, 0)
# ( 4) /one
# (-1)     /:id
# (-1)     -longer/:id

result = tree.find "/one-longer/10"

# expected to be `true`
puts result.found? # => false

Now, when keys are added in reverse order (longer first), lookup works since key is properly inserted before the other:

tree = Radix::Tree(Symbol).new
tree.add "/one-longer/:id", :two
tree.add "/one/:id", :one

puts tree.root.to_s(STDOUT, 0)
# ( 4) /one
# (-1)     -longer/:id
# (-1)     /:id

result = tree.find "/one-longer/10"

# works, result is `true`
puts result.found? # => true

This is caused by two nodes having the same priority, so insertion order wins 😞

It is required that we apply the same priority sorting to named and glob parameters as we apply to other keys: the longer the key, the higher it ranks.

Since nodes that contains named and glob keys might interfere with the lookup, they must remain lower than regular ones.

Need to review the prioritization technique to accommodate this change. 🤔

@luislavena luislavena added the bug label Jan 22, 2017
@luislavena luislavena self-assigned this Jan 22, 2017
luislavena added a commit that referenced this issue Feb 4, 2017
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
luislavena added a commit that referenced this issue Feb 4, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant