(#8596) Detect resource alias conflicts when titles do not match

The introduction of composite namevars caused the resource title used in
resource aliases to be set as an array, even when the resource only had one
namevar. This would fail to conflict with non-alias entries in the resource
table, which used a string for the title, even though the single element array
contained the same string.

Now, we flatten the key used in the resource table, so that single element
arrays are represented as strings, and will properly conflict with resource
titles.

Paired-With: Jacob Helwig <jacob@puppetlabs.com>

2 files changed, 60 insertions, 10 deletions

diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 0a63ff172..2fdd19b0c 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -98,7 +98,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     resource.ref =~ /^(.+)\[/
     class_name = $1 || resource.class.name
 
-    newref = [class_name, key]
+    newref = [class_name, key].flatten
 
     if key.is_a? String
       ref_string = "#{class_name}[#{key}]"
@@ -111,7 +111,10 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     # isn't sufficient.
     if existing = @resource_table[newref]
       return if existing == resource
-      raise(ArgumentError, "Cannot alias #{resource.ref} to #{key.inspect}; resource #{newref.inspect} already exists")
+      resource_definition = " at #{resource.file}:#{resource.line}" if resource.file and resource.line
+      existing_definition = " at #{existing.file}:#{existing.line}" if existing.file and existing.line
+      msg = "Cannot alias #{resource.ref} to #{key.inspect}#{resource_definition}; resource #{newref.inspect} already defined#{existing_definition}"
+      raise ArgumentError, msg
     end
     @resource_table[newref] = resource
     @aliases[resource.ref] ||= []
@@ -377,7 +380,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
       res = Puppet::Resource.new(nil, type)
     end
     title_key      = [res.type, res.title.to_s]
-    uniqueness_key = [res.type, res.uniqueness_key]
+    uniqueness_key = [res.type, res.uniqueness_key].flatten
     @resource_table[title_key] || @resource_table[uniqueness_key]
   end
 
diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb
index a15a912ef..4600022b3 100755
--- a/spec/unit/resource/catalog_spec.rb
+++ b/spec/unit/resource/catalog_spec.rb
@@ -507,7 +507,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
       lambda { @catalog.alias(@one, "other") }.should_not raise_error
     end
 
-    it "should create aliases for resources isomorphic resources whose names do not match their titles" do
+    it "should create aliases for isomorphic resources whose names do not match their titles" do
       resource = Puppet::Type::File.new(:title => "testing", :path => @basepath+"/something")
 
       @catalog.add_resource(resource)
@@ -515,7 +515,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
       @catalog.resource(:file, @basepath+"/something").should equal(resource)
     end
 
-    it "should not create aliases for resources non-isomorphic resources whose names do not match their titles" do
+    it "should not create aliases for non-isomorphic resources whose names do not match their titles" do
       resource = Puppet::Type.type(:exec).new(:title => "testing", :command => "echo", :path => %w{/bin /usr/bin /usr/local/bin})
 
       @catalog.add_resource(resource)
@@ -531,11 +531,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
       @catalog.resource("notify", "other").should equal(@one)
     end
 
-    it "should ignore conflicting aliases that point to the aliased resource" do
-      @catalog.alias(@one, "other")
-      lambda { @catalog.alias(@one, "other") }.should_not raise_error
-    end
-
     it "should fail to add an alias if the aliased name already exists" do
       @catalog.add_resource @one
       proc { @catalog.alias @two, "one" }.should raise_error(ArgumentError)
@@ -589,6 +584,58 @@ describe Puppet::Resource::Catalog, "when compiling" do
       @catalog.create_resource :file, args
       @catalog.resource("File[/yay]").should equal(resource)
     end
+
+    describe "when adding resources with multiple namevars" do
+      before :each do
+        Puppet::Type.newtype(:multiple) do
+          newparam(:color, :namevar => true)
+          newparam(:designation, :namevar => true)
+
+          def self.title_patterns
+            [ [
+                /^(\w+) (\w+)$/,
+                [
+                  [:color,  lambda{|x| x}],
+                  [:designation, lambda{|x| x}]
+                ]
+            ] ]
+          end
+        end
+      end
+
+      it "should add an alias using the uniqueness key" do
+        @resource = Puppet::Type.type(:multiple).new(:title => "some resource", :color => "red", :designation => "5")
+
+        @catalog.add_resource(@resource)
+        @catalog.resource(:multiple, "some resource").must == @resource
+        @catalog.resource("Multiple[some resource]").must == @resource
+        @catalog.resource("Multiple[red 5]").must == @resource
+      end
+
+      it "should conflict with a resource with the same uniqueness key" do
+        @resource = Puppet::Type.type(:multiple).new(:title => "some resource", :color => "red", :designation => "5")
+        @other    = Puppet::Type.type(:multiple).new(:title => "another resource", :color => "red", :designation => "5")
+
+        @catalog.add_resource(@resource)
+        expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias Multiple\[another resource\] to \["red", "5"\].*resource \["Multiple", "red", "5"\] already defined/)
+      end
+
+      it "should conflict when its uniqueness key matches another resource's title" do
+        @resource = Puppet::Type.type(:file).new(:title => "/tmp/foo")
+        @other    = Puppet::Type.type(:file).new(:title => "another file", :path => "/tmp/foo")
+
+        @catalog.add_resource(@resource)
+        expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias File\[another file\] to \["\/tmp\/foo"\].*resource \["File", "\/tmp\/foo"\] already defined/)
+      end
+
+      it "should conflict when its uniqueness key matches the uniqueness key derived from another resource's title" do
+        @resource = Puppet::Type.type(:multiple).new(:title => "red leader")
+        @other    = Puppet::Type.type(:multiple).new(:title => "another resource", :color => "red", :designation => "leader")
+
+        @catalog.add_resource(@resource)
+        expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias Multiple\[another resource\] to \["red", "leader"\].*resource \["Multiple", "red", "leader"\] already defined/)
+      end
+    end
   end
 
   describe "when applying" do