Allow values to be set to nil (override defaults) if user allows it

4 files changed, 186 insertions, 85 deletions

diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index db25ecd847..2d382d8381 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -16,6 +16,8 @@
 # limitations under the License.
 
 class Chef
+  NOT_PASSED = Object.new
+
   class DelayedEvaluator < Proc
   end
   module Mixin
@@ -81,34 +83,46 @@ class Chef
         DelayedEvaluator.new(&block)
       end
 
-      def set_or_return(symbol, arg, validation)
+      def set_or_return(symbol, value, validation)
         iv_symbol = "@#{symbol.to_s}".to_sym
-        if arg.nil? && self.instance_variable_defined?(iv_symbol) == true
-          ivar = self.instance_variable_get(iv_symbol)
-          if(ivar.is_a?(DelayedEvaluator))
-            validate({ symbol => ivar.call }, { symbol => validation })[symbol]
+
+        # If the user passed NOT_PASSED, or passed nil, then this is a get.
+        case value
+        when NOT_PASSED
+          is_get = true
+          value = nil
+        when nil
+          is_get = true unless explicitly_allows_nil?(symbol, validation)
+        end
+
+        if self.instance_variable_defined?(iv_symbol) && is_get
+          value = self.instance_variable_get(iv_symbol)
+          if value.is_a?(DelayedEvaluator)
+            validate({ symbol => value.call }, { symbol => validation })[symbol]
           else
-            ivar
+            value
           end
         else
-          if(arg.is_a?(DelayedEvaluator))
-            val = arg
-          else
-            val = validate({ symbol => arg }, { symbol => validation })[symbol]
+          if !value.is_a?(DelayedEvaluator)
+            value = validate({ symbol => value }, { symbol => validation })[symbol]
 
             # Handle the case where the "default" was a DelayedEvaluator. In
             # this case, the block yields an optional parameter of +self+,
             # which is the equivalent of "new_resource"
-            if val.is_a?(DelayedEvaluator)
-              val = val.call(self)
+            if value.is_a?(DelayedEvaluator)
+              value = value.call(self)
             end
           end
-          self.instance_variable_set(iv_symbol, val)
+          self.instance_variable_set(iv_symbol, value)
         end
       end
 
       private
 
+      def explicitly_allows_nil?(key, validation)
+        validation.has_key?(:is) && _pv_is({ key => nil }, key, validation[:is], raise_error: false)
+      end
+
       # Return the value of a parameter, or nil if it doesn't exist.
       def _pv_opts_lookup(opts, key)
         if opts.has_key?(key.to_s)
@@ -135,14 +149,11 @@ class Chef
       def _pv_equal_to(opts, key, to_be)
         value = _pv_opts_lookup(opts, key)
         unless value.nil?
-          passes = false
           to_be = Array(to_be)
           to_be.each do |tb|
-            passes = true if value == tb
-          end
-          unless passes
-            raise Exceptions::ValidationFailed, "Option #{key} must be equal to one of: #{to_be.join(", ")}!  You passed #{value.inspect}."
+            return true if value == tb
           end
+          raise Exceptions::ValidationFailed, "Option #{key} must be equal to one of: #{to_be.join(", ")}!  You passed #{value.inspect}."
         end
       end
 
@@ -150,14 +161,11 @@ class Chef
       def _pv_kind_of(opts, key, to_be)
         value = _pv_opts_lookup(opts, key)
         unless value.nil?
-          passes = false
           to_be = Array(to_be)
           to_be.each do |tb|
-            passes = true if value.kind_of?(tb)
-          end
-          unless passes
-            raise Exceptions::ValidationFailed, "Option #{key} must be a kind of #{to_be}!  You passed #{value.inspect}."
+            return true if value.kind_of?(tb)
           end
+          raise Exceptions::ValidationFailed, "Option #{key} must be a kind of #{to_be}!  You passed #{value.inspect}."
         end
       end
 
@@ -182,12 +190,14 @@ class Chef
       # both :cannot_be => [ :blank, :nil ]
       def _pv_cannot_be(opts, key, predicate_method_base_name)
         value = _pv_opts_lookup(opts, key)
-        Array(predicate_method_base_name).each do |method_name|
-          predicate_method = :"#{method_name}?"
+        if !value.nil?
+          Array(predicate_method_base_name).each do |method_name|
+            predicate_method = :"#{method_name}?"
 
-          if value.respond_to?(predicate_method)
-            if value.send(predicate_method)
-              raise Exceptions::ValidationFailed, "Option #{key} cannot be #{predicate_method_base_name}"
+            if value.respond_to?(predicate_method)
+              if value.send(predicate_method)
+                raise Exceptions::ValidationFailed, "Option #{key} cannot be #{predicate_method_base_name}"
+              end
             end
           end
         end
@@ -205,17 +215,10 @@ class Chef
       def _pv_regex(opts, key, regex)
         value = _pv_opts_lookup(opts, key)
         if !value.nil?
-          passes = false
           Array(regex).each do |r|
-            if value != nil
-              if r.match(value.to_s)
-                passes = true
-              end
-            end
-          end
-          unless passes
-            raise Exceptions::ValidationFailed, "Option #{key}'s value #{value} does not match regular expression #{regex.inspect}"
+            return true if r.match(value.to_s)
           end
+          raise Exceptions::ValidationFailed, "Option #{key}'s value #{value} does not match regular expression #{regex.inspect}"
         end
       end
 
@@ -223,7 +226,7 @@ class Chef
       def _pv_callbacks(opts, key, callbacks)
         raise ArgumentError, "Callback list must be a hash!" unless callbacks.kind_of?(Hash)
         value = _pv_opts_lookup(opts, key)
-        if value != nil
+        if !value.nil?
           callbacks.each do |message, zeproc|
             if zeproc.call(value) != true
               raise Exceptions::ValidationFailed, "Option #{key}'s value #{value} #{message}!"
@@ -243,18 +246,22 @@ class Chef
       alias :_pv_name_attribute :_pv_name_property
 
       # Compare the way "case" would (i.e. `===`)
-      def _pv_is(opts, key, to_be)
+      def _pv_is(opts, key, to_be, raise_error: true)
         value = _pv_opts_lookup(opts, key)
         to_be = [ to_be ].flatten(1)
         to_be.each do |tb|
           if tb.is_a?(Proc)
-            return if instance_exec(value, &tb)
+            return true if instance_exec(value, &tb)
           else
-            return if tb === value
+            return true if tb === value
           end
         end
 
-        raise Exceptions::ValidationFailed, "Option #{key} must be one of: #{to_be.join(", ")}!  You passed #{value.inspect}."
+        if raise_error
+          raise Exceptions::ValidationFailed, "Option #{key} must be one of: #{to_be.join(", ")}!  You passed #{value.inspect}."
+        else
+          false
+        end
       end
     end
   end
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 44247ca9f0..9496656705 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -58,8 +58,6 @@ class Chef
     include Chef::Mixin::ShellOut
     include Chef::Mixin::PowershellOut
 
-    NULL_ARG = Object.new
-
     #
     # The node the current Chef run is using.
     #
@@ -773,10 +771,10 @@ class Chef
     # @example With type and options
     #   property :x, String, default: 'hi'
     #
-    def self.property(name, type=NULL_ARG, **options)
+    def self.property(name, type=NOT_PASSED, **options)
       name = name.to_sym
 
-      if type != NULL_ARG
+      if type != NOT_PASSED
         if options[:is]
           options[:is] = ([ type ] + [ options[:is] ]).flatten(1)
         else
@@ -784,7 +782,7 @@ class Chef
         end
       end
 
-      define_method(name) do |value=nil|
+      define_method(name) do |value=NOT_PASSED|
         set_or_return(name, value, options)
       end
       define_method("#{name}=") do |value|
@@ -868,8 +866,8 @@ class Chef
     #   have.
     #
     attr_accessor :allowed_actions
-    def allowed_actions(value=NULL_ARG)
-      if value != NULL_ARG
+    def allowed_actions(value=NOT_PASSED)
+      if value != NOT_PASSED
         self.allowed_actions = value
       end
       @allowed_actions
@@ -1003,9 +1001,9 @@ class Chef
     #
     # @return [Symbol] The name of this resource type (e.g. `:execute`).
     #
-    def self.resource_name(name=NULL_ARG)
+    def self.resource_name(name=NOT_PASSED)
       # Setter
-      if name != NULL_ARG
+      if name != NOT_PASSED
         remove_canonical_dsl
 
         # Set the resource_name and call provides
@@ -1095,8 +1093,8 @@ class Chef
     #
     # @return [Symbol,Array<Symbol>] The default actions for the resource.
     #
-    def self.default_action(action_name=NULL_ARG)
-      unless action_name.equal?(NULL_ARG)
+    def self.default_action(action_name=NOT_PASSED)
+      unless action_name.equal?(NOT_PASSED)
         if action_name.is_a?(Array)
           @default_action = action_name.map { |arg| arg.to_sym }
         else
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index 31204c8c23..b9e9e78d3d 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -72,24 +72,105 @@ describe "Chef::Resource.property validation" do
     end
   end
 
-  def self.validation_test(validation, success_values, failure_values, getter_values=[])
-    with_property ":x, #{validation}" do
+  def self.validation_test(validation, success_values, failure_values, getter_values=[], *tags)
+    with_property ":x, #{validation}", *tags do
       success_values.each do |v|
         it "value #{v.inspect} is valid" do
+          resource.instance_eval { @x = 'default' }
           expect(resource.x v).to eq v
+          expect(resource.x).to eq v
         end
       end
       failure_values.each do |v|
         it "value #{v.inspect} is invalid" do
           expect { resource.x v }.to raise_error Chef::Exceptions::ValidationFailed
+          # resource.instance_eval { @x = 'default' }
+          # expect { resource.x v }.to raise_error Chef::Exceptions::ValidationFailed
         end
       end
       getter_values.each do |v|
         it "setting value to #{v.inspect} does not change the value" do
           Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          resource.x success_values.first
-          expect(resource.x v).to eq success_values.first
-          expect(resource.x).to eq success_values.first
+          resource.instance_eval { @x = 'default' }
+          expect(resource.x v).to eq 'default'
+          expect(resource.x).to eq 'default'
+        end
+      end
+    end
+  end
+
+  context "basic get, set, and nil set" do
+    with_property ":x, kind_of: String" do
+      context "when the variable already has a value" do
+        before do
+          resource.instance_eval { @x = 'default' }
+        end
+        it "get succeeds" do
+          expect(resource.x).to eq 'default'
+        end
+        it "set(nil) = get" do
+          expect(resource.x nil).to eq 'default'
+          expect(resource.x).to eq 'default'
+        end
+        it "set to valid value succeeds" do
+          expect(resource.x 'str').to eq 'str'
+          expect(resource.x).to eq 'str'
+        end
+        it "set to invalid value raises ValidationFailed" do
+          expect { resource.x 10 }.to raise_error Chef::Exceptions::ValidationFailed
+        end
+      end
+      context "when the variable does not have an initial value" do
+        it "get succeeds" do
+          expect(resource.x).to be_nil
+        end
+        it "set(nil) = get" do
+          expect(resource.x nil).to be_nil
+          expect(resource.x).to be_nil
+        end
+        it "set to valid value succeeds" do
+          expect(resource.x 'str').to eq 'str'
+          expect(resource.x).to eq 'str'
+        end
+        it "set to invalid value raises ValidationFailed" do
+          expect { resource.x 10 }.to raise_error Chef::Exceptions::ValidationFailed
+        end
+      end
+    end
+    with_property ":x, [ String, nil ]" do
+      context "when the variable already has a value" do
+        before do
+          resource.instance_eval { @x = 'default' }
+        end
+        it "get succeeds" do
+          expect(resource.x).to eq 'default'
+        end
+        it "set(nil) sets the value" do
+          expect(resource.x nil).to be_nil
+          expect(resource.x).to be_nil
+        end
+        it "set to valid value succeeds" do
+          expect(resource.x 'str').to eq 'str'
+          expect(resource.x).to eq 'str'
+        end
+        it "set to invalid value raises ValidationFailed" do
+          expect { resource.x 10 }.to raise_error Chef::Exceptions::ValidationFailed
+        end
+      end
+      context "when the variable does not have an initial value" do
+        it "get succeeds" do
+          expect(resource.x).to be_nil
+        end
+        it "set(nil) sets the value" do
+          expect(resource.x nil).to be_nil
+          expect(resource.x).to be_nil
+        end
+        it "set to valid value succeeds" do
+          expect(resource.x 'str').to eq 'str'
+          expect(resource.x).to eq 'str'
+        end
+        it "set to invalid value raises ValidationFailed" do
+          expect { resource.x 10 }.to raise_error Chef::Exceptions::ValidationFailed
         end
       end
     end
@@ -123,7 +204,8 @@ describe "Chef::Resource.property validation" do
 
     validation_test 'nil',
       [ nil ],
-      [ :a ]
+      [ :a ],
+      []
 
     validation_test '[ nil ]',
       [ nil ],
@@ -222,16 +304,19 @@ describe "Chef::Resource.property validation" do
       [ nil ]
 
     validation_test 'equal_to: nil',
-      [ nil ],
-      [ 'a' ]
+      [ ],
+      [ 'a' ],
+      [ nil ]
 
     validation_test 'equal_to: [ "a", nil ]',
-      [ 'a', nil ],
-      [ 'b' ]
+      [ 'a' ],
+      [ 'b' ],
+      [ nil ]
 
     validation_test 'equal_to: [ nil, "a" ]',
-      [ 'a', nil ],
-      [ 'b' ]
+      [ 'a' ],
+      [ 'b' ],
+      [ nil ]
 
     validation_test 'equal_to: []',
       [],
@@ -257,12 +342,14 @@ describe "Chef::Resource.property validation" do
       [ nil ]
 
     validation_test 'kind_of: NilClass',
-      [ nil ],
-      [ 'a' ]
+      [ ],
+      [ 'a' ],
+      [ nil ]
 
     validation_test 'kind_of: [ NilClass, String ]',
-      [ nil, 'a' ],
-      [ :a ]
+      [ 'a' ],
+      [ :a ],
+      [ nil ]
 
     validation_test 'kind_of: []',
       [],
@@ -314,8 +401,9 @@ describe "Chef::Resource.property validation" do
       [ 11, 4 ]
 
     validation_test 'callbacks: { "a" => proc { |x| x.nil? } }',
-      [ nil ],
-      [ 'a' ]
+      [ ],
+      [ 'a' ],
+      [ nil ]
 
     validation_test 'callbacks: {}',
       [ :a ],
@@ -335,6 +423,11 @@ describe "Chef::Resource.property validation" do
       [ 1 ],
       [ nil ]
 
+    validation_test 'respond_to: :to_s',
+      [ :a ],
+      [],
+      [ nil ]
+
     validation_test 'respond_to: [ :split, :to_s ]',
       [ 'hi' ],
       [ 1 ],
@@ -363,31 +456,33 @@ describe "Chef::Resource.property validation" do
 
   context "cannot_be" do
     validation_test 'cannot_be: :empty',
-      [ nil, 1, [1,2], { a: 10 } ],
-      [ [] ]
+      [ 1, [1,2], { a: 10 } ],
+      [ [] ],
+      [ nil ]
 
     validation_test 'cannot_be: "empty"',
-      [ nil, 1, [1,2], { a: 10 } ],
-      [ [] ]
+      [ 1, [1,2], { a: 10 } ],
+      [ [] ],
+      [ nil ]
 
     validation_test 'cannot_be: [ :empty, :nil ]',
       [ 1, [1,2], { a: 10 } ],
-      [ [], nil ],
+      [ [] ],
       [ nil ]
 
     validation_test 'cannot_be: [ "empty", "nil" ]',
       [ 1, [1,2], { a: 10 } ],
-      [ [], nil ],
+      [ [] ],
       [ nil ]
 
     validation_test 'cannot_be: [ :nil, :empty ]',
       [ 1, [1,2], { a: 10 } ],
-      [ [], nil ],
+      [ [] ],
       [ nil ]
 
     validation_test 'cannot_be: [ :empty, :nil, :blahblah ]',
       [ 1, [1,2], { a: 10 } ],
-      [ [], nil ],
+      [ [] ],
       [ nil ]
 
     validation_test 'cannot_be: []',
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index d9fc4fa41f..ff5e135c78 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -76,12 +76,13 @@ describe "Chef::Resource.property" do
       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
-    # 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
+    end
     it "can be updated" do
       expect(resource.bare_property 10).to eq 10
       expect(resource.bare_property 20).to eq 20