Make "property_name" in actions load current value as default

3 files changed, 107 insertions, 57 deletions

diff --git a/lib/chef/provider.rb b/lib/chef/provider.rb
index dcfc92645b..43d58740f2 100644
--- a/lib/chef/provider.rb
+++ b/lib/chef/provider.rb
@@ -211,10 +211,29 @@ class Chef
           extend Forwardable
           define_singleton_method(:to_s) { "#{resource_class} forwarder module" }
           define_singleton_method(:inspect) { to_s }
+          # Add a delegator for each explicit property that will get the *current* value
+          # of the property by default instead of the *actual* value.
+          resource.class.properties.each do |name, property|
+            class_eval(<<-EOM, __FILE__, __LINE__)
+              def #{name}(*args, &block)
+                # If no arguments were passed, we process "get" by defaulting
+                # the value to current_resource, not new_resource. This helps
+                # avoid issues where resources accidentally overwrite perfectly
+                # valid stuff with default values.
+                if args.empty? && !block
+                  if !new_resource.property_is_set?(__method__) && current_resource
+                    return current_resource.public_send(__method__)
+                  end
+                end
+                new_resource.public_send(__method__, *args, &block)
+              end
+            EOM
+          end
           dsl_methods =
              resource.class.public_instance_methods +
              resource.class.protected_instance_methods -
-             provider_class.instance_methods
+             provider_class.instance_methods -
+             resource.class.properties.keys
           def_delegators(:new_resource, *dsl_methods)
         end
         include @included_resource_dsl_module
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index cdb351f643..bb5c6050fe 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -1442,9 +1442,12 @@ class Chef
         use_inline_resources
         include_resource_dsl true
         define_singleton_method(:to_s) { "#{resource_class} action provider" }
-        define_singleton_method(:inspect) { to_s }
-        define_method(:load_current_resource) do
+        def self.inspect
+          to_s
+        end
+        def load_current_resource
           if new_resource.respond_to?(:load_current_value!)
+            # dup the resource and then reset desired-state properties.
             current_resource = new_resource.dup
 
             # We clear desired state in the copy, because it is supposed to be actual state.
diff --git a/spec/integration/recipes/resource_load_spec.rb b/spec/integration/recipes/resource_load_spec.rb
index f91039abb4..c29b877b59 100644
--- a/spec/integration/recipes/resource_load_spec.rb
+++ b/spec/integration/recipes/resource_load_spec.rb
@@ -23,9 +23,15 @@ describe "Resource.load_current_value" do
       def self.to_s; resource_name; end
       def self.inspect; resource_name.inspect; end
       property :x, default: lazy { "default #{Namer.incrementing_value}" }
+      def self.created_x=(value)
+        @created = value
+      end
+      def self.created_x
+        @created
+      end
       action :create do
+        new_resource.class.created_x = x
       end
-      default_action :nothing
     end
     result.resource_name resource_name
     result
@@ -44,64 +50,86 @@ describe "Resource.load_current_value" do
       end
     end
 
-    let(:resource) do
-      e = self
-      r = nil
-      converge {
-        r = public_send(e.resource_name, 'blah') do
-          x 'desired'
-        end
-      }
-      r
-    end
-
-    it "current_resource is passed name but not x" do
-      expect(resource.current_resource.x).to eq 'loaded 1 (name=blah)'
-    end
+    context "and a resource with x set to a desired value" do
+      let(:resource) do
+        e = self
+        r = nil
+        converge {
+          r = public_send(e.resource_name, 'blah') do
+            x 'desired'
+          end
+        }
+        r
+      end
 
-    it "resource.current_resource returns a different resource" do
-      expect(resource.current_resource.x).to eq 'loaded 1 (name=blah)'
-      expect(resource.x).to eq 'desired'
-    end
+      it "current_resource is passed name but not x" do
+        expect(resource.current_resource.x).to eq 'loaded 2 (name=blah)'
+      end
 
-    it "resource.current_resource constructs the resource anew each time" do
-      expect(resource.current_resource.x).to eq 'loaded 1 (name=blah)'
-      expect(resource.current_resource.x).to eq 'loaded 2 (name=blah)'
-    end
+      it "resource.current_resource returns a different resource" do
+        expect(resource.current_resource.x).to eq 'loaded 2 (name=blah)'
+        expect(resource.x).to eq 'desired'
+      end
 
-    context "and identity: :i and :d with desired_state: false" do
-      before {
-        resource_class.class_eval do
-          property :i, identity: true
-          property :d, desired_state: false
-        end
-      }
+      it "resource.current_resource constructs the resource anew each time" do
+        expect(resource.current_resource.x).to eq 'loaded 2 (name=blah)'
+        expect(resource.current_resource.x).to eq 'loaded 3 (name=blah)'
+      end
 
-      before {
-        resource.i 'desired_i'
-        resource.d 'desired_d'
-      }
+      it "the provider accesses the current value of x" do
+        expect(resource.class.created_x).to eq 'desired'
+      end
 
-      it "i, name and d are passed to load_current_value, but not x" do
-        expect(resource.current_resource.x).to eq 'loaded 1 (d=desired_d, i=desired_i, name=blah)'
+      context "and identity: :i and :d with desired_state: false" do
+        before {
+          resource_class.class_eval do
+            property :i, identity: true
+            property :d, desired_state: false
+          end
+        }
+
+        before {
+          resource.i 'desired_i'
+          resource.d 'desired_d'
+        }
+
+        it "i, name and d are passed to load_current_value, but not x" do
+          expect(resource.current_resource.x).to eq 'loaded 2 (d=desired_d, i=desired_i, name=blah)'
+        end
       end
-    end
 
-    context "and name_property: :i and :d with desired_state: false" do
-      before {
-        resource_class.class_eval do
-          property :i, name_property: true
-          property :d, desired_state: false
+      context "and name_property: :i and :d with desired_state: false" do
+        before {
+          resource_class.class_eval do
+            property :i, name_property: true
+            property :d, desired_state: false
+          end
+        }
+
+        before {
+          resource.i 'desired_i'
+          resource.d 'desired_d'
+        }
+
+        it "i, name and d are passed to load_current_value, but not x" do
+          expect(resource.current_resource.x).to eq 'loaded 2 (d=desired_d, i=desired_i, name=blah)'
         end
-      }
+      end
+    end
 
-      before {
-        resource.i 'desired_i'
-        resource.d 'desired_d'
-      }
+    context "and a resource with no values set" do
+      let(:resource) do
+        e = self
+        r = nil
+        converge {
+          r = public_send(e.resource_name, 'blah') do
+          end
+        }
+        r
+      end
 
-      it "i, name and d are passed to load_current_value, but not x" do
-        expect(resource.current_resource.x).to eq 'loaded 1 (d=desired_d, i=desired_i, name=blah)'
+      it "the provider accesses values from load_current_value" do
+        expect(resource.class.created_x).to eq 'loaded 1 (name=blah)'
       end
     end
 
@@ -132,7 +160,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_resource.x).to eq 'loaded 1 (name=blah)'
+        expect(subresource.current_resource.x).to eq 'loaded 2 (name=blah)'
       end
       it "load_current_value yields a copy of the child class" do
         expect(subresource.current_resource).to be_kind_of(subresource_class)
@@ -151,8 +179,8 @@ describe "Resource.load_current_value" do
 
       it "the overridden load_current_value is used" do
         current_resource = subresource.current_resource
-        expect(current_resource.x).to eq 'default 2'
-        expect(current_resource.y).to eq 'loaded_y 1 (name=blah)'
+        expect(current_resource.x).to eq 'default 3'
+        expect(current_resource.y).to eq 'loaded_y 2 (name=blah)'
       end
     end
 
@@ -169,8 +197,8 @@ 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_resource
-        expect(current_resource.x).to eq 'loaded 1 (name=blah)'
-        expect(current_resource.y).to eq 'loaded_y 2 (name=blah, x=loaded 1 (name=blah))'
+        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))'
       end
     end
   end