Always allow nil set; only warn if not using "is"jk/simpler_nil_set

4 files changed, 72 insertions, 73 deletions

diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index e3c7657b1b..26a26090e0 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -457,7 +457,7 @@ class Chef
 
         def call(resource, value=NOT_PASSED)
           # setting to nil does a get
-          if value.nil? && !explicitly_accepts_nil?(resource)
+          if value.nil?
             get(resource)
           else
             super
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 89e4ffe0e6..8c2776928e 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -253,13 +253,13 @@ class Chef
         return get(resource)
       end
 
-      if value.nil? && !explicitly_accepts_nil?(resource)
+      if value.nil? && !options.has_key?(:is)
         # In Chef 12, value(nil) does a *get* instead of a set, so we
         # warn if the value would have been changed. In Chef 13, it will be
         # equivalent to value = nil.
         result = get(resource)
         if !result.nil?
-          Chef.log_deprecation("#{name} nil currently does not overwrite the value of #{name}. This will change in Chef 13, and the value will be set to nil instead. Please change your code to explicitly accept nil using \"property :#{name}, [MyType, nil]\", or stop setting this value to nil.")
+          Chef.log_deprecation("#{name} nil currently does not overwrite the value of #{name}. This will change in Chef 13, and the value will be set to nil instead. If `nil` is a valid value for your property, please change it to use a type (property kind_of: [ String, NilClass ] becomes property [ String, nil ]) or use `is` instead of `equal_to` or `kind_of` to avoid the warning. If `nil` is not a valid value for your property, please stop setting it to nil.")
         end
         result
       else
@@ -332,8 +332,6 @@ class Chef
 
           value = coerce(resource, value)
 
-          # We don't validate defaults
-
           # If the value is mutable (non-frozen), we set it on the instance
           # so that people can mutate it.  (All constant default values are
           # frozen.)
@@ -367,7 +365,18 @@ class Chef
     def set(resource, value)
       unless value.is_a?(DelayedEvaluator)
         value = coerce(resource, value)
-        validate(resource, value)
+        if value.nil?
+          # If the value is `nil`, and is not a valid value for this property,
+          # it means we need to reset the property.
+          begin
+            validate(resource, value)
+          rescue Chef::Exceptions::ValidationFailed
+            reset(resource)
+            return
+          end
+        else
+          validate(resource, value)
+        end
       end
       set_value(resource, value)
     end
@@ -411,6 +420,8 @@ class Chef
     #
     # Does no special handling for lazy values.
     #
+    # `nil` is never coerced.
+    #
     # @param resource [Chef::Resource] The resource we're coercing against
     #   (to provide context for the coerce).
     # @param value The value to coerce.
@@ -421,7 +432,7 @@ class Chef
     #   this property.
     #
     def coerce(resource, value)
-      if options.has_key?(:coerce)
+      if options.has_key?(:coerce) && !value.nil?
         value = exec_in_resource(resource, options[:coerce], value)
       end
       value
@@ -509,49 +520,6 @@ class Chef
     #
     attr_reader :options
 
-    #
-    # Find out whether this type accepts nil explicitly.
-    #
-    # A type accepts nil explicitly if "is" allows nil, it validates as nil, *and* is not simply
-    # an empty type.
-    #
-    # A type is presumed to accept nil if it does coercion (which must handle nil).
-    #
-    # These examples accept nil explicitly:
-    # ```ruby
-    # property :a, [ String, nil ]
-    # property :a, [ String, NilClass ]
-    # property :a, [ String, proc { |v| v.nil? } ]
-    # ```
-    #
-    # This does not (because the "is" doesn't exist or doesn't have nil):
-    #
-    # ```ruby
-    # property :x, String
-    # ```
-    #
-    # These do not, even though nil would validate fine (because they do not
-    # have "is"):
-    #
-    # ```ruby
-    # property :a
-    # property :a, equal_to: [ 1, 2, 3, nil ]
-    # property :a, kind_of: [ String, NilClass ]
-    # property :a, respond_to: [ ]
-    # property :a, callbacks: { "a" => proc { |v| v.nil? } }
-    # ```
-    #
-    # @param resource [Chef::Resource] The resource we're coercing against
-    #   (to provide context for the coerce).
-    #
-    # @return [Boolean] Whether this value explicitly accepts nil.
-    #
-    # @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))
-    end
-
     def get_value(resource)
       if instance_variable_name
         resource.instance_variable_get(instance_variable_name)
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index a54a38eb85..ff5009e01a 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -99,6 +99,14 @@ describe "Chef::Resource.property validation" do
           expect(resource.x).to eq 'default'
         end
       end
+      if tags.include?(:nil_is_reset)
+        it "setting value to nil resets the value" do
+          resource.instance_eval { @x = 'default' }
+          expect(resource.property_is_set?(:x)).to be_truthy
+          expect(resource.x(nil)).not_to eq('default')
+          expect(resource.property_is_set?(:x)).to be_falsey
+        end
+      end
     end
   end
 
@@ -186,32 +194,38 @@ describe "Chef::Resource.property validation" do
     validation_test 'String',
       [ 'hi' ],
       [ 10 ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test ':a',
       [ :a ],
       [ :b ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test ':a, is: :b',
       [ :a, :b ],
       [ :c ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test ':a, is: [ :b, :c ]',
       [ :a, :b, :c ],
       [ :d ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test '[ :a, :b ], is: :c',
       [ :a, :b, :c ],
       [ :d ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test '[ :a, :b ], is: [ :c, :d ]',
       [ :a, :b, :c, :d ],
       [ :e ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test 'nil',
       [ nil ],
@@ -224,7 +238,8 @@ describe "Chef::Resource.property validation" do
     validation_test '[]',
       [],
       [ :a ],
-      [ nil ]
+      [],
+      :nil_is_reset
   end
 
   # is
@@ -233,34 +248,41 @@ describe "Chef::Resource.property validation" do
     validation_test 'is: String',
       [ 'a', '' ],
       [ :a, 1 ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     # Value
     validation_test 'is: :a',
       [ :a ],
       [ :b ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test 'is: [ :a, :b ]',
       [ :a, :b ],
       [ [ :a, :b ] ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test 'is: [ [ :a, :b ] ]',
       [ [ :a, :b ] ],
       [ :a, :b ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     # Regex
     validation_test 'is: /abc/',
       [ 'abc', 'wowabcwow' ],
       [ '', 'abac' ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     # Property
     validation_test 'is: Chef::Property.new(is: :a)',
       [ :a ],
-      [ :b, nil ]
+      [ :b ],
+      [],
+      :nil_is_reset
 
     # RSpec Matcher
     class Globalses
@@ -270,13 +292,15 @@ describe "Chef::Resource.property validation" do
     validation_test "is: Globalses.eq(10)",
       [ 10 ],
       [ 1 ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     # Proc
     validation_test 'is: proc { |x| x }',
       [ true, 1 ],
       [ false ],
-      [ nil ]
+      [],
+      :nil_is_reset
 
     validation_test 'is: proc { |x| x > blah }',
       [ 10 ],
@@ -293,7 +317,8 @@ describe "Chef::Resource.property validation" do
     validation_test 'is: []',
       [],
       [ :a ],
-      [ nil ]
+      [],
+      :nil_is_reset
   end
 
   # Combination
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index 095dcc8e98..4a90298b70 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -79,12 +79,18 @@ describe "Chef::Resource.property" do
       expect(resource.bare_property 10).to eq 10
       expect(resource.bare_property).to eq 10
     end
-    it "emits a deprecation warning and does a get, if set to nil" do
-      expect(resource.bare_property 10).to eq 10
-      expect { resource.bare_property nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      expect(resource.bare_property nil).to eq 10
-      expect(resource.bare_property).to eq 10
+    context "when it already has a value" do
+      before { resource.bare_property 10 }
+      it "emits a deprecation warning and does a get, if set to nil" do
+        expect { resource.bare_property nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
+        Chef::Config[:treat_deprecation_warnings_as_errors] = false
+        expect(resource.bare_property nil).to eq 10
+        expect(resource.bare_property).to eq 10
+      end
+    end
+    it "does a get, if set to nil" do
+      expect(resource.bare_property nil).to be_nil
+      expect(resource.property_is_set?(:bare_property)).to be_falsey
     end
     it "can be updated" do
       expect(resource.bare_property 10).to eq 10
@@ -922,7 +928,7 @@ describe "Chef::Resource.property" do
         expect(Namer.current_index).to eq 1
       end
       it "does not emit a deprecation warning if set to nil" do
-        expect(resource.x nil).to eq "1"
+        expect(resource.x nil).to eq nil
       end
       it "coercion sets the value (and coercion does not run on get)" do
         expect(resource.x 10).to eq "101"