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

added undeclared params validator #810 #1158

Closed
wants to merge 6 commits into from

Conversation

hobofan
Copy link

@hobofan hobofan commented Sep 18, 2015

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:

@dblock
Copy link
Member

dblock commented Sep 19, 2015

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
Copy link
Member

dblock commented Sep 19, 2015

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

@hobofan
Copy link
Author

hobofan commented Sep 19, 2015

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
Copy link
Author

hobofan commented Sep 22, 2015

@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
Copy link
Member

dblock commented Sep 22, 2015

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
Copy link
Author

hobofan commented Sep 23, 2015

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 hobofan changed the title [WIP] added declared_only validator #810 added declared_only validator #810 Sep 23, 2015
@hobofan hobofan changed the title added declared_only validator #810 added undeclared params validator #810 Sep 23, 2015
@hobofan
Copy link
Author

hobofan commented Sep 23, 2015

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

return k, convert_hash.call(v).to_h
end
end
end
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 refactored out of here.

@dblock
Copy link
Member

dblock commented Sep 24, 2015

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
Copy link
Author

hobofan commented Sep 24, 2015

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
Copy link

wrtsprt commented Jan 7, 2016

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
Copy link
Author

hobofan commented Jan 7, 2016

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
Copy link
Member

dblock commented Jan 27, 2016

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
Copy link
Author

hobofan commented Jan 16, 2019

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

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.

3 participants