Finishing all code changes to split resource collection into 2 data containers

17 files changed, 131 insertions, 184 deletions

diff --git a/lib/chef/formatters/doc.rb b/lib/chef/formatters/doc.rb
index 4a08b9d095..2e102a82c6 100644
--- a/lib/chef/formatters/doc.rb
+++ b/lib/chef/formatters/doc.rb
@@ -143,7 +143,7 @@ class Chef
 
       # Called before convergence starts
       def converge_start(run_context)
-        puts_line "Converging #{run_context.resource_collection.all_resources.size} resources"
+        puts_line "Converging #{run_context.resource_list.all_resources.size} resources"
       end
 
       # Called when the converge phase is finished.
diff --git a/lib/chef/formatters/minimal.rb b/lib/chef/formatters/minimal.rb
index a189cc67eb..eaf7e8f40f 100644
--- a/lib/chef/formatters/minimal.rb
+++ b/lib/chef/formatters/minimal.rb
@@ -144,7 +144,7 @@ class Chef
 
       # Called before convergence starts
       def converge_start(run_context)
-        puts "Converging #{run_context.resource_collection.all_resources.size} resources"
+        puts "Converging #{run_context.resource_list.all_resources.size} resources"
       end
 
       # Called when the converge phase is finished.
diff --git a/lib/chef/provider/breakpoint.rb b/lib/chef/provider/breakpoint.rb
index 224e2758eb..caa8bd5231 100644
--- a/lib/chef/provider/breakpoint.rb
+++ b/lib/chef/provider/breakpoint.rb
@@ -25,9 +25,9 @@ class Chef
 
       def action_break
         if defined?(Shell) && Shell.running?
-          run_context.resource_collection.iterator.pause
+          run_context.resource_list.iterator.pause
           @new_resource.updated_by_last_action(true)
-          run_context.resource_collection.iterator
+          run_context.resource_list.iterator
         end
       end
 
diff --git a/lib/chef/provider/deploy.rb b/lib/chef/provider/deploy.rb
index db147278c2..d55dc35f94 100644
--- a/lib/chef/provider/deploy.rb
+++ b/lib/chef/provider/deploy.rb
@@ -375,7 +375,8 @@ class Chef
 
       def gem_resource_collection_runner
         gems_collection = Chef::ResourceCollection.new
-        gem_packages.each { |rbgem| gems_collection << rbgem }
+        # We don't need to populated the resource_set because converge simply runs through each resource and applies them
+        gem_packages.each { |rbgem| gems_collection.resource_list << rbgem }
         gems_run_context = run_context.dup
         gems_run_context.resource_collection = gems_collection
         Chef::Runner.new(gems_run_context)
diff --git a/lib/chef/provider/lwrp_base.rb b/lib/chef/provider/lwrp_base.rb
index 135a3f6b7c..ce55b00729 100644
--- a/lib/chef/provider/lwrp_base.rb
+++ b/lib/chef/provider/lwrp_base.rb
@@ -62,7 +62,7 @@ class Chef
           return_value
         ensure
           @run_context = saved_run_context
-          if temp_run_context.resource_collection.any? {|r| r.updated? }
+          if temp_run_context.resource_list.any? {|r| r.updated? }
             new_resource.updated_by_last_action(true)
           end
         end
diff --git a/lib/chef/provider/route.rb b/lib/chef/provider/route.rb
index 208a4f4139..d430860f04 100644
--- a/lib/chef/provider/route.rb
+++ b/lib/chef/provider/route.rb
@@ -162,7 +162,7 @@ class Chef::Provider::Route < Chef::Provider
       case node[:platform]
       when "centos", "redhat", "fedora"
         # walk the collection
-        run_context.resource_collection.each do |resource|
+        run_context.resource_list.each do |resource|
           if resource.is_a? Chef::Resource::Route
             # default to eth0
             if resource.device
diff --git a/lib/chef/recipe.rb b/lib/chef/recipe.rb
index 32578da5ab..599c451ce4 100644
--- a/lib/chef/recipe.rb
+++ b/lib/chef/recipe.rb
@@ -79,7 +79,7 @@ class Chef
     # Used by the DSL to look up resources when executing in the context of a
     # recipe.
     def resources(*args)
-      run_context.resource_collection.find(*args)
+      run_context.resource_set.find(*args)
     end
 
     # This was moved to Chef::Node#tag, redirecting here for compatability
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 52c0b95591..377a0c4aef 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -49,22 +49,22 @@ class Chef
 
       # If resource and/or notifying_resource is not a resource object, this will look them up in the resource collection
       # and fix the references from strings to actual Resource objects.
-      def resolve_resource_reference(resource_collection)
+      def resolve_resource_reference(resource_set)
         return resource if resource.kind_of?(Chef::Resource) && notifying_resource.kind_of?(Chef::Resource)
 
         if not(resource.kind_of?(Chef::Resource))
-          fix_resource_reference(resource_collection)
+          fix_resource_reference(resource_set)
         end
 
         if not(notifying_resource.kind_of?(Chef::Resource))
-          fix_notifier_reference(resource_collection)
+          fix_notifier_reference(resource_set)
         end
       end
 
       # This will look up the resource if it is not a Resource Object.  It will complain if it finds multiple
       # resources, can't find a resource, or gets invalid syntax.
-      def fix_resource_reference(resource_collection)
-        matching_resource = resource_collection.find(resource)
+      def fix_resource_reference(resource_set)
+        matching_resource = resource_set.find(resource)
         if Array(matching_resource).size > 1
           msg = "Notification #{self} from #{notifying_resource} was created with a reference to multiple resources, "\
           "but can only notify one resource. Notifying resource was defined on #{notifying_resource.source_line}"
@@ -92,8 +92,8 @@ F
 
       # This will look up the notifying_resource if it is not a Resource Object.  It will complain if it finds multiple
       # resources, can't find a resource, or gets invalid syntax.
-      def fix_notifier_reference(resource_collection)
-        matching_notifier = resource_collection.find(notifying_resource)
+      def fix_notifier_reference(resource_set)
+        matching_notifier = resource_set.find(notifying_resource)
         if Array(matching_notifier).size > 1
           msg = "Notification #{self} from #{notifying_resource} was created with a reference to multiple notifying "\
           "resources, but can only originate from one resource.  Destination resource was defined "\
@@ -298,10 +298,9 @@ F
       end
     end
 
-    # 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(resource_type, instance_name)
+        prior_resource = run_context.resource_set.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 #{::Chef::ResourceSet.create_key(resource_type, resource_name)} from prior resource (CHEF-3694)")
@@ -446,8 +445,8 @@ F
     # resolve_resource_reference on each in turn, causing them to
     # resolve lazy/forward references.
     def resolve_notification_references
-      run_context.immediate_notifications(self).each { |n| n.resolve_resource_reference(run_context.resource_collection) }
-      run_context.delayed_notifications(self).each {|n| n.resolve_resource_reference(run_context.resource_collection) }
+      run_context.immediate_notifications(self).each { |n| n.resolve_resource_reference(run_context.resource_set) }
+      run_context.delayed_notifications(self).each {|n| n.resolve_resource_reference(run_context.resource_set) }
     end
 
     def notifies_immediately(action, resource_spec)
@@ -467,7 +466,7 @@ F
     end
 
     def resources(*args)
-      run_context.resource_collection.find(*args)
+      run_context.resource_set.find(*args)
     end
 
     def subscribes(action, resources, timing=:delayed)
diff --git a/lib/chef/resource_collection.rb b/lib/chef/resource_collection.rb
index 2cac42703e..f62245686a 100644
--- a/lib/chef/resource_collection.rb
+++ b/lib/chef/resource_collection.rb
@@ -19,148 +19,52 @@
 
 require 'chef/resource_set'
 require 'chef/resource_list'
+require 'chef/resource_collection/resource_collection_serialization'
 
+##
+# TODO add class documentation
 class Chef
   class ResourceCollection
+    include ResourceCollectionSerialization
+
+    attr_reader :resource_set, :resource_list
 
     def initialize
       @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
-      @resource_list.all_resources
-    end
-
-    def [](index)
-      @resource_list[index]
-    end
-
-    def []=(index, arg)
-      @resource_list[index] = arg
-    end
-
-    def <<(*args)
-      @resource_list.send(:<<, *args)
-    end
-
-    # 'push' is an alias method to <<
-    alias_method :push, :<<
-
-    def insert(resource)
-      @resource_list.insert(resource)
-    end
-
-    def insert_at(insert_at_index, *resources)
-      @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
-      @resource_list.each
-    end
-
-    def execute_each_resource(&resource_exec_block)
-      @resource_list.execute_each_resource(&resource_exec_block)
-    end
-
-    def each_index
-      @resource_list.each_index
-    end
-
-    def empty?
-      @resources_list.empty?
-    end
-
-    def lookup(resource_type, instance_name)
-      @resource_set.lookup(resource_type, instance_name)
-    end
-
-    def find(*args)
-      @resource_list.find(*args)
-    end
+    # TODO insert calls we might need?
+    # :insert, :insert_at, :[]=, :<<, :push
+    # :insert_as
 
-    # resources is a poorly named, but we have to maintain it for back
-    # compat.
-    alias_method :resources, :find
+    # TODO when there were 2 resources with the same key in resource_set, how do we handle notifications since they get copied?
+    # Did the old class only keep the last seen reference?
 
-    private
+    # TODO do we need to implement a dup method?  Run_context was shallowly copying resource_collection before
 
-      def find_resource_by_hash(arg)
-        results = Array.new
-        arg.each do |resource_name, name_list|
-          names = name_list.kind_of?(Array) ? name_list : [ name_list ]
-          names.each do |name|
-            res_name = "#{resource_name.to_s}[#{name}]"
-            results << lookup(res_name)
-          end
-        end
-        return results
-      end
+    RESOURCE_LIST_METHODS = Enumerable.instance_methods +
+        [:all_resources, :[], :each, :execute_each_resource, :each_index, :empty?]
+    RESOURCE_SET_METHODS = [:lookup, :find, :resources]
 
-      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 |name|
-            resource_name = "#{resource_type}[#{name}]"
-            results << lookup(resource_name)
-          end
-        when SINGLE_RESOURCE_MATCH
-          resource_type = $1
-          name = $2
-          resource_name = "#{resource_type}[#{name}]"
-          results << lookup(resource_name)
-        else
-          raise ArgumentError, "Bad string format #{arg}, you must have a string like resource_type[name]!"
-        end
-        return results
+    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
-
-      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
+      if block
+        proxy.send(name, *args, &block)
+      else
+        proxy.send(name, *args)
       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_collection/resource_collection_serialization.rb b/lib/chef/resource_collection/resource_collection_serialization.rb
new file mode 100644
index 0000000000..708e72d848
--- /dev/null
+++ b/lib/chef/resource_collection/resource_collection_serialization.rb
@@ -0,0 +1,53 @@
+#
+# 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.
+#
+class Chef
+  class ResourceCollection
+    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
+
+      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
+end
diff --git a/lib/chef/resource_list.rb b/lib/chef/resource_list.rb
index 883906d01a..1392c6b557 100644
--- a/lib/chef/resource_list.rb
+++ b/lib/chef/resource_list.rb
@@ -18,9 +18,11 @@
 
 require 'chef/resource'
 require 'chef/resource_collection/stepable_iterator'
+require 'chef/resource_collection/resource_collection_serialization'
 
 class Chef
   class ResourceList
+    include ResourceCollection::ResourceCollectionSerialization
     include Enumerable
 
     attr_reader :iterator
@@ -81,7 +83,7 @@ class Chef
     end
 
     def execute_each_resource(&resource_exec_block)
-      @iterator = StepableIterator.for_collection(@resources)
+      @iterator = ResourceCollection::StepableIterator.for_collection(@resources)
       @iterator.each_with_index do |resource, idx|
         @insert_after_idx = idx
         yield resource
@@ -98,17 +100,5 @@ class Chef
       @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
index 45e69918cf..e0e2038f78 100644
--- a/lib/chef/resource_set.rb
+++ b/lib/chef/resource_set.rb
@@ -17,9 +17,11 @@
 #
 
 require 'chef/resource'
+require 'chef/resource_collection/resource_collection_serialization'
 
 class Chef
   class ResourceSet
+    include ResourceCollection::ResourceCollectionSerialization
 
     # Matches a multiple resource lookup specification,
     # e.g., "service[nginx,unicorn]"
@@ -33,15 +35,22 @@ class Chef
       @resources_by_key = Hash.new
     end
 
+    def keys
+      @resources_by_key.keys
+    end
+
     def insert_as(resource_type, instance_name, resource)
-      assert_chef_resource(resource)
-      @resources_by_key[create_key(resource_type, instance_name)] = resource
+      is_chef_resource(resource)
+      key = ResourceSet.create_key(resource_type, instance_name)
+      @resources_by_key[key] = 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)]
+      raise ArgumentError, "Must pass resource_type as a String or Symbol and instance_name as a String" unless
+          (resource_type.kind_of?(String) || resource_type.kind_of?(Symbol)) && instance_name.kind_of?(String)
+      key = ResourceSet.create_key(resource_type, instance_name)
+      res = @resources_by_key[key]
       unless res
         raise Chef::Exceptions::ResourceNotFound, "Cannot find a resource matching #{key} (did you define it first?)"
       end
@@ -79,7 +88,6 @@ class Chef
 
     # 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
@@ -111,8 +119,6 @@ class Chef
       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
@@ -150,13 +156,5 @@ class Chef
       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 39773f2ceb..b77a2af9b0 100644
--- a/lib/chef/run_context.rb
+++ b/lib/chef/run_context.rb
@@ -18,8 +18,6 @@
 # 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'
@@ -51,10 +49,14 @@ 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
+
+    def resource_set
+      resource_collection.resource_set
+    end
+
+    def resource_list
+      resource_collection.resource_list
+    end
 
     # A Hash containing the immediate notifications triggered by resources
     # during the converge phase of the chef run.
diff --git a/lib/chef/run_status.rb b/lib/chef/run_status.rb
index 0f181426b0..d47a35459c 100644
--- a/lib/chef/run_status.rb
+++ b/lib/chef/run_status.rb
@@ -70,13 +70,13 @@ class Chef::RunStatus
 
   # The list of all resources in the current run context's +resource_collection+
   def all_resources
-    @run_context && @run_context.resource_collection.all_resources
+    @run_context && @run_context.resource_list.all_resources
   end
 
   # The list of all resources in the current run context's +resource_collection+
   # that are marked as updated
   def updated_resources
-    @run_context && @run_context.resource_collection.select { |r| r.updated }
+    @run_context && @run_context.resource_list.select { |r| r.updated }
   end
 
   # The backtrace from +exception+, if any
diff --git a/lib/chef/runner.rb b/lib/chef/runner.rb
index 6125fe59e1..e27e250b21 100644
--- a/lib/chef/runner.rb
+++ b/lib/chef/runner.rb
@@ -72,12 +72,12 @@ class Chef
     # +run_action+ for each resource in turn.
     def converge
       # Resolve all lazy/forward references in notifications
-      run_context.resource_collection.each do |resource|
+      run_context.resource_list.each do |resource|
         resource.resolve_notification_references
       end
 
       # Execute each resource.
-      run_context.resource_collection.execute_each_resource do |resource|
+      run_context.resource_list.execute_each_resource do |resource|
         Array(resource.action).each {|action| run_action(resource, action)}
       end
 
diff --git a/lib/chef/shell/ext.rb b/lib/chef/shell/ext.rb
index bc4e955169..ea11d9d1a8 100644
--- a/lib/chef/shell/ext.rb
+++ b/lib/chef/shell/ext.rb
@@ -244,7 +244,7 @@ E
                   :skip_back    => "move back in the run list",
                   :skip_forward => "move forward in the run list"
       def chef_run
-        Shell.session.resource_collection.iterator
+        Shell.session.resource_list.iterator
       end
 
       desc "resets the current recipe"
@@ -547,7 +547,7 @@ E
       desc "list all the resources on the current recipe"
       def resources(*args)
         if args.empty?
-          pp run_context.resource_collection.instance_variable_get(:@resources_by_name).keys
+          pp run_context.resource_set.keys
         else
           pp resources = original_resources(*args)
           resources
diff --git a/lib/chef/shell/shell_session.rb b/lib/chef/shell/shell_session.rb
index 73e6c34ebb..6486a5ebeb 100644
--- a/lib/chef/shell/shell_session.rb
+++ b/lib/chef/shell/shell_session.rb
@@ -69,8 +69,8 @@ module Shell
       @node.consume_attributes(@node_attributes)
     end
 
-    def resource_collection
-      run_context.resource_collection
+    def resource_list
+      run_context.resource_list
     end
 
     def run_context