Skip to content

Fix is_array check for non-contiguous sub and parent entities #691

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

jdmurphy
Copy link
Contributor

@jdmurphy jdmurphy commented Jul 30, 2018

While reviewing an API made with Grape, I noticed that an entity created for a POST request param body did not have the correct markings for array and object. In digging into the code, I found that the parse_request_params was dependent on the ordering of the params coming in. Specifically, since we initialize array_key outside the loop, and then set it below, it should exist for elements of an array, but I found that my params were in an order that caused sub elements to not be by their parents. For example (from tests so that I don't have to leak my API internals):

[
  ['id', { required: true, type: 'String' }],
  ['description', { required: false, type: 'String' }],
  ['stuffs', { required: true, type: 'Array' }],
  ['stuffs[owners]', { required: true, type: 'Array' }],
  ['stuffs[creators]', { required: true, type: 'Array' }],
  ['stuffs[owners][id]', { required: true, type: 'String' }],
  ['stuffs[creators][id]', { required: true, type: 'String' }]
]

would be updated to (notice 'stuffs[owners][id]').

[
  ['id', { required: true, type: 'String' }],
  ['description', { required: false, type: 'String' }],
  ['stuffs', { required: true, type: 'Array', is_array: true }],
  ['stuffs[owners]', { required: true, type: 'Array', is_array: true }],
  ['stuffs[creators]', { required: true, type: 'Array', is_array: true }],
  ['stuffs[owners][id]', { required: true, type: 'String' }],
  ['stuffs[creators][id]', { required: true, type: 'String', is_array: true }]
]

I updated parse_request_params to keep all array keys and then see if the name of current param matches any of them exactly or matches any of the array keys with a [ appended which would indicate it is a sub element.

@jdmurphy jdmurphy force-pushed the param_order_issue branch from 50be6d8 to 2d398f6 Compare July 30, 2018 19:34
@coveralls
Copy link

coveralls commented Jul 30, 2018

Coverage Status

Coverage decreased (-0.5%) to 99.529% when pulling 2d398f6 on jdmurphy:param_order_issue into baa8614 on ruby-grape:master.

@LeFnord
Copy link
Member

LeFnord commented Aug 2, 2018

thanks again @jdmurphy 😄 …

@LeFnord LeFnord merged commit f7376f7 into ruby-grape:master Aug 2, 2018
@jdmurphy
Copy link
Contributor Author

@LeFnord, any chance we could get a release since there are a few fixes lined up since the last one?

@LeFnord
Copy link
Member

LeFnord commented Aug 21, 2018

yeap sure, will cut a new release the next days … 😄

LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants