Fixing #620 - class names and node names now throw an error when they conflict

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

5 files changed, 52 insertions, 17 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 68cb87256..76de82a67 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,6 @@
+    Class names and node names now conflict (#620).
+
+0.23.0
     Modified the fileserver to cache file information, so that
     each file isn't being read on every connection.  Also,
     added londo's patch from #678 to avoid reading entire files
diff --git a/lib/puppet/parser/ast/hostclass.rb b/lib/puppet/parser/ast/hostclass.rb
index 5416c1071..642645824 100644
--- a/lib/puppet/parser/ast/hostclass.rb
+++ b/lib/puppet/parser/ast/hostclass.rb
@@ -25,7 +25,7 @@ class Puppet::Parser::AST
             args = hash[:arguments]
 
             # Verify that we haven't already been evaluated
-            if scope.setclass?(self)
+            if scope.class_scope(self)
                 Puppet.debug "%s class already evaluated" % @type
                 return nil
             end
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index c67825bb3..6feeefc46 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -115,11 +115,19 @@ class Puppet::Parser::Scope
     # that subclasses can set their parent scopes to be the scope of
     # their parent class.
     def class_scope(klass)
-        if klass.respond_to?(:classname)
+        scope = if klass.respond_to?(:classname)
             @classtable[klass.classname]
         else
             @classtable[klass]
         end
+
+        return nil unless scope
+
+        if scope.nodescope? and ! klass.is_a?(AST::Node)
+            raise Puppet::ParseError, "Node %s has already been evaluated; cannot evaluate class with same name" % [klass.classname]
+        end
+
+        scope
     end
 
     # Return the list of collections.
@@ -442,6 +450,14 @@ class Puppet::Parser::Scope
         return Puppet::Parser::Scope.new(hash)
     end
 
+    # Is this class for a node?  This is used to make sure that
+    # nodes and classes with the same name conflict (#620), which
+    # is required because of how often the names are used throughout
+    # the system, including on the client.
+    def nodescope?
+        defined?(@nodescope) and @nodescope
+    end
+
     # Return the list of remaining overrides.
     def overrides
         #@overridetable.collect { |name, overs| overs }.flatten
@@ -452,14 +468,6 @@ class Puppet::Parser::Scope
         @definedtable.values
     end
 
-    def setclass?(obj)
-        if obj.respond_to?(:classname)
-            @classtable.has_key?(obj.classname)
-        else
-            @classtable[obj]
-        end
-    end
-
     # Store the fact that we've evaluated a given class.  We use a hash
     # that gets inherited from the top scope down, rather than a global
     # hash.  We store the object ID, not class name, so that we
@@ -474,6 +482,10 @@ class Puppet::Parser::Scope
         else
             raise Puppet::DevError, "Invalid class %s" % obj.inspect
         end
+        if obj.is_a?(AST::Node)
+            @nodescope = true
+        end
+        nil
     end
 
     # Set all of our facts in the top-level scope.
diff --git a/test/language/ast/hostclass.rb b/test/language/ast/hostclass.rb
index 03b11c9b5..56c1bc7b9 100755
--- a/test/language/ast/hostclass.rb
+++ b/test/language/ast/hostclass.rb
@@ -34,7 +34,7 @@ class TestASTHostClass < Test::Unit::TestCase
             klass.evaluate(:scope => scope)
         end
 
-        assert(scope.setclass?(klass), "Class was not considered evaluated")
+        assert(scope.class_scope(klass), "Class was not considered evaluated")
 
         tmp = scope.findresource("File[/tmp]")
         assert(tmp, "Could not find file /tmp")
@@ -74,8 +74,8 @@ class TestASTHostClass < Test::Unit::TestCase
             moresub.evaluate(:scope => scope)
         end
 
-        assert(scope.setclass?(newbase), "Did not eval newbase")
-        assert(scope.setclass?(newsub), "Did not eval newsub")
+        assert(scope.class_scope(newbase), "Did not eval newbase")
+        assert(scope.class_scope(newsub), "Did not eval newsub")
 
         yay = scope.findresource("File[/tmp/yay]")
         assert(yay, "Did not find file /tmp/yay")
diff --git a/test/language/scope.rb b/test/language/scope.rb
index 7fb14786e..f0feee156 100755
--- a/test/language/scope.rb
+++ b/test/language/scope.rb
@@ -325,13 +325,13 @@ class TestScope < Test::Unit::TestCase
 
         base = scope.findclass("base")
         assert(base, "Could not find base class")
-        assert(! scope.setclass?(base), "Class incorrectly set")
+        assert(! scope.class_scope(base), "Class incorrectly set")
         assert(! scope.classlist.include?("base"), "Class incorrectly in classlist")
         assert_nothing_raised do
             scope.setclass base
         end
 
-        assert(scope.setclass?(base), "Class incorrectly unset")
+        assert(scope.class_scope(base), "Class incorrectly unset")
         assert(scope.classlist.include?("base"), "Class not in classlist")
 
         # Make sure we can retrieve the scope.
@@ -344,7 +344,7 @@ class TestScope < Test::Unit::TestCase
             scope.setclass "string"
         end
 
-        assert(! scope.setclass?("string"), "string incorrectly set")
+        assert(! scope.class_scope("string"), "string incorrectly set")
 
         # Set "" in the class list, and make sure it doesn't show up in the return
         top = scope.findclass("")
@@ -411,7 +411,7 @@ class TestScope < Test::Unit::TestCase
         end
 
         [myclass, otherclass].each do |klass|
-            assert(scope.setclass?(klass),
+            assert(scope.class_scope(klass),
                 "%s was not set" % klass.classname)
         end
     end
@@ -760,6 +760,26 @@ Host <<||>>"
         assert_equal("", scope.lookupvar("testing", true),
             "undef was not returned as '' when string")
     end
+
+    # #620 - Nodes and classes should conflict, else classes don't get evaluated
+    def test_nodes_and_classes_name_conflict
+        scope = mkscope
+        
+        node = AST::Node.new :classname => "test", :namespace => ""
+        scope.setclass(node)
+
+        assert(scope.nodescope?, "Scope was not marked a node scope when a node was set")
+
+        # Now make a subscope that will be a class scope
+        klass = AST::HostClass.new :classname => "test", :namespace => ""
+        kscope = klass.subscope(scope)
+
+        # Now make sure we throw a failure, because we're trying to do a class and node
+        # with the same name
+        assert_raise(Puppet::ParseError, "Did not fail on class and node with same name") do
+            kscope.class_scope(klass)
+        end
+    end
 end
 
 # $Id$