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

Allow validation of nested parameters. #236

Merged

Conversation

tim-vandecasteele
Copy link
Contributor

Based on the feedback of #222, I tried my own way at validation of nested parameters:

Using the group syntax which allows for:

group :user do
  requires :user_name
  group :admin do
    requires :first_admin_name, :last_admin_name
  end
end

In this implementation, I'm passing the scope to the validators, so they can take the relevant parameters.

Passing the scope to the validators, so they can take the relevant parameters.

Using the group syntax which allows for:

group :user do
  group :admin do
    requires :first_name, :last_name
  end
end
@travisbot
Copy link

This pull request passes (merged d021e22 into 9bd4f23).

@bobbytables
Copy link
Contributor

I think adding a 4'th parameter to the Validator class is starting to make it inundated. It might be better to start moving things into an options hash.

One thing I can see in the future is params requirements getting out of hand. Maybe we can build on this and give the ability to give it a class for defining requirements.

Example:

params UserParams

Where UserParams is:

class UserParams < Grape::ParamsScope
  group do
  end
end

@bobbytables
Copy link
Contributor

Other than that, I love the concept of this and your implementation looks pretty good. I'd love to use this.

@kristianmandrup
Copy link

Never use 4 arguments. Usually 2 should be enough, a primary and an options IMO. Also, I recommend not to have more than 4 lines in any method... purist!

@dblock
Copy link
Member

dblock commented Sep 4, 2012

This looks like what doctor ordered, thanks @tim-vandecasteele. I want an update to the README, please and I'll merge it unless there're objections.

@travisbot
Copy link

This pull request passes (merged ec1fe76 into 9bd4f23).

@travisbot
Copy link

This pull request passes (merged 026c26e into 9bd4f23).

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

nice implementation, glad someone found the time for it !
There is a missing piece though, the documentation hash should includes the group, currently it does not:

I used this as reference:

require 'grape'

class TestAPI < Grape::API

  desc "add a user"
  params do
    group :user do
      requires :name, type: String
      requires :type, type: String
    end
  end
  post '/add_user' do

  end

end


p TestAPI.endpoints[0].options[:route_options][:params]

The current output of this script is:

{
  "name"=>{:required=>true, :type=>"String"},
  "type"=>{:required=>true, :type=>"String"}
}

Where the expected output would be more like this for me:

{
  "user" => {
    "name"=>{:required=>true, :type=>"String"},
    "type"=>{:required=>true, :type=>"String"}
  }
}

documentation can properly access these parameters.
@tim-vandecasteele
Copy link
Contributor Author

In fact I hit the same thing when trying to update grape-swagger. For now I adapted this by giving a full_name to a parameter, meaning you would get something like

{
  "name"=>{:required=>true, :type=>"String", :full_name=>"user[name]"},
  "type"=>{:required=>true, :type=>"String", :full_name=>"user[type]"}
}

Although I agree a nested hash would be better (for one to avoid clashes in parameters between different groups).

I'd have to check if I can change the params hash, although I'd think that will have a much bigger impact.

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

oh, that's funny: https://github.com/schmurfy/grape_apidoc/tree/ ;)

@travisbot
Copy link

This pull request passes (merged 3715500 into 9bd4f23).

@dblock
Copy link
Member

dblock commented Sep 5, 2012

I'm merging this, thank you. It also wraps up the parameter block implementation sufficiently IMO to make a release soon. Anybody things we're still missing something?

dblock added a commit that referenced this pull request Sep 5, 2012
…meters

Allow validation of nested parameters.
@dblock dblock merged commit 7e375f0 into ruby-grape:master Sep 5, 2012
@dblock
Copy link
Member

dblock commented Sep 5, 2012

... maybe whitelisting discussed in #219 ...

@dpsk
Copy link
Contributor

dpsk commented Sep 5, 2012

Thanks a lot for this feature!

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

I really don't like the user[name] syntax but it is better than nothing I guess.

@dpsk
Copy link
Contributor

dpsk commented Sep 5, 2012

Yes, definitely better, i'm looking for solution like for 2 days and almost start to write it from scratch.
Not really related to this issue question, why i can't use validations in before block?

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

what do you want to do in the before block exactly ?
The validations implemented do not need any action from the user, just declare the rules and they will be enforced.

Do you want some global validations for your api ?

@dpsk
Copy link
Contributor

dpsk commented Sep 5, 2012

I have same parametrs in every action of my api that should be sent, so i want to declare validations for them in one place.
Nested validations from your merge works perfectly when i drop them inside namespace block, but if i drop them outside nothing happen. I'm not sure if that is acceptable to declare validations in the root level of api.

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

global validations for every endpoint in one api class would indeed be interesting to have.

@dpsk
Copy link
Contributor

dpsk commented Sep 5, 2012

Is it a big feature? Maybe you could guide me a bit and i will send a pull request with it?

@dblock
Copy link
Member

dblock commented Sep 5, 2012

+1 @dpsk I have pagination parameters that apply in many API calls, would be silly to have to repeat them everywhere. I think I would want to write something like this:

params do
   include pagination_params
end

This could be as simple as to look for a PaginationParams module and evaluate it in the context of the params block.

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

You can have a look at "lib/validations.rb" if you want to give it a try, it should not be too hard to implement.
How it works is that the api class has a settings class variables holding the parameters for the next defined endpoint, when the endpoint is created this hash like variables is cloned into it and emptied for the next endpoint definition.

My first thought would be to add another global hash holding options for all the endpoints and not only the next one and which would be merged into the endpoint specific one before the endpoint is created.

we could have something like:

global_params do
  requires :user_id, type: Integer
end

params do
  # ...
end

@dblock
Copy link
Member

dblock commented Sep 5, 2012

@schmurfy If you take my pagination example API that won't work. I don't want all my APIs to get this, just those that support pagination. A fully global feature isn't a bad idea either, authentication is a good example where someone just wants a global parameter set.

@schmurfy
Copy link
Contributor

schmurfy commented Sep 5, 2012

I agree this is not a one size fit all, maybe there is room for both ^^
yours is definitely better if you need to reuse validations maybe even between two apis, maybe my example above could even be implemented on top of an include style feature just to simplify what I think is a common use case for it while still allowing more complex behaviors.

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

Successfully merging this pull request may close these issues.

7 participants