Use object ID when detected unprocessed Resources (#5604)

* Use object ID when detected unprocessed Resources

In the Data Collector, when detecting unprocessed resources, a Set
was built up using the resource object itself as the Set element.
Internally, Set adds this to a Hash.

We allow users to create custom resources that could contain a property
named "hash" which in turn wires up a `#hash` instance method on the
Resource. Ruby expects object's `#hash` method to return a Fixnum that
it uses internally. So if a resource had a "hash" property that returned
a String, bad things happened.

With this change, we make our own Hash and use the resource's object ID
in the key so we don't have to worry about the resource's `#hash` method
getting called and throwing an exception. I will send up a separate
change that warns users when they choose a property name that is already
an existing method name.

Fixes #5565.

Signed-off-by: Adam Leff <adam@leff.co>

2 files changed, 72 insertions, 9 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index df3bce6167..acd42c355e 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -430,26 +430,33 @@ class Chef
       end
 
       def detect_unprocessed_resources
-        # create a Set containing all resource+action combinations from
-        # the Resource Collection
-        collection_resources = Set.new
+        # create a Hash (for performance reasons, rather than an Array) containing all
+        # resource+action combinations from the Resource Collection
+        #
+        # We use the object ID instead of the resource itself in the Hash key because
+        # we currently allow users to create a property called "hash" which creates
+        # a #hash instance method on the resource. Ruby expects that to be a Fixnum,
+        # so bad things happen when adding an object to an Array or a Hash if it's not.
+        collection_resources = {}
         run_context.resource_collection.all_resources.each do |resource|
           Array(resource.action).each do |action|
-            collection_resources.add([resource, action])
+            collection_resources[[resource.__id__, action]] = resource
           end
         end
 
-        # Delete from the Set any resource+action combination we have
+        # Delete from the Hash any resource+action combination we have
         # already processed.
         all_resource_reports.each do |report|
-          collection_resources.delete([report.resource, report.action])
+          collection_resources.delete([report.resource.__id__, report.action])
         end
 
-        # The items remaining in the Set are unprocessed resource+actions,
+        # The items remaining in the Hash are unprocessed resource+actions,
         # so we'll create new resource reports for them which default to
         # a state of "unprocessed".
-        collection_resources.each do |resource, action|
-          add_resource_report(create_resource_report(resource, action))
+        collection_resources.each do |key, resource|
+          # The Hash key is an array of the Resource's object ID and the action.
+          # We need to pluck out the action.
+          add_resource_report(create_resource_report(resource, key[1]))
         end
       end
 
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index 4ae227ba3c..f3f7ffb30f 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -682,4 +682,60 @@ describe Chef::DataCollector::Reporter do
       end
     end
   end
+
+  describe "#detect_unprocessed_resources" do
+    context "when resources do not override core methods" do
+      it "adds resource reports for any resources that have not yet been processed" do
+        resource_a  = Chef::Resource::Service.new("processed service")
+        resource_b  = Chef::Resource::Service.new("unprocessed service")
+
+        resource_a.action = [ :enable, :start ]
+        resource_b.action = :start
+
+        run_context = Chef::RunContext.new(Chef::Node.new, Chef::CookbookCollection.new, nil)
+        run_context.resource_collection.insert(resource_a)
+        run_context.resource_collection.insert(resource_b)
+
+        allow(reporter).to receive(:run_context).and_return(run_context)
+
+        # process the actions for resource_a, but not resource_b
+        reporter.resource_up_to_date(resource_a, :enable)
+        reporter.resource_completed(resource_a)
+        reporter.resource_up_to_date(resource_a, :start)
+        reporter.resource_completed(resource_a)
+        expect(reporter.all_resource_reports.size).to eq(2)
+
+        # detect unprocessed resources, which should find that resource_b has not yet been processed
+        reporter.send(:detect_unprocessed_resources)
+        expect(reporter.all_resource_reports.size).to eq(3)
+      end
+    end
+
+    context "when a resource overrides a core method, such as #hash" do
+      it "does not raise an exception" do
+        resource_a  = Chef::Resource::Service.new("processed service")
+        resource_b  = Chef::Resource::Service.new("unprocessed service")
+
+        resource_a.action = :start
+        resource_b.action = :start
+
+        run_context = Chef::RunContext.new(Chef::Node.new, Chef::CookbookCollection.new, nil)
+        run_context.resource_collection.insert(resource_a)
+        run_context.resource_collection.insert(resource_b)
+
+        allow(reporter).to receive(:run_context).and_return(run_context)
+
+        # override the #hash method on resource_a to return a String instead of
+        # a Fixnum. Without the fix in chef/chef#5604, this would raise an
+        # exception when getting added to the Set/Hash.
+        resource_a.define_singleton_method(:hash) { "a string" }
+
+        # process the actions for resource_a, but not resource_b
+        reporter.resource_up_to_date(resource_a, :start)
+        reporter.resource_completed(resource_a)
+
+        expect { reporter.send(:detect_unprocessed_resources) }.not_to raise_error
+      end
+    end
+  end
 end