Fixing #606 -- now only components mention @children.

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

6 files changed, 202 insertions, 206 deletions

diff --git a/lib/puppet/metatype/attributes.rb b/lib/puppet/metatype/attributes.rb
index 50ee53c2b..679d6fc2f 100644
--- a/lib/puppet/metatype/attributes.rb
+++ b/lib/puppet/metatype/attributes.rb
@@ -524,17 +524,10 @@ class Puppet::Type
     # when an error has been encountered
     def delete(attr)
         attr = symbolize(attr)
-        case attr
-        when Puppet::Type
-            if @children.include?(attr)
-                @children.delete(attr)
-            end
+        if @parameters.has_key?(attr)
+            @parameters.delete(attr)
         else
-            if @parameters.has_key?(attr)
-                @parameters.delete(attr)
-            else
-                raise Puppet::DevError.new("Undefined attribute '#{attr}' in #{self}")
-            end
+            raise Puppet::DevError.new("Undefined attribute '#{attr}' in #{self}")
         end
     end
 
diff --git a/lib/puppet/metatype/container.rb b/lib/puppet/metatype/container.rb
index b48f8018e..7c44a7def 100644
--- a/lib/puppet/metatype/container.rb
+++ b/lib/puppet/metatype/container.rb
@@ -1,5 +1,4 @@
 class Puppet::Type
-    attr_accessor :children
 
     # this is a retarded hack method to get around the difference between
     # component children and file children
@@ -31,48 +30,14 @@ class Puppet::Type
         elsif defined? @parent and @parent.parentof?(child)
             debug "My parent is parent of child"
             return true
-        elsif @children.include?(child)
-            debug "child is already in children array"
-            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)
-                devfail "Already the parent of %s[%s]" % [child.class.name, child.title]
-            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
-        }
-    end
-
     # Remove an object.  The argument determines whether the object's
     # subscriptions get eliminated, too.
     def remove(rmdeps = true)
-        # Our children remove themselves from our @children array (else the object
-        # we called this on at the top would not be removed), so we duplicate the
-        # array and iterate over that.  If we don't do this, only half of the
-        # objects get removed.
-        @children.dup.each { |child|
-            child.remove(rmdeps)
-        }
-
-        @children.clear
-
         # This is hackish (mmm, cut and paste), but it works for now, and it's
         # better than warnings.
         @parameters.each do |name, obj|
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index b745c6322..2b49492b2 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -106,40 +106,6 @@ class Type < Puppet::Element
         define_method(:validate, &block)
         #@validate = block
     end
-
-    # iterate across all children, and then iterate across properties
-    # we do children first so we're sure that all dependent objects
-    # are checked first
-    # we ignore parameters here, because they only modify how work gets
-    # done, they don't ever actually result in work specifically
-    def each
-        # we want to return the properties in the order that each type
-        # specifies it, because it may (as in the case of File#create)
-        # be important
-        if self.class.depthfirst?
-            @children.each { |child|
-                yield child
-            }
-        end
-        self.eachproperty { |property|
-            yield property
-        }
-        unless self.class.depthfirst?
-            @children.each { |child|
-                yield child
-            }
-        end
-    end
-
-    # Recurse deeply through the tree, but only yield types, not properties.
-    def delve(&block)
-        self.each do |obj|
-            if obj.is_a? Puppet::Type
-                obj.delve(&block)
-            end
-        end
-        block.call(self)
-    end
     
     # create a log at specified level
     def log(msg)
@@ -157,7 +123,6 @@ class Type < Puppet::Element
     public
 
     def initvars
-        @children = []
         @evalcount = 0
         @tags = []
 
diff --git a/lib/puppet/type/component.rb b/lib/puppet/type/component.rb
index cafd1bbe7..82ee8c056 100644
--- a/lib/puppet/type/component.rb
+++ b/lib/puppet/type/component.rb
@@ -7,153 +7,227 @@ require 'puppet/type'
 require 'puppet/transaction'
 require 'puppet/pgraph'
 
-module Puppet
-    newtype(:component) do
-        include Enumerable
-
-        newparam(:name) do
-            desc "The name of the component.  Generally optional."
-            isnamevar
+Puppet::Type.newtype(:component) do
+    include Enumerable
+    attr_accessor :children
+
+    newparam(:name) do
+        desc "The name of the component.  Generally optional."
+        isnamevar
+    end
+
+    newparam(:type) do
+        desc "The type that this component maps to.  Generally some kind of
+            class from the language."
+
+        defaultto "component"
+    end
+
+    # Remove a child from the component.
+    def delete(child)
+        if @children.include?(child)
+            @children.delete(child)
+            return true
+        else
+            return super
         end
+    end
 
-        newparam(:type) do
-            desc "The type that this component maps to.  Generally some kind of
-                class from the language."
-
-            defaultto "component"
+    # Recurse deeply through the tree, but only yield types, not properties.
+    def delve(&block)
+        self.each do |obj|
+            if obj.is_a?(self.class)
+                obj.delve(&block)
+            end
         end
-
-        # Remove a child from the component.
-        def delete(child)
-            if @children.include?(child)
-                @children.delete(child)
-                return true
+        block.call(self)
+    end
+
+    # Return each child in turn.
+    def each
+        @children.each { |child| yield child }
+    end
+
+    # flatten all children, sort them, and evaluate them in order
+    # this is only called on one component over the whole system
+    # this also won't work with scheduling, but eh
+    def evaluate
+        self.finalize unless self.finalized?
+        transaction = Puppet::Transaction.new(self)
+        transaction.component = self
+        return transaction
+    end
+
+    # Do all of the polishing off, mostly doing autorequires and making
+    # dependencies.  This will get run once on the top-level component,
+    # and it will do everything necessary.
+    def finalize
+        started = {}
+        finished = {}
+        
+        # First do all of the finish work, which mostly involves
+        self.delve do |object|
+            # Make sure we don't get into loops
+            if started.has_key?(object)
+                debug "Already finished %s" % object.title
+                next
             else
-                return false
+                started[object] = true
+            end
+            unless finished.has_key?(object)
+                object.finish
+                finished[object] = true
             end
         end
 
-        # Return each child in turn.
-        def each
-            @children.each { |child| yield child }
-        end
+        @finalized = true
+    end
 
-        # flatten all children, sort them, and evaluate them in order
-        # this is only called on one component over the whole system
-        # this also won't work with scheduling, but eh
-        def evaluate
-            self.finalize unless self.finalized?
-            transaction = Puppet::Transaction.new(self)
-            transaction.component = self
-            return transaction
+    def finalized?
+        if defined? @finalized
+            return @finalized
+        else
+            return false
         end
-
-        # Do all of the polishing off, mostly doing autorequires and making
-        # dependencies.  This will get run once on the top-level component,
-        # and it will do everything necessary.
-        def finalize
-            started = {}
-            finished = {}
-            
-            # First do all of the finish work, which mostly involves
-            self.delve do |object|
-                # Make sure we don't get into loops
-                if started.has_key?(object)
-                    debug "Already finished %s" % object.title
-                    next
-                else
-                    started[object] = true
-                end
-                unless finished.has_key?(object)
-                    object.finish
-                    finished[object] = true
-                end
-            end
-
-            @finalized = true
+    end
+
+    # Initialize a new component
+    def initialize(args)
+        @children = []
+        super(args)
+    end
+
+    def initvars
+        super
+        @children = []
+    end
+
+    def parent=(parent)
+        if self.parentof?(parent)
+            devfail "%s[%s] is already the parent of %s[%s]" %
+                [self.class.name, self.title, parent.class.name, parent.title]
         end
-
-        def finalized?
-            if defined? @finalized
-                return @finalized
-            else
-                return false
-            end
+        @parent = parent
+    end
+
+    # Add a hook for testing for recursion.
+    def parentof?(child)
+        if super(child)
+            return true
+        elsif @children.include?(child)
+            debug "child is already in children array"
+            return true
+        else
+            return false
         end
+    end
 
-        # Initialize a new component
-        def initialize(args)
+    def push(*childs)
+        unless defined? @children
             @children = []
-            super(args)
         end
-        
-        # Component paths are special because they function as containers.
-        def pathbuilder
-            tmp = []
-            if defined? @parent and @parent
-                tmp += [@parent.pathbuilder, self.title]
+        childs.each { |child|
+            # Make sure we don't have any loops here.
+            if parentof?(child)
+                devfail "Already the parent of %s[%s]" % [child.class.name, child.title]
+            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
+        }
+    end
+    
+    # Component paths are special because they function as containers.
+    def pathbuilder
+        tmp = []
+        if defined? @parent and @parent
+            tmp += [@parent.pathbuilder, self.title]
+        else
+            # The top-level name is always main[top], so we don't bother with
+            # that.
+            if self.title == "main[top]"
+                tmp << "" # This empty field results in "//" in the path
             else
-                # The top-level name is always main[top], so we don't bother with
-                # that.
-                if self.title == "main[top]"
-                    tmp << "" # This empty field results in "//" in the path
-                else
-                    tmp << self.title
-                end
+                tmp << self.title
             end
-            
-            tmp
         end
-
-        # We have a different way of setting the title
-        def title
-            unless defined? @title
-                if self[:type] == self[:name] # this is the case for classes
-                    @title = self[:type]
-                elsif self[:name] =~ /\[.+\]/ # most components already have ref info in the name
-                    @title = self[:name]
-                else # else, set it up
-                    @title = "%s[%s]" % [self[:type].capitalize, self[:name]]
-                end
+        
+        tmp
+    end
+
+    # Remove an object.  The argument determines whether the object's
+    # subscriptions get eliminated, too.
+    def remove(rmdeps = true)
+        # Our children remove themselves from our @children array (else the object
+        # we called this on at the top would not be removed), so we duplicate the
+        # array and iterate over that.  If we don't do this, only half of the
+        # objects get removed.
+        @children.dup.each { |child|
+            child.remove(rmdeps)
+        }
+
+        @children.clear
+
+        # Get rid of params and provider, too.
+        super
+
+        @parent = nil
+    end
+
+    # We have a different way of setting the title
+    def title
+        unless defined? @title
+            if self[:type] == self[:name] # this is the case for classes
+                @title = self[:type]
+            elsif self[:name] =~ /\[.+\]/ # most components already have ref info in the name
+                @title = self[:name]
+            else # else, set it up
+                @title = "%s[%s]" % [self[:type].capitalize, self[:name]]
             end
-            return @title
-        end
-
-        def refresh
-            @children.collect { |child|
-                if child.respond_to?(:refresh)
-                    child.refresh
-                    child.log "triggering %s" % :refresh
-                end
-            }
         end
+        return @title
+    end
+
+    def refresh
+        @children.collect { |child|
+            if child.respond_to?(:refresh)
+                child.refresh
+                child.log "triggering %s" % :refresh
+            end
+        }
+    end
+    
+    # Convert to a graph object with all of the container info.
+    def to_graph
+        graph = Puppet::PGraph.new
         
-        # Convert to a graph object with all of the container info.
-        def to_graph
-            graph = Puppet::PGraph.new
-            
-            delver = proc do |obj|
-                obj.each do |child|
-                    if child.is_a?(Puppet::Type)
-                        graph.add_edge!(obj, child)
-                        delver.call(child)
-                    end
+        delver = proc do |obj|
+            obj.each do |child|
+                graph.add_edge!(obj, child)
+                if child.is_a?(self.class)
+                    delver.call(child)
                 end
             end
-            
-            delver.call(self)
-            
-            return graph
         end
-
-        def to_s
-            if self.title =~ /\[/
-                return self.title
-            else
-                return "component(%s)" % self.title
-            end
+        
+        delver.call(self)
+        
+        return graph
+    end
+
+    def to_s
+        if self.title =~ /\[/
+            return self.title
+        else
+            return "component(%s)" % self.title
         end
-	end
+    end
 end
 
 # $Id$
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index bfa10dda2..aaee68ab9 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -1136,8 +1136,7 @@ module Puppet
         # There are some cases where all of the work does not get done on
         # file creation/modification, so we have to do some extra checking.
         def property_fix
-            self.each do |thing|
-                next unless thing.is_a? Puppet::Property
+            properties.each do |thing|
                 next unless [:mode, :owner, :group].include?(thing.name)
 
                 # Make sure we get a new stat objct
diff --git a/test/ral/manager/type.rb b/test/ral/manager/type.rb
index 3872ef637..355b81c4d 100755
--- a/test/ral/manager/type.rb
+++ b/test/ral/manager/type.rb
@@ -285,7 +285,7 @@ class TestType < Test::Unit::TestCase
     def test_object_recursion
         comp = Puppet.type(:component).create(:name => "top")
 
-        file = Puppet.type(:file).create(:path => tempfile, :ensure => :file)
+        file = Puppet.type(:component).create(:name => "middle")
 
         assert_raise(Puppet::DevError) do
             comp.push(comp)