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

Refactor and extend coercion and type validation #1167

Merged
merged 1 commit into from
Sep 30, 2015

Conversation

dslh
Copy link
Contributor

@dslh dslh commented Sep 28, 2015

Addresses #1164, #690, #689, #693.
Depends on solnic/virtus#343

Grape::ParameterTypes is renamed Grape::Validations::Types
to reflect that it should probably be bundled with an eventual
grape-validations gem. It is expanded to include two new
categories of types, 'special' and 'recognized' (see
'lib/grape/validations/types.rb'). CoerceValidator now makes
use of Virtus::Attribute::value_coerced?, simplifying its
internals.

CustomTypeCoercer is introduced, attempting to standardize
support for custom types by decoupling coercion and type-checking
logic from the type class supplied to
Grape::Dsl::Parameters::requires.

JSON, Array[JSON] and Rack::Multipart::UploadedFile (a.k.a
File) are designated 'special' types, for which special
implementations of Virtus::Attribute are provided.

Instances of Virtus::Attribute built with Virtus::Attribute.build
may now also be passed as the type parameter for requires.
A number of pre-rolled attributes are available providing coercion
for Date and DateTime objects from various formats in
lib/grape/validations/types/date.rb and date_time.rb.

@dslh dslh force-pushed the validation_types branch 2 times, most recently from 3b1a461 to ec9c6d9 Compare September 28, 2015 14:54
@dslh
Copy link
Contributor Author

dslh commented Sep 28, 2015

I've had a go at making better use of virtus to make type validation a bit more flexible. I would say this PR needs few more things done before it should be accepted:

  • The pending PR to virtus needs to be accepted. Pointing virtus at the PR branch in the Gemfile resolves all regression errors. Could probably work around this in the mean time if necessary.
  • Is Grape::Validations::Types a good namespace name?
  • See CustomTypeCoercer#infer_type_check. There's support there for more involved type checking than just value.class == primitive, but I'm unsure what the conventions should be. This should be agreed on and documented under Grape::Dsl::Parameters#requires. Alternatively, arbitrary type-check control can be achieved by supplying a Virtus::Attribute subtype overriding value_coerce?, see lib/grape/validations/types/json.rb and file.rb. This could also perhaps be better documented.
  • The pre-rolled attributes in lib/grape/validations/types/date.rb and date_time.rb are as-yet undocumented. That's because these 'convenience' types must currently be accessed as type: Grape::Validations::Types::Date::Iso8601... I'm unsure of the best way to make these more readily available to ParamsScope.
  • I'm not 100% sure that coercion was tested to the point where I could confidently say no regressions have been introduced around validating nulls, blank strings, and the like. Removing the special type check for Rack::Multipart::UploadedFile for example, did not cause any failures until I added to the spec. This might need to be reviewed.
  • I do not have a good understanding of how some of the more exotic uses of the type parameter interact with the automatic documentation system. If anyone can point me in the right direction I would be happy to revise the code as needed.

I think what's missing now is a coerce_from: :param or %w{array of params} and you have a type system that can be adapted to most requirements.

# one String argument and returning the parsed value in its correct type.
# @param type [Class] type to check
# @return [Boolean] whether or not the type can be used as a custom type
def self.custom_type?(type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be just custom? to match the other names maybe. Not feeling strongly. Maybe some other methods should be private and this would not.

@dblock
Copy link
Member

dblock commented Sep 29, 2015

I really like this code and I think it's important to get some or all of it in. I would vote to even monkey-patch Virtus in Grape until there's a version out there with the PR, or implementing a work-around in Grape that explicitly does what that PR does. Either way, bring this PR to a green state and I'll merge it trusting our specs not to introduce any regressions.

@dslh dslh force-pushed the validation_types branch 2 times, most recently from 07fce88 to 6f55a61 Compare September 30, 2015 14:21
Addresses ruby-grape#1164, ruby-grape#690, ruby-grape#689, ruby-grape#693.

`Grape::ParameterTypes` is renamed `Grape::Validations::Types`
to reflect that it should probably be bundled with an eventual
`grape-validations` gem. It is expanded to include two new
categories of types, 'special' and 'recognized' (see
'lib/grape/validations/types.rb'). `CoerceValidator` now makes
use of `Virtus::Attribute::value_coerced?`, simplifying its
internals.

`CustomTypeCoercer` is introduced, attempting to standardize
support for custom types by decoupling coercion and type-checking
logic from the `type` class supplied to
`Grape::Dsl::Parameters::requires`. The process for inferring
which logic to use for each type and coercion method is encoded
in `lib/grape/validations/types/build_coercer.rb`.

`JSON`, `Array[JSON]` and `Rack::Multipart::UploadedFile (a.k.a
`File`) are designated 'special' types, for which special
implementations of `Virtus::Attribute` are provided.

Instances of `Virtus::Attribute` built with `Virtus::Attribute.build`
may now also be passed as the `type` parameter for `requires`.

Depends on a monkey patch to `Virtus::Attribute::Collection`, included
in `lib/grape/validations/types/virtus_collection_patch.rb`. See pull
request 343 on solnic/virtus for more details.
@dslh
Copy link
Contributor Author

dslh commented Sep 30, 2015

Tidied up a bit:

  • lib/grape/validations/types/virtus_collection_patch.rb duplicates the virtus PR, works with both fixed and unfixed virtus versions so we can remove it after a fixed virtus gem is published.
  • Logic from CoerceValidator#converter moved to module function Types.build_coercer. Not crazy about the name, coercer_for maybe?
  • I've removed Types::Date and Types::DateTime, the specs seem to run foul of bugs in certain ruby versions and it's not really core grape stuff anyway. If there's any interest I could put together a 'grape-formats' gem with these and a few other more or less common formats.

Most of the inference that CustomTypeCoercer does on the type parameter still isn't documented in the README. I don't have any strong opinion as to how it should work, maybe leaving it buried will encourage proposals.

@@ -5,6 +5,7 @@

* Your contribution here.

* [#1167](https://github.com/ruby-grape/grape/pull/1167): Refactor and extend coercion and type validation system - [@dslh](https://github.com/dslh).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've added type: File, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, doc'd and spec'd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should be added to changelog as a line, like that we support File now out of the box.

@dblock
Copy link
Member

dblock commented Sep 30, 2015

I'm merging this, great work. Lets iterate on top of it.

dblock added a commit that referenced this pull request Sep 30, 2015
Refactor and extend coercion and type validation
@dblock dblock merged commit 03d7236 into ruby-grape:master Sep 30, 2015
@dslh dslh deleted the validation_types branch September 30, 2015 15:32
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.

2 participants