Fixing #157. Objects are no longer allowed to be their own parents, and there are checks in both directions to stop this.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1246 980ebf18-57e1-0310-9a29-db15c13687c0

4 files changed, 69 insertions, 25 deletions

diff --git a/lib/puppet/transportable.rb b/lib/puppet/transportable.rb
index 501dd8d89..e7459ee36 100644
--- a/lib/puppet/transportable.rb
+++ b/lib/puppet/transportable.rb
@@ -222,12 +222,6 @@ module Puppet
                     #Puppet.debug "%s[%s] has no parameters" % [@type, @name]
                 end
                 container = Puppet.type(:component).create(trans)
-
-                if parent
-                    container.parent = parent
-                    #Puppet.warning "parent is of type %s" % parent.class
-                    #trans[:parent] = parent
-                end
             else
                 hash = {
                     :name => self.name,
@@ -243,9 +237,9 @@ module Puppet
                     #Puppet.debug "%s[%s] has no parameters" % [@type, @name]
                 end
 
-                if parent
-                    hash[:parent] = parent
-                end
+                #if parent
+                #    hash[:parent] = parent
+                #end
                 container = Puppet.type(:component).create(hash)
             end
             #Puppet.info container.inspect
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 16ffac085..78bfb955b 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -21,9 +21,9 @@ class Type < Puppet::Element
     # In naming methods, I have tried to consistently name the method so
     # that it is clear whether it operates on all attributes (thus has 'attr' in
     # the method name, or whether it operates on a specific type of attributes.
-    attr_accessor :children, :parent
+    attr_accessor :children
     attr_accessor :file, :line
-    attr_reader :tags
+    attr_reader :tags, :parent
 
     attr_writer :implicit
     def implicit?
@@ -986,11 +986,40 @@ class Type < Puppet::Element
         return @parameters[name].value
     end
 
+    def parent=(parent)
+        if self.parentof?(parent)
+            raise Puppet::DevError, "Objects can not be their own parents"
+        end
+        @parent = parent
+    end
+
+    # Add a hook for testing for recursion.
+    def parentof?(child)
+        if (self == child) or
+            (defined? @parent and @parent.parentof?(child)) or
+            @children.include?(child)
+                return true
+        else
+            return false
+        end
+    end
+
     def push(*childs)
         unless defined? @children
             @children = []
         end
         childs.each { |child|
+            # Make sure we don't have any loops here.
+            if parentof?(child)
+                raise Puppet::DevError, "Objects can not be their own parents"
+            end
+            unless child.is_a?(Puppet::Element)
+                self.debug "Got object of type %s" % child.class
+                self.devfail(
+                    "Containers can only contain Puppet::Elements, not %s" %
+                    child.class
+                )
+            end
             @children.push(child)
             child.parent = self
         }
diff --git a/lib/puppet/type/component.rb b/lib/puppet/type/component.rb
index b10fd2471..9123bbe75 100644
--- a/lib/puppet/type/component.rb
+++ b/lib/puppet/type/component.rb
@@ -141,20 +141,6 @@ module Puppet
             return @name
         end
 
-        def push(*ary)
-            ary.each { |child|
-                unless child.is_a?(Puppet::Element)
-                    self.debug "Got object of type %s" % child.class
-                    self.devfail(
-                        "Containers can only contain Puppet::Elements, not %s" %
-                        child.class
-                    )
-                end
-                @children.push child
-                child.parent = self
-            }
-        end
-
         def refresh
             @children.collect { |child|
                 if child.respond_to?(:refresh)
diff --git a/test/types/type.rb b/test/types/type.rb
index f98ca2355..d9e5d7577 100644
--- a/test/types/type.rb
+++ b/test/types/type.rb
@@ -285,6 +285,41 @@ class TestType < Test::Unit::TestCase
             assert_nil(klass[obj.name], "object %s was not removed" % obj.name)
         end
     end
+
+    # Verify that objects can't be their own children.
+    def test_object_recursion
+        comp = Puppet.type(:component).create(:name => "top")
+
+        file = Puppet.type(:file).create(:path => tempfile, :ensure => :file)
+
+        assert_raise(Puppet::DevError) do
+            comp.push(comp)
+        end
+
+        assert_raise(Puppet::DevError) do
+            file.push(file)
+        end
+
+        assert_raise(Puppet::DevError) do
+            comp.parent = comp
+        end
+
+        assert_raise(Puppet::DevError) do
+            file.parent = file
+        end
+
+        assert_nothing_raised {
+            comp.push(file)
+        }
+
+        assert_raise(Puppet::DevError) do
+            file.push(comp)
+        end
+
+        assert_raise(Puppet::DevError) do
+            comp.parent = file
+        end
+    end
 end
 
 # $Id$