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.

[dslh]

I've bundled up some code that was cut from #1167 into its own project, grape-formats. It provides some tokens that can be used for the type option of parameter declarations, to denote that the parameter is a Date or DateTime that should be parsed using one of the standard ruby parsing functions declared on the class.

There's no gem released yet since it depends on unreleased changes to grape itself, and anyway would be good to get some feedback or hear if there's any interest. It's so small it barely seems worth the time but it could be a good starting point for a collection of various ISO/RFC formats and so on.

Note that at this point you can also just drop the Date or DateTime class in directly as the type parameter and it will work; in that case the class parse method will be used for coercion I believe. This collection is intended for those who need finer control over the date/timestamp formats they permit.

[dblock]

I think it looks great.

[dnesteryuk]

@dblock I believe this one doesn't require any work any more, Date and DateTime implement the parse method, so they can be used as custom types.

class API < Grape::API
  prefix :api
  version 'v1', using: :path
  params do
    requires :datetime, type: DateTime
    requires :date, type: Date
  end
  post '/' do
    declared = declared(params)

    puts declared[:datetime].inspect
    puts declared[:date].inspect
  end
end

Input:

{
  datetime: '3rd Feb 2020 04:05:06 PM',
  date: '2020-10-01'
}

Output:

Mon, 03 Feb 2020 16:05:06 +0000
Thu, 01 Oct 2020

Output in case of invalid values:

datetime is invalid, date is invalid