Setting nil to properties with implicit nil sets default value

- This makes converting core resources to properties safer

- This makes it easier to apply wrapping properties to subresources

property :foo, String, default: "foo"

This is where the change lies, and writing a nil here will now actually
write a "foo" to the variable.

property :foo, [ String, nil ], default: "foo"

This is unchanged.  Writing nil writes nil.

property :foo, String

Technically this is changed, since it writes the default value, but
since nil.equal?(nil) in a very deep way no behavior changes.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

3 files changed, 65 insertions, 18 deletions

diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index c955dd3b12..19fc4e678f 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -131,10 +131,6 @@ class Chef
 
       private
 
-      def explicitly_allows_nil?(key, validation)
-        validation.has_key?(:is) && _pv_is({ key => nil }, key, validation[:is], raise_error: false)
-      end
-
       def _validation_message(key, default)
         @validation_message.has_key?(key) ? @validation_message[key] : default
       end
@@ -321,6 +317,7 @@ class Chef
       #   ```
       #
       def _pv_callbacks(opts, key, callbacks)
+        puts caller
         raise ArgumentError, "Callback list must be a hash!" unless callbacks.kind_of?(Hash)
         value = _pv_opts_lookup(opts, key)
         if !value.nil?
@@ -407,7 +404,7 @@ class Chef
       #   x nil      #=> invalid
       #   ```
       #
-      def _pv_is(opts, key, to_be, raise_error: true)
+      def _pv_is(opts, key, to_be)
         return true if !opts.has_key?(key.to_s) && !opts.has_key?(key.to_sym)
         value = _pv_opts_lookup(opts, key)
         to_be = [ to_be ].flatten(1)
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 942fff0ee9..52abdd18c9 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -344,7 +344,7 @@ class Chef
             # Otherwise, we have to validate it now.
             value = input_to_stored_value(resource, default, is_default: true)
           end
-          value = stored_value_to_output(resource, value, is_default: true)
+          value = stored_value_to_output(resource, value)
 
           # If the value is mutable (non-frozen), we set it on the instance
           # so that people can mutate it.  (All constant default values are
@@ -573,7 +573,9 @@ class Chef
     # @api private
     def explicitly_accepts_nil?(resource)
       options.has_key?(:coerce) ||
-        (options.has_key?(:is) && resource.send(:_pv_is, { name => nil }, name, options[:is], raise_error: false))
+        (options.has_key?(:is) && Chef::Mixin::ParamsValidate.send(:_pv_is, { name => nil }, name, options[:is]))
+    rescue Chef::Exceptions::ValidationFailed, Chef::Exceptions::CannotValidateStaticallyError
+      false
     end
 
     # @api private
@@ -649,25 +651,26 @@ class Chef
     end
 
     def input_to_stored_value(resource, value, is_default: false)
+      if value.nil? && !is_default && !explicitly_accepts_nil?(resource)
+        value = default
+      end
       unless value.is_a?(DelayedEvaluator)
-        value = coerce_and_validate(resource, value, is_default: is_default)
+        value = coerce_and_validate(resource, value)
       end
       value
     end
 
-    def stored_value_to_output(resource, value, is_default: false)
+    def stored_value_to_output(resource, value)
       # Crack open lazy values before giving the result to the user
       if value.is_a?(DelayedEvaluator)
         value = exec_in_resource(resource, value)
-        value = coerce_and_validate(resource, value, is_default: is_default)
+        value = coerce_and_validate(resource, value)
       end
       value
     end
 
-    # Coerces and validates the value. If the value is a default, it will warn
-    # the user that invalid defaults are bad mmkay, and return it as if it were
-    # valid.
-    def coerce_and_validate(resource, value, is_default: false)
+    # Coerces and validates the value.
+    def coerce_and_validate(resource, value)
       result = coerce(resource, value)
       validate(resource, result)
 
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index 882ea3353b..b05d8c4e17 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -617,8 +617,9 @@ describe "Chef::Resource.property validation" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
-      it "value nil is invalid" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
+      it "value nil sets to the default" do
+        # this mildly complicated because the default of a name property is a lazy evaluator to the actual resource.name
+        expect(resource.x nil).to be_a(Chef::DelayedEvaluator)
       end
     end
 
@@ -630,8 +631,54 @@ describe "Chef::Resource.property validation" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
-      it "value nil is invalid" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
+      it "value nil sets the default" do
+        expect(resource.x nil).to eq 10
+        expect(resource.x).to eq 10
+      end
+    end
+  end
+
+  context "nil setting default" do
+    with_property ":x, String" do
+      it "if x is not specified, the default is returned" do
+        expect(resource.x).to eq nil
+      end
+      it "value '2' is valid" do
+        expect(resource.x "2").to eq "2"
+        expect(resource.x).to eq "2"
+      end
+      it "value nil sets the default" do
+        resource.x "2"
+        expect(resource.x nil).to eq nil
+        expect(resource.x).to eq nil
+      end
+    end
+    with_property ":x, String, default: '1'" do
+      it "if x is not specified, the default is returned" do
+        expect(resource.x).to eq "1"
+      end
+      it "value '2' is valid" do
+        expect(resource.x "2").to eq "2"
+        expect(resource.x).to eq "2"
+      end
+      it "value nil sets the default" do
+        resource.x "2"
+        expect(resource.x nil).to eq "1"
+        expect(resource.x).to eq "1"
+      end
+    end
+    with_property ":x, [ String, nil ] , default: '1'" do
+      it "if x is not specified, the default is returned" do
+        expect(resource.x).to eq "1"
+      end
+      it "value '2' is valid" do
+        expect(resource.x "2").to eq "2"
+        expect(resource.x).to eq "2"
+      end
+      it "value nil sets to nil" do
+        resource.x "2"
+        expect(resource.x nil).to eq nil
+        expect(resource.x).to eq nil
       end
     end
   end