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

Issue with same param name in different given blocks #1885

Open
andreacfm opened this issue May 14, 2019 · 6 comments
Open

Issue with same param name in different given blocks #1885

andreacfm opened this issue May 14, 2019 · 6 comments
Labels

Comments

@andreacfm
Copy link
Contributor

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

@dblock
Copy link
Member

dblock commented May 14, 2019

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

Turn this into a spec, maybe implement/fix?

@dblock dblock added the bug? label May 14, 2019
@andreacfm
Copy link
Contributor Author

@dblock I will add a spec.

@andreacfm
Copy link
Contributor Author

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

@dblock
Copy link
Member

dblock commented May 15, 2019

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

MacksMind added a commit to upper-hand/grape that referenced this issue Jun 15, 2019
@MacksMind
Copy link

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

@barunio
Copy link

barunio commented Feb 21, 2021

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.

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