Ensure Data Collector resource report exists before updatingadamleff/fix-data-collector

In certain cases when using custom resources, we have seen evidence
of resource_updated and resource_failed events getting fired before
the DataCollector instance has a valid resource report created to
track such updates/failures.

This change ensures a resource report exists before attempting to
modify it, regardless of what order these events may fire.

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

2 files changed, 22 insertions, 25 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index 82c41edfdc..dbf1150dc9 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -174,12 +174,13 @@ class Chef
       # resource, and we only care about tracking top-level resources.
       def resource_current_state_loaded(new_resource, action, current_resource)
         return if nested_resource?(new_resource)
-        update_current_resource_report(create_resource_report(new_resource, action, current_resource))
+        initialize_resource_report_if_needed(new_resource, action, current_resource)
       end
 
       # see EventDispatch::Base#resource_up_to_date
       # Mark our ResourceReport status accordingly
       def resource_up_to_date(new_resource, action)
+        initialize_resource_report_if_needed(new_resource, action)
         current_resource_report.up_to_date unless nested_resource?(new_resource)
       end
 
@@ -190,15 +191,15 @@ class Chef
       def resource_skipped(new_resource, action, conditional)
         return if nested_resource?(new_resource)
 
-        resource_report = create_resource_report(new_resource, action)
-        resource_report.skipped(conditional)
-        update_current_resource_report(resource_report)
+        initialize_resource_report_if_needed(new_resource, action)
+        current_resource_report.skipped(conditional)
       end
 
       # see EventDispatch::Base#resource_updated
       # Flag the current ResourceReport instance as updated (as long as it's
       # a top-level resource).
       def resource_updated(new_resource, action)
+        initialize_resource_report_if_needed(new_resource, action)
         current_resource_report.updated unless nested_resource?(new_resource)
       end
 
@@ -207,6 +208,7 @@ class Chef
       # long as it's a top-level resource, and update the run error text
       # with the proper Formatter.
       def resource_failed(new_resource, action, exception)
+        initialize_resource_report_if_needed(new_resource, action)
         current_resource_report.failed(exception) unless nested_resource?(new_resource)
         update_error_description(
           Formatters::ErrorMapper.resource_failed(
@@ -224,7 +226,7 @@ class Chef
         if current_resource_report && !nested_resource?(new_resource)
           current_resource_report.finish
           add_resource_report(current_resource_report)
-          update_current_resource_report(nil)
+          clear_current_resource_report
         end
       end
 
@@ -402,10 +404,6 @@ class Chef
         @run_status = run_status
       end
 
-      def update_current_resource_report(resource_report)
-        @current_resource_report = resource_report
-      end
-
       def update_error_description(discription_hash)
         @error_descriptions = discription_hash
       end
@@ -414,6 +412,11 @@ class Chef
         @deprecations << { message: message, url: url, location: location }
       end
 
+      def initialize_resource_report_if_needed(new_resource, action, current_resource = nil)
+        return unless current_resource_report.nil?
+        @current_resource_report = create_resource_report(new_resource, action, current_resource)
+      end
+
       def create_resource_report(new_resource, action, current_resource = nil)
         Chef::DataCollector::ResourceReport.new(
           new_resource,
@@ -422,6 +425,10 @@ class Chef
         )
       end
 
+      def clear_current_resource_report
+        @current_resource_report = nil
+      end
+
       def detect_unprocessed_resources
         # create a Set containing all resource+action combinations from
         # the Resource Collection
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index b3e2d931a7..4ae227ba3c 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -371,12 +371,10 @@ describe Chef::DataCollector::Reporter do
     end
 
     context "when resource is not a nested resource" do
-      it "creates the resource report and stores it as the current one" do
+      it "initializes the resource report" do
         allow(reporter).to receive(:nested_resource?).and_return(false)
-        expect(reporter).to receive(:create_resource_report)
+        expect(reporter).to receive(:initialize_resource_report_if_needed)
           .with(new_resource, action, current_resource)
-          .and_return(resource_report)
-        expect(reporter).to receive(:update_current_resource_report).with(resource_report)
         reporter.resource_current_state_loaded(new_resource, action, current_resource)
       end
     end
@@ -418,7 +416,6 @@ describe Chef::DataCollector::Reporter do
 
     before do
       allow(reporter).to receive(:nested_resource?)
-      allow(reporter).to receive(:create_resource_report).and_return(resource_report)
       allow(resource_report).to receive(:skipped)
     end
 
@@ -431,17 +428,10 @@ describe Chef::DataCollector::Reporter do
     end
 
     context "when the resource is not a nested resource" do
-      it "creates the resource report and stores it as the current one" do
+      it "initializes the resource report and marks it as skipped" do
         allow(reporter).to receive(:nested_resource?).and_return(false)
-        expect(reporter).to receive(:create_resource_report)
-          .with(new_resource, action)
-          .and_return(resource_report)
-        expect(reporter).to receive(:update_current_resource_report).with(resource_report)
-        reporter.resource_skipped(new_resource, action, conditional)
-      end
-
-      it "marks the resource report as skipped" do
-        allow(reporter).to receive(:nested_resource?).with(new_resource).and_return(false)
+        allow(reporter).to receive(:current_resource_report).and_return(resource_report)
+        expect(reporter).to receive(:initialize_resource_report_if_needed).with(new_resource, action)
         expect(resource_report).to receive(:skipped).with(conditional)
         reporter.resource_skipped(new_resource, action, conditional)
       end
@@ -548,7 +538,7 @@ describe Chef::DataCollector::Reporter do
         end
 
         it "nils out the current resource report" do
-          expect(reporter).to receive(:update_current_resource_report).with(nil)
+          expect(reporter).to receive(:clear_current_resource_report)
           reporter.resource_completed(new_resource)
         end
       end