added undeclared params validator #810

[hobofan]

Only allow declared parameters as proposed in #810.

Works like this:

params do
  optional :beer
  optional :wine
  optional :juice
  declared_only
end

Now if there is a parameter present that is not beer, wine or juice a validation exception will be thrown.
Also tested for nested parameters.

It is not quite finished yet:

• I plan on adding a except option to skip the validator for some parameters. Especially in combination with the bug described in Nested mount points don't inherit parent declared params #1050 / namespace param validation doesn't propagate to nested Grape modules #1085 , a param that is accepted on every endpoint would have to either be declared in every params block or will throw an exception with this validator
• Not tested/tried it for Arrays yet.
• Some feedback on the name of declared_only would be nice. I not sure if it fits in well.

[dblock]

I like it. I think that the DSL could be clearer and be on params, maybe turn it around, for example params undeclared: :error do .... I think that if a param is accepted with every endpoint it should be in a helpers ... and reused via use, introducing except will defeat the purpose of this IMO.

I think I would like to be able to turn this globally on for my entire API.

I'd like a way to turn this on gradually, we have 400 endpoints in a project and we would love to make params declared only, so maybe something like params undeclared: :warn?

This will also let us specify a lambda, so params undeclared: ->(params) { }.

[dblock]

Something that's not declared is smuggled? Should undeclared be smuggled? j/k

[hobofan]

Great, I would also prefer to have it on params, but decided to follow the conventions of the existing validators (even though it works somewhat different than the other validators).

[hobofan]

@dblock

I'd like a way to turn this on gradually, we have 400 endpoints in a project and we would love to make params declared only, so maybe something like params undeclared: :warn?

I've put it in there now, but it isn't easily testable and I haven't really found any similar behaviour in grape yet.

[dblock]

I really like this. I think it needs README and CHANGELOG and it would be good to go. Testing via expecting a warning is good AFAIK.

[hobofan]

I had to change a bit more, to get the validation for generic hashes working in an expected way.

E.g.:

params undeclared: :error do
  optional :config, type: Hash
end

If you only specify the type Hash, you probably want to allow any Hash.

In contrast, if you declared you Hash with a block like

params undeclared: :error do
  optional :config, type: Hash do
    requires :language
  end
end

you have a strict expectation of how the Hash should look like.

As for the warning test, I tried my best (even ugly stuff like expect_any_instance_of) but didn't manage to get a correct expectation running.

[hobofan]

When trying to integrate it into our own API I noticed that it failed when a Hash had more than one nested parameter defined.
This is now fixed but requires more involved Hash inclusion checking for the declared params. On the other hand this opens up the possibility to expand given blocks to work with nested blocks like:

params do
  optional :a, type: Hash do
    optional :b
  end
  given :a => :b do
    requires :c
  end
end

[dblock]

Should be refactored out of here.

[dblock]

Maybe there's a simpler implementation here that calculates a hash difference (only caring about keys), I haven't dug into things like https://github.com/liufengyun/hashdiff or others, but it sounds like a good way to abstract the problem, no?

[hobofan]

Yes, having declared_param? accept a path as array of keys might make things simpler. I'll look into it, but might be a week until I have the time.

[wrtsprt]

What is the status on this? Is there anything we can do to help?

This seems to be a quite important feature to me. Especially with being able to turn that on by default ('strict mode') for the whole API.

[hobofan]

I sadly won't have time to work on it in the forseeable future, sorry.

I have no problem if anyone picks up where I left off and submits a new PR.

[dblock]

I am still very interested in this, it's a super useful feature. Was thinking maybe we should have strict: true on params as a more rigid DSL, undeclared: still feels odd to me. Looking for someone to finish this PR!

[hobofan]

Closing this, as I'm currently clearing out old/outdated PRs of mine.