Make sure node are referenced by their names

This patch uses the unused AST::HostName as the only way to reference
a node in the AST nodes array.
The AST::HostName respect the hash properties of the underlying
string, to keep the O(1) hash properties.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

lib/puppet/parser/ast/leaf.rb

@@ -90,11 +90,27 @@ class Puppet::Parser::AST
         def initialize(hash)
             super
 
-            unless @value =~ %r{^[0-9a-zA-Z\-]+(\.[0-9a-zA-Z\-]+)*$}
+            @value = @value.to_s.downcase
+            if @value =~ /[^-\w.]/
                 raise Puppet::DevError,
                     "'%s' is not a valid hostname" % @value
             end
         end
+
+        def to_classname
+            return @value
+        end
+
+        # implementing eql? and hash so that when an HostName is stored
+        # in a hash it has the same hashing properties as the underlying value
+        def eql?(value)
+            value = value.value if value.is_a?(HostName)
+            return @value.eql?(value)
+        end
+
+        def hash
+            return @value.hash
+        end
     end
 
     # A simple variable.  This object is only used during interpolation;

lib/puppet/parser/ast/node.rb

@@ -11,11 +11,6 @@ class Puppet::Parser::AST::Node < Puppet::Parser::AST::HostClass
     def initialize(options)
         @parentclass = nil
         super
-
-        # Do some validation on the node name
-        if @name =~ /[^-\w.]/
-            raise Puppet::ParseError, "Invalid node name %s" % @name
-        end
     end
 
     def namespace

lib/puppet/parser/grammar.ra

@@ -684,11 +684,15 @@ classname:       NAME { result = val[0][:value] }
 
 # Multiple hostnames, as used for node names.  These are all literal
 # strings, not AST objects.
-hostnames:    hostname
-  | hostnames COMMA hostname {
+hostnames:    nodename
+  | hostnames COMMA nodename {
     result = val[0]
     result = [result] unless result.is_a?(Array)
-    result << val[2][:value]
+    result << val[2]
+}
+
+nodename: hostname {
+    result = ast AST::HostName, :value => val[0]
 }
 
 hostname: NAME { result = val[0][:value] }

lib/puppet/parser/loaded_code.rb

@@ -14,11 +14,11 @@ class Puppet::Parser::LoadedCode
     end
 
     def add_node(name, code)
-        @nodes[munge_name(name)] = code
+        @nodes[check_name(name)] = code
     end
 
     def node(name)
-        @nodes[munge_name(name)]
+        @nodes[check_name(name)]
     end
 
     def nodes?
@@ -88,4 +88,13 @@ class Puppet::Parser::LoadedCode
     def munge_name(name)
         name.to_s.downcase
     end
+
+    # Check that the given (node) name is an HostName instance
+    # We're doing this so that hashing of node in the @nodes hash
+    # is consistent (see AST::HostName#hash and AST::HostName#eql?)
+    # and that the @nodes hash still keep its O(1) get/put properties.
+    def check_name(name)
+        name = Puppet::Parser::AST::HostName.new(:value => name) unless name.is_a?(Puppet::Parser::AST::HostName)
+        name
+    end
 end

lib/puppet/parser/parser_support.rb

@@ -354,7 +354,7 @@ class Puppet::Parser::Parser
         names = [names] unless names.instance_of?(Array)
         doc = lexer.getcomment
         names.collect do |name|
-            name = name.to_s.downcase
+            name = AST::HostName.new :value => name unless name.is_a?(AST::HostName)
             if other = @loaded_code.node(name)
                 error("Node %s is already defined at %s:%s; cannot redefine" % [other.name, other.file, other.line])
             end
@@ -372,7 +372,7 @@ class Puppet::Parser::Parser
                 args[:parentclass] = options[:parent]
             end
             node = ast(AST::Node, args)
-            node.classname = name
+            node.classname = name.to_classname
             @loaded_code.add_node(name, node)
             node
         end

spec/unit/parser/ast/leaf.rb

@@ -136,7 +136,9 @@ end
 describe Puppet::Parser::AST::HostName do
     before :each do
         @scope = stub 'scope'
-        @value = stub 'value', :is_a? => true, :=~ => true
+        @value = stub 'value', :=~ => false
+        @value.stubs(:to_s).returns(@value)
+        @value.stubs(:downcase).returns(@value)
         @host = Puppet::Parser::AST::HostName.new( :value => @value)
     end
 
@@ -144,7 +146,48 @@ describe Puppet::Parser::AST::HostName do
         lambda { Puppet::Parser::AST::HostName.new( :value => "not an hostname!" ) }.should raise_error
     end
 
+    it "should stringify the value" do
+        value = stub 'value', :=~ => false
+
+        value.expects(:to_s).returns("test")
+
+        Puppet::Parser::AST::HostName.new(:value => value)
+    end
+
+    it "should downcase the value" do
+        value = stub 'value', :=~ => false
+        value.stubs(:to_s).returns("UPCASED")
+        host = Puppet::Parser::AST::HostName.new(:value => value)
+
+        host.value == "upcased"
+    end
+
     it "should evaluate to its value" do
         @host.evaluate(@scope).should == @value
     end
+
+    it "should implement to_classname" do
+        @host.should respond_to(:to_classname)
+    end
+
+    it "should return the downcased nodename as classname" do
+        host = Puppet::Parser::AST::HostName.new( :value => "KLASSNAME" )
+        host.to_classname.should == "klassname"
+    end
+
+    it "should delegate eql? to the underlying value if it is an HostName" do
+        @value.expects(:eql?).with("value")
+        @host.eql?("value")
+    end
+
+    it "should delegate eql? to the underlying value if it is not an HostName" do
+        value = stub 'compared', :is_a? => true, :value => "value"
+        @value.expects(:eql?).with("value")
+        @host.eql?(value)
+    end
+
+    it "should delegate hash to the underlying value" do
+        @value.expects(:hash)
+        @host.hash
+    end
 end

spec/unit/parser/loaded_code.rb

@@ -37,49 +37,49 @@ describe Puppet::Parser::LoadedCode do
     describe "when finding a qualified instance" do
         it "should return any found instance if the instance name is fully qualified" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar", "yay"
-            loader.find("namespace", "::foo::bar", :node).should == "yay"
+            loader.add_hostclass "foo::bar", "yay"
+            loader.find("namespace", "::foo::bar", :hostclass).should == "yay"
         end
 
         it "should return nil if the instance name is fully qualified and no such instance exists" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.find("namespace", "::foo::bar", :node).should be_nil
+            loader.find("namespace", "::foo::bar", :hostclass).should be_nil
         end
 
         it "should return the partially qualified object if it exists in the provided namespace" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar::baz", "yay"
-            loader.find("foo", "bar::baz", :node).should == "yay"
+            loader.add_hostclass "foo::bar::baz", "yay"
+            loader.find("foo", "bar::baz", :hostclass).should == "yay"
         end
 
         it "should return the unqualified object if it exists in the provided namespace" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar", "yay"
-            loader.find("foo", "bar", :node).should == "yay"
+            loader.add_hostclass "foo::bar", "yay"
+            loader.find("foo", "bar", :hostclass).should == "yay"
         end
 
         it "should return the unqualified object if it exists in the parent namespace" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar", "yay"
-            loader.find("foo::bar::baz", "bar", :node).should == "yay"
+            loader.add_hostclass "foo::bar", "yay"
+            loader.find("foo::bar::baz", "bar", :hostclass).should == "yay"
         end
 
         it "should should return the partially qualified object if it exists in the parent namespace" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar::baz", "yay"
-            loader.find("foo::bar", "bar::baz", :node).should == "yay"
+            loader.add_hostclass "foo::bar::baz", "yay"
+            loader.find("foo::bar", "bar::baz", :hostclass).should == "yay"
         end
 
         it "should return the qualified object if it exists in the root namespace" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar::baz", "yay"
-            loader.find("foo::bar", "foo::bar::baz", :node).should == "yay"
+            loader.add_hostclass "foo::bar::baz", "yay"
+            loader.find("foo::bar", "foo::bar::baz", :hostclass).should == "yay"
         end
 
         it "should return nil if the object cannot be found" do
             loader = Puppet::Parser::LoadedCode.new
-            loader.add_node "foo::bar::baz", "yay"
-            loader.find("foo::bar", "eh", :node).should be_nil
+            loader.add_hostclass "foo::bar::baz", "yay"
+            loader.find("foo::bar", "eh", :hostclass).should be_nil
         end
     end
 
@@ -110,4 +110,47 @@ describe Puppet::Parser::LoadedCode do
     it "should indicate whether no nodes are defined" do
         Puppet::Parser::LoadedCode.new.should_not be_nodes
     end
+
+    describe "when adding nodes" do
+        it "should create an HostName if nodename is a string" do
+            Puppet::Parser::AST::HostName.expects(:new).with(:value => "foo")
+            loader = Puppet::Parser::LoadedCode.new
+            loader.add_node("foo", "bar")
+        end
+
+        it "should not create an HostName if nodename is an HostName" do
+            name = Puppet::Parser::AST::HostName.new(:value => "foo")
+
+            Puppet::Parser::AST::HostName.expects(:new).with(:value => "foo").never
+
+            loader = Puppet::Parser::LoadedCode.new
+            loader.add_node(name, "bar")
+        end
+    end
+
+    describe "when finding nodes" do
+        it "should create an HostName if nodename is a string" do
+            Puppet::Parser::AST::HostName.expects(:new).with(:value => "foo")
+            loader = Puppet::Parser::LoadedCode.new
+            loader.node("foo")
+        end
+
+        it "should not create an HostName if nodename is an HostName" do
+            name = Puppet::Parser::AST::HostName.new(:value => "foo")
+
+            Puppet::Parser::AST::HostName.expects(:new).with(:value => "foo").never
+
+            loader = Puppet::Parser::LoadedCode.new
+            loader.node(name)
+        end
+
+        it "should be able to find nobe by HostName" do
+            namein = Puppet::Parser::AST::HostName.new(:value => "foo")
+            nameout = Puppet::Parser::AST::HostName.new(:value => "foo")
+            loader = Puppet::Parser::LoadedCode.new
+
+            loader.add_node(namein, "bar")
+            loader.node(nameout) == "bar"
+        end
+    end
 end

spec/unit/parser/parser.rb

@@ -7,7 +7,8 @@ describe Puppet::Parser do
     ast = Puppet::Parser::AST
 
     before :each do
-        @parser = Puppet::Parser::Parser.new :environment => "development"
+        @loaded_code = Puppet::Parser::LoadedCode.new
+        @parser = Puppet::Parser::Parser.new :environment => "development", :loaded_code => @loaded_code
         @true_ast = Puppet::Parser::AST::Boolean.new :value => true
     end
 
@@ -225,4 +226,91 @@ describe Puppet::Parser do
             @parser.ast(Puppet::Parser::AST::Definition)
         end
     end
+
+    describe "when creating a node" do
+        before :each do
+            @lexer = stub 'lexer'
+            @lexer.stubs(:getcomment)
+            @parser.stubs(:lexer).returns(@lexer)
+            @node = stub_everything 'node'
+            @parser.stubs(:ast).returns(@node)
+            @parser.stubs(:node).returns(nil)
+
+            @nodename = stub 'nodename', :is_a? => false, :to_classname => "node"
+            @nodename.stubs(:is_a?).with(Puppet::Parser::AST::HostName).returns(true)
+        end
+
+        it "should get the lexer stacked comments" do
+            @lexer.expects(:getcomment)
+
+            @parser.newnode(@nodename)
+        end
+
+        it "should create an HostName if needed" do
+            Puppet::Parser::AST::HostName.expects(:new).with(:value => "node").returns(@nodename)
+
+            @parser.newnode("node")
+        end
+
+        it "should raise an error if the node already exists" do
+            @loaded_code.stubs(:node).with(@nodename).returns(@node)
+
+            lambda { @parser.newnode(@nodename) }.should raise_error
+        end
+
+        it "should store the created node in the loaded code" do
+            @loaded_code.expects(:add_node).with(@nodename, @node)
+
+            @parser.newnode(@nodename)
+        end
+
+        it "should create the node with code if provided" do
+            @parser.stubs(:ast).with { |*args| args[1][:code] == :code }.returns(@node)
+
+            @parser.newnode(@nodename, :code => :code)
+        end
+
+        it "should create the node with a parentclass if provided" do
+            @parser.stubs(:ast).with { |*args| args[1][:parent] == :parent }.returns(@node)
+
+            @parser.newnode(@nodename, :parent => :parent)
+        end
+
+        it "should set the node classname from the HostName" do
+            @nodename.stubs(:to_classname).returns(:classname)
+
+            @node.expects(:classname=).with(:classname)
+
+            @parser.newnode(@nodename)
+        end
+
+        it "should return an array of nodes" do
+            @parser.newnode(@nodename).should == [@node]
+        end
+    end
+
+    describe "when retrieving a specific node" do
+        it "should delegate to the loaded_code node" do
+            @loaded_code.expects(:node).with("node")
+
+            @parser.node("node")
+        end
+    end
+
+    describe "when retrieving a specific class" do
+        it "should delegate to the loaded code" do
+            @loaded_code.expects(:hostclass).with("class")
+
+            @parser.hostclass("class")
+        end
+    end
+
+    describe "when retrieving a specific definitions" do
+        it "should delegate to the loaded code" do
+            @loaded_code.expects(:definition).with("define")
+
+            @parser.definition("define")
+        end
+    end
+
  end

test/lib/puppettest/support/collection.rb

@@ -18,7 +18,7 @@ module PuppetTest::Support::Collection
                 query = nil
 
                 assert_nothing_raised("Could not parse '#{str}'") do
-                    query = parser.parse(code).classes[""].code[0].query
+                    query = parser.parse(code).hostclass("").code[0].query
                 end
 
                 yield str, res, query