From b2366a11103a53518704b54ab58c5b9b8e45a241 Mon Sep 17 00:00:00 2001
From: Lamont Granquist <lamont@scriptkiddie.org>
Date: Tue, 31 Mar 2020 18:01:44 -0700
Subject: Add after_resource to pair with current_resource

For custom resources which correctly implement load_current_value the
new after_resource comes for "free" and load_current_value will be
called twice to load the current_resource (should be renamed the
"before_resource" but that ship sailed) and then the after_resource.

Appropriate wiring is added for a new event and capturing that into
the action_collection and then the data_collector.  The resource_reporter
has not and will not be updated.

For old style resources which are difficult to convert to custom
resources (thinking here particularly of the 1:N model of the service
resource in core chef, or stuff that just may be a lot of work like the
file resource) then they can override the load_after_resource to
manually wire up their own after_resource.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
---
 lib/chef/action_collection.rb                  |  23 ++--
 lib/chef/data_collector/run_end_message.rb     |  10 +-
 lib/chef/event_dispatch/base.rb                |   3 +
 lib/chef/provider.rb                           |  12 +++
 lib/chef/resource/action_class.rb              |  33 +++++-
 spec/integration/recipes/resource_load_spec.rb | 141 +++++++++++++++++++++++--
 spec/unit/data_collector_spec.rb               |  57 ++++++----
 7 files changed, 242 insertions(+), 37 deletions(-)

diff --git a/lib/chef/action_collection.rb b/lib/chef/action_collection.rb
index 7b1997cfaf..eb0d3bef58 100644
--- a/lib/chef/action_collection.rb
+++ b/lib/chef/action_collection.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2018-2019, Chef Software Inc.
+# Copyright:: Copyright 2018-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -24,10 +24,6 @@ class Chef
 
     class ActionRecord
 
-      # XXX: this is buggy since we (ab)use this resource for "after" state and it may be
-      # inaccurate and it may be mutated by the user.  A third after_resource should be added
-      # to new_resource + current_resource to properly implement this.
-      #
       # @return [Chef::Resource] The declared resource state.
       #
       attr_accessor :new_resource
@@ -38,6 +34,10 @@ class Chef
       # implementation, but must be handled), or for unprocessed resources.
       attr_accessor :current_resource
 
+      # @return [Chef::Resource] the after_resource object (after-state).  This can be nil for
+      # non custom-resources or resources that do not implement load_after_resource.
+      attr_accessor :after_resource
+
       # @return [Symbol] # The action that was run (or scheduled to run in the case of "unprocessed" resources).
       attr_accessor :action
 
@@ -161,7 +161,7 @@ class Chef
       pending_updates << ActionRecord.new(new_resource, action, pending_updates.length)
     end
 
-    # Hook called after a resource is loaded.  If load_current_resource fails, this hook will
+    # Hook called after a current resource is loaded.  If load_current_resource fails, this hook will
     # not be called and current_resource will be nil, and the resource_failed hook will be called.
     #
     # (see EventDispatch::Base#)
@@ -172,6 +172,17 @@ class Chef
       current_record.current_resource = current_resource
     end
 
+    # Hook called after an after resource is loaded.  If load_after_resource fails, this hook will
+    # not be called and after_resource will be nil, and the resource_failed hook will be called.
+    #
+    # (see EventDispatch::Base#)
+    #
+    def resource_after_state_loaded(new_resource, action, after_resource)
+      return if consumers.empty?
+
+      current_record.after_resource = after_resource
+    end
+
     # Hook called after an action is determined to be up to date.
     #
     # (see EventDispatch::Base#)
diff --git a/lib/chef/data_collector/run_end_message.rb b/lib/chef/data_collector/run_end_message.rb
index 762058bfdb..cfd4b9d8b3 100644
--- a/lib/chef/data_collector/run_end_message.rb
+++ b/lib/chef/data_collector/run_end_message.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2012-2019, Chef Software Inc.
+# Copyright:: Copyright 2012-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -106,12 +106,13 @@ class Chef
         def action_record_for_json(action_record)
           new_resource = action_record.new_resource
           current_resource = action_record.current_resource
+          after_resource = action_record.after_resource
 
           hash = {
             "type" => new_resource.resource_name.to_sym,
             "name" => new_resource.name.to_s,
             "id" => safe_resource_identity(new_resource),
-            "after" => safe_state_for_resource_reporter(new_resource),
+            "after" => safe_state_for_resource_reporter(after_resource || new_resource),
             "before" => safe_state_for_resource_reporter(current_resource),
             "duration" => action_record.elapsed_time.nil? ? "" : (action_record.elapsed_time * 1000).to_i.to_s,
             "delta" => new_resource.respond_to?(:diff) && updated_or_failed?(action_record) ? new_resource.diff : "",
@@ -120,6 +121,11 @@ class Chef
             "status" => action_record_status_for_json(action_record),
           }
 
+          # don't use the new_resource for the after_resource if it is skipped or failed
+          if action_record.status == :skipped || action_record.status == :failed || action_record.status == :unprocessed
+            hash["after"] = {}
+          end
+
           if new_resource.cookbook_name
             hash["cookbook_name"]    = new_resource.cookbook_name
             hash["cookbook_version"] = new_resource.cookbook_version.version
diff --git a/lib/chef/event_dispatch/base.rb b/lib/chef/event_dispatch/base.rb
index cb1cc80017..f7b706cb2c 100644
--- a/lib/chef/event_dispatch/base.rb
+++ b/lib/chef/event_dispatch/base.rb
@@ -266,6 +266,9 @@ class Chef
       # Called after #load_current_resource has run.
       def resource_current_state_loaded(resource, action, current_resource); end
 
+      # Called after #load_after_resource has run.
+      def resource_after_state_loaded(resource, action, after_resource); end
+
       # Called when resource current state load is skipped due to the provider
       # not supporting whyrun mode.
       def resource_current_state_load_bypassed(resource, action, current_resource); end
diff --git a/lib/chef/provider.rb b/lib/chef/provider.rb
index 07cdea6dfc..3783bd9d5f 100644
--- a/lib/chef/provider.rb
+++ b/lib/chef/provider.rb
@@ -35,6 +35,7 @@ class Chef
 
     attr_accessor :new_resource
     attr_accessor :current_resource
+    attr_accessor :after_resource
     attr_accessor :run_context
 
     attr_reader :recipe_name
@@ -94,6 +95,7 @@ class Chef
       @new_resource = new_resource
       @action = action
       @current_resource = nil
+      @after_resource = nil
       @run_context = run_context
       @converge_actions = nil
 
@@ -148,6 +150,13 @@ class Chef
 
     def cleanup_after_converge; end
 
+    def load_after_resource
+      # This is a backwards compatible hack, custom resources properly wire up a new after_resource
+      # via load_current_value.  It is acceptible for old style resources that cannot be easily made
+      # into custom resources to override this method and provide a proper after_resource.
+      @after_resource = @new_resource
+    end
+
     # the :nothing action which is available on all resources by default
     def action_nothing
       logger.trace("Doing nothing for #{@new_resource}")
@@ -191,6 +200,9 @@ class Chef
       set_updated_status
 
       cleanup_after_converge
+
+      load_after_resource
+      events.resource_after_state_loaded(@new_resource, @action, @after_resource)
     end
 
     def process_resource_requirements
diff --git a/lib/chef/resource/action_class.rb b/lib/chef/resource/action_class.rb
index 1b4ddd453b..23418df4ed 100644
--- a/lib/chef/resource/action_class.rb
+++ b/lib/chef/resource/action_class.rb
@@ -64,7 +64,38 @@ class Chef
         @current_resource = current_resource
       end
 
-      # @todo: remove in Chef-15
+      def load_after_resource
+        if new_resource.respond_to?(:load_current_value!)
+          # dup the resource and then reset desired-state properties.
+          after_resource = new_resource.dup
+
+          # We clear desired state in the copy, because it is supposed to be actual state.
+          # We keep identity properties and non-desired-state, which are assumed to be
+          # "control" values like `recurse: true`
+          after_resource.class.properties.each_value do |property|
+            if property.desired_state? && !property.identity? && !property.name_property?
+              property.reset(after_resource)
+            end
+          end
+
+          # Call the actual load_current_value! method. If it raises
+          # CurrentValueDoesNotExist, set after_resource to `nil`.
+          begin
+            # If the user specifies load_current_value do |desired_resource|, we
+            # pass in the desired resource as well as the current one.
+            if after_resource.method(:load_current_value!).arity > 0
+              after_resource.load_current_value!(new_resource)
+            else
+              after_resource.load_current_value!
+            end
+          rescue Chef::Exceptions::CurrentValueDoesNotExist
+            after_resource = nil
+          end
+        end
+
+        @after_resource = after_resource
+      end
+
       def self.include_resource_dsl?
         true
       end
diff --git a/spec/integration/recipes/resource_load_spec.rb b/spec/integration/recipes/resource_load_spec.rb
index 3ca71c3a03..52a108a766 100644
--- a/spec/integration/recipes/resource_load_spec.rb
+++ b/spec/integration/recipes/resource_load_spec.rb
@@ -65,17 +65,17 @@ describe "Resource.load_current_value" do
       end
 
       it "current_resource is passed name but not x" do
-        expect(resource.current_value.x).to eq "loaded 2 (name=blah)"
+        expect(resource.current_value.x).to eq "loaded 3 (name=blah)"
       end
 
       it "resource.current_value returns a different resource" do
-        expect(resource.current_value.x).to eq "loaded 2 (name=blah)"
+        expect(resource.current_value.x).to eq "loaded 3 (name=blah)"
         expect(resource.x).to eq "desired"
       end
 
       it "resource.current_value constructs the resource anew each time" do
-        expect(resource.current_value.x).to eq "loaded 2 (name=blah)"
         expect(resource.current_value.x).to eq "loaded 3 (name=blah)"
+        expect(resource.current_value.x).to eq "loaded 4 (name=blah)"
       end
 
       it "the provider accesses the current value of x" do
@@ -96,7 +96,7 @@ describe "Resource.load_current_value" do
         end
 
         it "i, name and d are passed to load_current_value, but not x" do
-          expect(resource.current_value.x).to eq "loaded 2 (d=desired_d, i=desired_i, name=blah)"
+          expect(resource.current_value.x).to eq "loaded 3 (d=desired_d, i=desired_i, name=blah)"
         end
       end
 
@@ -114,7 +114,7 @@ describe "Resource.load_current_value" do
         end
 
         it "i, name and d are passed to load_current_value, but not x" do
-          expect(resource.current_value.x).to eq "loaded 2 (d=desired_d, i=desired_i, name=blah)"
+          expect(resource.current_value.x).to eq "loaded 3 (d=desired_d, i=desired_i, name=blah)"
         end
       end
     end
@@ -146,7 +146,7 @@ describe "Resource.load_current_value" do
 
     context "and a child resource class with no load_current_value" do
       it "the parent load_current_value is used" do
-        expect(subresource.current_value.x).to eq "loaded 2 (name=blah)"
+        expect(subresource.current_value.x).to eq "loaded 3 (name=blah)"
       end
       it "load_current_value yields a copy of the child class" do
         expect(subresource.current_value).to be_kind_of(subresource_class)
@@ -165,8 +165,8 @@ describe "Resource.load_current_value" do
 
       it "the overridden load_current_value is used" do
         current_resource = subresource.current_value
-        expect(current_resource.x).to eq "default 3"
-        expect(current_resource.y).to eq "loaded_y 2 (name=blah)"
+        expect(current_resource.x).to eq "default 4"
+        expect(current_resource.y).to eq "loaded_y 3 (name=blah)"
       end
     end
 
@@ -183,10 +183,131 @@ describe "Resource.load_current_value" do
 
       it "the original load_current_value is called as well as the child one" do
         current_resource = subresource.current_value
-        expect(current_resource.x).to eq "loaded 3 (name=blah)"
-        expect(current_resource.y).to eq "loaded_y 4 (name=blah, x=loaded 3 (name=blah))"
+        expect(current_resource.x).to eq "loaded 5 (name=blah)"
+        expect(current_resource.y).to eq "loaded_y 6 (name=blah, x=loaded 5 (name=blah))"
+      end
+    end
+  end
+end
+
+describe "simple load_current_value tests" do
+  let(:resource_class) do
+    Class.new(Chef::Resource) do
+      attr_writer :index # this is our hacky global state
+      def index; @index ||= 1; end
+
+      property :myindex, Integer
+
+      load_current_value do |new_resource|
+        myindex new_resource.index
+      end
+
+      action :run do
+        new_resource.index += 1
+      end
+    end
+  end
+
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:new_resource) { resource_class.new("test", run_context) }
+  let(:provider) { new_resource.provider_for_action(:run) }
+
+  it "calling the action on the provider sets the current_resource" do
+    expect(events).to receive(:resource_current_state_loaded).with(new_resource, :run, anything)
+    provider.run_action(:run)
+    expect(provider.current_resource.myindex).to eql(1)
+  end
+
+  it "calling the action on the provider sets the after_resource" do
+    expect(events).to receive(:resource_after_state_loaded).with(new_resource, :run, anything)
+    provider.run_action(:run)
+    expect(provider.after_resource.myindex).to eql(2)
+  end
+end
+
+describe "simple load_current_resource tests" do
+  let(:provider_class) do
+    Class.new(Chef::Provider) do
+      provides :no_load_current_value
+      def load_current_resource
+        @current_resource = new_resource.dup
+        @current_resource.myindex = 1
+      end
+      action :run do
+      end
+    end
+  end
+
+  let(:resource_class) do
+    provider_class # vivify the provider_class
+    Class.new(Chef::Resource) do
+      provides :no_load_current_value
+      property :myindex, Integer
+    end
+  end
+
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:new_resource) { resource_class.new("test", run_context) }
+  let(:provider) { new_resource.provider_for_action(:run) }
+
+  it "calling the action on the provider sets the current_resource" do
+    expect(events).to receive(:resource_current_state_loaded).with(new_resource, :run, anything)
+    provider.run_action(:run)
+    expect(provider.current_resource.myindex).to eql(1)
+  end
+
+  it "calling the action on the provider sets the after_resource" do
+    expect(events).to receive(:resource_after_state_loaded).with(new_resource, :run, new_resource)
+    provider.run_action(:run)
+    expect(provider.after_resource.myindex).to eql(nil)
+  end
+end
+
+describe "simple load_current_resource and load_after_resource tests" do
+  let(:provider_class) do
+    Class.new(Chef::Provider) do
+      provides :load_after
+      def load_current_resource
+        @current_resource = new_resource.dup
+        @current_resource.myindex = 1
+      end
+
+      def load_after_resource
+        @after_resource = new_resource.dup
+        @after_resource.myindex = 2
       end
+      action :run do
+      end
+    end
+  end
+
+  let(:resource_class) do
+    provider_class # autovivify provider class
+    Class.new(Chef::Resource) do
+      provides :load_after
+      property :myindex, Integer
     end
   end
 
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:new_resource) { resource_class.new("test", run_context) }
+  let(:provider) { new_resource.provider_for_action(:run) }
+
+  it "calling the action on the provider sets the current_resource" do
+    expect(events).to receive(:resource_current_state_loaded).with(new_resource, :run, anything)
+    provider.run_action(:run)
+    expect(provider.current_resource.myindex).to eql(1)
+  end
+
+  it "calling the action on the provider sets the after_resource" do
+    expect(events).to receive(:resource_after_state_loaded).with(new_resource, :run, anything)
+    provider.run_action(:run)
+    expect(provider.after_resource.myindex).to eql(2)
+  end
 end
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index 3a1eb5b521..bc234d56aa 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2019-2019, Chef Software Inc.
+# Copyright:: Copyright 2019-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -19,6 +19,12 @@ require_relative "../spec_helper"
 require "chef/data_collector"
 require "socket"
 
+#
+# FIXME FIXME FIXME:  What we need to do here is have the ability to construct a real resource collection
+# with some test resources that will correctly be up-to-date/updated/skipped/failed/unprocessed and really
+# converge a client with them (sort of chefspec-like construction of a Chef::Client but with the actions
+# actually running -- another testing requirement similar to the integration testing framework in cheffish as well)
+#
 describe Chef::DataCollector do
   before(:each) do
     Chef::Config[:enable_reporting] = true
@@ -30,9 +36,23 @@ describe Chef::DataCollector do
 
   let(:data_collector) { Chef::DataCollector::Reporter.new(events) }
 
-  let(:new_resource) { Chef::Resource::File.new("/tmp/a-file.txt") }
+  let(:new_resource) do
+    new_resource = Chef::Resource::File.new("/tmp/a-file.txt")
+    new_resource.checksum nil
+    new_resource
+  end
+
+  let(:current_resource) do
+    current_resource = Chef::Resource::File.new("/tmp/a-file.txt")
+    current_resource.checksum "1234123412341234123412341234123412341234123412341234123412341234"
+    current_resource
+  end
 
-  let(:current_resource) { Chef::Resource::File.new("/tmp/a-file.txt") }
+  let(:after_resource) do
+    after_resource = Chef::Resource::File.new("/tmp/a-file.txt")
+    after_resource.checksum "6789678967896789678967896789678967896789678967896789678967896789"
+    after_resource
+  end
 
   let(:events) { Chef::EventDispatch::Dispatcher.new }
 
@@ -130,10 +150,10 @@ describe Chef::DataCollector do
     new_resource.respond_to?(:diff) && %w{updated failed}.include?(status)
   end
 
-  def resource_record_for(current_resource, new_resource, action, status, duration)
+  def resource_record_for(new_resource, before_resource, after_resource, action, status, duration)
     {
-      "after" => new_resource.state_for_resource_reporter,
-      "before" => current_resource&.state_for_resource_reporter,
+      "after" => after_resource&.state_for_resource_reporter || {},
+      "before" => before_resource&.state_for_resource_reporter || {},
       "cookbook_name" => cookbook_name,
       "cookbook_version" => cookbook_version.version,
       "delta" => resource_has_diff(new_resource, status) ? new_resource.diff : "",
@@ -634,13 +654,14 @@ describe Chef::DataCollector do
       context "when the run contains a file resource that is up-to-date" do
         let(:total_resource_count) { 1 }
         let(:updated_resource_count) { 0 }
-        let(:resource_record) { [ resource_record_for(current_resource, new_resource, :create, "up-to-date", "1234") ] }
+        let(:resource_record) { [ resource_record_for(new_resource, current_resource, after_resource, :create, "up-to-date", "1234") ] }
         let(:status) { "success" }
 
         before do
           events.resource_action_start(new_resource, :create)
           events.resource_current_state_loaded(new_resource, :create, current_resource)
           events.resource_up_to_date(new_resource, :create)
+          events.resource_after_state_loaded(new_resource, :create, after_resource)
           new_resource.instance_variable_set(:@elapsed_time, 1.2345)
           events.resource_completed(new_resource)
           events.converge_complete
@@ -653,13 +674,14 @@ describe Chef::DataCollector do
       context "when the run contains a file resource that is updated" do
         let(:total_resource_count) { 1 }
         let(:updated_resource_count) { 1 }
-        let(:resource_record) { [ resource_record_for(current_resource, new_resource, :create, "updated", "1234") ] }
+        let(:resource_record) { [ resource_record_for(new_resource, current_resource, after_resource, :create, "updated", "1234") ] }
         let(:status) { "success" }
 
         before do
           events.resource_action_start(new_resource, :create)
           events.resource_current_state_loaded(new_resource, :create, current_resource)
           events.resource_updated(new_resource, :create)
+          events.resource_after_state_loaded(new_resource, :create, after_resource)
           new_resource.instance_variable_set(:@elapsed_time, 1.2345)
           events.resource_completed(new_resource)
           events.converge_complete
@@ -679,7 +701,7 @@ describe Chef::DataCollector do
           allow(r).to receive(:cookbook_version).and_return(cookbook_version)
           r
         end
-        let(:resource_record) { [ resource_record_for(implementation_resource, implementation_resource, :create, "updated", "2345"), resource_record_for(current_resource, new_resource, :create, "updated", "1234") ] }
+        let(:resource_record) { [ resource_record_for(implementation_resource, implementation_resource, implementation_resource, :create, "updated", "2345"), resource_record_for(new_resource, current_resource, after_resource, :create, "updated", "1234") ] }
         let(:status) { "success" }
 
         before do
@@ -689,10 +711,12 @@ describe Chef::DataCollector do
           events.resource_action_start(implementation_resource , :create)
           events.resource_current_state_loaded(implementation_resource, :create, implementation_resource)
           events.resource_updated(implementation_resource, :create)
+          events.resource_after_state_loaded(implementation_resource, :create, implementation_resource)
           implementation_resource.instance_variable_set(:@elapsed_time, 2.3456)
           events.resource_completed(implementation_resource)
 
           events.resource_updated(new_resource, :create)
+          events.resource_after_state_loaded(new_resource, :create, after_resource)
           new_resource.instance_variable_set(:@elapsed_time, 1.2345)
           events.resource_completed(new_resource)
           events.converge_complete
@@ -706,7 +730,7 @@ describe Chef::DataCollector do
         let(:total_resource_count) { 1 }
         let(:updated_resource_count) { 0 }
         let(:resource_record) do
-          rec = resource_record_for(current_resource, new_resource, :create, "skipped", "1234")
+          rec = resource_record_for(new_resource, nil, nil, :create, "skipped", "1234")
           rec["conditional"] = "not_if { #code block }" # FIXME: "#code block" is poor, is there some way to fix this?
           [ rec ]
         end
@@ -715,7 +739,6 @@ describe Chef::DataCollector do
         before do
           conditional = (new_resource.not_if { true }).first
           events.resource_action_start(new_resource, :create)
-          events.resource_current_state_loaded(new_resource, :create, current_resource)
           events.resource_skipped(new_resource, :create, conditional)
           new_resource.instance_variable_set(:@elapsed_time, 1.2345)
           events.resource_completed(new_resource)
@@ -730,7 +753,7 @@ describe Chef::DataCollector do
         let(:total_resource_count) { 1 }
         let(:updated_resource_count) { 0 }
         let(:resource_record) do
-          rec = resource_record_for(current_resource, new_resource, :create, "skipped", "1234")
+          rec = resource_record_for(new_resource, nil, nil, :create, "skipped", "1234")
           rec["conditional"] = 'not_if "true"'
           [ rec ]
         end
@@ -739,7 +762,6 @@ describe Chef::DataCollector do
         before do
           conditional = (new_resource.not_if "true").first
           events.resource_action_start(new_resource, :create)
-          events.resource_current_state_loaded(new_resource, :create, current_resource)
           events.resource_skipped(new_resource, :create, conditional)
           new_resource.instance_variable_set(:@elapsed_time, 1.2345)
           events.resource_completed(new_resource)
@@ -756,7 +778,7 @@ describe Chef::DataCollector do
         let(:total_resource_count) { 1 }
         let(:updated_resource_count) { 0 }
         let(:resource_record) do
-          rec = resource_record_for(current_resource, new_resource, :create, "failed", "1234")
+          rec = resource_record_for(new_resource, current_resource, nil, :create, "failed", "1234")
           rec["error_message"] = "imperial to metric conversion error"
           [ rec ]
         end
@@ -783,7 +805,7 @@ describe Chef::DataCollector do
         let(:total_resource_count) { 1 }
         let(:updated_resource_count) { 0 }
         let(:resource_record) do
-          rec = resource_record_for(current_resource, new_resource, :create, "failed", "1234")
+          rec = resource_record_for(new_resource, nil, nil, :create, "failed", "1234")
           rec["before"] = {}
           rec["error_message"] = "imperial to metric conversion error"
           [ rec ]
@@ -818,10 +840,9 @@ describe Chef::DataCollector do
           res
         end
         let(:resource_record) do
-          rec1 = resource_record_for(current_resource, new_resource, :create, "failed", "1234")
+          rec1 = resource_record_for(new_resource, current_resource, nil, :create, "failed", "1234")
           rec1["error_message"] = "imperial to metric conversion error"
-          rec2 = resource_record_for(nil, unprocessed_resource, :nothing, "unprocessed", "")
-          rec2["before"] = {}
+          rec2 = resource_record_for(unprocessed_resource, nil, nil, :nothing, "unprocessed", "")
           [ rec1, rec2 ]
         end
         let(:status) { "failure" }
-- 
cgit v1.2.1


From f24948d75a8f0c08d4705e7bc194bfd4b2a3a7d3 Mon Sep 17 00:00:00 2001
From: Lamont Granquist <lamont@scriptkiddie.org>
Date: Tue, 31 Mar 2020 19:47:10 -0700
Subject: remove duping for load_current_value and code duplication

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
---
 lib/chef/resource/action_class.rb            | 71 ++++++++--------------------
 spec/unit/resource/chocolatey_source_spec.rb |  5 +-
 2 files changed, 24 insertions(+), 52 deletions(-)

diff --git a/lib/chef/resource/action_class.rb b/lib/chef/resource/action_class.rb
index 23418df4ed..eefa968197 100644
--- a/lib/chef/resource/action_class.rb
+++ b/lib/chef/resource/action_class.rb
@@ -29,71 +29,42 @@ class Chef
         "#{new_resource || "<no resource>"} action #{action ? action.inspect : "<no action>"}"
       end
 
-      #
-      # If load_current_value! is defined on the resource, use that.
-      #
-      def load_current_resource
+      def return_load_current_value
+        resource = nil
         if new_resource.respond_to?(:load_current_value!)
-          # dup the resource and then reset desired-state properties.
-          current_resource = new_resource.dup
+          resource = new_resource.class.new(new_resource.name, new_resource.run_context)
 
-          # We clear desired state in the copy, because it is supposed to be actual state.
-          # We keep identity properties and non-desired-state, which are assumed to be
-          # "control" values like `recurse: true`
-          current_resource.class.properties.each_value do |property|
-            if property.desired_state? && !property.identity? && !property.name_property?
-              property.reset(current_resource)
+          # copy the non-desired state, the identity properties and name property to the new resource
+          # (the desired state values must be loaded by load_current_value)
+          resource.class.properties.each_value do |property|
+            if !property.desired_state? || property.identity? || property.name_property?
+              property.set(resource, new_resource.send(property.name)) if new_resource.class.properties[property.name].is_set?(new_resource)
             end
           end
 
-          # Call the actual load_current_value! method. If it raises
-          # CurrentValueDoesNotExist, set current_resource to `nil`.
+          # we support optionally passing the new_resource as an arg to load_current_value and
+          # load_current_value can raise in order to clear the current_resource to nil
           begin
-            # If the user specifies load_current_value do |desired_resource|, we
-            # pass in the desired resource as well as the current one.
-            if current_resource.method(:load_current_value!).arity > 0
-              current_resource.load_current_value!(new_resource)
+            if resource.method(:load_current_value!).arity > 0
+              resource.load_current_value!(new_resource)
             else
-              current_resource.load_current_value!
+              resource.load_current_value!
             end
           rescue Chef::Exceptions::CurrentValueDoesNotExist
-            current_resource = nil
+            resource = nil
           end
         end
+        resource
+      end
 
-        @current_resource = current_resource
+      # build the before state (current_resource)
+      def load_current_resource
+        @current_resource = return_load_current_value
       end
 
+      # build the after state (after_resource)
       def load_after_resource
-        if new_resource.respond_to?(:load_current_value!)
-          # dup the resource and then reset desired-state properties.
-          after_resource = new_resource.dup
-
-          # We clear desired state in the copy, because it is supposed to be actual state.
-          # We keep identity properties and non-desired-state, which are assumed to be
-          # "control" values like `recurse: true`
-          after_resource.class.properties.each_value do |property|
-            if property.desired_state? && !property.identity? && !property.name_property?
-              property.reset(after_resource)
-            end
-          end
-
-          # Call the actual load_current_value! method. If it raises
-          # CurrentValueDoesNotExist, set after_resource to `nil`.
-          begin
-            # If the user specifies load_current_value do |desired_resource|, we
-            # pass in the desired resource as well as the current one.
-            if after_resource.method(:load_current_value!).arity > 0
-              after_resource.load_current_value!(new_resource)
-            else
-              after_resource.load_current_value!
-            end
-          rescue Chef::Exceptions::CurrentValueDoesNotExist
-            after_resource = nil
-          end
-        end
-
-        @after_resource = after_resource
+        @after_resource = return_load_current_value
       end
 
       def self.include_resource_dsl?
diff --git a/spec/unit/resource/chocolatey_source_spec.rb b/spec/unit/resource/chocolatey_source_spec.rb
index 60bd773594..aad9c4c283 100644
--- a/spec/unit/resource/chocolatey_source_spec.rb
+++ b/spec/unit/resource/chocolatey_source_spec.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2018, Chef Software, Inc.
+# Copyright:: Copyright 2018-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -49,9 +49,10 @@ describe Chef::Resource::ChocolateySource do
   before(:each) do
     disable_provider # vivify before mocking
     enable_provider
+    current_resource
     allow(resource).to receive(:provider_for_action).and_return(disable_provider)
     allow(resource).to receive(:provider_for_action).and_return(enable_provider)
-    allow(resource).to receive(:dup).and_return(current_resource)
+    allow(resource.class).to receive(:new).and_return(current_resource)
     @original_env = ENV.to_hash
     ENV["ALLUSERSPROFILE"] = 'C:\ProgramData'
   end
-- 
cgit v1.2.1