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

Params: optional arrays within hashes/JSON objects broken #2001

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dleavitt
Copy link

See failing tests below for more specifics.

To reproduce
In a params block, define an array param within a Hash or JSON param:

params do
  optional :data, type: Hash do
    optional :array, type: Array do
      optional :name, type: String
    end
  end
end

When the request body looks like this: { data: {} }
I'd expect that calling declared(params) within the route would return something like this:
{ data: { array: [] } }
or maybe this:
{ data: {} }

What's actually returned is this:
{ data: { array: { name: nil } } }

A hash rather than an array.

@grape-bot
Copy link

grape-bot commented Feb 28, 2020

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#2001](https://github.com/ruby-grape/grape/pull/2001): Params: optional arrays within hashes/json objects broken - [@dleavitt](https://github.com/dleavitt).

Generated by 🚫 danger

@myxoh
Copy link
Member

myxoh commented Mar 4, 2020

Makes sense, thanks for the test!

@dblock
Copy link
Member

dblock commented Mar 4, 2020

@dleavitt now that we have a test (thanks), try fixing it?

post '/nesty', { data: { } }
expect(last_response.status).to eq 201
# {"data"=>{"json"=>{"name"=>nil}}}
expect(JSON.parse(last_response.body)['data']['json']).not_to be_a Hash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess here we should expect the hash. @dleavitt could you verify this test?

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

Successfully merging this pull request may close these issues.

None yet

5 participants