Merge pull request #3295 from chef/lcg/dont-mutate-new-resource

don't mutate the new resource

6 files changed, 64 insertions, 18 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fe53d3ee41..fed2167bf2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,8 +6,10 @@
 
 * Convert bootstrap template to use sh #2877
 * [Issue #3316](https://github.com/chef/chef/issues/3316) Fix idempotency issues with the `windows_package` resource
+* [pr#3295](https://github.com/chef/chef/pull/3295): Stop mutating `new_resource.checksum` in file providers.  Fixes some ChecksumMismatch exceptions like [issue#3168](https://github.com/chef/chef/issues/3168)
 
 ## 12.3.0
+
 * [pr#3160](https://github.com/chef/chef/pull/3160): Use Chef Zero in
   socketless mode for local mode, add `--no-listen` flag to disable port
   binding
diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb
index c070d29458..fda8ad2e5e 100644
--- a/lib/chef/provider/file.rb
+++ b/lib/chef/provider/file.rb
@@ -386,10 +386,11 @@ class Chef
 
       def update_file_contents
         do_backup unless needs_creating?
-        deployment_strategy.deploy(tempfile.path, ::File.realpath(@new_resource.path))
-        Chef::Log.info("#{@new_resource} updated file contents #{@new_resource.path}")
+        deployment_strategy.deploy(tempfile.path, ::File.realpath(new_resource.path))
+        Chef::Log.info("#{new_resource} updated file contents #{new_resource.path}")
         if managing_content?
-          @new_resource.checksum(checksum(@new_resource.path)) # for reporting
+          # save final checksum for reporting.
+          new_resource.final_checksum = checksum(new_resource.path)
         end
       end
 
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index d934ec8c47..3d319617d1 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -467,7 +467,7 @@ class Chef
     #
     # @return [Hash{Symbol => Object}] A Hash of attribute => value for the
     #   Resource class's `state_attrs`.
-    def state
+    def state_for_resource_reporter
       self.class.state_attrs.inject({}) do |state_attrs, attr_name|
         state_attrs[attr_name] = send(attr_name)
         state_attrs
@@ -475,6 +475,15 @@ class Chef
     end
 
     #
+    # Since there are collisions with LWRP parameters named 'state' this
+    # method is not used by the resource_reporter and is most likely unused.
+    # It certainly cannot be relied upon and cannot be fixed.
+    #
+    # @deprecated
+    #
+    alias_method :state, :state_for_resource_reporter
+
+    #
     # The value of the identity attribute, if declared. Falls back to #name if
     # no identity attribute is declared.
     #
diff --git a/lib/chef/resource/file.rb b/lib/chef/resource/file.rb
index 53a6a160af..b36fcc2135 100644
--- a/lib/chef/resource/file.rb
+++ b/lib/chef/resource/file.rb
@@ -38,6 +38,15 @@ class Chef
 
       attr_writer :checksum
 
+      #
+      # The checksum of the rendered file.  This has to be saved on the
+      # new_resource for the 'after' state for reporting but we cannot
+      # mutate the new_resource.checksum which would change the
+      # user intent in the new_resource if the resource is reused.
+      #
+      # @returns [String] Checksum of the file we actually rendered
+      attr_accessor :final_checksum
+
       provides :file
 
       def initialize(name, run_context=nil)
@@ -129,6 +138,15 @@ class Chef
           @verifications
         end
       end
+
+      def state_for_resource_reporter
+        state_attrs = super()
+        # fix up checksum state with final_checksum saved by the provider
+        if checksum.nil? && final_checksum
+          state_attrs[:checksum] = final_checksum
+        end
+        state_attrs
+      end
     end
   end
 end
diff --git a/lib/chef/resource_reporter.rb b/lib/chef/resource_reporter.rb
index 1816fc857d..96cc01d814 100644
--- a/lib/chef/resource_reporter.rb
+++ b/lib/chef/resource_reporter.rb
@@ -62,8 +62,8 @@ class Chef
         as_hash["type"]   = new_resource.class.dsl_name
         as_hash["name"]   = new_resource.name.to_s
         as_hash["id"]     = new_resource.identity.to_s
-        as_hash["after"]  = state(new_resource)
-        as_hash["before"] = current_resource ? state(current_resource) : {}
+        as_hash["after"]  = new_resource.state_for_resource_reporter
+        as_hash["before"] = current_resource ? current_resource.state_for_resource_reporter : {}
         as_hash["duration"] = (elapsed_time * 1000).to_i.to_s
         as_hash["delta"]  = new_resource.diff if new_resource.respond_to?("diff")
         as_hash["delta"]  = "" if as_hash["delta"].nil?
@@ -89,13 +89,6 @@ class Chef
       def success?
         !self.exception
       end
-
-      def state(r)
-        r.class.state_attrs.inject({}) do |state_attrs, attr_name|
-          state_attrs[attr_name] = r.send(attr_name)
-          state_attrs
-        end
-      end
     end # End class ResouceReport
 
     attr_reader :updated_resources
diff --git a/spec/support/shared/unit/provider/file.rb b/spec/support/shared/unit/provider/file.rb
index 86f32c9e89..018be0a015 100644
--- a/spec/support/shared/unit/provider/file.rb
+++ b/spec/support/shared/unit/provider/file.rb
@@ -529,26 +529,49 @@ shared_examples_for Chef::Provider::File do
                                   :for_reporting => diff_for_reporting )
             allow(diff).to receive(:diff).with(resource_path, tempfile_path).and_return(true)
             expect(provider).to receive(:diff).at_least(:once).and_return(diff)
-            expect(provider).to receive(:managing_content?).at_least(:once).and_return(true)
             expect(provider).to receive(:checksum).with(tempfile_path).and_return(tempfile_sha256)
-            expect(provider).to receive(:checksum).with(resource_path).and_return(tempfile_sha256)
+            allow(provider).to receive(:managing_content?).and_return(true)
+            allow(provider).to receive(:checksum).with(resource_path).and_return(tempfile_sha256)
+            expect(resource).not_to receive(:checksum).with(tempfile_sha256)  # do not mutate the new resource
             expect(provider.deployment_strategy).to receive(:deploy).with(tempfile_path, normalized_path)
           end
           context "when the file was created" do
             before { expect(provider).to receive(:needs_creating?).at_least(:once).and_return(true) }
-            it "does not backup the file and does not produce a diff for reporting" do
+            it "does not backup the file" do
               expect(provider).not_to receive(:do_backup)
               provider.send(:do_contents_changes)
+            end
+
+            it "does not produce a diff for reporting" do
+              provider.send(:do_contents_changes)
               expect(resource.diff).to be_nil
             end
+
+            it "renders the final checksum correctly for reporting" do
+              provider.send(:do_contents_changes)
+              expect(resource.state_for_resource_reporter[:checksum]).to eql(tempfile_sha256)
+            end
           end
           context "when the file was not created" do
-            before { expect(provider).to receive(:needs_creating?).at_least(:once).and_return(false) }
-            it "backs up the file and produces a diff for reporting" do
+            before do
+              allow(provider).to receive(:do_backup)  # stub do_backup
+              expect(provider).to receive(:needs_creating?).at_least(:once).and_return(false)
+            end
+
+            it "backs up the file" do
               expect(provider).to receive(:do_backup)
               provider.send(:do_contents_changes)
+            end
+
+            it "produces a diff for reporting" do
+              provider.send(:do_contents_changes)
               expect(resource.diff).to eq(diff_for_reporting)
             end
+
+            it "renders the final checksum correctly for reporting" do
+              provider.send(:do_contents_changes)
+              expect(resource.state_for_resource_reporter[:checksum]).to eql(tempfile_sha256)
+            end
           end
         end