diff options
author | John Keiser <john@johnkeiser.com> | 2015-12-18 14:48:45 -0800 |
---|---|---|
committer | John Keiser <john@johnkeiser.com> | 2016-01-27 10:23:20 -0800 |
commit | 7520f3d36d2c8029c6c2996dd9289e9f74b9e6d3 (patch) | |
tree | 8e9aa1e623d3c92794dfc502be7350e8aa760e92 /lib/chef | |
parent | c3f1021fc6107808826461279cfd6eb055aa6162 (diff) | |
download | chef-7520f3d36d2c8029c6c2996dd9289e9f74b9e6d3.tar.gz |
Fix nil with properties:
1. Warn when default values are invalid.
2. Never validate nil (on set or get) if there is no default.
3. Emit "will be invalid in Chef 13" warning when setting an invalid nil value.
Diffstat (limited to 'lib/chef')
-rw-r--r-- | lib/chef/exceptions.rb | 1 | ||||
-rw-r--r-- | lib/chef/mixin/params_validate.rb | 12 | ||||
-rw-r--r-- | lib/chef/property.rb | 129 | ||||
-rw-r--r-- | lib/chef/resource/chef_gem.rb | 4 | ||||
-rw-r--r-- | lib/chef/resource/file.rb | 2 |
5 files changed, 110 insertions, 38 deletions
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb index f9a717471c..08d916064f 100644 --- a/lib/chef/exceptions.rb +++ b/lib/chef/exceptions.rb @@ -71,6 +71,7 @@ class Chef class RoleNotFound < RuntimeError; end class DuplicateRole < RuntimeError; end class ValidationFailed < ArgumentError; end + class CannotValidateStaticallyError < ArgumentError; end class InvalidPrivateKey < ArgumentError; end class MissingKeyAttribute < ArgumentError; end class KeyCommandInputError < ArgumentError; end diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb index 841cf9e6a2..3585def853 100644 --- a/lib/chef/mixin/params_validate.rb +++ b/lib/chef/mixin/params_validate.rb @@ -22,7 +22,6 @@ require "chef/delayed_evaluator" class Chef module Mixin module ParamsValidate - # Takes a hash of options, along with a map to validate them. Returns the original # options hash, plus any changes that might have been made (through things like setting # default values in the validation map) @@ -333,6 +332,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 opts[key] = self.instance_variable_get(:"@name") end end @@ -403,6 +403,7 @@ class Chef to_be.each do |tb| case tb when Proc + raise CannotValidateStaticallyError, "is: proc { } must be evaluated once for each resource" if self == Chef::Mixin::ParamsValidate return true if instance_exec(value, &tb) when Property validate(opts, { key => tb.validation_options }) @@ -436,12 +437,21 @@ 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 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 opts[key.to_sym] = instance_exec(opts[key], &coercer) end end + # We allow Chef::Mixin::ParamsValidate.validate(), but we will raise an + # error if you try to do anything requiring there to be an actual resource. + # This way, you can statically validate things if you have constant validation + # (which is the norm). + extend self + + # Used by #set_or_return to avoid emitting a deprecation warning for # "value nil" and to keep default stickiness working exactly the same # @api private diff --git a/lib/chef/property.rb b/lib/chef/property.rb index 8b5c6560a9..437ee10147 100644 --- a/lib/chef/property.rb +++ b/lib/chef/property.rb @@ -87,16 +87,21 @@ class Chef # is fully initialized. # def initialize(**options) - options.each { |k,v| options[k.to_sym] = v if k.is_a?(String) } + options.each { |k,v| options[k.to_sym] = v; options.delete(k) if k.is_a?(String) } + @options = options + options[:name] = options[:name].to_sym if options[:name] + options[:instance_variable_name] = options[:instance_variable_name].to_sym if options[:instance_variable_name] + # Replace name_attribute with name_property if options.has_key?(:name_attribute) # If we have both name_attribute and name_property and they differ, raise an error if options.has_key?(:name_property) - raise ArgumentError, "Cannot specify both name_property and name_attribute together on property #{options[:name]}#{options[:declared_in] ? " of resource #{options[:declared_in].resource_name}" : ""}." + raise ArgumentError, "Cannot specify both name_property and name_attribute together on property #{self}." end # replace name_property with name_attribute in place options = Hash[options.map { |k,v| k == :name_attribute ? [ :name_property, v ] : [ k,v ] }] + @options = options end # Only pick the first of :default, :name_property and :name_attribute if @@ -109,17 +114,22 @@ class Chef options.delete(:name_property) preferred_default = :default end - Chef.log_deprecation("Cannot specify both default and name_property together on property #{options[:name]}#{options[:declared_in] ? " of resource #{options[:declared_in].resource_name}" : ""}. Only one (#{preferred_default}) will be obeyed. In Chef 13, this will become an error.") + Chef.log_deprecation("Cannot specify both default and name_property together on property #{self}. Only one (#{preferred_default}) will be obeyed. In Chef 13, this will become an error. Please remove one or the other from the property.") end - @options = options - - options[:name] = options[:name].to_sym if options[:name] - options[:instance_variable_name] = options[:instance_variable_name].to_sym if options[:instance_variable_name] + # Validate the default early, so the user gets a good error message, and + # cache it so we don't do it again if so + begin + # If we can validate it all the way to output, do it. + @stored_default = input_to_stored_value(nil, default, is_default: true) + rescue Chef::Exceptions::CannotValidateStaticallyError + # If the validation is not static (i.e. has procs), we will have to + # coerce and validate the default each time we run + end end def to_s - name + "#{name}#{declared_in ? " of resource #{declared_in.resource_name}" : ""}" end # @@ -253,14 +263,26 @@ class Chef return get(resource) end - if value.nil? && !explicitly_accepts_nil?(resource) + if value.nil? # 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.") + + # 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.log_deprecation("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.log_deprecation("nil is an invalid value for #{self}. In Chef 13, this warning will change to an error. Error: #{$!}") end + result else # Anything else, such as myprop(value) is a set @@ -291,16 +313,14 @@ class Chef # this property, or if the value is required and not set. # def get(resource) + # If it's set, return it (and evaluate any lazy values) if is_set?(resource) value = get_value(resource) - if value.is_a?(DelayedEvaluator) - value = exec_in_resource(resource, value) - value = coerce(resource, value) - validate(resource, value) - end - value + value = stored_value_to_output(resource, value) else + # We are getting the default value. + # If the user does something like this: # # ``` @@ -326,14 +346,14 @@ class Chef end if has_default? - value = default - if value.is_a?(DelayedEvaluator) - value = exec_in_resource(resource, value) + # If we were able to cache the stored_default, grab it. + if defined?(@stored_default) + value = @stored_default + else + # Otherwise, we have to validate it now. + value = input_to_stored_value(resource, default, is_default: true) end - - value = coerce(resource, value) - - # We don't validate defaults + value = stored_value_to_output(resource, value, is_default: true) # If the value is mutable (non-frozen), we set it on the instance # so that people can mutate it. (All constant default values are @@ -366,11 +386,7 @@ class Chef # this property. # def set(resource, value) - unless value.is_a?(DelayedEvaluator) - value = coerce(resource, value) - validate(resource, value) - end - set_value(resource, value) + set_value(resource, input_to_stored_value(resource, value)) end # @@ -423,7 +439,10 @@ class Chef # def coerce(resource, value) if options.has_key?(:coerce) - value = exec_in_resource(resource, options[:coerce], value) + # If we have no default value, `nil` is never coerced or validated + unless !has_default? && value.nil? + value = exec_in_resource(resource, options[:coerce], value) + end end value end @@ -442,7 +461,10 @@ class Chef # this property. # def validate(resource, value) - resource.validate({ name => value }, { name => validation_options }) + # If we have no default value, `nil` is never coerced or validated + unless value.nil? && !has_default? + (resource || Chef::Mixin::ParamsValidate).validate({ name => value }, { name => validation_options }) + end end # @@ -595,14 +617,53 @@ class Chef value = resource.instance_exec(*args, &proc) end else - value = proc.call + # If we don't have a resource yet, we can't exec in resource! + raise Chef::Exceptions::CannotValidateStaticallyError, "Cannot validate or coerce without a resource" + end + end + + def input_to_stored_value(resource, value, is_default: false) + unless value.is_a?(DelayedEvaluator) + value = coerce_and_validate(resource, value, is_default: is_default) end + value + end + def stored_value_to_output(resource, value, is_default: false) + # Crack open lazy values before giving the result to the user if value.is_a?(DelayedEvaluator) - value = coerce(resource, value) - validate(resource, value) + value = exec_in_resource(resource, value) + value = coerce_and_validate(resource, value, is_default: is_default) end value end + + # Coerces and validates the value. If the value is a default, it will warn + # the user that invalid defaults are bad mmkay, and return it as if it were + # 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.log_deprecation("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.log_deprecation("Default value #{value.inspect} is invalid for property #{self}. In Chef 13 this will become an error: #{$!}.") + end + else + raise + end + end + + result + end end end diff --git a/lib/chef/resource/chef_gem.rb b/lib/chef/resource/chef_gem.rb index 233ae66026..655534684c 100644 --- a/lib/chef/resource/chef_gem.rb +++ b/lib/chef/resource/chef_gem.rb @@ -26,9 +26,9 @@ class Chef property :gem_binary, default: "#{RbConfig::CONFIG['bindir']}/gem", callbacks: { - "The chef_gem resource is restricted to the current gem environment, use gem_package to install to other environments." => proc { false } + "The chef_gem resource is restricted to the current gem environment, use gem_package to install to other environments." => proc { |v| v == properties[:gem_binary].default } } - property :compile_time, [ true, false ], default: lazy { Chef::Config[:chef_gem_compile_time] }, desired_state: false + property :compile_time, [ true, false, nil ], default: lazy { Chef::Config[:chef_gem_compile_time] }, desired_state: false def after_created # Chef::Resource.run_action: Caveat: this skips Chef::Runner.run_action, where notifications are handled diff --git a/lib/chef/resource/file.rb b/lib/chef/resource/file.rb index 0a530e27c9..52cb409226 100644 --- a/lib/chef/resource/file.rb +++ b/lib/chef/resource/file.rb @@ -49,7 +49,7 @@ class Chef allowed_actions :create, :delete, :touch, :create_if_missing property :path, String, name_property: true, identity: true - property :atomic_update, [ true, false ], desired_state: false, default: Chef::Config[:file_atomic_update] + property :atomic_update, [ true, false ], desired_state: false, default: lazy { Chef::Config[:file_atomic_update] } property :backup, [ Integer, false ], desired_state: false, default: 5 property :checksum, [ /^[a-zA-Z0-9]{64}$/, nil ] property :content, [ String, nil ], desired_state: false |