-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add scenario to test coercion of grape entity #2036
base: master
Are you sure you want to change the base?
Conversation
This is a first attempt at exposing a regression in Grape 1.3.1 compared with 1.2.4 with respect to specifying subclasses of Grape::Entity as parameters.
Here's an example of a CHANGELOG.md entry: * [#2036](https://github.com/ruby-grape/grape/pull/2036): Add scenario to test coercion of grape entity - [@keithpitty](https://github.com/keithpitty). Generated by 🚫 danger |
The addition of a GrapeEntityCoercer to handle subclasses of Grape::Entity enables grape entity parameters to be specified without resulting in the coercing logic to mark them as invalid.
Note: This is still a work in progress. Whilst I've enabled a basic test case for a |
A grape entity can expose an attribute with a different key. So we need to handle that case as well.
Validation of grape entities now handles an array. However, we still need to ensure that the coerced values are returned.
I wonder whether |
@dblock good question. I guess it prompts me to ask whether coercing an object which is an instance of a subclass of Given you're more familiar with the grape codebase than me, I'd be interested to hear more of your thoughts about this. |
@keithpitty @dblock I would pretend that |
I am saying the same thing. Grape does not depend on grape-entity, but handles grape-entity when it's loaded as needed for rendering. Grape-entity depends on Grape and should be introducing a parser that gets automatically registered when grape-entity is loaded. I think the right thing to do would be to see a dependency on grape-entity removed from specs, entity specs moved into an integration test that loads grape-entity, and ensure both parsing and rendering work. Thoughts? |
cc: @LeFnord for visibility |
Hi … wondering a bit about the approach, for my understanding the usage of Entities for params, should only avoid duplication mean, from the Entity the stuff from the documentation key should be used for params, so I would expect by using it, a kind of copy over from the Entity to the parameter, and let do grape the rest a usual expecting this copy over is implemented in grape-entity |
Grape uses dry-types but is not too opinionated about coercion otherwise - what I am saying is that we're not trying to force everyone to convert a spec (eg. grape-entity) into Grape's way of checking parameters (dry-types), therefore introducing a concept of custom coercion. So while you certainly can copy-over, I don't think you have to. Either approach works. |
This is a first attempt at exposing a regression in Grape 1.3.1 compared with 1.2.4 with respect to specifying subclasses of
Grape::Entity
as parameters.My investigation so far has revealed that there is no provision within
Grape::Validations::Types
to handle subclasses ofGrape::Entity
. Presumably a newGrape::Validations::Types::GrapeEntityCoercer
class needs to be created. This would be instantiated withinGrape::Validations::Types::create_coercer_instance
.