From b5f8bf30da24d39e29323f95b444577cf08a3d6f Mon Sep 17 00:00:00 2001 From: John Keiser Date: Sun, 13 Dec 2015 12:07:33 -0800 Subject: Always allow nil set; only warn if not using "is" --- lib/chef/mixin/params_validate.rb | 2 +- lib/chef/property.rb | 66 +++++++++-------------------------- spec/unit/property/validation_spec.rb | 57 +++++++++++++++++++++--------- spec/unit/property_spec.rb | 20 +++++++---- 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" -- cgit v1.2.1