Chef-13: Nillable properties

remove deprecations and now properties are nillable and passing a nil is
now always a set, not a get.

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

5 files changed, 93 insertions, 175 deletions

diff --git a/lib/chef/constants.rb b/lib/chef/constants.rb
index f32c3e6654..d75b632173 100644
--- a/lib/chef/constants.rb
+++ b/lib/chef/constants.rb
@@ -1,6 +1,6 @@
 #
 # Author:: John Keiser <jkeiser@chef.io>
-# Copyright:: Copyright 2015-2016, Chef Software Inc.
+# Copyright:: Copyright 2015-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index 0db058c3ab..d90e38b916 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -18,6 +18,7 @@
 require "chef/constants"
 require "chef/property"
 require "chef/delayed_evaluator"
+require "chef/exceptions"
 
 class Chef
   module Mixin
@@ -332,7 +333,7 @@ class Chef
       def _pv_name_property(opts, key, is_name_property = true)
         if is_name_property
           if opts[key].nil?
-            raise CannotValidateStaticallyError, "name_property cannot be evaluated without a resource." if self == Chef::Mixin::ParamsValidate
+            raise Exceptions::CannotValidateStaticallyError, "name_property cannot be evaluated without a resource." if self == Chef::Mixin::ParamsValidate
             opts[key] = instance_variable_get(:"@name")
           end
         end
@@ -404,7 +405,7 @@ class Chef
         passed = to_be.any? do |tb|
           case tb
           when Proc
-            raise CannotValidateStaticallyError, "is: proc { } must be evaluated once for each resource" if self == Chef::Mixin::ParamsValidate
+            raise Exceptions::CannotValidateStaticallyError, "is: proc { } must be evaluated once for each resource" if self == Chef::Mixin::ParamsValidate
             instance_exec(value, &tb)
           when Property
             begin
@@ -448,10 +449,10 @@ class Chef
       #
       def _pv_coerce(opts, key, coercer)
         if opts.has_key?(key.to_s)
-          raise CannotValidateStaticallyError, "coerce must be evaluated for each resource." if self == Chef::Mixin::ParamsValidate
+          raise Exceptions::CannotValidateStaticallyError, "coerce must be evaluated for each resource." if self == Chef::Mixin::ParamsValidate
           opts[key.to_s] = instance_exec(opts[key], &coercer)
         elsif opts.has_key?(key.to_sym)
-          raise CannotValidateStaticallyError, "coerce must be evaluated for each resource." if self == Chef::Mixin::ParamsValidate
+          raise Exceptions::CannotValidateStaticallyError, "coerce must be evaluated for each resource." if self == Chef::Mixin::ParamsValidate
           opts[key.to_sym] = instance_exec(opts[key], &coercer)
         end
       end
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index ac226bc815..85e184497b 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -74,7 +74,7 @@ class Chef
     #     return `true` if the property is set *or* if `name` is set.
     #   @option options [Boolean] :nillable `true` opt-in to Chef-13 style behavior where
     #     attempting to set a nil value will really set a nil value instead of issuing
-    #     a warning and operating like a getter
+    #     a warning and operating like a getter [DEPRECATED]
     #   @option options [Object] :default The value this property
     #     will return if the user does not set one. If this is `lazy`, it will
     #     be run in the context of the instance (and able to access other
@@ -264,32 +264,8 @@ class Chef
     #
     def call(resource, value = NOT_PASSED)
       if value == NOT_PASSED
-        return get(resource)
-      end
-
-      if value.nil? && !nillable?
-        # 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, nil_set: true)
-
-        # Warn about this becoming a set in Chef 13.
-        begin
-          input_to_stored_value(resource, value)
-          # If nil is valid, and it would change the value, warn that this will change to a set.
-          if !result.nil?
-            Chef.deprecated(:custom_resource, "An attempt was made to change #{name} from #{result.inspect} to nil by calling #{name}(nil). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil.")
-          end
-        rescue Chef::Exceptions::DeprecatedFeatureError
-          raise
-        rescue
-          # If nil is invalid, warn that this will become an error.
-          Chef.deprecated(:custom_resource, "nil is an invalid value for #{self}. In Chef 13, this warning will change to an error. Error: #{$!}")
-        end
-
-        result
+        get(resource)
       else
-        # Anything else, such as myprop(value) is a set
         set(resource, value)
       end
     end
@@ -318,10 +294,11 @@ class Chef
     #
     def get(resource, nil_set: false)
       # If it's set, return it (and evaluate any lazy values)
+      value = nil
+
       if is_set?(resource)
         value = get_value(resource)
         value = stored_value_to_output(resource, value)
-
       else
         # We are getting the default value.
 
@@ -366,13 +343,14 @@ class Chef
           if !value.frozen? && !value.nil?
             set_value(resource, value)
           end
-
-          value
-
-        elsif required?
-          raise Chef::Exceptions::ValidationFailed, "#{name} is required"
         end
       end
+
+      if value.nil? && required?
+        raise Chef::Exceptions::ValidationFailed, "#{name} is required"
+      else
+        value
+      end
     end
 
     #
@@ -391,7 +369,13 @@ class Chef
     #   this property.
     #
     def set(resource, value)
-      set_value(resource, input_to_stored_value(resource, value))
+      value = set_value(resource, input_to_stored_value(resource, value))
+
+      if value.nil? && required?
+        raise Chef::Exceptions::ValidationFailed, "#{name} is required"
+      else
+        value
+      end
     end
 
     #
@@ -444,8 +428,8 @@ class Chef
     #
     def coerce(resource, value)
       if options.has_key?(:coerce)
-        # If we have no default value, `nil` is never coerced or validated
-        unless !has_default? && value.nil?
+        # nil is never coerced
+        unless value.nil?
           value = exec_in_resource(resource, options[:coerce], value)
         end
       end
@@ -466,8 +450,8 @@ class Chef
     #   this property.
     #
     def validate(resource, value)
-      # If we have no default value, `nil` is never coerced or validated
-      unless value.nil? && !has_default?
+      # nils are not validated unless we have an explicit default value
+      if !value.nil? || has_default?
         if resource
           resource.validate({ name => value }, { name => validation_options })
         else
@@ -651,7 +635,7 @@ class Chef
       # Emit the deprecation.
       resource_name = declared_in.respond_to?(:resource_name) ? declared_in.resource_name : declared_in
       Chef.deprecated(:property_name_collision, "Property `#{name}` of resource `#{resource_name}` overwrites an existing method. " \
-        "Please use a different property name. This will raise an exception in Chef 13.")
+                      "Please use a different property name. This will raise an exception in Chef 13.")
     end
 
     def exec_in_resource(resource, proc, *args)
@@ -688,31 +672,9 @@ class Chef
     # valid.
     def coerce_and_validate(resource, value, is_default: false)
       result = coerce(resource, value)
-      begin
-        # If the input is from a default, we need to emit an invalid default warning on validate.
-        validate(resource, result)
-      rescue Chef::Exceptions::CannotValidateStaticallyError
-        # This one gets re-raised
-        raise
-      rescue
-        # Anything else is just an invalid default: in those cases, we just
-        # warn and return the (possibly coerced) value to the user.
-        if is_default
-          if value.nil?
-            Chef.deprecated(:custom_resource, "Default value nil is invalid for property #{self}. Possible fixes: 1. Remove 'default: nil' if nil means 'undefined'. 2. Set a valid default value if there is a reasonable one. 3. Allow nil as a valid value of your property (for example, 'property #{name.inspect}, [ String, nil ], default: nil'). Error: #{$!}")
-          else
-            Chef.deprecated(:custom_resource, "Default value #{value.inspect} is invalid for property #{self}. In Chef 13 this will become an error: #{$!}.")
-          end
-        else
-          raise
-        end
-      end
+      validate(resource, result)
 
       result
     end
-
-    def nillable?
-      !!options[:nillable]
-    end
   end
 end
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index 4e1b252863..13afcdfbc2 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -100,13 +100,10 @@ describe "Chef::Resource.property validation" do
         expect(resource.x).to be_nil
       end
       unless tags.include?(:nillable)
-        it "changing x to nil warns that the get will change to a set in Chef 13 and does not change the value" do
+        it "changing x to nil does a set" do
           resource.instance_eval { @x = "default" }
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /An attempt was made to change x from "default" to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          expect(resource.x nil).to eq "default"
-          expect(resource.x).to eq "default"
+          expect(resource.x nil).to eq nil
+          expect(resource.x).to eq nil
         end
       end
     end
@@ -116,13 +113,10 @@ describe "Chef::Resource.property validation" do
           expect(resource.x nil).to be_nil
           expect(resource.x).to be_nil
         end
-        it "changing x to nil warns that the get will change to a set in Chef 13 and does not change the value" do
+        it "changing x to nil does a set" do
           resource.instance_eval { @x = "default" }
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /An attempt was made to change x from "default" to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          expect(resource.x nil).to eq "default"
-          expect(resource.x).to eq "default"
+          expect(resource.x nil).to eq nil
+          expect(resource.x).to eq nil
         end
       end
     elsif tags.include?(:nillable)
@@ -134,26 +128,17 @@ describe "Chef::Resource.property validation" do
           expect(resource.x).to eq nil
         end
       end
-    else
+    elsif tags.include?(:delayed_nil_default_failure)
       it "property :x, #{validation}, default: nil warns that the default is invalid" do
-        expect { resource_class.class_eval("property :x, #{validation}, default: nil", __FILE__, __LINE__) }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-          /Default value nil is invalid for property x of resource chef_resource_property_spec_(\d+). Possible fixes: 1. Remove 'default: nil' if nil means 'undefined'. 2. Set a valid default value if there is a reasonable one. 3. Allow nil as a valid value of your property \(for example, 'property :x, \[ String, nil \], default: nil'\)./
+        expect { resource_class.class_eval("property :x, #{validation}, default: nil", __FILE__, __LINE__) }.not_to raise_error
+        expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed, /Property x must be one of: .*  You passed nil./
       end
-      context "With property :x, #{validation}, default: nil" do
-        before do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          resource_class.class_eval("property :x, #{validation}, default: nil", __FILE__, __LINE__)
-          Chef::Config[:treat_deprecation_warnings_as_errors] = true
-        end
-
-        it "changing x to nil emits a warning that the value is invalid and does not change the value" do
-          resource.instance_eval { @x = "default" }
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /nil is an invalid value for x of resource chef_resource_property_spec_(\d+). In Chef 13, this warning will change to an error./
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          expect(resource.x nil).to eq "default"
-          expect(resource.x).to eq "default"
-        end
+    elsif tags.include?(:skip_nil_default)
+      # intentionally left blank
+    else
+      it "property :x, #{validation}, default: nil warns that the default is invalid" do
+        expect { resource_class.class_eval("property :x, #{validation}, default: nil", __FILE__, __LINE__) }.to raise_error Chef::Exceptions::ValidationFailed,
+          /Property x must be one of: .*  You passed nil./
       end
     end
   end
@@ -174,12 +159,10 @@ describe "Chef::Resource.property validation" do
         it "set to invalid value raises ValidationFailed" do
           expect { resource.x 10 }.to raise_error Chef::Exceptions::ValidationFailed
         end
-        it "set to nil emits a deprecation warning and does a get" do
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+        it "set to nil does a set" do
           resource.x "str"
-          expect(resource.x nil).to eq "str"
-          expect(resource.x).to eq "str"
+          expect(resource.x nil).to eq nil
+          expect(resource.x).to eq nil
         end
       end
       context "when the variable does not have an initial value" do
@@ -206,12 +189,9 @@ describe "Chef::Resource.property validation" do
         it "get succeeds" do
           expect(resource.x).to eq "default"
         end
-        it "set(nil) emits a warning that the value will be set, but does not set the value" do
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /An attempt was made to change x from "default" to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          expect(resource.x nil).to eq "default"
-          expect(resource.x).to eq "default"
+        it "set(nil) does a set" do
+          expect(resource.x nil).to eq nil
+          expect(resource.x).to eq nil
         end
         it "set to valid value succeeds" do
           expect(resource.x "str").to eq "str"
@@ -328,11 +308,17 @@ describe "Chef::Resource.property validation" do
     # Proc
     validation_test "is: proc { |x| x }",
       [ true, 1 ],
-      [ false ]
+      [ false ],
+      # this is somewhat complicated, we test adding `default: nil` and that the default fails, but with a proc the
+      # validation is delayed until access, so we have to test after access not after declaring the default
+      :delayed_nil_default_failure
 
     validation_test "is: proc { |x| x > blah }",
       [ 10 ],
-      [ -1 ]
+      [ -1 ],
+      # here the test of adding `default: nil` just causes the proc to explode because nil gets passed to the proc
+      # which throws a NoMethodError on NilClass for the `>` method.
+      :skip_nil_default
 
     validation_test "is: nil",
       [ ],
@@ -589,15 +575,21 @@ describe "Chef::Resource.property validation" do
       it "value nil emits a validation failed error because it must have a value" do
         expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
       end
-      context "and value is set to something other than nil" do
-        before { resource.x 10 }
-        it "value nil emits a deprecation warning and does a get" do
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          resource.x 1
-          expect(resource.x nil).to eq 1
-          expect(resource.x).to eq 1
-        end
+    end
+
+    with_property ":x, String, 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 not valid (required means 'not nil')" 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 1 is invalid" do
+        expect { resource.x 1 }.to raise_error Chef::Exceptions::ValidationFailed
       end
     end
 
@@ -625,12 +617,8 @@ describe "Chef::Resource.property validation" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
-      it "value nil emits a deprecation warning and does a get" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        resource.x 1
-        expect(resource.x nil).to eq 1
-        expect(resource.x).to eq 1
+      it "value nil is invalid" do
+        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
       end
     end
 
@@ -643,11 +631,7 @@ describe "Chef::Resource.property validation" do
         expect(resource.x).to eq 1
       end
       it "value nil is invalid" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        resource.x 1
-        expect(resource.x nil).to eq 1
-        expect(resource.x).to eq 1
+        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
       end
     end
   end
@@ -675,15 +659,13 @@ describe "Chef::Resource.property validation" do
         end
 
         it "value '1' is invalid" do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
           expect { resource.x "1" }.to raise_error Chef::Exceptions::ValidationFailed
         end
 
-        it "value nil does a get" do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+        it "value nil does a set" do
           resource.x 1
           resource.x nil
-          expect(resource.x).to eq 1
+          expect(resource.x).to eq nil
         end
       end
     end
@@ -709,10 +691,10 @@ describe "Chef::Resource.property validation" do
           expect { resource.x "1" }.to raise_error Chef::Exceptions::ValidationFailed
         end
 
-        it "value nil does a get" do
+        it "value nil does a set" do
           resource.x 1
           resource.x nil
-          expect(resource.x).to eq 1
+          expect(resource.x).to eq nil
         end
       end
     end
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index 0e7bf53ec5..9f3ab43a2f 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -82,12 +82,10 @@ 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
+    it "nil does a set" 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
+      expect(resource.bare_property nil).to eq nil
+      expect(resource.bare_property).to eq nil
     end
     it "can be updated" do
       expect(resource.bare_property 10).to eq 10
@@ -585,24 +583,8 @@ describe "Chef::Resource.property" do
     end
 
     context "validation of defaults" do
-      it "When a class is declared with property :x, String, default: 10, a warning is emitted" do
-        expect { resource_class.class_eval { property :x, String, default: 10 } }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-          /Default value 10 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Property x must be one of: String!  You passed 10./
-      end
-      context "With property :x, String, default: 10" do
-        before do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          resource_class.class_eval { property :x, String, default: 10 }
-          Chef::Config[:treat_deprecation_warnings_as_errors] = true
-        end
-
-        it "when x is set, no error is raised" do
-          expect(resource.x "hi").to eq "hi"
-          expect(resource.x).to eq "hi"
-        end
-        it "when x is retrieved, no validation error is raised" do
-          expect(resource.x).to eq 10
-        end
+      it "When a class is declared with property :x, String, default: 10, it immediately fails validation" do
+        expect { resource_class.class_eval { property :x, String, default: 10 } }.to raise_error Chef::Exceptions::ValidationFailed
       end
 
       with_property ":x, String, default: lazy { Namer.next_index }" do
@@ -613,12 +595,9 @@ describe "Chef::Resource.property" do
           expect(resource.x "hi").to eq "hi"
           expect(resource.x).to eq "hi"
         end
-        it "when x is retrieved, an invalid default warning is emitted and the value is returned" do
-          expect { resource.x }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-            /Default value 1 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Property x must be one of: String!  You passed 1./
+        it "when x is retrieved, it fails validation" do
+          expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
           expect(Namer.current_index).to eq 1
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          expect(resource.x).to eq 2
         end
       end
 
@@ -731,11 +710,8 @@ describe "Chef::Resource.property" do
           end
         end
         with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
-          it "when x is retrieved, it is coerced and emits an invalid default warning, but still returns the value" do
-            expect { resource.x }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-              /Default value 10 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Property x must be one of: Integer!  You passed "101"./
-            Chef::Config[:treat_deprecation_warnings_as_errors] = false
-            expect(resource.x).to eq "102"
+          it "when x is retrieved, it is coerced and fails validation" 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
@@ -744,11 +720,8 @@ describe "Chef::Resource.property" do
           end
         end
         with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-          it "when x is retrieved, it is coerced and emits an invalid default warning; the value is still returned." do
-            expect { resource.x }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
-              /Default value 10 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Property x must be one of: Integer!  You passed "101"./
-            Chef::Config[:treat_deprecation_warnings_as_errors] = false
-            expect(resource.x).to eq "102"
+          it "when x is retrieved, it is coerced and fails validation" 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
@@ -1057,7 +1030,7 @@ describe "Chef::Resource.property" do
           include Chef::Mixin::Properties
           property_type(is: [:a, :b], default: :c)
         end
-      end.to raise_error(Chef::Exceptions::DeprecatedFeatureError, /Default value :c is invalid for property <property type>./)
+      end.to raise_error(Chef::Exceptions::ValidationFailed)
       expect do
         module ::PropertySpecPropertyTypes
           include Chef::Mixin::Properties