first attempt at refactoring ResourceCollection into two objects

6 files changed, 345 insertions, 174 deletions

diff --git a/lib/chef/dsl/recipe.rb b/lib/chef/dsl/recipe.rb
index 476d9db666..8578f41d92 100644
--- a/lib/chef/dsl/recipe.rb
+++ b/lib/chef/dsl/recipe.rb
@@ -86,21 +86,8 @@ class Chef
 
         resource = build_resource(type, name, created_at, &resource_attrs_block)
 
-        # Some resources (freebsd_package) can be invoked with multiple names
-        # (package || freebsd_package).
-        # https://github.com/opscode/chef/issues/1773
-        # For these resources we want to make sure
-        # their key in resource collection is same as the name they are declared
-        # as. Since this might be a breaking change for resources that define
-        # customer to_s methods, we are working around the issue by letting
-        # resources know of their created_as_type until this issue is fixed in
-        # Chef 12:
-        # https://github.com/opscode/chef/issues/1817
-        if resource.respond_to?(:created_as_type=)
-          resource.created_as_type = type
-        end
-
-        run_context.resource_collection.insert(resource)
+        run_context.resource_set.insert_as(type, name, resource)
+        run_context.resource_list << resource
         resource
       end
 
@@ -124,7 +111,7 @@ class Chef
         # This behavior is very counter-intuitive and should be removed.
         # See CHEF-3694, https://tickets.opscode.com/browse/CHEF-3694
         # Moved to this location to resolve CHEF-5052, https://tickets.opscode.com/browse/CHEF-5052
-        resource.load_prior_resource
+        resource.load_prior_resource(type, name)
         resource.cookbook_name = cookbook_name
         resource.recipe_name = recipe_name
         # Determine whether this resource is being created in the context of an enclosing Provider
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 70abfbcdb0..52c0b95591 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -27,6 +27,7 @@ require 'chef/guard_interpreter/resource_guard_interpreter'
 require 'chef/resource/conditional'
 require 'chef/resource/conditional_action_not_nothing'
 require 'chef/resource_collection'
+require 'chef/resource_set'
 require 'chef/resource_platform_map'
 require 'chef/node'
 require 'chef/platform'
@@ -297,12 +298,13 @@ F
       end
     end
 
-    def load_prior_resource
+    # TODO can we perform this without having the resource know its declared_type?
+    def load_prior_resource(resource_type, instance_name)
       begin
-        prior_resource = run_context.resource_collection.lookup(self.to_s)
+        prior_resource = run_context.resource_collection.lookup(resource_type, instance_name)
         # if we get here, there is a prior resource (otherwise we'd have jumped
         # to the rescue clause).
-        Chef::Log.warn("Cloning resource attributes for #{self.to_s} from prior resource (CHEF-3694)")
+        Chef::Log.warn("Cloning resource attributes for #{::Chef::ResourceSet.create_key(resource_type, resource_name)} from prior resource (CHEF-3694)")
         Chef::Log.warn("Previous #{prior_resource}: #{prior_resource.source_line}") if prior_resource.source_line
         Chef::Log.warn("Current  #{self}: #{self.source_line}") if self.source_line
         prior_resource.instance_variables.each do |iv|
@@ -483,7 +485,7 @@ F
     end
 
     def validate_resource_spec!(resource_spec)
-      run_context.resource_collection.validate_lookup_spec!(resource_spec)
+      ::Chef::ResourceSet.validate_lookup_spec!(resource_spec)
     end
 
     def is(*args)
diff --git a/lib/chef/resource_collection.rb b/lib/chef/resource_collection.rb
index cc14a03962..2cac42703e 100644
--- a/lib/chef/resource_collection.rb
+++ b/lib/chef/resource_collection.rb
@@ -17,209 +17,83 @@
 # limitations under the License.
 #
 
-require 'chef/resource'
-require 'chef/resource_collection/stepable_iterator'
+require 'chef/resource_set'
+require 'chef/resource_list'
 
 class Chef
   class ResourceCollection
-    include Enumerable
-
-    # Matches a multiple resource lookup specification,
-    # e.g., "service[nginx,unicorn]"
-    MULTIPLE_RESOURCE_MATCH = /^(.+)\[(.+?),(.+)\]$/
-
-    # Matches a single resource lookup specification,
-    # e.g., "service[nginx]"
-    SINGLE_RESOURCE_MATCH = /^(.+)\[(.+)\]$/
-
-    attr_reader :iterator
 
     def initialize
-      @resources = Array.new
-      @resources_by_name = Hash.new
-      @insert_after_idx = nil
+      @resource_set = ResourceSet.new
+      @resource_list = ResourceList.new
     end
 
+    # TODO proxy all calls with
+    # http://simonecarletti.com/blog/2010/05/understanding-ruby-and-rails-proxy-patter-delegation-and-basicobject/
+
+    # TODO fundamentally we want to write objects into 2 different data containers.  We can proxy reads, but it is
+    # much harder to proxy writes through 1 object.
+
     def all_resources
-      @resources
+      @resource_list.all_resources
     end
 
     def [](index)
-      @resources[index]
+      @resource_list[index]
     end
 
     def []=(index, arg)
-      is_chef_resource(arg)
-      @resources[index] = arg
-      @resources_by_name[arg.to_s] = index
+      @resource_list[index] = arg
     end
 
     def <<(*args)
-      args.flatten.each do |a|
-        is_chef_resource(a)
-        @resources << a
-        @resources_by_name[a.to_s] = @resources.length - 1
-      end
-      self
+      @resource_list.send(:<<, *args)
     end
 
     # 'push' is an alias method to <<
     alias_method :push, :<<
 
     def insert(resource)
-      if @insert_after_idx
-        # in the middle of executing a run, so any resources inserted now should
-        # be placed after the most recent addition done by the currently executing
-        # resource
-        insert_at(@insert_after_idx + 1, resource)
-        @insert_after_idx += 1
-      else
-        is_chef_resource(resource)
-        @resources << resource
-        @resources_by_name[resource.to_s] = @resources.length - 1
-      end
+      @resource_list.insert(resource)
     end
 
     def insert_at(insert_at_index, *resources)
-      resources.each do |resource|
-        is_chef_resource(resource)
-      end
-      @resources.insert(insert_at_index, *resources)
-      # update name -> location mappings and register new resource
-      @resources_by_name.each_key do |key|
-        @resources_by_name[key] += resources.size if @resources_by_name[key] >= insert_at_index
-      end
-      resources.each_with_index do |resource, i|
-        @resources_by_name[resource.to_s] = insert_at_index + i
-      end
+      @resource_list.insert_at(insert_at_index, *resources)
+    end
+
+    def insert_as(resource_type, instance_name, resource)
+      # TODO how does this compete with the above 2 methods?  How do I combine them?
+      @resource_set.insert_as(resource_type, instance_name, resource)
     end
 
     def each
-      @resources.each do |resource|
-        yield resource
-      end
+      @resource_list.each
     end
 
     def execute_each_resource(&resource_exec_block)
-      @iterator = StepableIterator.for_collection(@resources)
-      @iterator.each_with_index do |resource, idx|
-        @insert_after_idx = idx
-        yield resource
-      end
+      @resource_list.execute_each_resource(&resource_exec_block)
     end
 
     def each_index
-      @resources.each_index do |i|
-        yield i
-      end
+      @resource_list.each_index
     end
 
     def empty?
-      @resources.empty?
+      @resources_list.empty?
     end
 
-    def lookup(resource)
-      lookup_by = nil
-      if resource.kind_of?(Chef::Resource)
-        lookup_by = resource.to_s
-      elsif resource.kind_of?(String)
-        lookup_by = resource
-      else
-        raise ArgumentError, "Must pass a Chef::Resource or String to lookup"
-      end
-      res = @resources_by_name[lookup_by]
-      unless res
-        raise Chef::Exceptions::ResourceNotFound, "Cannot find a resource matching #{lookup_by} (did you define it first?)"
-      end
-      @resources[res]
+    def lookup(resource_type, instance_name)
+      @resource_set.lookup(resource_type, instance_name)
     end
 
-    # Find existing resources by searching the list of existing resources.  Possible
-    # forms are:
-    #
-    # find(:file => "foobar")
-    # find(:file => [ "foobar", "baz" ])
-    # find("file[foobar]", "file[baz]")
-    # find("file[foobar,baz]")
-    #
-    # Returns the matching resource, or an Array of matching resources.
-    #
-    # Raises an ArgumentError if you feed it bad lookup information
-    # Raises a Runtime Error if it can't find the resources you are looking for.
     def find(*args)
-      results = Array.new
-      args.each do |arg|
-        case arg
-        when Hash
-          results << find_resource_by_hash(arg)
-        when String
-          results << find_resource_by_string(arg)
-        else
-          msg = "arguments to #{self.class.name}#find should be of the form :resource => 'name' or resource[name]"
-          raise Chef::Exceptions::InvalidResourceSpecification, msg
-        end
-      end
-      flat_results = results.flatten
-      flat_results.length == 1 ? flat_results[0] : flat_results
+      @resource_list.find(*args)
     end
 
     # resources is a poorly named, but we have to maintain it for back
     # compat.
     alias_method :resources, :find
 
-    # Returns true if +query_object+ is a valid string for looking up a
-    # resource, or raises InvalidResourceSpecification if not.
-    # === Arguments
-    # * query_object should be a string of the form
-    # "resource_type[resource_name]", a single element Hash (e.g., :service =>
-    # "apache2"), or a Chef::Resource (this is the happy path). Other arguments
-    # will raise an exception.
-    # === Returns
-    # * true returns true for all valid input.
-    # === Raises
-    # * Chef::Exceptions::InvalidResourceSpecification for all invalid input.
-    def validate_lookup_spec!(query_object)
-      case query_object
-      when Chef::Resource
-        true
-      when SINGLE_RESOURCE_MATCH, MULTIPLE_RESOURCE_MATCH
-        true
-      when Hash
-        true
-      when String
-        raise Chef::Exceptions::InvalidResourceSpecification,
-          "The string `#{query_object}' is not valid for resource collection lookup. Correct syntax is `resource_type[resource_name]'"
-      else
-        raise Chef::Exceptions::InvalidResourceSpecification,
-          "The object `#{query_object.inspect}' is not valid for resource collection lookup. " +
-          "Use a String like `resource_type[resource_name]' or a Chef::Resource object"
-      end
-    end
-
-    # Serialize this object as a hash
-    def to_hash
-      instance_vars = Hash.new
-      self.instance_variables.each do |iv|
-        instance_vars[iv] = self.instance_variable_get(iv)
-      end
-      {
-        'json_class' => self.class.name,
-        'instance_vars' => instance_vars
-      }
-    end
-
-    def to_json(*a)
-      Chef::JSONCompat.to_json(to_hash, *a)
-    end
-
-    def self.json_create(o)
-      collection = self.new()
-      o["instance_vars"].each do |k,v|
-        collection.instance_variable_set(k.to_sym, v)
-      end
-      collection
-    end
-
     private
 
       def find_resource_by_hash(arg)
@@ -263,4 +137,30 @@ class Chef
         true
       end
   end
+
+  module ResourceCollectionSerialization
+    # Serialize this object as a hash
+    def to_hash
+      instance_vars = Hash.new
+      self.instance_variables.each do |iv|
+        instance_vars[iv] = self.instance_variable_get(iv)
+      end
+      {
+          'json_class' => self.class.name,
+          'instance_vars' => instance_vars
+      }
+    end
+
+    def to_json(*a)
+      Chef::JSONCompat.to_json(to_hash, *a)
+    end
+
+    def self.json_create(o)
+      collection = self.new()
+      o["instance_vars"].each do |k,v|
+        collection.instance_variable_set(k.to_sym, v)
+      end
+      collection
+    end
+  end
 end
diff --git a/lib/chef/resource_list.rb b/lib/chef/resource_list.rb
new file mode 100644
index 0000000000..883906d01a
--- /dev/null
+++ b/lib/chef/resource_list.rb
@@ -0,0 +1,114 @@
+#
+# Author:: Tyler Ball (<tball@getchef.com>)
+# Copyright:: Copyright (c) 2014 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'chef/resource'
+require 'chef/resource_collection/stepable_iterator'
+
+class Chef
+  class ResourceList
+    include Enumerable
+
+    attr_reader :iterator
+
+    def initialize
+      @resources = Array.new
+      @insert_after_idx = nil
+    end
+
+    def all_resources
+      @resources
+    end
+
+    def [](index)
+      @resources[index]
+    end
+
+    def []=(index, arg)
+      is_chef_resource(arg)
+      @resources[index] = arg
+    end
+
+    def <<(*args)
+      args.flatten.each do |a|
+        is_chef_resource(a)
+        @resources << a
+      end
+      self
+    end
+
+    # 'push' is an alias method to <<
+    alias_method :push, :<<
+
+    def insert(resource)
+      if @insert_after_idx
+        # in the middle of executing a run, so any resources inserted now should
+        # be placed after the most recent addition done by the currently executing
+        # resource
+        insert_at(@insert_after_idx + 1, resource)
+        @insert_after_idx += 1
+      else
+        is_chef_resource(resource)
+        @resources << resource
+      end
+    end
+
+    def insert_at(insert_at_index, *resources)
+      resources.each do |resource|
+        is_chef_resource(resource)
+      end
+      @resources.insert(insert_at_index, *resources)
+    end
+
+    def each
+      @resources.each do |resource|
+        yield resource
+      end
+    end
+
+    def execute_each_resource(&resource_exec_block)
+      @iterator = StepableIterator.for_collection(@resources)
+      @iterator.each_with_index do |resource, idx|
+        @insert_after_idx = idx
+        yield resource
+      end
+    end
+
+    def each_index
+      @resources.each_index do |i|
+        yield i
+      end
+    end
+
+    def empty?
+      @resources.empty?
+    end
+
+    # TODO the json serialization/unserialization could be put into a module
+
+    private
+
+    # TODO put into common module
+    def is_chef_resource(arg)
+      unless arg.kind_of?(Chef::Resource)
+        raise ArgumentError, "Cannot insert a #{arg.class} into a resource collection: must be a subclass of Chef::Resource"
+      end
+      true
+    end
+
+  end
+end
diff --git a/lib/chef/resource_set.rb b/lib/chef/resource_set.rb
new file mode 100644
index 0000000000..45e69918cf
--- /dev/null
+++ b/lib/chef/resource_set.rb
@@ -0,0 +1,162 @@
+#
+# Author:: Tyler Ball (<tball@getchef.com>)
+# Copyright:: Copyright (c) 2014 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'chef/resource'
+
+class Chef
+  class ResourceSet
+
+    # Matches a multiple resource lookup specification,
+    # e.g., "service[nginx,unicorn]"
+    MULTIPLE_RESOURCE_MATCH = /^(.+)\[(.+?),(.+)\]$/
+
+    # Matches a single resource lookup specification,
+    # e.g., "service[nginx]"
+    SINGLE_RESOURCE_MATCH = /^(.+)\[(.+)\]$/
+
+    def initialize
+      @resources_by_key = Hash.new
+    end
+
+    def insert_as(resource_type, instance_name, resource)
+      assert_chef_resource(resource)
+      @resources_by_key[create_key(resource_type, instance_name)] = resource
+    end
+
+    # TODO when do we need to lookup a resource by resource?  That smells like we should still have the resource say how its indexed, or we shouldn't be doing that.
+    def lookup(resource_type, instance_name)
+      raise ArgumentError, "Must pass a String to lookup" unless key.kind_of?(String)
+      res = @resources_by_key[create_key(resource_type, instance_name)]
+      unless res
+        raise Chef::Exceptions::ResourceNotFound, "Cannot find a resource matching #{key} (did you define it first?)"
+      end
+      res
+    end
+
+    # Find existing resources by searching the list of existing resources.  Possible
+    # forms are:
+    #
+    # find(:file => "foobar")
+    # find(:file => [ "foobar", "baz" ])
+    # find("file[foobar]", "file[baz]")
+    # find("file[foobar,baz]")
+    #
+    # Returns the matching resource, or an Array of matching resources.
+    #
+    # Raises an ArgumentError if you feed it bad lookup information
+    # Raises a Runtime Error if it can't find the resources you are looking for.
+    def find(*args)
+      results = Array.new
+      args.each do |arg|
+        case arg
+          when Hash
+            results << find_resource_by_hash(arg)
+          when String
+            results << find_resource_by_string(arg)
+          else
+            msg = "arguments to #{self.class.name}#find should be of the form :resource => 'name' or 'resource[name]'"
+            raise Chef::Exceptions::InvalidResourceSpecification, msg
+        end
+      end
+      flat_results = results.flatten
+      flat_results.length == 1 ? flat_results[0] : flat_results
+    end
+
+    # resources is a poorly named, but we have to maintain it for back
+    # compat.
+    # TODO do we anymore?
+    alias_method :resources, :find
+
+    # Returns true if +query_object+ is a valid string for looking up a
+    # resource, or raises InvalidResourceSpecification if not.
+    # === Arguments
+    # * query_object should be a string of the form
+    # "resource_type[resource_name]", a single element Hash (e.g., :service =>
+    # "apache2"), or a Chef::Resource (this is the happy path). Other arguments
+    # will raise an exception.
+    # === Returns
+    # * true returns true for all valid input.
+    # === Raises
+    # * Chef::Exceptions::InvalidResourceSpecification for all invalid input.
+    def self.validate_lookup_spec!(query_object)
+      case query_object
+        when Chef::Resource
+          true
+        when SINGLE_RESOURCE_MATCH, MULTIPLE_RESOURCE_MATCH
+          true
+        when Hash
+          true
+        when String
+          raise Chef::Exceptions::InvalidResourceSpecification,
+                "The string `#{query_object}' is not valid for resource collection lookup. Correct syntax is `resource_type[resource_name]'"
+        else
+          raise Chef::Exceptions::InvalidResourceSpecification,
+                "The object `#{query_object.inspect}' is not valid for resource collection lookup. " +
+                    "Use a String like `resource_type[resource_name]' or a Chef::Resource object"
+      end
+    end
+
+    # TODO the json serialization/unserialization could be put into a module
+
+    def self.create_key(resource_type, instance_name)
+      "#{resource_type}[#{instance_name}]"
+    end
+
+    private
+
+    def find_resource_by_hash(arg)
+      results = Array.new
+      arg.each do |resource_type, name_list|
+        instance_names = name_list.kind_of?(Array) ? name_list : [ name_list ]
+        instance_names.each do |instance_name|
+          results << lookup(resource_type, instance_name)
+        end
+      end
+      return results
+    end
+
+    def find_resource_by_string(arg)
+      results = Array.new
+      case arg
+        when MULTIPLE_RESOURCE_MATCH
+          resource_type = $1
+          arg =~ /^.+\[(.+)\]$/
+          resource_list = $1
+          resource_list.split(",").each do |instance_name|
+            results << lookup(resource_type, instance_name)
+          end
+        when SINGLE_RESOURCE_MATCH
+          resource_type = $1
+          name = $2
+          results << lookup(resource_type, name)
+        else
+          raise ArgumentError, "Bad string format #{arg}, you must have a string like resource_type[name]!"
+      end
+      return results
+    end
+
+    # TODO put into common module
+    def assert_chef_resource(arg)
+      unless arg.kind_of?(Chef::Resource)
+        raise ArgumentError, "Cannot insert a #{arg.class} into a resource collection: must be a subclass of Chef::Resource"
+      end
+      true
+    end
+
+  end
+end
diff --git a/lib/chef/run_context.rb b/lib/chef/run_context.rb
index bbe2f9eba0..39773f2ceb 100644
--- a/lib/chef/run_context.rb
+++ b/lib/chef/run_context.rb
@@ -18,6 +18,8 @@
 # limitations under the License.
 
 require 'chef/resource_collection'
+require 'chef/resource_set'
+require 'chef/resource_list'
 require 'chef/cookbook_version'
 require 'chef/node'
 require 'chef/role'
@@ -49,6 +51,10 @@ class Chef
     # The Chef::ResourceCollection for this run. Populated by evaluating
     # recipes, which is triggered by #load. (See also: CookbookCompiler)
     attr_accessor :resource_collection
+    # TODO do I even want to expose these, or run everything through the facade?  Writers don't really care about
+    # populating both, I mostly separated them
+    attr_accessor :resource_set
+    attr_accessor :resource_list
 
     # A Hash containing the immediate notifications triggered by resources
     # during the converge phase of the chef run.