Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rescue StandardError from explicit values validator procs #1679

Merged
merged 5 commits into from
Aug 25, 2017
Merged

Rescue StandardError from explicit values validator procs #1679

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bdbd846
Rescue StandardError from explicit values validator procs
Aug 24, 2017
b88c3c0
Update CHANGELOG.md
jlfaber Aug 24, 2017
36ef7dc
Update CHANGELOG.md
jlfaber Aug 24, 2017
73a28e6
Code Review feedback
Aug 24, 2017
8361901
Code Review feedback
Aug 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

* [#1652](/~https://github.com/ruby-grape/grape/pull/1652): Fix missing backtrace that was not being bubbled up to the `error_formatter` - [@dcsg](/~https://github.com/dcsg).
* [#1661](/~https://github.com/ruby-grape/grape/pull/1661): Handle deeply-nested dependencies correctly - [@rnubel](/~https://github.com/rnubel), [@jnardone](/~https://github.com/jnardone).
* [#1679](/~https://github.com/ruby-grape/grape/pull/1679): Treat StandardError from explicit values validator proc as false - [@jlfaber](/~https://github.com/jlfaber).

### 1.0.0 (7/3/2017)

Expand Down
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,7 +1179,8 @@ end

Alternatively, a Proc with arity one (i.e. taking one argument) can be used to explicitly validate
each parameter value. In that case, the Proc is expected to return a truthy value if the parameter
value is valid.
value is valid. The parameter will be considered invalid if the Proc returns a falsy value or if it
raises a StandardError.

```ruby
params do
Expand Down
6 changes: 5 additions & 1 deletion lib/grape/validations/validators/values.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@dblock dblock Aug 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we also rescuing this @values.call here in that case? (and would need a spec if that makes sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true if values.nil?
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rescue StandardError => e, even if it's implicit to make it clear?

return false
end
param_array.all? { |param| values.include?(param) }
end

Expand Down
25 changes: 25 additions & 0 deletions spec/grape/validations/validators/values_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ class API < Grape::API
{ type: params[:type] }
end

params do
requires :number, type: Integer, values: ->(v) { v > 0 }
end
get '/lambda_int_val' do
{ number: params[:number] }
end

params do
requires :type, values: -> { [] }
end
Expand Down Expand Up @@ -357,6 +364,24 @@ def app
expect(last_response.body).to eq({ error: 'type does not have a valid value' }.to_json)
end

it 'does not allow non-numeric string value for int value using lambda' do
get('/lambda_int_val', number: 'foo')
expect(last_response.status).to eq 400
expect(last_response.body).to eq({ error: 'number is invalid, number does not have a valid value' }.to_json)
end

it 'does not allow nil for int value using lambda' do
get('/lambda_int_val', number: nil)
expect(last_response.status).to eq 400
expect(last_response.body).to eq({ error: 'number does not have a valid value' }.to_json)
end

it 'allows numeric string for int value using lambda' do
get('/lambda_int_val', number: '3')
expect(last_response.status).to eq 200
expect(last_response.body).to eq({ number: 3 }.to_json)
end

it 'allows value using lambda' do
get('/lambda_val', type: 'valid-type1')
expect(last_response.status).to eq 200
Expand Down