-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
added exactly_one_of validation #637
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
Conversation
For an API project, I needed a validation to make sure exactly one parameter out of a group is chosen, so wrote a slight adaption to the mutually_exclusive params validation. Mutually_exclusive ensures that none of the mutually exclusive params can be there at the same time; exactly_one_of ensures (you guessed it) that exactly one of them will be there. Since I already wrote it, I thought I'd just share it because it might be useful for other people as well. Suggestions for improvements are welcome :) |
@@ -29,3 +29,4 @@ en: | |||
unknown_options: 'unknown options: %{options}' | |||
incompatible_option_values: '%{option1}: %{value1} is incompatible with %{option2}: %{value2}' | |||
mutual_exclusion: 'are mutually exclusive' | |||
exactly_one: "You must select exactly one option." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message is not consistent with the other. It should probably be lowercase without a period.
This is great. See my comment about the error message. Add a test that shows that the error message includes the actual list of parameters, too. Will be good to merge then. |
For me, this PR very similar to #626 (codebase espacially). params do
optional :beer
optional :wine
mutually_exclusive :beer, :wine, but_require_exactly_one: true
end |
I think I'd rather want a clear |
@Morred so cool to see this building on top of mutually_exclusive ;) |
@dblock , sugar for DSL, it's OK. But I see only one difference in the implementation right now: #mutual_exclusion.rb
def validate!(params)
@params = params
if two_or_more_exclusive_params_are_present
raise Grape::Exceptions::Validation, param: "#{keys_in_common.map(&:to_sym)}", message_key: :mutual_exclusion
end
params
end
#exactly_one_of.rb
def validate!(params)
@params = params
if two_or_more_restricted_params_are_present
raise Grape::Exceptions::Validation, param: "#{keys_in_common.map(&:to_sym)}", message_key: :exactly_one
end
if none_of_restricted_params_is_present
raise Grape::Exceptions::Validation, param: 'no_params', message_key: :exactly_one
end
params
end Why we should add some min/max options? |
How about keeping the new DSL while keeping things dry by extending MutualExclusionValidator? module Grape
module Validations
class ExactlyOneOfValidator < MutualExclusionValidator
def validate!(params)
super
if none_of_restricted_params_is_present
raise Grape::Exceptions::Validation, param: 'no_params', message_key: :exactly_one
end
params
end
private
def none_of_restricted_params_is_present
keys_in_common.length < 1
end
end
end
end |
Thanks for the feedback, everyone! I changed it so now exactly_one_of inherits from mutually_exclusive, since it's based on that anyway. That way we can keep the exactly_one of in the DSL without having all the duplicate code. What do you think? Also wrote the additional test. |
params do | ||
optional :beer | ||
optional :wine | ||
ȩxactly_one_of :beer, :wine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
I am ready to merge this, see typo and error message comment. Maybe someone else can suggest a better error message, too ? |
end | ||
|
||
def all_keys | ||
attrs.map(&:to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
param: "#{all_keys.map(&:to_sym)}" # => attrs.map(&:to_s).map(&:to_sym), looks weird
if you add this method you can hide details of implementation:
raise Grape::Exceptions::Validation, param: all_keys, message_key: :exactly_one
...
def all_keys
attrs.map(&:to_sym)
end
but for me, just attrs.map(&:to_sym)
is good enough.
looks better, but I'd like to see specs only for added behaviour (most of them are already present in Anyway, if @dblock is ready to merge this, no problem 💛 |
updated readme updated changelog make exactly_one_of inherit from mutually_exclusive, add test fixed typo, small changes
Fixed the typo, changed the error message and the attrs.to_sym thing. I specifically added those specs to the exactly_one_of because I wanted to see whether everything still works the same way when the exactly_one_of sits on top of the mutually_exclusive. If you guys think that's too much, I can just remove them :) |
@Morred , thanks :)
Ok, you override How about to use shared examples? It's just a question. ( more complex to readability - but DRY in spec) # shared/mutually_exclusive_examples.rb
shared_examples_for 'mutually exclusive #validate!' do
let(:scope) do
...
end
describe Grape::Validations::MutualExclusionValidator do
describe '#validate!' do
it_behaves_like 'mutually exclusive #validate!'
end
end
describe Grape::Validations::ExactlyOneOfValidator do
describe '#validate!' do
it_behaves_like 'mutually exclusive #validate!' do
context 'when none of the restricted params is selected' do
let(:params) { { somethingelse: true } }
it 'raises a validation exception' do
expect {
validator.validate! params
}.to raise_error(Grape::Exceptions::Validation)
end
end
end
end
end @dblock, @oliverbarnes any thoughts?) |
I'm merging this. I am not too hung up on DRYing tests, but I definitely think @dm1try has a point. |
added exactly_one_of validation
Do we expect this to work inside a
Because (at master) this appears not to work. |
It should work within a group, so this is likely a bug. Open a new issue with a spec please. |
updated readme
updated changelog