Fix unknown validator exception when using requires/optional with Entity

[mscrivo]

when that entity specifies keywords that grape interprets as custom validators, ie: is_array, param_type, etc.

There have been a couple issues opened about this in the past with no real resolution: #1527 and #1302

This PR changes it so that it skips looking for a custom validator for those special documentation keywords.

This allows you to do something like:

desc 'Create some entity',
     entity: SomeEntity,
     params: SomeEntity.documentation
params do
  requires :none,
           except: %i[field1 field2],
           using:
             SomeEntity.documentation
end
post do
 ...
end

and SomeEntity can specify documentation like:

      class SomeEntity < Grape::Entity
        expose :id, documentation: { type: Integer, format: 'int64', desc: 'ID' }
        expose :name, documentation: { type: String, desc: 'Name', require: true, param_type: 'body' }
        expose :array_field,
               documentation: {
                 type: String,
                 desc: 'Array Field',
                 require: true,
                 is_array: true,
                 param_type: 'body'
               }
      end

and it won't crash on startup and give you correct documentation and param validation to boot.

Open questions:

1. Is this an exhaustive list of keywords?
2. Is there a better way to maintain/get the list of keywords?
3. Is there a better approach altogether?

[dblock]

Start by writing a test for this? Let's find a better way to implement it, too.

[mscrivo]

Let's find a better way to implement it, too

Sure, but I was hoping for some guidance there since I don't know the codebase as well as others.

Start by writing a test for this?

Will do, but I don't think it makes sense to until we have an acceptable approach

[dblock]

I tend to do TDD for things like this, but in either case I don't know what the right fix is. Hardcoding the list that Grape knows nothing about is probably not. I've rarely used this feature too, but it looks like documentation had to match parameter options by design.

[mscrivo]

I tend to do TDD for things like this, but in either case I don't know what the right fix is. Hardcoding the list that Grape knows nothing about is probably not. I've rarely used this feature too, but it looks like documentation had to match parameter options by design.

There in lies the problem, how do you write a test for this, when the issue is in the integration point between the multiple repos? Seems like the fix needs to be in grape, but proper testing of it would be done in grape-swagger-entity?

I don't know, tbh, I'm confused why grape is even split into so many repos. Seems like it would be a much more robust offering if all combined into one gem with solid integration points. Documentation would be much better too since you wouldn't have to constantly jump between 4 different READMEs to figure out what combination of things you need. But I admit my naivety here, as I really don't know anything about the project's history and thus context for various design decisions.

[dblock]

I tend to do TDD for things like this, but in either case I don't know what the right fix is. Hardcoding the list that Grape knows nothing about is probably not. I've rarely used this feature too, but it looks like documentation had to match parameter options by design.

There in lies the problem, how do you write a test for this, when the issue is in the integration point between the multiple repos? Seems like the fix needs to be in grape, but proper testing of it would be done in grape-swagger-entity?

You can add an integration test that uses a thirdparty gem here: https://github.com/ruby-grape/grape/tree/master/spec/integration

I don't know, tbh, I'm confused why grape is even split into so many repos. Seems like it would be a much more robust offering if all combined into one gem with solid integration points. Documentation would be much better too since you wouldn't have to constantly jump between 4 different READMEs to figure out what combination of things you need. But I admit my naivety here, as I really don't know anything about the project's history and thus context for various design decisions.

Cause Grape doesn't want to be Rails! :) but open to more specific suggestions and of course contributions

[mscrivo]

I've made it a tiny bit more explicit as to what's going on in this code, ie.

• That there exists some list of reserved keywords for documentation hashes that is:
  • Not codified in any one place
  • Does not have any validators associated with it

I've modified the existing tests around requires :all and requires :none such that they fail when they reserved words are not excluded from custom validator lookups and they do indeed fail if I remove the list.

[dblock]

Let's add a test that exercises every one of these options? Maybe something like

ParamsScope:: RESERVED_DOCUMENTATION_KEYWORDS.each do |keywords|
   context keyword do
       it ..

[dblock]

Let's make this a constant RESERVED_KEYWORDS. Maybe we should also rename it? These are documentation options so RESERVED_DOCUMENTATION_KEYWORDS? WDYT?

[mscrivo]

Sounds good to me!

[mscrivo]

Let's add a test that exercises every one of these options? Maybe something like

ParamsScope:: RESERVED_DOCUMENTATION_KEYWORDS.each do |keywords|
   context keyword do
       it ..

I see where you're coming from, but my counterpoint is that each keyword is a different type, ie is_array & required are bools, whereas param_type is a string with 3 options and the others are just arbitrary strings, so it'd be a bit awkward to write a generic test going through each keyword with it knowing what it's type should be without creating a map of some sort. But that seems overly complex and I don't think it really increases test coverage either, since if you have a test that covers at least one of the keywords, you've covered them all, since the functionality of skipping is the same for all.

[dblock]

Thanks for helping with this @mscrivo, merged.