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

exactly_one_of seems not work for nested parameters #1288

Open
mmkhmk opened this issue Feb 23, 2016 · 10 comments
Open

exactly_one_of seems not work for nested parameters #1288

mmkhmk opened this issue Feb 23, 2016 · 10 comments
Labels

Comments

@mmkhmk
Copy link

mmkhmk commented Feb 23, 2016

Hello,

I'm currently creating an API with the latest version(0.14.0) but exactly_one_of doesn't work as expected for nested parameters.

Here is the sample.

params do
  optional :additionalFacets, type: Hash do
    requires: label, type: String
    requires: facet, type: Hash do
      requires :field, type: String
      requires :type, type: String

      optional :limit, type: Integer
      optional :start, type: Integer

      exactly_one_of :limit, :start
    end
  end
end

In this case I meant the validation of exactly_one_of works only when additionalFacets are given.
But it works when it is not given as well.

Can you help check this?

@mmkhmk mmkhmk changed the title exactly_one_of seems not work in a group inside group exactly_one_of seems not work for nested parameters Feb 23, 2016
@dblock
Copy link
Member

dblock commented Feb 23, 2016

It sounds like a bug, try writing a spec that reproduces this?

@dblock dblock added the bug? label Feb 23, 2016
@mmkhmk
Copy link
Author

mmkhmk commented Feb 24, 2016

Here is the spec that reproduces it.

context 'nested params in nested params' do
  before do
    subject.params do
      requires :nested, type: Hash do
        requires :nested_in_nested, type: Hash do
          optional :beer_nested
          optional :wine_nested
          optional :juice_nested
          exactly_one_of :beer_nested, :wine_nested, :juice_nested
        end
      end
      optional :nested2, type: Hash do
        requires :nested_in_nested2, type: Array do
          optional :beer_nested2
          optional :wine_nested2
          optional :juice_nested2
          exactly_one_of :beer_nested2, :wine_nested2, :juice_nested2
        end
      end
    end
    subject.get '/exactly_one_of_nested' do
      'exactly_one_of works!'
    end
  end

  it 'errors when none is present' do
    get '/exactly_one_of_nested'
    expect(last_response.status).to eq(400)
    expect(last_response.body).to eq 'nested is missing, nested[nested_in_nested] is missing, beer_nested, wine_nested, juice_nested are missing, exactly one parameter must be provided'
  end

  it 'succeeds when one is present' do
    get '/exactly_one_of_nested', nested: { nested_in_nested: { beer_nested: 'string' } }
    expect(last_response.status).to eq(200)
    expect(last_response.body).to eq 'exactly_one_of works!'
  end

  it 'errors when two or more are present' do
    get '/exactly_one_of_nested', nested: { nested_in_nested: { beer_nested: 'string' } }, nested2: { nested_in_nested2: [{ beer_nested2: 'string', wine_nested2: 'anotherstring' }] }
    expect(last_response.status).to eq(400)
    expect(last_response.body).to eq 'beer_nested2, wine_nested2 are mutually exclusive'
  end
end

Result

Failures:

  1) Grape::Validations params exactly one of nested params in a group errors when none are present
     Failure/Error: expect(last_response.body).to eq 'nested is missing, nested[nested_in_nested] is missing, beer_nested, wine_nested, juice_nested are missing, exactly one parameter must be provided'

       expected: "nested is missing, nested[nested_in_nested] is missing, beer_nested, wine_nested, juice_nested are missing, exactly one parameter must be provided"
            got: "nested is missing, nested[nested_in_nested] is missing, beer_nested, wine_nested, juice_nested are missing, exactly one parameter must be provided, beer_nested2, wine_nested2, juice_nested2 are missing, exactly one parameter must be provided"

       (compared using ==)
     # ./spec/grape/failed_validations_spec.rb:40:in `block (5 levels) in <top (required)>'

  2) Grape::Validations params exactly one of nested params in a group succeeds when one is present
     Failure/Error: expect(last_response.status).to eq(200)

       expected: 200
            got: 400

       (compared using ==)
     # ./spec/grape/failed_validations_spec.rb:45:in `block (5 levels) in <top (required)>'

Finished in 0.10893 seconds (files took 0.44235 seconds to load)
3 examples, 2 failures

@jlfaber
Copy link
Contributor

jlfaber commented Apr 4, 2017

I just tried this spec on both the current HEAD and v0.19.1 and it passes with no problem.

@dblock
Copy link
Member

dblock commented Apr 4, 2017

Closing, @mmkhmk if you can reproduce on HEAD please PR a failing spec.

@dblock dblock closed this as completed Apr 4, 2017
@milgner
Copy link
Contributor

milgner commented Mar 27, 2018

I was just able to reproduce this with 1.0. See https://gist.github.com/milgner/d3c229c8dfd6670720403a92066f15f5 for a small, self-contained example. Find a zip of the full application with Gemfile etc at https://oc.illunis.net/index.php/s/ejXzHAYkCcojcCw

@dblock
Copy link
Member

dblock commented Mar 27, 2018

@milgner Can you PR a failing spec from that example on HEAD?

@dblock
Copy link
Member

dblock commented Mar 27, 2018

I'll reopen for now.

@dblock dblock reopened this Mar 27, 2018
@milgner
Copy link
Contributor

milgner commented Mar 27, 2018

I have a couple of free days coming up, so that shouldn't be a problem 😄

@milgner
Copy link
Contributor

milgner commented Mar 28, 2018

I added a test case and am currently looking into fixing it. It seems like the problem is that while Validations::Base does a check on @scope.meets_dependency?, a similar check is missing from ExactlyOneOfValidator.

I'm not familiar enough with the codebase to make really qualified recommendations, but it seems like it might help to move the part that checks meets_dependency from Base#validate!(params) into Base#validate(request)?

See milgner/grape@8d8586e for the failing spec.

@milgner
Copy link
Contributor

milgner commented Mar 28, 2018

It looks like the problem might have been introduced in #1625, which changed the behaviour of should_validate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants