Global unique node names #9

merged 1 commit into from Aug 23, 2012


None yet

3 participants

ysf commented Aug 23, 2012

While using RubyTree at work I noticed that the node names, though the documentation states "The node name is expected to be unique within the tree.", are not unique if you add an already added node/name to another child. There are several ways to fix this, the easiest implementation, is to check if the tree, where the child is added too, already contains a node with this name.

Because I'm not allowed to spend too much worktime on open source projects I stuck with the simple implementation I mentioned and hope that this is of any use to you anyway. While testing, the "add node to self as child" case was fixed too so I changed the test likewise.

Adding a node might still be exploitable by something like this, I hope you get the idea, could happen anytime you add a node or merge two trees:

@root     ='root', "Root node")
@child1   ='child1', '1st child')
@child2   ='child2', '2nd child')
@conflict ='conflict', 'Should do problems')

@child1 << @conflict # should work because @conflict is not in root-tree
@child2 << @conflict # same here.

@root << @child1 # no problem so far
@root << @child2 # now two nodes with the name 'conflict' exist in root tree

Thanks a lot for RubyTree and best regards, ysf :)


This pull request fails (merged 76b6d24 into a76aec1).

@evolve75 evolve75 was assigned Aug 23, 2012
@evolve75 evolve75 commented on the diff Aug 23, 2012
@@ -313,7 +313,9 @@ def <<(child)
def add(child, at_index = -1)
raise ArgumentError, "Attempting to add a nil node" unless child # Only handles the immediate child scenario
raise ArgumentError, "Attempting add node to itself" if self == child
- raise "Child #{} already added!" if @children_hash.has_key?(
+ # Lazy mans unique test, won't test if children of child are unique in this tree too.
+ self.root.each { |node| raise "Child #{} already added!" if == }
evolve75 Aug 23, 2012 Owner

We will need to make this configurable. The issue is that while this is the correct behavior (in terms of the assertion/check), there is existing code which is using duplicate node names already, and this fix will break that code.

@evolve75 evolve75 merged commit 3af3a3a into evolve75:master Aug 23, 2012

1 check failed

default The Travis build failed
@sirsean sirsean referenced this pull request Jan 23, 2014

Unique node names #24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment