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.

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

Raise proper validation errors on invalid parameter types #543

Closed
bwalex opened this issue Dec 29, 2013 · 6 comments
Closed

Raise proper validation errors on invalid parameter types #543

bwalex opened this issue Dec 29, 2013 · 6 comments

Comments

@bwalex
Copy link
Contributor

bwalex commented Dec 29, 2013

I've run into several issues with validations. For half of the things I don't have a real use case, but it should still work.

  1. The following block works both when opts is an array of opts and when it is a single opt (made to work this way in validation for array in params #333). I think there should be a way to specify whether it is actually an array of hashes or a single nested hash.
    params do
      requires :name, type: String, desc: "Foobarness."
      requires :opts do
        requires :opt_a, type: Integer, desc: "Option A"
        optional :opt_b, type: Integer, desc: "Option B"
      end
  1. In the same example, if someone passes in something like the following, it blows up with the stack trace below because it doesn't check that "opts" is actually a Hash.
{
  "name" : "foobar",
  "opts" : "moo"
}
2013-12-29 22:01:30 +0000: Rack app error: #<NoMethodError: undefined method `has_key?' for "":String>
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations/presence.rb:10:in `validate_param!'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:25:in `block in validate!'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:42:in `block (2 levels) in each'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:41:in `each'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:41:in `block in each'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:40:in `each'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:40:in `each'
  1. With the following params block and request, it also blows up, but in a different way, presumably due to the nested parameters:
    params do
      requires :name, type: String, desc: "Foobarness."
      group :tags do
        requires :id, type: Integer, desc: "Tag ID"
        group :foobars do
          optional :fooname, type: String, desc: "Test string"
          optional :fooval, type: String, desc: "Test string"
        end
      end
    end
{
  "name" : "foo",
  "tags" : [
  ]
}
2013-12-29 22:04:01 +0000: Rack app error: #<TypeError: no implicit conversion of Symbol into Integer>
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:136:in `[]'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:136:in `params'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:35:in `initialize'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:22:in `new'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/validations.rb:22:in `validate!'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/endpoint.rb:389:in `block in run'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/endpoint.rb:387:in `each'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/endpoint.rb:387:in `run'
/usr/lib/ruby/gems/2.0.0/gems/grape-0.6.1/lib/grape/endpoint.rb:154:in `block in call!'
@dblock
Copy link
Member

dblock commented Dec 30, 2013

Agreed on (1). Maybe by introducing a type? I should be able to write:

requires :opts, type: Array do
  requires :opt_a, type: Integer, desc: "Option A"
  optional :opt_b, type: Integer, desc: "Option B"
end

For both (2) and (3): these are bugs. The parameter passed is of the wrong type, so you should get an exception that says so.

Leaving this open as-is. It would be a great start if you could make these into Grape tests and opened PRs with the failing ones. Of course, feel free to fix :)

@dblock
Copy link
Member

dblock commented Dec 30, 2013

I've renamed this to "Raise proper validation errors on invalid parameter types", I think it captures the problem better.

@bwalex
Copy link
Contributor Author

bwalex commented Dec 31, 2013

Agreed - it was just intended as a placeholder until I get around to either writing some tests and/or fixing it. I'll get back to you as soon as I make some progress on this.

@bwalex
Copy link
Contributor Author

bwalex commented Dec 31, 2013

I've made a fair bit of progress on this, both on tests and fixing/implementing it. However, I've run into a bit of an issue with how query params are handled.

In particular, if something requires an array block, an empty array should be valid. But with how query parameters work, the empty array doesn't really exist, so grape never sees it on the (GET) request.

The way I've implemented it, which is by adding validations for presence and type on the 'requires', etc with block, this fails the 'presence' validation.

In my opinion that's actually fair enough, since it isn't actually present even as an empty array in the request. It's just a quirk to keep in mind with query parameters and groups/requires with blocks, etc. It should work as expected with body parameters, e.g. json, where an empty array is something perfectly valid.

@dblock
Copy link
Member

dblock commented Jan 1, 2014

I think the presence issue is much smaller, and you can open it once we have a fix for the bigger problem.

bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
… types

 * Also adds a :type option to group/requires/optional with block,
   which can either be Array or Hash. It defaults to Array.

 * Fixes (2) and (3) as mentioned in ruby-grape#543

 * There is a quirk around query parameters: an empty array should
   pass validation if the array itself is required, but with how
   query parameters work, it doesn't. It does work fine with body
   parameters using JSON or similar, which do have a concept of an
   empty array.
bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 1, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 2, 2014
… types

 * Also adds a :type option to group/requires/optional with block,
   which can either be Array or Hash. It defaults to Array.

 * Fixes (2) and (3) as mentioned in ruby-grape#543

 * There is a quirk around query parameters: an empty array should
   pass validation if the array itself is required, but with how
   query parameters work, it doesn't. It does work fine with body
   parameters using JSON or similar, which do have a concept of an
   empty array.
dblock pushed a commit that referenced this issue Jan 2, 2014
 * Also adds a :type option to group/requires/optional with block,
   which can either be Array or Hash. It defaults to Array.

 * Fixes (2) and (3) as mentioned in #543

 * There is a quirk around query parameters: an empty array should
   pass validation if the array itself is required, but with how
   query parameters work, it doesn't. It does work fine with body
   parameters using JSON or similar, which do have a concept of an
   empty array.
bwalex added a commit to bwalex/grape that referenced this issue Jan 2, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 2, 2014
bwalex added a commit to bwalex/grape that referenced this issue Jan 2, 2014
dblock added a commit that referenced this issue Jan 2, 2014
README.md and UPGRADE.md update for #543, #545
@dblock
Copy link
Member

dblock commented Jan 3, 2014

Closing, fixed in 0.7.0.

@dblock dblock closed this as completed Jan 3, 2014
salimane added a commit to salimane/grape that referenced this issue Jan 8, 2014
* upstream/master:
  add posibility to define reusable named params
  Added ability to restrict declared(params) to the local endpoint with include_parent_namespaces: false.
  Fix for ruby-grape#464: gracefully handle invalid version headers
  Rename exception classes to match README.
  Define errors in context/before blocks.
  Updated/renamed UPGRADING.
  README.md and UPGRADE.md update for ruby-grape#543, ruby-grape#545
  Address ruby-grape#543 - raise proper validation errors on array/hash types
  Remove unnecessary test case.
  Rename children --> subclasses to match the name used in the code.
  Fix typo in rescue_from unit test (Communication --> Communications).
  Rescue subclasses in error middleware.
  Fix for travis-ci/travis-ci#1800.
  Added Ruby 2.1 support.
  Lock RSpec version, failing with rspec-expectations 3.x beta.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants