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 exactly_one_of validation #637

Merged
merged 1 commit into from
Apr 29, 2014
Merged

added exactly_one_of validation #637

merged 1 commit into from
Apr 29, 2014

Conversation

Morred
Copy link
Contributor

@Morred Morred commented Apr 25, 2014

updated readme

updated changelog

@Morred
Copy link
Contributor Author

Morred commented Apr 25, 2014

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

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.

@dblock
Copy link
Member

dblock commented Apr 25, 2014

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.

@dm1try
Copy link
Member

dm1try commented Apr 25, 2014

For me, this PR very similar to #626 (codebase espacially).
Could we just extend mutually_exclusive validator?

params do
  optional :beer
  optional :wine
  mutually_exclusive :beer, :wine, but_require_exactly_one: true
end

@dblock
Copy link
Member

dblock commented Apr 25, 2014

I think I'd rather want a clear exactly_one_of DSL, but maybe the implementation can definitely be shared with a validator that has min/max options?

@oliverbarnes
Copy link
Contributor

@Morred so cool to see this building on top of mutually_exclusive ;)

@dm1try
Copy link
Member

dm1try commented Apr 26, 2014

@dblock , sugar for DSL, it's OK. But I see only one difference in the implementation right now: mutually_exclusive => optional, exactly_one_of => required

#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?

@oliverbarnes
Copy link
Contributor

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

@Morred
Copy link
Contributor Author

Morred commented Apr 27, 2014

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

@dblock
Copy link
Member

dblock commented Apr 27, 2014

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

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.

@dm1try
Copy link
Member

dm1try commented Apr 27, 2014

looks better, but I'd like to see specs only for added behaviour (most of them are already present in mutual_exclusion_spec.rb).

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
@Morred
Copy link
Contributor Author

Morred commented Apr 28, 2014

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 :)

@dm1try
Copy link
Member

dm1try commented Apr 28, 2014

@Morred , thanks :)

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

Ok, you override validate! method and, I think, it does make sense.

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?)

@dblock
Copy link
Member

dblock commented Apr 29, 2014

I'm merging this. I am not too hung up on DRYing tests, but I definitely think @dm1try has a point.

dblock added a commit that referenced this pull request Apr 29, 2014
added exactly_one_of validation
@dblock dblock merged commit 1eee799 into ruby-grape:master Apr 29, 2014
@davetapley
Copy link

Do we expect this to work inside a group? E.g:

params do
  group :libation do
    optional :beer
    optional :wine
    exactly_one_of :beer, :wine, 
  end
end

Because (at master) this appears not to work.
I get the exactly one parameter must be provided error giving either libation[beer]= or libation[wine]=, yet it will be accepted with just beer= or wine= (i.e. the validation ignores the group).

@dblock
Copy link
Member

dblock commented Jul 14, 2014

It should work within a group, so this is likely a bug. Open a new issue with a spec please.

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.

5 participants