Fix exception being thrown when dependent parameter is a Hash #1740
Conversation
CHANGELOG.md
Outdated
| @@ -8,6 +8,8 @@ | |||
|
|
|||
| * Your contribution here. | |||
|
|
|||
| * [#1740](https://github.com/ruby-grape/grape/pull/1740): Fix dependent parameter validation using `given` when parameter is a `Hash` - [@jvortmann](https://github.com/jvortmann). | |||
There was a problem hiding this comment.
Nitpick, put "Your contribution here" below the line? Just more consistent with how we've been doing it.
| @@ -466,6 +466,20 @@ def initialize(value) | |||
| end.to raise_error(Grape::Exceptions::UnknownParameter) | |||
| end | |||
|
|
|||
| it 'DOES NOT raise an error if the dependent parameter is a Hash' do | |||
There was a problem hiding this comment.
Lets not shout. So "does not raise error if ..." Thanks :)
There was a problem hiding this comment.
No problem.
The idea was not to shout, but to make it more visible when running using "documentation" format.
| it 'DOES NOT raise an error if the dependent parameter is a Hash' do | ||
| expect do | ||
| subject.params do | ||
| optional :d, type: Integer |
There was a problem hiding this comment.
I am nitpicking, but is this optional :d needed for the spec?
There was a problem hiding this comment.
I did to populate declared_params with something more than a Hash.
Just removed as I believe there are other specs covering thath.
lib/grape/dsl/parameters.rb
Outdated
| private | ||
|
|
||
| def first_hash_key_or_param(parameter) | ||
| parameter.is_a?(Hash) ? parameter.keys[0] : parameter |
There was a problem hiding this comment.
Maybe .keys.first is more idiomatic?
There was a problem hiding this comment.
Yes, it might.
The case is that I've just move the line that were in the given method as is.
Should I change the line then?
When using `given` and the paramter type is a Hash, a Grape::Exceptions::UnknownParameter is thrown. This is a wrong behavior due to the fact that the method `declared_param?(param)` do not deal properly with params that are hashes (even after flattening the declared params). This commit fixes this by checking if the param is a Hash and thus returning its first key. This approach was already used in other part of the code, so it was extracted and reused on `declared_params?(param)`.
|
@dblock just did the changes :) |
|
Merged, thank you. |
|
@dblock nice :) What are the plans to cut a release? Is there a particular time-box for that? |
|
No plans, maintainers cut releases fairly frequently so stay tuned. |
|
I have the same problem, so a gem release with this fix would be pleased |
When using
givenand the paramter type is a Hash,a Grape::Exceptions::UnknownParameter is thrown. This is a wrong behavior
due to the fact that the method
declared_param?(param)do not dealproperly with params that are hashes (even after flattening the
declared params).
This commit fixes this by checking if the param is a Hash and thus
returning its first key. This approach was already used in other part of
the code, so it was extracted and reused on
declared_params?(param).