Dependent parameters with specific values don't support Arrays

[squarebracket]

With params like this:

        optional :captions, type: Array do
          requires :output, type: String, value_set: ['cea608', 'cea708', 'dvbsub', 'teletext']
          given output: ->(val) { val == 'cea608' } do
            requires :language, type: String
            requires :input_pid, type: String
          end
          given output: ->(val) { val == 'dvbsub' } do
            requires :input_pid, type: String
          end
        end

grape has an exception. It's because of this code in lib/grape/validations/params_scope.rb:

        @dependent_on.each do |dependency|
          if dependency.is_a?(Hash)
            dependency_key = dependency.keys[0]
            proc = dependency.values[0]
            return false unless proc.call(params(parameters).try(:[], dependency_key))
          elsif params(parameters).try(:[], dependency).blank?
            return false
          end
        end if @dependent_on

Since parameters is an array here, and not a hash, it can't do [] since it wants an integer. This code needs to be amended to support arrays. From what I've been able to figure out on the code, the try(:[]) method should be applied to every item in the array params(parameters).

I'm looking at the spec tests in spec/grape/validations/params_scope_spec.rb to try to figure out what to copy/paste so that I can work on implementing the behaviour. I guess lines 476 -> 586, but without the error conditions?

[rnubel]

I think you're right; the use case of type: Array seems to just be an oversight. This test reproduces your error:

  context 'when validations are dependent on a parameter within an array param' do
    before do
      subject.params do
        requires :foos, type: Array  do
          optional :foo_type, :baz_type
          given :foo_type do
            requires :bar
          end
        end
      end
      subject.get('/test') { declared(params).to_json }
    end

    it 'applies the constraint within each value' do
      get '/test', foos: [ { foo_type: 'a', bar: 'b' }, { baz_type: 'c' } ]

      expect(last_response.status).to eq(200)
    end
  end

I'm taking a quick look into a fix while on the train; I'll update if I find anything.

[rnubel]

This technically fixes it, but is a bit rough: ruby-grape:master...rnubel:fix/given-within-array-param I will revisit tomorrow and make a PR, or you can run with it if you see a better path.

[squarebracket]

It appears that I am running into two problems with the current master branch:

1. Using arrays with given, which your patch does seem to solve
2. Using multiple given conditions on a single key, with a parameter that is an array.

There is a spec test starting at line 500 that has

          subject.params do
            send a_decl, :a, **a_opts
            given(a: ->(val) { val == 'x' }) { requires :b, **b_opts }
            given(a: ->(val) { val == 'y' }) { requires :c, **b_opts }
          end

So I imagine it's possible. I tried hacking array handling into should_validate?, but that was applying each given clause to every item in the item. I was printing out stack traces to see if I could figure out how everything was being processed, but maybe your patch will point me in the right direction.

[squarebracket]

I wasn't able to figure out how the pieces fit together. Can someone with more familiarity with the innards explain the call flow? From what I gather, should_validate? decides whether or not to apply @dependent_on to the current parameters. So, I guess at some point during the logic, what should be happening is that parameters should be just one of the hashes, one of the @dependent_on procs should return true and apply its block requirements, and the other @dependent_on proc should return false and thus not apply?

[timothysu]

+1

The branch linked does not actually fix the issue, modifying your test into the following tests fails because it seems that the array of hashes are being flattened into one hash. It thinks [ { foo: 'a' }, { bar: 'b', baz: 'c' } ] is valid despite bar not being in the same hash as foo. It should be validating on each individual within the array.

context 'when validations are for array elements' do
    before do
      subject.params do
        group :foos, type: Array do
          optional :foo
          optional :baz
          given :foo do
            requires :bar
          end
        end
      end
      subject.get('/test') { declared(params).to_json }
    end

    it 'applies the constraint for each element' do
      get '/test', foos: [ { foo: 'a', bar: 'b' }, { baz: 'c' } ]
      expect(last_response.status).to eq(200)

      get '/test', foos: [ { foo: 'a' }, { baz: 'c' } ]
      puts last_response.body
      expect(last_response.status).to eq(400)
      expect(last_response.body).to eq('foos[0][bar] is missing')

      get '/test', foos: [ { foo: 'a' }, { bar: 'b', baz: 'c' } ]
      puts last_response.body # Returns {"foos":[{"foo":"a","baz":"c","bar":"b"}]}
      expect(last_response.status).to eq(400)
      expect(last_response.body).to eq('foos[0][bar] is missing')
    end
  end

[jlfaber]

Don't have time to test this theory out right now, but I ran into a similar issue earlier and it turned out that the flattening behavior was being caused by the test code and not the server code. IIRC get calls were using URL query parameters and had some unexpected flattening behavior with arrays. I solved it by forcing the test code to post/put a JSON payload instead which preserved the desired array structure.

[rnubel]

@timothysu actually, the problem you're seeing is just a testing issue like @jlfaber mentions, due to two things (both come down to the fact that standard HTTP url-encoded parameters can't represent a list of hashes properly). First, even when using JSON, GET requests cannot send a body, so url-encoded is the only option; so the only way to demonstrate it is with a request like so:

    it 'applies the constraint within each value' do
      post  '/test',
        { foos: [ { foo_type: 'a' }, { bar: 'not_needed', baz_type: 'c' } ] }.to_json,
        { 'CONTENT_TYPE' => 'application/json' }

      expect(last_response.status).to eq(400)
      expect(last_response.body).to eq('foos[0][bar] is missing')
    end

This test passes on the branch linked above.

[dblock]

@rnubel is correct, I'm going to close this.