Cleaning up based on review comments

5 files changed, 199 insertions, 217 deletions

diff --git a/lib/chef/dsl/recipe.rb b/lib/chef/dsl/recipe.rb
index b110371036..72de9e3662 100644
--- a/lib/chef/dsl/recipe.rb
+++ b/lib/chef/dsl/recipe.rb
@@ -86,7 +86,7 @@ class Chef
 
         resource = build_resource(type, name, created_at, &resource_attrs_block)
 
-        run_context.resource_collection.insert(resource, type, name)
+        run_context.resource_collection.insert(resource, resource_type:type, instance_name:name)
         resource
       end
 
diff --git a/lib/chef/resource_collection.rb b/lib/chef/resource_collection.rb
index 2e202bd6eb..8a02651d7d 100644
--- a/lib/chef/resource_collection.rb
+++ b/lib/chef/resource_collection.rb
@@ -21,6 +21,7 @@ require 'chef/resource_collection/resource_set'
 require 'chef/resource_collection/resource_list'
 require 'chef/resource_collection/resource_collection_serialization'
 require 'chef/log'
+require 'forwardable'
 
 ##
 # ResourceCollection currently handles two tasks:
@@ -29,6 +30,7 @@ require 'chef/log'
 class Chef
   class ResourceCollection
     include ResourceCollectionSerialization
+    extend Forwardable
 
     def initialize
       @resource_set = ResourceSet.new
@@ -42,13 +44,13 @@ class Chef
     # If you know the at_location but not the resource_type or instance_name, pass them in as nil
     # This method is meant to be the 1 insert method necessary in the future.  It should support all known use cases
     #   for writing into the ResourceCollection.
-    def insert(resource, resource_type=nil, instance_name=nil, at_location=nil)
+    def insert(resource, resource_type:nil, instance_name:nil, at_location:nil)
       if at_location
         @resource_list.insert_at(at_location, resource)
       else
         @resource_list.insert(resource)
       end
-      unless resource_type.nil? || instance_name.nil?
+      if !(resource_type.nil? && instance_name.nil?)
         @resource_set.insert_as(resource, resource_type, instance_name)
       else
         @resource_set.insert_as(resource)
@@ -85,28 +87,15 @@ class Chef
     # @deprecated
     alias_method :push, :<<
 
-    # Read-only methods are simple to proxy - doing that below
+    # Read-only methods are simple to delegate - doing that below
 
     RESOURCE_LIST_METHODS = Enumerable.instance_methods +
         [:iterator, :all_resources, :[], :each, :execute_each_resource, :each_index, :empty?] -
         [:find] # find needs to run on the set
     RESOURCE_SET_METHODS = [:lookup, :find, :resources, :keys, :validate_lookup_spec!]
 
-    def method_missing(name, *args, &block)
-      if RESOURCE_LIST_METHODS.include?(name)
-        proxy = @resource_list
-      elsif RESOURCE_SET_METHODS.include?(name)
-        proxy = @resource_set
-      else
-        raise NoMethodError.new("ResourceCollection does not proxy `#{name}`", name, args)
-      end
-      if block
-        proxy.send(name, *args, &block)
-      else
-        proxy.send(name, *args)
-      end
-
-    end
+    def_delegators :@resource_list, *RESOURCE_LIST_METHODS
+    def_delegators :@resource_set, *RESOURCE_SET_METHODS
 
   end
 end
diff --git a/lib/chef/resource_collection/resource_set.rb b/lib/chef/resource_collection/resource_set.rb
index e3287951d7..6425c2ab08 100644
--- a/lib/chef/resource_collection/resource_set.rb
+++ b/lib/chef/resource_collection/resource_set.rb
@@ -40,8 +40,10 @@ class Chef
         @resources_by_key.keys
       end
 
-      def insert_as(resource, resource_type=resource.resource_name, instance_name=resource.name)
+      def insert_as(resource, resource_type=nil, instance_name=nil)
         is_chef_resource!(resource)
+        resource_type ||= resource.resource_name
+        instance_name ||= resource.name
         key = ResourceSet.create_key(resource_type, instance_name)
         @resources_by_key[key] = resource
       end
diff --git a/spec/unit/resource_collection/resource_set_spec.rb b/spec/unit/resource_collection/resource_set_spec.rb
index 8a9c8aa80a..29b676f85a 100644
--- a/spec/unit/resource_collection/resource_set_spec.rb
+++ b/spec/unit/resource_collection/resource_set_spec.rb
@@ -18,200 +18,182 @@
 
 require 'spec_helper'
 
-class Chef
-  class ResourceCollection
-    describe ResourceSet do
-      let(:subject) { ResourceSet.new }
-
-      let(:zen_master_name) { "Neo" }
-      let(:zen_master2_name) { "Morpheus" }
-      let(:zen_follower_name) { "Squid" }
-      let(:zen_master) {
-        r = Chef::Resource::ZenMaster.new(zen_master_name)
-        r
-      }
-      let(:zen_master2) {
-        r = Chef::Resource::ZenMaster.new(zen_master2_name)
-        r
-      }
-      let(:zen_follower) {
-        r = Chef::Resource::ZenFollower.new(zen_follower_name)
-        r
-      }
-
-      describe "initialize" do
-        it "should return a Chef::ResourceCollection" do
-          expect(subject).to be_instance_of(ResourceSet)
-        end
-      end
-
-      describe "keys" do
-        it "should return an empty list for an empty ResourceSet" do
-          expect(subject.keys).to eq([])
-        end
-
-        it "should return the keys for a non-empty ResourceSet" do
-          subject.insert_as(zen_master)
-          expect(subject.keys).to eq(["zen_master[Neo]"])
-        end
-
-        it "should return the keys for a non-empty ResourceSet with custom type and name" do
-          subject.insert_as(zen_master, "OtherResource", "other_resource")
-          expect(subject.keys).to eq(["OtherResource[other_resource]"])
-        end
-      end
-
-      describe "insert_as, lookup and find" do
-        # To validate insert_as you need lookup, and vice-versa - putting all tests in 1 context to avoid duplication
-        it "should accept only Chef::Resources" do
-          expect { subject.insert_as(zen_master) }.to_not raise_error
-          expect { subject.insert_as("string") }.to raise_error
-        end
-
-        it "should allow you to lookup resources by a default .to_s" do
-          subject.insert_as(zen_master)
-          expect(subject.lookup(zen_master.to_s)).to equal(zen_master)
-        end
-
-        it "should use a custom type and name to insert" do
-          subject.insert_as(zen_master, "OtherResource", "other_resource")
-          expect(subject.lookup("OtherResource[other_resource]")).to equal(zen_master)
-        end
-
-        it "should raise an exception if you send something strange to lookup" do
-          expect { subject.lookup(:symbol) }.to raise_error(ArgumentError)
-        end
-
-        it "should raise an exception if it cannot find a resource with lookup" do
-          expect { subject.lookup(zen_master.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
-        end
-
-        it "should find a resource by type symbol and name" do
-          subject.insert_as(zen_master)
-          expect(subject.find(:zen_master => zen_master_name)).to equal(zen_master)
-        end
-
-        it "should find a resource by type symbol and array of names" do
-          subject.insert_as(zen_master)
-          subject.insert_as(zen_master2)
-          check_by_names(subject.find(:zen_master => [zen_master_name,zen_master2_name]), zen_master_name, zen_master2_name)
-        end
-
-        it "should find a resource by type symbol and array of names with custom names" do
-          subject.insert_as(zen_master, :zzz, "name1")
-          subject.insert_as(zen_master2, :zzz, "name2")
-          check_by_names(subject.find( :zzz => ["name1","name2"]), zen_master_name, zen_master2_name)
-        end
-
-        it "should find resources of multiple kinds (:zen_master => a, :zen_follower => b)" do
-          subject.insert_as(zen_master)
-          subject.insert_as(zen_follower)
-          check_by_names(subject.find(:zen_master => [zen_master_name], :zen_follower => [zen_follower_name]),
-                         zen_master_name, zen_follower_name)
-        end
-
-        it "should find resources of multiple kinds (:zen_master => a, :zen_follower => b with custom names)" do
-          subject.insert_as(zen_master, :zzz, "name1")
-          subject.insert_as(zen_master2, :zzz, "name2")
-          subject.insert_as(zen_follower, :yyy, "name3")
-          check_by_names(subject.find(:zzz => ["name1","name2"], :yyy => ["name3"]),
-                         zen_master_name, zen_follower_name, zen_master2_name)
-        end
-
-        it "should find a resource by string zen_master[a]" do
-          subject.insert_as(zen_master)
-          expect(subject.find("zen_master[#{zen_master_name}]")).to eq(zen_master)
-        end
-
-        it "should find a resource by string zen_master[a] with custom names" do
-          subject.insert_as(zen_master, :zzz, "name1")
-          expect(subject.find("zzz[name1]")).to eq(zen_master)
-        end
-
-        it "should find resources by strings of zen_master[a,b]" do
-          subject.insert_as(zen_master)
-          subject.insert_as(zen_master2)
-          check_by_names(subject.find("zen_master[#{zen_master_name},#{zen_master2_name}]"),
-                         zen_master_name, zen_master2_name)
-        end
-
-        it "should find resources by strings of zen_master[a,b] with custom names" do
-          subject.insert_as(zen_master, :zzz, "name1")
-          subject.insert_as(zen_master2, :zzz, "name2")
-          check_by_names(subject.find("zzz[name1,name2]"),
-                         zen_master_name, zen_master2_name)
-        end
-
-        it "should find resources of multiple types by strings of zen_master[a]" do
-          subject.insert_as(zen_master)
-          subject.insert_as(zen_follower)
-          check_by_names(subject.find("zen_master[#{zen_master_name}]", "zen_follower[#{zen_follower_name}]"),
-                         zen_master_name, zen_follower_name)
-        end
-
-        it "should find resources of multiple types by strings of zen_master[a] with custom names" do
-          subject.insert_as(zen_master, :zzz, "name1")
-          subject.insert_as(zen_master2, :zzz, "name2")
-          subject.insert_as(zen_follower, :yyy, "name3")
-          check_by_names(subject.find("zzz[name1,name2]", "yyy[name3]"),
-                         zen_master_name, zen_follower_name,zen_master2_name)
-        end
-
-        it "should only keep the last copy when multiple instances of a Resource are inserted" do
-          subject.insert_as(zen_master)
-          expect(subject.find("zen_master[#{zen_master_name}]")).to eq(zen_master)
-          new_zm =zen_master.dup
-          new_zm.retries = 10
-          expect(new_zm).to_not eq(zen_master)
-          subject.insert_as(new_zm)
-          expect(subject.find("zen_master[#{zen_master_name}]")).to eq(new_zm)
-        end
-
-        it "should raise an exception if you pass a bad name to resources" do
-          expect { subject.find("michael jackson") }.to raise_error(ArgumentError)
-        end
-
-        it "should raise an exception if you pass something other than a string or hash to resource" do
-          expect { subject.find([Array.new]) }.to raise_error(ArgumentError)
-        end
-
-        it "raises an error when attempting to find a resource that does not exist" do
-          expect { subject.find("script[nonesuch]") }.to raise_error(Chef::Exceptions::ResourceNotFound)
-        end
-      end
-
-      describe "validate_lookup_spec!" do
-        it "accepts a string of the form 'resource_type[resource_name]'" do
-          expect(subject.validate_lookup_spec!("resource_type[resource_name]")).to be_true
-        end
-
-        it "accepts a single-element :resource_type => 'resource_name' Hash" do
-          expect(subject.validate_lookup_spec!(:service => "apache2")).to be_true
-        end
-
-        it "accepts a chef resource object" do
-          expect(subject.validate_lookup_spec!(zen_master)).to be_true
-        end
-
-        it "rejects a malformed query string" do
-          expect { subject.validate_lookup_spec!("resource_type[missing-end-bracket") }.to \
-            raise_error(Chef::Exceptions::InvalidResourceSpecification)
-        end
-
-        it "rejects an argument that is not a String, Hash, or Chef::Resource" do
-          expect { subject.validate_lookup_spec!(Object.new) }.to \
-            raise_error(Chef::Exceptions::InvalidResourceSpecification)
-        end
-
-      end
-
-      def check_by_names(results, *names)
-        expect(results.size).to eq(names.size)
-        names.each do |name|
-          expect(results.detect{|r| r.name == name}).to_not eq(nil)
-        end
-      end
+describe Chef::ResourceCollection::ResourceSet do
+  let(:collection) { Chef::ResourceCollection::ResourceSet.new }
+
+  let(:zen_master_name) { "Neo" }
+  let(:zen_master2_name) { "Morpheus" }
+  let(:zen_follower_name) { "Squid" }
+  let(:zen_master) { Chef::Resource::ZenMaster.new(zen_master_name) }
+  let(:zen_master2) { Chef::Resource::ZenMaster.new(zen_master2_name) }
+  let(:zen_follower) { Chef::Resource::ZenFollower.new(zen_follower_name) }
+
+  describe "initialize" do
+    it "should return a Chef::ResourceCollection" do
+      expect(collection).to be_instance_of(Chef::ResourceCollection::ResourceSet)
+    end
+  end
+
+  describe "keys" do
+    it "should return an empty list for an empty ResourceSet" do
+      expect(collection.keys).to eq([])
+    end
 
+    it "should return the keys for a non-empty ResourceSet" do
+      collection.instance_variable_get(:@resources_by_key)["key"] = nil
+      expect(collection.keys).to eq(["key"])
     end
   end
+
+  describe "insert_as, lookup and find" do
+    # To validate insert_as you need lookup, and vice-versa - putting all tests in 1 context to avoid duplication
+    it "should accept only Chef::Resources" do
+      expect { collection.insert_as(zen_master) }.to_not raise_error
+      expect { collection.insert_as("string") }.to raise_error(ArgumentError)
+    end
+
+    it "should allow you to lookup resources by a default .to_s" do
+      collection.insert_as(zen_master)
+      expect(collection.lookup(zen_master.to_s)).to equal(zen_master)
+    end
+
+    it "should use a custom type and name to insert" do
+      collection.insert_as(zen_master, "OtherResource", "other_resource")
+      expect(collection.lookup("OtherResource[other_resource]")).to equal(zen_master)
+    end
+
+    it "should raise an exception if you send something strange to lookup" do
+      expect { collection.lookup(:symbol) }.to raise_error(ArgumentError)
+    end
+
+    it "should raise an exception if it cannot find a resource with lookup" do
+      expect { collection.lookup(zen_master.to_s) }.to raise_error(Chef::Exceptions::ResourceNotFound)
+    end
+
+    it "should find a resource by type symbol and name" do
+      collection.insert_as(zen_master)
+      expect(collection.find(:zen_master => zen_master_name)).to equal(zen_master)
+    end
+
+    it "should find a resource by type symbol and array of names" do
+      collection.insert_as(zen_master)
+      collection.insert_as(zen_master2)
+      check_by_names(collection.find(:zen_master => [zen_master_name,zen_master2_name]), zen_master_name, zen_master2_name)
+    end
+
+    it "should find a resource by type symbol and array of names with custom names" do
+      collection.insert_as(zen_master, :zzz, "name1")
+      collection.insert_as(zen_master2, :zzz, "name2")
+      check_by_names(collection.find( :zzz => ["name1","name2"]), zen_master_name, zen_master2_name)
+    end
+
+    it "should find resources of multiple kinds (:zen_master => a, :zen_follower => b)" do
+      collection.insert_as(zen_master)
+      collection.insert_as(zen_follower)
+      check_by_names(collection.find(:zen_master => [zen_master_name], :zen_follower => [zen_follower_name]),
+                     zen_master_name, zen_follower_name)
+    end
+
+    it "should find resources of multiple kinds (:zen_master => a, :zen_follower => b with custom names)" do
+      collection.insert_as(zen_master, :zzz, "name1")
+      collection.insert_as(zen_master2, :zzz, "name2")
+      collection.insert_as(zen_follower, :yyy, "name3")
+      check_by_names(collection.find(:zzz => ["name1","name2"], :yyy => ["name3"]),
+                     zen_master_name, zen_follower_name, zen_master2_name)
+    end
+
+    it "should find a resource by string zen_master[a]" do
+      collection.insert_as(zen_master)
+      expect(collection.find("zen_master[#{zen_master_name}]")).to eq(zen_master)
+    end
+
+    it "should find a resource by string zen_master[a] with custom names" do
+      collection.insert_as(zen_master, :zzz, "name1")
+      expect(collection.find("zzz[name1]")).to eq(zen_master)
+    end
+
+    it "should find resources by strings of zen_master[a,b]" do
+      collection.insert_as(zen_master)
+      collection.insert_as(zen_master2)
+      check_by_names(collection.find("zen_master[#{zen_master_name},#{zen_master2_name}]"),
+                     zen_master_name, zen_master2_name)
+    end
+
+    it "should find resources by strings of zen_master[a,b] with custom names" do
+      collection.insert_as(zen_master, :zzz, "name1")
+      collection.insert_as(zen_master2, :zzz, "name2")
+      check_by_names(collection.find("zzz[name1,name2]"),
+                     zen_master_name, zen_master2_name)
+    end
+
+    it "should find resources of multiple types by strings of zen_master[a]" do
+      collection.insert_as(zen_master)
+      collection.insert_as(zen_follower)
+      check_by_names(collection.find("zen_master[#{zen_master_name}]", "zen_follower[#{zen_follower_name}]"),
+                     zen_master_name, zen_follower_name)
+    end
+
+    it "should find resources of multiple types by strings of zen_master[a] with custom names" do
+      collection.insert_as(zen_master, :zzz, "name1")
+      collection.insert_as(zen_master2, :zzz, "name2")
+      collection.insert_as(zen_follower, :yyy, "name3")
+      check_by_names(collection.find("zzz[name1,name2]", "yyy[name3]"),
+                     zen_master_name, zen_follower_name,zen_master2_name)
+    end
+
+    it "should only keep the last copy when multiple instances of a Resource are inserted" do
+      collection.insert_as(zen_master)
+      expect(collection.find("zen_master[#{zen_master_name}]")).to eq(zen_master)
+      new_zm =zen_master.dup
+      new_zm.retries = 10
+      expect(new_zm).to_not eq(zen_master)
+      collection.insert_as(new_zm)
+      expect(collection.find("zen_master[#{zen_master_name}]")).to eq(new_zm)
+    end
+
+    it "should raise an exception if you pass a bad name to resources" do
+      expect { collection.find("michael jackson") }.to raise_error(ArgumentError)
+    end
+
+    it "should raise an exception if you pass something other than a string or hash to resource" do
+      expect { collection.find([Array.new]) }.to raise_error(ArgumentError)
+    end
+
+    it "raises an error when attempting to find a resource that does not exist" do
+      expect { collection.find("script[nonesuch]") }.to raise_error(Chef::Exceptions::ResourceNotFound)
+    end
+  end
+
+  describe "validate_lookup_spec!" do
+    it "accepts a string of the form 'resource_type[resource_name]'" do
+      expect(collection.validate_lookup_spec!("resource_type[resource_name]")).to be_true
+    end
+
+    it "accepts a single-element :resource_type => 'resource_name' Hash" do
+      expect(collection.validate_lookup_spec!(:service => "apache2")).to be_true
+    end
+
+    it "accepts a chef resource object" do
+      expect(collection.validate_lookup_spec!(zen_master)).to be_true
+    end
+
+    it "rejects a malformed query string" do
+      expect { collection.validate_lookup_spec!("resource_type[missing-end-bracket") }.to \
+        raise_error(Chef::Exceptions::InvalidResourceSpecification)
+    end
+
+    it "rejects an argument that is not a String, Hash, or Chef::Resource" do
+      expect { collection.validate_lookup_spec!(Object.new) }.to \
+        raise_error(Chef::Exceptions::InvalidResourceSpecification)
+    end
+
+  end
+
+  def check_by_names(results, *names)
+    expect(results.size).to eq(names.size)
+    names.each do |name|
+      expect(results.detect{|r| r.name == name}).to_not eq(nil)
+    end
+  end
+
 end
diff --git a/spec/unit/resource_collection_spec.rb b/spec/unit/resource_collection_spec.rb
index 631e3730d8..81a203af6f 100644
--- a/spec/unit/resource_collection_spec.rb
+++ b/spec/unit/resource_collection_spec.rb
@@ -26,6 +26,10 @@ describe Chef::ResourceCollection do
     @resource = Chef::Resource::ZenMaster.new("makoto")
   end
 
+  it "should throw an error when calling a non-delegated method" do
+    expect { @rc.not_a_method }.to raise_error(NoMethodError)
+  end
+
   describe "initialize" do
     it "should return a Chef::ResourceCollection" do
       @rc.should be_kind_of(Chef::ResourceCollection)
@@ -35,7 +39,7 @@ describe Chef::ResourceCollection do
   describe "[]" do
     it "should accept Chef::Resources through [index]" do
       lambda { @rc[0] = @resource }.should_not raise_error
-      lambda { @rc[0] = "string" }.should raise_error
+      lambda { @rc[0] = "string" }.should raise_error(ArgumentError)
     end
 
     it "should allow you to fetch Chef::Resources by position" do
@@ -47,7 +51,7 @@ describe Chef::ResourceCollection do
   describe "push" do
     it "should accept Chef::Resources through pushing" do
       lambda { @rc.push(@resource) }.should_not raise_error
-      lambda { @rc.push("string") }.should raise_error
+      lambda { @rc.push("string") }.should raise_error(ArgumentError)
     end
   end
 
@@ -60,7 +64,12 @@ describe Chef::ResourceCollection do
   describe "insert" do
     it "should accept only Chef::Resources" do
       lambda { @rc.insert(@resource) }.should_not raise_error
-      lambda { @rc.insert("string") }.should raise_error
+      lambda { @rc.insert("string") }.should raise_error(ArgumentError)
+    end
+
+    it "should accept named arguments in any order" do
+      @rc.insert(@resource, at_location:0, instance_name:'foo', resource_type:'bar')
+      expect(@rc[0]).to eq(@resource)
     end
 
     it "should append resources to the end of the collection when not executing a run" do
@@ -92,7 +101,7 @@ describe Chef::ResourceCollection do
     it "should accept only Chef::Resources" do
       lambda { @rc.insert_at(0, @resource, @resource) }.should_not raise_error
       lambda { @rc.insert_at(0, "string") }.should raise_error
-      lambda { @rc.insert_at(0, @resource, "string") }.should raise_error
+      lambda { @rc.insert_at(0, @resource, "string") }.should raise_error(ArgumentError)
     end
 
     it "should toss an error if it receives a bad index" do