validation / coercion support

[schmurfy]

Hi,
I implemented this for my personal need:

require 'rubygems'
require 'bundler/setup'

require 'grape'

class TestAPI < Grape::API
  default_format :json

  requires :name, :company
  optional :a_number, :regexp => /^[0-9]+$/
  get do
    "Hello #{params[:name]}"
  end

  optional :arr, :coerce => Array[Integer]
  get '/coerce' do
    params[:arr].inject(&:+)
  end

end

use Rack::CommonLogger
run TestAPI

Which can be used like this:

curl -d '{"arr" : ["1", "56"]}' -X GET 127.0.0.1:3000/
57

The only dependency added is virtus for the coercion, the validation is done with custom classes since there is no lightweight validations library out there.

Are you interested in merging this in the core ?

That is one thing I always miss the rare times I want to play with rails but for an api server I consider this a must have (but maybe I am the only one ;) )

I have a fully working code with tests but I am not completely happy with how I integrated it.

[ppadron]

I suppose these are the changes, right?
schmurfy@4800c93

I think it would be a great to have support for validations, but why not doing it based on the :params Hash passed to the desc method? It is already the standard way of describing an endpoint [1], and this way we would not have two separate places to define parameters.

Some "pseudocode":

class TestAPI < Grape::API

  # enable/disable validation support
  validation true

  desc "Does something", :params => {
    :name     => { :desc => "Your name", :required => true },
    :numbers => { :desc => "Numbers", :required => true, :coerce => Array[Integer] }
  }
  get do
    ...
  end

end

For custom validators, they could be added just like helpers:

validators do
  def my_custom_validator
    ...
  end
end

What do you think?

[1] - Interesting example on generating API docs/reference: http://code.dblock.org/grape-describing-and-documenting-an-api

[schmurfy]

Yes these are my changes, to be honest I completely missed the desc function ;)
The README for the grape library is a really huge and hard to swallow and I skipped a large part and headed right into the source code to get my answers (I was wondering if there was any way to add route specific "options").

The syntax you propose makes sense except I am not a big fan of nested hashes, not the best thing when it comes to readability but I like the idea of self documented API though. The "desc" name might be misleading in this case too since we are doing more than just documenting

What the "validation true" directive would do in your example ?

Here is another option:

class TestAPI < Grape::API

  desc "Does something"  
  params do
    required :name, :desc=> "Tour name"
    required :numbers, :desc => "Numbers", :coerce => Array[Integer]
    optional :age,  :desc => "Your Age"
  end

  get do
    ...
  end

end

This may be a taste issue but I find this more readable than the hash form.
In this example the params block and its content could add informations into the @last_description variable filled by desc.

For adding validators having a way to add user's one easily would be nice yes :)
I focused on the core functionalities to get something out and start a discussion on it but I don't consider it a finished work.

[schmurfy]

I did some testing with the auto documenting api idea but with https://github.com/wordnik/swagger-ui (untouched) and I managed to have a basic api working and documented.

I did some changes to have requires/optional methods update the @last_description hash with the data they have (btw the whole desc hash really feels like a quick & dirty hack or a last minute addition to the library, is it ?).

There are some things missing but here is what I have: https://dl.dropbox.com/u/1313066/github/grape_api_doc.png ,
I have seen the api-sandbox project but decided for swagger after trying both.
Here is another screen with a request/response: https://dl.dropbox.com/u/1313066/github/grape_api_doc2.png

I did it by writing a rack application serving both the ui and the json descriptions expected by it.

[dblock]

The grape community would love something in or out of grape (a gem?) that gets users from zero to a documented API one can explore in zero lines of code!

[schmurfy]

I will create a separate gem for the self documenting aspect since I plan to use it anyway but first things first !
I turned this issue in a pull request adding my branch to either merge this as is or discuss how it could be merged.

I did some changes to integrate with the desc method, as an example instead of writing this :

class TestAPI < Grape::API  
  desc "say hello", :params => {
    :name     => { :desc => "Your name", :type => "string" },
    :a_number => { :desc => "A number", :type => "integer" },
  }

  requires :name, :coerce => String
  optional :a_number, :coerce => Integer
  get do
    {:message => "Hello #{params[:name]}"}
  end

end

You can now write this:

class TestAPI < Grape::API  
  desc "say hello"
  requires :name, :desc => "Your name", :coerce => String
  optional :a_number, :desc => "A number", :coerce => Integer

  get do
    {:message => "Hello #{params[:name]}"}
  end

end

which will define the validation, the coercion and document the parameters in the same call, the relevant information will be added into @last_description[:params] if the hash exits (if desc was called just before).

[ppadron]

@schmurfy in the example I wrote, the validation true would signal that the parameters described for the endpoints should be validated. If disabled, validation would be skipped and the parameter descriptions would be used only for documentation purposes.

I agree with you that (deeply) nested hashes can be not-so-easy to read, but IMHO it would be better if parameters could be defined/described in a single place, instead of having calls to requires and optional inside the API context and stacking them into @last_description. Then again, this is just my opinion, and I'd be thankful to have this implemented in Grape regardless of how the rules are defined. Thanks!

[schmurfy]

My problem is making clear that your are not only describing your interface but actually adding constraints on the parameters which will be enforced, that's my main reason for keeping requires/optional separated from "desc".

When I see this:

  desc "say hello", :params => {
    :name     => { :desc => "Your name", :type => "string" },
    :a_number => { :desc => "A number", :type => "integer" },
  }

This is not obvious for me that this will have a direct effect on the parameters. I am not against another syntax I just want it to be clear to understand at first sight (and without nested hashes ;) ).

It would be nice to have other inputs on this matter.

[dblock]

I think you are onto something very nice here ;) The desc blocks are free-formed, which gives flexibility, but also makes it a bit harder to keep consistent. I think that writing something like this would be a natural extension of what we have now:

desc "Reticulates splines." do
  params do
     required :id, type: Integer, description: "Spline id."
     optional :direction, type: String, description: "Direction to reticulate."
     optional :vertices, type: Array[Integer], description: "Optional vertices."
  end
end

This would force coercion for parameters that have a type and required/optional validation for those marked required.

Making desc a block would give us a completely new path to implement this, and would keep the old syntax available for those that want it.

[schmurfy]

@dblock And what do you think of this:

desc "Reticulates splines.", :notes => "It may eventually work if you are lucky."
params do
   required :id, type: Integer, description: "Spline id."
   optional :direction, type: String, description: "Direction to reticulate."
   optional :vertices, type: Array[Integer], description: "Optional vertices."
end

I really don't like having validations inside a block (or hash) calling itself "description", it does not really shows what will be the result of what you wrote. For me a description is just that and won't have any effect at runtime which, in this case, is misleading.

Another thing I would like to add is deep validation support as I did in goliath which would look like this:

  required "user.id", type: Integer

Grape already does a really good job at parsing json and this would allow validation anything as needed, I already got usecases for that for a project currently using Goliath.

PS: (the "notes" option can be used by the swagger-ui interface, I should have a first version of a gem integrating it with grape ready in the next few days)

[dblock]

@schmurfy, Je pense que c'est tres bien! Go for it.

[schmurfy]

@dblock :)
Code updated to use params {} construct and I reworked the internals to clean things up a little.

I also added a small text about validations in the big readme.

@ppadron I did some test with your idea of allowing helper style validators but having a proper class hierarchy fits them better and adding new validators is now really easy:

module Grape
  module Validations

    class LetterAValidator < SingleOptionValidator
      def validate_param!(attr_name, params)
        if params[attr_name] != "a"
          throw :error, :status => 400, :message => "#{attr_name} must be 'a' !"
        end
      end
    end

  end
end

Which can then be used like the others:

params do
  requires :index, :letter => "a"

end
get do

end

I checked again to see if I could find a general purpose validations library I could reuse instead of recoding yet another one but I found none which are lightweight and fits the needs (while I really like Virtus I am not really a big fan of Aequitas in its current form).
If anyone has a candidate I am all ears.

[schmurfy]

I uploaded an early release of my api viewer experiment: https://github.com/schmurfy/grape_apidoc , for now it depends on my branch.

[dblock]

I think this is really good. I'll leave this open for a while to get some comments, I'd especially like @mbleigh to do a read through this.

I think that we should decide that users should be declaring params this way, and update the README accordingly, deleting any reference to the way you would declare an optional params value in the desc hash. Otherwise it's confusing for a novice which way to go. This is definitely a very DSL-y way which is what Grape is all about.

[dblock]

I think you want to create a validators folder and put all these validators into separate files.

Update: saw you did this further. I believe the SingleOptionValidator should move out too.

[schmurfy]

I kept SingleOptionValidator there to clearly separate the foundation classes and the real validators but I can move it too if you prefer.

[dblock]

I don't feel strongly about it. Keep it.

[dblock]

I would even drop Virtus, it's like saying that JSON is created with multi_json, who cares, really? :) I would write something like this:

Declaring a parameter type causes the value to be coerced into that type, where possible. No validation will occur. Invalid input, such as "foo" for an Integer, will be left unmodified and passed through into the API.

[schmurfy]

I love Virtus and I want to share ;)
Seriously that's fine with me, I based my new version on what you propose and drop any mention of Virtus.

I added a note about a way to do the validation if wanted.
I also added another note saying the behavior might change since I hope we can make the validation automatic when declaring a coercion in the future.

[schmurfy]

I really did not like the non validation part of the coercion (which kind of kills the whole purpose) so I did some more tests and managed to add an automatic validation which should hopefully work for anything virtus can coerce.

[dblock]

You can do this without a begin? I learned something :)

[schmurfy]

Yes I was also glad when I found that, nice shortcut.

[dblock]

Move the definition of CoerceAPI into here as a before block just like in the presence_spec.rb. These classes pollute the global scope and have unpredictable results. Or namespace it the same way as CoerceTest. We should pick one way of doing things, right now there're let's and modules and examples and all kinds of other fun stuff, I'll go cleanup master into one of them (probably module-based).

[schmurfy]

I will move the class definition in the CoerseTest module I added, I planned to do that but got distracted.
The way I usually prefer to fo this is what I did in my first version (create a class and bind it to an instance variable) but when I moved the Boolean class declaration I had to change the code otherwise Boolean was not in scope and so would raise a justified error :/

[dblock]

This has been merged, thank you very much.

I had to do a bit of work to make specs pass on Ruby 1.8.7, notably you can't do @app = Class.new(Grape::API) do &block, in Ruby 1.8.7 the contents of the block are not executed in the context of new class declaration. The fix was simply to namespace it as a regular class.

I think we should deprecate the old params syntax properly, next. I am not sure what shape that would take in code.

[schmurfy]

Nice 1.8 bug :/
Too bad this syntax is quite handy to keep tests isolated.

in the current state the params block I added also add the informations into the desc hash as before when done manually so in a way both syntax will result in the same thing (it was the shortest path). One way to deprecate passing a :params hash to desc could be to simply check in the desc method what was passed to it and add a warning accordingly.

There is one question I was asking myself: is there a use case where one would want to document the parameters but without having coercion in place ?
The answer to this question could be a factor to decide on the next step regarding the accepted way of documenting params.

[dblock]

I think the answer to your question is "yes". Syntax-wise this is enabled easily with

required :id, :type => Integer # required, coerced and validated
optional :name # not required, not coerced and not validated

Want to take a stab at this?

Given this there's absolutely no need to preserve the old syntax for params except for backwards compatibility, IMO. We still need to merge the params from the actual HTTP path somehow into this.

[adamgotterer]

What do you guys think about adding a way to control the validation message? Either by capturing in a rescue or giving a handler for each type of validation error?

[dblock]

If it raises a specific exception, I ought to be able to catch it the normal rescue_from way, no?

[adamgotterer]

Looking at the current validation code for presence - https://github.com/schmurfy/grape/blob/908bc2d7eeffc49c36afd7d26d72721ce0f59b65/lib/grape/validations/presence.rb it just calls :error. Doesn't raise anything that you can capture.

[dblock]

So that would be a worthy improvement, raise a real exception here and be able to rescue_from it.

[schmurfy]

I am not familiar with catch/ throw but this is how it was working in other places in the code so I did the same, I have nothing against using exception but I am curious what you want to do with this.
For the params without coercion I was thinking about a use case where one would want to have its API documented (for example with my swagger ui gem) but do not want the coercion. I don't see any reason to do that but who knows maybe someone here have one :)

The code you wrote with an optional without any other option should already work I think (I do not have the code at hand).

[adamgotterer]

@schmurfy the reason for raising an exception is so that you can catch it elsewhere and determine how you want to handle it. For example: currently there's no way to change the response error message. If you could identify when this error is thrown you could alter the message to something else.

Another unrelated thought. I've been playing around with writing custom validation handlers. Is it worth considering having a similar syntax to Entities and using with to specify the custom handler? Right now a custom validator with no options needs to be called as :validator_name => true. Could be with: CustomValidator. Not sure how you'd want to handle the params part, with: { ValidatorClass => { 'param' => 'value' }}?

[schmurfy]

I based ths syntax on the activemodel one because that's the most straightforward I currently found, in your example the tru may be an hash like that:

requires :name, :my_validator => {:option1 => 42, :options2 => "a cat"}

And it also allows chaining validators without having to write a cascade of nested hashs.
I will make a try at adding exception on the validation, just to be sure what you would want is something like this ?
(here ValidationError would be the parent exception for all of them and each validator raises its own)

rescue_from ValidationError do |err|
  if err.is_a?(MissingParameterError)
    # do something
  end
end

[adamgotterer]

Was just a thought, I don't hate the syntax. It is like ActiveModel, so it makes sense.

Rescue syntax looks good to me. Might want to wait for dB or one of the other main contributors to comment.

[dblock]

Rescue syntax looks awesome. But this validator business smells like my original params implementation, too many hashes and not enough DSL. Maybe something like this?

requires :name
validates_presence_of :name
validates :name, :presence => true
validate :name, :with => CustomValidator

Looks familiar, no? :)

[adamgotterer]

How would pass additional params? Maybe anything else passed is considered a validation param?

validate :name, :with => CustomLengthValidator, :max => 10, :min => 3

[dblock]

Yes, that's the ActiveRecord syntax, I would even make sure one can use an ActiveRecord validator out of the box.

Note that they use validates with an s, probably a good idea since we have requireS.

[schmurfy]

Since I started using the new actimodel syntax (validates) I really feel more at home than with all these validate_xxx which quickly become a mess in my models that is why I settled for this one.

For custom validator we could simply have a custom type:

requires :name, :regexp => /.../, :custom => MyValidator, :custom => proc{|...| ...  }

Here there would be a CustomValidator class taking a proc or a class and either calling it with the value or calling a method on that class.

With my current implementation we could also add the validate syntax you propose too:

requires :name
validate :name, :regexp => /.../
validate :name, :with => CustomValidator

That's too verbose for me but if someone wants it I can add it without too much efforts.

(requires automatically inject :presence => true in the validations list)

[dblock]

I'm with you @schmurfy , implement what you think works best.

[adamgotterer]

@schmurfy can you explain the difference between the type and coerce params? Is type primarily for documentation?

[schmurfy]

in the current implementation type is just an alias for coerce to bridge with the previous hash syntax, they do exactly the same thing.

[adamgotterer]

@schmurfy are you working on the exception changes? I'm happy to take a stab at it if you don't have time.

@dblock question for you. It's currently doing a throw :error which turns into a message. In order to capture with rescue_from it will need to turn into an exception. I was thinking we could derive from a base Grape Error Exception that could have a default behavior if theres no rescue set. We would likely be able to roll that out across the framework as well. What do you think?

[schmurfy]

@adamgotterer I lack the time currently, feel free to give it a try

I think you can raise an exception in the validation classes and find a high level place to turn it into a throw, that is why I was thinking of as a first step.

I am not sure but the main difference between throw and raise is that throw does not keep a backtrace and therefore is much faster which mays be why it was chosen here.

[dblock]

Nobody wanted this to be that fast :) I like #221.

[schmurfy]

I would not have chosen exception if I had done it myself but when trying to integrate pull requests into a project as a first step at least it is usually easier to try to act friendly with the existing "features" than trying to change everything at once ^^

I find raise/rescue far more easier to manage and more powerful.
That's great to see that I was not the only one with validation needs too :)