Fix ValuesValidator and simplify code #1567
Conversation
jlfaber
commented
Jan 31, 2017
jlfaber
commented
- Reject any input when value or proc output is empty array
- Properly apply except to array inputs
|
This PR incorporates the failing test from #1566 (Thanks @faucct!) but addresses two additional issues. #1566 was focused on the case where the values Proc returned an empty array. As noted there, that should cause any input to be rejected. I noted that the static equivalent of that case (when values: is statically set to an empty array) should behave in the same way. As I was reviewing the code, I also noticed that the except: implementation seemed to have a bug: when validating an input array, the validator only rejected the input if ALL of the elements of the array were exceptional. Instead, it should reject the input if ANY of the elements are exceptional. @faucct, would you mind reviewing this fix to make sure it addresses your concerns? I also notice that travis-ci failed both this PR and #1566. I suspect that failure may not be related to these changes since it only affects a single configuration (Ruby 2.4.0 + rails_edge.gemfil). |
|
It sure does. I am closing my PR. |
* Reject any input when value or proc output is empty array * Properly apply except to array inputs
jlfaber
force-pushed
the
fix_values_validator
branch
from
e422259 to
0e6f94e
Compare
| if !param_array.empty? && param_array.all? { |param| excepts.include?(param) } | ||
| raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: except_message | ||
| end | ||
| raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: except_message \ |
There was a problem hiding this comment.
You should make it a guard clause and this way you don't have to put it on multiple lines, so:
return if excepts.nil? || params_array.none? ...
raise ...There was a problem hiding this comment.
I could do that for the values check (the second one) but not for the excepts check (the first one) since I don't want a missing excepts to skip the values check. But, honestly, I kind of like the symmetry of the two checks -- I think it makes them a little quicker to read and understand. Your call, though. If you want me to switch the second one to a return unless I'll be happy to do that. Please advise.
10 similar comments
|
I tried rerunning the travis-ci build by force pushing a new commit that was identical to the original one. This one passed successfully. Which is, of course, concerning. The two builds are: https://travis-ci.org/ruby-grape/grape/builds/197028860 (failed) https://travis-ci.org/ruby-grape/grape/builds/197053271 (passed) To reiterate, the source they built from was identical. @dblock any ideas what might be going on here? |
|
@jlfaber Anything using delegators via forwardable.rb is potentially broken, see https://bugs.ruby-lang.org/issues/13107. Fix will come in Ruby 2.4.1. |
|
Merged, thank you. |
|
You're welcome. Thanks again @faucct for the initial find and spec. |
