Fix set_or_return order: default->coerce->validate

3 files changed, 188 insertions, 147 deletions

diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index e076b83460..3edc761ffc 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -67,7 +67,7 @@ class Chef
             true
           when Hash
             validation.each do |check, carg|
-              check_method = "_pv_#{check.to_s}"
+              check_method = "_pv_#{check}"
               if self.respond_to?(check_method, true)
                 self.send(check_method, opts, key, carg)
               else
@@ -87,52 +87,87 @@ class Chef
         symbol = symbol.to_sym
         iv_symbol = :"@#{symbol}"
 
+        # Steal default, coerce, name_property and required from validation
+        # so that we can handle the order in which they are applied
+        validation = validation.dup
+        if validation.has_key?(:default)
+          default = validation.delete(:default)
+        elsif validation.has_key?('default')
+          default = validation.delete('default')
+        else
+          default = NOT_PASSED
+        end
+        coerce    = validation.delete(:coerce)
+        coerce  ||= validation.delete('coerce')
+        name_property   = validation.delete(:name_property)
+        name_property ||= validation.delete('name_property')
+        name_property ||= validation.delete(:name_attribute)
+        name_property ||= validation.delete('name_attribute')
+        required   = validation.delete(:required)
+        required ||= validation.delete('required')
+
+        opts = {}
         # If the user passed NOT_PASSED, or passed nil, then this is a get.
         if value == NOT_PASSED || (value.nil? && !explicitly_allows_nil?(symbol, validation))
 
           # Get the value if there is one
           if self.instance_variable_defined?(iv_symbol)
-            value = self.instance_variable_get(iv_symbol)
-            if value.is_a?(DelayedEvaluator)
-              # Pass optional first parameter of self
-              value = value.call(self)
-              value = validate({ symbol => value }, { symbol => validation })[symbol]
+            opts[symbol] = self.instance_variable_get(iv_symbol)
+
+            # Handle lazy values
+            if opts[symbol].is_a?(DelayedEvaluator)
+              if opts[symbol].arity >= 1
+                opts[symbol] = opts[symbol].call(self)
+              else
+                opts[symbol] = opts[symbol].call
+              end
+
+              # Coerce and validate the default value
+              _pv_required(opts, symbol, required, explicitly_allows_nil?(symbol, validation)) if required
+              _pv_coerce(opts, symbol, coerce) if coerce
+              validate(opts, { symbol => validation })
             end
 
           # Get the default value
           else
-            validated = validate({}, { symbol => validation })
-            if validated.has_key?(symbol)
-              value = validated[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 value.is_a?(DelayedEvaluator)
-                if value.arity >= 1
-                  value = value.call(self)
+            _pv_required(opts, symbol, required, explicitly_allows_nil?(symbol, validation)) if required
+            _pv_default(opts, symbol, default) unless default == NOT_PASSED
+            _pv_name_property(opts, symbol, name_property)
+
+            if opts.has_key?(symbol)
+              # Handle lazy defaults.
+              if opts[symbol].is_a?(DelayedEvaluator)
+                if opts[symbol].arity >= 1
+                  opts[symbol] = opts[symbol].call(self)
                 else
-                  value = instance_eval(&value)
+                  opts[symbol] = instance_eval(&opts[symbol])
                 end
               end
 
+              # Coerce and validate the default value
+              _pv_required(opts, symbol, required, explicitly_allows_nil?(symbol, validation)) if required
+              _pv_coerce(opts, symbol, coerce) if coerce
+              validate(opts, { symbol => validation })
+
               # Defaults are presently "stickily" set on the instance
-              self.instance_variable_set(iv_symbol, value)
-            else
-              value = nil
+              self.instance_variable_set(iv_symbol, opts[symbol])
             end
           end
 
         # Set the value
         else
-          unless value.is_a?(DelayedEvaluator)
-            value = validate({ symbol => value }, { symbol => validation })[symbol]
+          opts[symbol] = value
+          unless opts[symbol].is_a?(DelayedEvaluator)
+            # Coerce and validate the value
+            _pv_required(opts, symbol, required, explicitly_allows_nil?(symbol, validation)) if required
+            _pv_coerce(opts, symbol, coerce) if coerce
+            validate(opts, { symbol => validation })
           end
 
-          self.instance_variable_set(iv_symbol, value)
+          self.instance_variable_set(iv_symbol, opts[symbol])
         end
 
-        value
+        opts[symbol]
       end
 
       private
@@ -153,14 +188,11 @@ class Chef
       end
 
       # Raise an exception if the parameter is not found.
-      def _pv_required(opts, key, is_required=true)
+      def _pv_required(opts, key, is_required=true, explicitly_allows_nil=false)
         if is_required
-          if (opts.has_key?(key.to_s) && !opts[key.to_s].nil?) ||
-              (opts.has_key?(key.to_sym) && !opts[key.to_sym].nil?)
-            true
-          else
-            raise Exceptions::ValidationFailed, "Required argument #{key} is missing!"
-          end
+          return true if opts.has_key?(key.to_s) && (explicitly_allows_nil || !opts[key.to_s].nil?)
+          return true if opts.has_key?(key.to_sym) && (explicitly_allows_nil || !opts[key.to_sym].nil?)
+          raise Exceptions::ValidationFailed, "Required argument #{key} is missing!"
         end
       end
 
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index 14be7dc271..e32147bd38 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -3,7 +3,7 @@ require 'support/shared/integration/integration_helper'
 describe "Chef::Resource.property validation" do
   include IntegrationSupport
 
-  class Namer
+  module Namer
     @i = 0
     def self.next_resource_name
       "chef_resource_property_spec_#{@i += 1}"
@@ -519,24 +519,26 @@ describe "Chef::Resource.property validation" do
       it "if x is not specified, retrieval fails" do
         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
       end
-      it "value nil is invalid" do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
-      end
       it "value 1 is valid" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
+      it "value nil does a get" do
+        Chef::Config[:treat_deprecation_warnings_as_errors] = false
+        resource.x 1
+        resource.x nil
+        expect(resource.x).to eq 1
+      end
     end
 
     with_property ':x, [String, nil], required: true' do
       it "if x is not specified, retrieval fails" do
         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
       end
-      # it "value nil is valid" do
-      #   expect(resource.x nil).to be_nil
-      #   expect(resource.x).to be_nil
-      # end
+      it "value nil is valid" do
+        expect(resource.x nil).to be_nil
+        expect(resource.x).to be_nil
+      end
       it "value '1' is valid" do
         expect(resource.x '1').to eq '1'
         expect(resource.x).to eq '1'
@@ -546,32 +548,34 @@ describe "Chef::Resource.property validation" do
       end
     end
 
-    # with_property ':x, name_property: true, required: true' do
-    with_property ':x, required: true, name_property: true' do
+    with_property ':x, name_property: true, required: true' do
       it "if x is not specified, retrieval fails" do
         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
       end
-      it "value nil is invalid" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
-      end
       it "value 1 is valid" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
+      it "value nil does a get" do
+        resource.x 1
+        resource.x nil
+        expect(resource.x).to eq 1
+      end
     end
 
-    # with_property ':x, default: 10, required: true' do
-    with_property ':x, required: true, default: 10' do
+    with_property ':x, default: 10, required: true' do
       it "if x is not specified, retrieval fails" do
         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
       end
-      it "value nil is invalid" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
-      end
       it "value 1 is valid" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
+      it "value nil does a get" do
+        resource.x 1
+        resource.x nil
+        expect(resource.x).to eq 1
+      end
     end
   end
 
@@ -607,9 +611,11 @@ describe "Chef::Resource.property validation" do
           expect { resource.x '1' }.to raise_error Chef::Exceptions::ValidationFailed
         end
 
-        it "value nil is invalid" do
+        it "value nil does a get" do
           Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
+          resource.x 1
+          resource.x nil
+          expect(resource.x).to eq 1
         end
       end
     end
@@ -635,8 +641,10 @@ describe "Chef::Resource.property validation" do
           expect { resource.x '1' }.to raise_error Chef::Exceptions::ValidationFailed
         end
 
-        it "value nil is invalid" do
-          expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
+        it "value nil does a get" do
+          resource.x 1
+          resource.x nil
+          expect(resource.x).to eq 1
         end
       end
     end
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index 919be5b61b..8c01bf7b49 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -3,7 +3,7 @@ require 'support/shared/integration/integration_helper'
 describe "Chef::Resource.property" do
   include IntegrationSupport
 
-  class Namer
+  module Namer
     @i = 0
     def self.next_resource_name
       "chef_resource_property_spec_#{@i += 1}"
@@ -281,9 +281,9 @@ describe "Chef::Resource.property" do
         resource.x lazy { 10 }
         expect(resource.property_is_set?(:x)).to be_truthy
       end
-      it "when x is retrieved, property_is_set?(:x) is true" do
+      it "when x is retrieved, property_is_set?(:x) is false" do
         resource.x
-        expect(resource.property_is_set?(:x)).to be_truthy
+        expect(resource.property_is_set?(:x)).to be_falsey
       end
     end
 
@@ -445,92 +445,94 @@ describe "Chef::Resource.property" do
           expect(resource.x 'hi').to eq 'hi'
           expect(resource.x).to eq 'hi'
         end
-        # it "when x is retrieved, a validation error is raised" do
-        #   expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-        #   expect(Namer.current_index).to eq 1
-        # end
+        it "when x is retrieved, a validation error is raised" do
+          expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
+          expect(Namer.current_index).to eq 1
+        end
       end
 
-      # with_property ":x, default: lazy { Namer.next_index }, is: proc { |v| Namer.next_index; true }" do
-      #   it "when x is retrieved, validation is run each time" do
-      #     expect(resource.x).to eq 1
-      #     expect(Namer.current_index).to eq 2
-      #     expect(resource.x).to eq 3
-      #     expect(Namer.current_index).to eq 4
-      #   end
-      # end
-    end
-
-    # context "coercion of defaults" do
-    #   with_property ':x, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
-    #     it "when the resource is created, the proc is not yet run" do
-    #       resource
-    #       expect(Namer.current_index).to eq 0
-    #     end
-    #     it "when x is set, coercion is run" do
-    #       expect(resource.x 'hi').to eq 'hi1'
-    #       expect(resource.x).to eq 'hi1'
-    #       expect(Namer.current_index).to eq 1
-    #     end
-    #     it "when x is retrieved, coercion is run, no more than once" do
-    #       expect(resource.x).to eq '101'
-    #       expect(resource.x).to eq '101'
-    #       expect(Namer.current_index).to eq 1
-    #     end
-    #   end
-    #
-    #   with_property ':x, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-    #     it "when the resource is created, the proc is not yet run" do
-    #       resource
-    #       expect(Namer.current_index).to eq 0
-    #     end
-    #     it "when x is set, coercion is run" do
-    #       expect(resource.x 'hi').to eq 'hi1'
-    #       expect(resource.x).to eq 'hi1'
-    #       expect(Namer.current_index).to eq 1
-    #     end
-    #   end
-    #
-    #   with_property ':x, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }, is: proc { |v| Namer.next_index; true }' do
-    #     it "when x is retrieved, coercion is run each time" do
-    #       expect(resource.x).to eq '101'
-    #       expect(Namer.current_index).to eq 2
-    #       expect(resource.x).to eq '103'
-    #       expect(Namer.current_index).to eq 4
-    #     end
-    #   end
-    #
-    #   context "validation and coercion of defaults" do
-    #     with_property ':x, String, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
-    #       it "when x is retrieved, it is coerced before validating and passes" do
-    #         expect(resource.x).to eq '101'
-    #       end
-    #     end
-    #     with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
-    #       it "when x is retrieved, it is coerced before validating and fails" do
-    #         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-    #       end
-    #     end
-    #     with_property ':x, String, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-    #       it "when x is retrieved, it is coerced before validating and passes" do
-    #         expect(resource.x).to eq '101'
-    #       end
-    #     end
-    #     with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-    #       it "when x is retrieved, it is coerced before validating and fails" do
-    #         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-    #       end
-    #     end
-    #     with_property ':x, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }, is: proc { |v| Namer.next_index; true }' do
-    #       it "when x is retrieved, coercion and validation is run on each access" do
-    #         expect(resource.x).to eq '101'
-    #         expect(Namer.current_index).to eq 2
-    #         expect(resource.x).to eq '103'
-    #         expect(Namer.current_index).to eq 4
-    #       end
-    #     end
-    #   end
-    # end
+      with_property ":x, default: lazy { Namer.next_index }, is: proc { |v| Namer.next_index; true }" do
+        it "validation is only run the first time" do
+          expect(resource.x).to eq 1
+          expect(Namer.current_index).to eq 2
+          expect(resource.x).to eq 1
+          expect(Namer.current_index).to eq 2
+        end
+      end
+    end
+
+    context "coercion of defaults" do
+      with_property ':x, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
+        it "when the resource is created, the proc is not yet run" do
+          resource
+          expect(Namer.current_index).to eq 0
+        end
+        it "when x is set, coercion is run" do
+          expect(resource.x 'hi').to eq 'hi1'
+          expect(resource.x).to eq 'hi1'
+          expect(Namer.current_index).to eq 1
+        end
+        it "when x is retrieved, coercion is run, no more than once" do
+          expect(resource.x).to eq '101'
+          expect(resource.x).to eq '101'
+          expect(Namer.current_index).to eq 1
+        end
+      end
+
+      with_property ':x, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
+        it "when the resource is created, the proc is not yet run" do
+          resource
+          expect(Namer.current_index).to eq 0
+        end
+        it "when x is set, coercion is run" do
+          expect(resource.x 'hi').to eq 'hi1'
+          expect(resource.x).to eq 'hi1'
+          expect(Namer.current_index).to eq 1
+        end
+      end
+
+      with_property ':x, proc { |v| Namer.next_index; true }, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
+        it "coercion is only run the first time x is retrieved" do
+          expect(Namer.current_index).to eq 0
+          expect(resource.x).to eq '101'
+          expect(Namer.current_index).to eq 2
+          expect(resource.x).to eq '101'
+          expect(Namer.current_index).to eq 2
+        end
+      end
+
+      context "validation and coercion of defaults" do
+        with_property ':x, String, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
+          it "when x is retrieved, it is coerced before validating and passes" do
+            expect(resource.x).to eq '101'
+          end
+        end
+        with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
+          it "when x is retrieved, it is coerced before validating and fails" do
+            expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
+          end
+        end
+        with_property ':x, String, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
+          it "when x is retrieved, it is coerced before validating and passes" do
+            expect(resource.x).to eq '101'
+          end
+        end
+        with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
+          it "when x is retrieved, it is coerced before validating and fails" do
+            expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
+          end
+        end
+        with_property ':x, proc { |v| Namer.next_index; true }, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
+          it "coercion and validation is only run the first time x is retrieved" do
+            expect(Namer.current_index).to eq 0
+            expect(resource.x).to eq '101'
+            expect(Namer.current_index).to eq 2
+            expect(resource.x).to eq '101'
+            expect(Namer.current_index).to eq 2
+          end
+        end
+      end
+    end
   end
 
   context "Chef::Resource#lazy" do
@@ -769,12 +771,11 @@ describe "Chef::Resource.property" do
           expect(resource.x).to eq 10
         end
       end
-      # with_property ":x, #{name}: true, default: 10" do
-      #   it "chooses default over #{name}" do
-      #     # expect(resource.x).to eq 10
-      #     expect(resource.x).to eq 10
-      #   end
-      # end
+      with_property ":x, #{name}: true, default: 10" do
+        it "chooses default over #{name}" do
+          expect(resource.x).to eq 10
+        end
+      end
     end
   end
 end