Rescue StandardError from explicit values validator procs #1679
Conversation
| return param_array.all? { |param| values.call(param) } if values.is_a? Proc | ||
| begin | ||
| return param_array.all? { |param| values.call(param) } if values.is_a? Proc | ||
| rescue => _e |
There was a problem hiding this comment.
Maybe rescue StandardError => e, even if it's implicit to make it clear?
|
@dblock - Fair point. Yeah, it definitely hides the details of the exception. How would you feel about doing this inside the rescue? warn "Error '#{e}' raised while validating attribute '#{attr_name}'" |
| @@ -43,7 +43,11 @@ def validate_param!(attr_name, params) | |||
| def check_values(param_array) | |||
| values = @values.is_a?(Proc) && @values.arity.zero? ? @values.call : @values | |||
There was a problem hiding this comment.
Why aren't we also rescuing this @values.call here in that case? (and would need a spec if that makes sense)
There was a problem hiding this comment.
I think that an error raised from an arity-zero proc is a different and more serious condition. The output there is expected to be an array of valid values (or invalid values in the case of except_values) and a raised error means that the validator is completely broken. Yes, we could return an empty array and warn in that case, but that doesn't feel like the right thing to do. In the values case it wouldn't ever accept anything; and, worse, in the except_values case it would always accept everything.
The primary reason I want to rescue in the arity-one case is because of the likelihood that unexpected input values (non-numeric string or nil in the example above) will raise errors in simple validation code that doesn't do thorough type-safety checks. But in the arity-zero case, there is no input value so that's not a consideration.
|
Added a warning for the arity-one case only. Let me know if you feel strongly about rescuing in the arity-zero case. Otherwise, this should be ready to merge. |
|
Merged. Thank you. |
Example:
Right now, sending a non-numeric string or a nil value for
numberwill result in a stack trace because neither String nor NilClass have aneven?method. One could address this with an additional type check in the proc, but that would result in a lot of extra code that doesn't really contribute to the overall clarity of the declaration.Instead, we should rescue any StandardError raised from an arity-one values validator proc and treat it as a false result.