Expand parameter validation to type DateTime

[dblock]

Dates may be posted as Integer, ISO strings, Date, DateTime, etc.

[d4rky-pl]

+1

[pankajbatra]

+1

[vdaluz]

+1

[dcortes22]

+1

[pragmaticivan]

Can you give more details? +1

[u2]

More details?

[ngouy]

up

[samotarnik]

@dblock
I've been playing with this for the past couple of days. From what I understand, there are two separate issues here, namely coercion and validation.

Coercion is currently done via Virtus which brings along some interesting consequences. For example

Virtus::Attribute.build(DateTime).coerce(1234) #=> #<DateTime: 1970-01-01T00:20:34+00:00 ((2440588j,1234s,0n),+0s,2299161j)> 
Virtus::Attribute.build(Date).coerce(1234) #=> 1234
Virtus::Attribute.build(Time).coerce(1234) #=> 1234

and

Virtus::Attribute.build(DateTime).coerce('1234') #=> "1234"
Virtus::Attribute.build(Date).coerce('1234') #=> "1234"
Virtus::Attribute.build(Time).coerce('1234') #=> "1234"

There are several options here. One is to augment Grape to handle such cases. Another is to fix Virtus to handle these better. In any case, in my opinion, Grape should somehow be aware that coercion actually failed, i.e. that you demanded a DateTime but got a String instead. I suspect that with dates/datetimes, though, there will always be one more format to cover, so the most appropriate thing would probably be to define a custom parameter type for your custom datetime format and handle the parsing there.

Validations are a different beast. The :include? method (used in the ValuesValidator) is not really appropriate to test continuous types/variables. For example, this works

a = DateTime.new(2015,1,1,0,0,0); b = DateTime.new(2015,2,1,0,0,0); c = DateTime.new(2015,1,10,0,0,0)
(a..b).include?(c) #=> true

but these do not

a = DateTime.new(2015,1,1,0,0,0); b = DateTime.new(2015,1,1,10,0,0); c = DateTime.new(2015,1,1,5,0,0)
(a..b).include?(c) #=> false

a = Time.new(2015,1,1,0,0,0); b = Time.new(2015,2,1,0,0,0); c = Time.new(2015,1,10,0,0,0)
(a..b).include?(c) #=> TypeError

One possible thing to do here would be to use the :between? method implemented in the Comparable.

a = DateTime.new(2015,1,1,0,0,0); b = DateTime.new(2015,2,1,0,0,0); c = DateTime.new(2015,1,10,0,0,0)
c.between?(a,b) #=> true

a = DateTime.new(2015,1,1,0,0,0); b = DateTime.new(2015,1,1,10,0,0); c = DateTime.new(2015,1,1,5,0,0)
c.between?(a,b) #=> true

a = Time.new(2015,1,1,0,0,0); b = Time.new(2015,2,1,0,0,0); c = Time.new(2015,1,10,0,0,0)
c.between?(a,b) #=> true

which is what I did here. That is, I implemented a BetweenValidator, that behaves basically the same as the ValuesValidator but takes an array of (two) endpoints (i.e. interval boundaries) as an argument and uses the mentioned :between? method (i.e. works with any type that implements Comparable). It supports default values, lambdas and also ranges (although I really don't like that and would probably remove it). For instance:

params do
  requires :id, type: Integer, between: 5..9, default: 7
  optional :created_at, type: DateTime, between: [DateTime.new(2014,1,1), DateTime.new(2015,1,1)]
  optional :price, type: Float, between: [0.0, -> { rand(5.5..9.9) }]
end

The implementation is pretty rough at the moment, I mainly wanted to see what you guys think about the proposed DSL. (That is why I haven't updated the documentation and/or created a PR, yet.)

Looking further, a possible way would be to have a single class (that iterates across passed params and validation values) for validations which would get the criteria (different test functions) injected as a dependency. This way, it would be somewhat easy to create additional validators.

Finally, a remark and a question. If I run the entire test suite, the specs pass, but if I try to run particular tests in isolation, some of them fail. Cf.:

bundle exec rspec spec/grape/validations/params_scope_spec.rb

(I'm using Ruby 2.1.5 and RSpec 3.3.0).

And why are validators (and also other things) tested via integration and not unit specs? I would expect a validator spec to set up a validator class with the desired arguments and then expect the validate_param! method (not) to fail instead of going through the actual requests.

Lots to discuss here, I guess.

[dblock]

In any case, in my opinion, Grape should somehow be aware that coercion actually failed.

That seems like a reasonable statement. So probably when parameter validation fails, it would include a coercion error?

Lets look at your between implementation ...

Feel free to PR a fix for the running of a standalone spec and refactoring of said specs as you see better, of course.

[samotarnik]

Right.

So, Virtus::Attribute has a "strict mode" support, where it raises an error if the coercion fails (see here). Perhaps, that should be used in Grape's CoerceValidator class (glancing at it, I'm not sure that is currently the case).

Regarding the validators' specs, my idea of how these should look like is something along the following lines

it 'fails in a particular scenario' do
  attrs = ...
  opts = ...
  required = ...
  scope = ...
  validator = Grape::Validations::ValuesValidator.new(attrs, opts, required, scope)
  expect{ validator.validate! }.to raise_error Grape::Exceptions::Validation
end

By going through the requests, things are slower and not isolated to the point one would, in my opinion, want. And I suspect there might be some problems because of this already (e.g. params_scope_spec.rb)---see below.

Finally, I think there are problems with tests isolation in general. Please, compare

bundle exec rspec

with (at least in my case seed 35635 raises two errors)

bundle exec rspec --order rand:35635

but also

find spec/ -name *_spec.rb | xargs -i bundle exec rspec --order rand {}

where I get different errors than in the second case. This should be sorted out first, I would add the --order rand option into the .rspec file and then the specs should be also fixed. Should I add an issue for this---I'm not sure one exists at this point?

[dblock]

Yes, please at least open an issue.