grape icon indicating copy to clipboard operation
grape copied to clipboard

Issue with same param name in different given blocks

Open andreacfm opened this issue 5 years ago • 7 comments

        .....
        given name: ->(val) do
          val == 'name'
        end do
          requires :values, type: Array[String]
        end

        given name: ->(val) do
          val == 'another_name'
        end do
          requires :values, type: Array[Json]
        end
....

Seems like the check on the values param in the second block override the first. When both the blocks are executed in the same call the second check on values fails as invalid. Sounds like a bug?

andreacfm avatar May 14 '19 15:05 andreacfm

Looks like a non-feature of supporting two given blocks with the same key?

Turn this into a spec, maybe implement/fix?

dblock avatar May 14 '19 15:05 dblock

@dblock I will add a spec.

andreacfm avatar May 14 '19 18:05 andreacfm

@dblock can you point me to the best place for adding the test case?

andreacfm avatar May 14 '19 21:05 andreacfm

I would say spec/grape/validations/params_scope_spec.rb has most of the given specs.

dblock avatar May 15 '19 03:05 dblock

^^ My workaround should NOT be used. It appears to work by breaking parameter validation.

MacksMind avatar Jun 15 '19 16:06 MacksMind

I spent time looking into this today, and I think I understand the problem, but I believe the fix would require a non-trivial change to how this library works.

Parameter declarations are set when the class is loaded, not when a request is made. This manifests as a problem related to given but it’s not exactly that.

Suppose you were to do this:

requires :type, type: String

given type: ->(val) { val == 'typeA' } do
  requires :attributes, type: Hash do
    requires :foo, type: String, values: ['foo-a', 'foo-b']
    requires :bar, type: String
  end
end

given type: ->(val) { val == 'typeB' } do
  requires :attributes, type: Hash do
    requires :foo, type: Integer, values: [1, 2]
  end
end

In terms of validations, this actually works as expected:

# Requests with these params will pass validation, as we want
{type: 'typeA', attributes: { foo: 'foo-a', bar: 'z' }}
{type: 'typeB', attributes: { foo: 1 }}

# Requests with these params will fail to validate, as we want
{type: 'typeA', attributes: { foo: 'foo-a' }}        # requires bar param
{type: 'typeA', attributes: { foo: 1, bar: 'y' }}    # foo needs to be in ['foo-a', 'foo-b']
{type: 'typeB', attributes: { foo: 'foo-a' }}        # foo needs to be in [1, 2]

This is good, but the good news stops there. If we try to use declared() to access our declared params, things fall apart:

# If we make a request with these params, 
{type: 'typeA', attributes: { foo: 'foo-a', bar: 'z' }}

# Then declared(params) gives us:
{type: 'typeA', attributes: { foo: 'foo-a' }}  # Note how the `bar` param is missing even though we declared it!

The reason is that declared params are determined when the class is loaded, so the second requires :attributes block overwrites the first one. The given block is entirely irrelevant when the parameter stack is being determined at load-time, and from the perspective of the declared() helper, the requires :bar declaration doesn't exist.

In order to solve this problem, I think we would need for two things to happen:

  1. The block contents of one parameter should be appended to the block contents of another parameter, rather than overwriting them.
  2. We would need for the declared() helper to appropriately take into account the results of the proc supplied to given.

barunio avatar Feb 21 '21 03:02 barunio