Fix NoMethodError with none Hash params

[ksss]

params do
  requires :foos, type: Array do
    optional :bar
    given :bar do
      requires :baz
    end
  end
end
get '/test' { 'ok' }

client.get('/test', foos: ['abc'])
#=> NoMethodError (undefined method `with_indifferent_access' for "abc":String)

[dblock]

I don't think the fix is correct. In the example case the type of items in foos is expected to be a Hash, implied by an optional :bar. Passing a String should yield an error. I agree the current error is not great and I'd want Grape to do better.

[dblock]

I believe this should be a 400. The first item in foos does not match the expected input of my API.

[ksss]

I followed this behavior.

params do
  optional :foo
  given :foo do
    requires :bar
  end
end
get('/test') { 'ok' }

client.get '/test', 'hello'
last_response.status #=> 200
last_response.body #=> "ok"

Is this also not intentional?

[dblock]

Hm. So we have this spec today? I feel that's wrong. Maybe @namusyaka or @dm1try have an opinion?

[dm1try]

@dblock I do not see any contradiction in the examples above, in the latter case foo is not provided, so it's not validated. in the original case foos is provided, so it's validated.

according to the right behavior, if we assume that the implicit requirement for type: Array with block is "array of objects"(Array[Object]) then you are right, we should return a validation error(the string is not an "object", provided type is wrong).
in this PR, a type requirement is "array of something"(Array[Any]) and this feels wrong, as for me.

[dblock]

Ok I think I agree. So what @dm1try is saying is that the spec above that @ksss is suggesting is correct by design, but that the spec in this PR is not because see above. So I'll take back my "i feel that's wrong".

@ksss does that make sense?

[ksss]

@dm1try Thank you for reviewing.
@dblock That makes sense.
I expect that scope in this PR is no errors occur on validation step.

[dblock]

@dm1try Just to confirm, you're saying this PR is good to go as is? Merge if yes.

[dm1try]

@dblock I think it's fine, at least it's better than NoMethodError.

though, in the future this spec should pass:

  context 'group values' do
    before do
      subject.params do
        requires :value, type: Hash do
        end
      end
      subject.get('/hash') { }

      subject.params do
        requires :value, type: Array do
        end
      end
      subject.get('/array_of_hashes') { }
    end

    it 'should be consistent' do
      get '/hash', value: ""
      expect(last_response.status).to eq(400)
      expect(last_response.body).to eq("value is invalid")

      get '/array_of_hashes', value: [""]
      expect(last_response.status).to eq(400)
      expect(last_response.body).to eq("value is invalid")
    end
  end

I've tested the idea to set implicit Array[Hash] type here

        if(block && (validations[:type].nil? || validations[:type] == Array))
          validations[:type] = Array[Hash]
        end

and while it works the case above, it fails for the cases where we rely on Virtus coerce logic for the hash

irb -Ilib -rgrape
irb(main):001:0> Virtus::Attribute.build(Array[Hash]).coerce({just: "hash"})
=> [{:just=>nil, "hash"=>nil}]
irb(main):002:0> Virtus::Attribute.build(Array).coerce({just: "hash"})
=> [[:just, "hash"]]

so this needs more investigation