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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow array query parameter for a single value #6542

Conversation

mrmodise
Copy link
Contributor

@mrmodise mrmodise commented Oct 11, 2020

PR fixes an issue where by passing only a single query parameter throws an error with code INVALID_PARAMETER_VALUE and message should be array

Fixes: #6514

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@mrmodise mrmodise force-pushed the fix-array-query-parameter-with-single-value branch from 55f3d49 to 1783d44 Compare October 11, 2020 18:09
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Thank you @mrmodise for the pull request 鉂わ笍

I'd like to discuss several implementation & testing details, PTAL at my comment below.

I'd also like to discuss one important edge case: let's say we have a parameter accepting a two-dimensional array (an array of an arrays of strings). How are we going to coerce such values? Request values to consider:

  • a "full" value: [[1, 2], [3, 4]]
  • a single item at top level, containing an array: [[1, 2]]
  • multi values at top level, containing a single time array: [[1], [3]]
  • a single item array at top level containing a single item array: [[1]]

Can you please check how is swagger-ui (API Explorer) converting such values into query string parameters and ensure that our coercion code handles them correctly?

Using qs with {arrayFormat: 'repeat'}:

> qs.stringify({foo: [1]}, {arrayFormat: 'repeat'})
'foo=1' // this is your use case, right?
> qs.stringify({foo: [[1,2],[3,4]]}, {arrayFormat: 'repeat'})
'foo=1&foo=2&foo=3&foo=4' // uh oh, information was lost :(
> qs.stringify({foo: [[1,2]]}, {arrayFormat: 'repeat'})
'foo=1&foo=2'
> qs.stringify({foo: [[1],[3]]}, {arrayFormat: 'repeat'})
'foo=1&foo=3'
> qs.stringify({foo: [[1]]}, {arrayFormat: 'repeat'})
'foo=1'

Based on the above, I am not sure if it's a good idea to support coercion of singular values into an array for all array types. I guess it should be fine if we limit coercion only to itemType of string, number or boolean as suggested by @raymondfeng.

packages/rest/src/coercion/coerce-parameter.ts Outdated Show resolved Hide resolved
packages/rest/src/coercion/coerce-parameter.ts Outdated Show resolved Hide resolved
@bajtos bajtos added the REST Issues related to @loopback/rest package and REST transport in general label Oct 12, 2020
@mrmodise mrmodise force-pushed the fix-array-query-parameter-with-single-value branch from d047fdc to a133736 Compare October 12, 2020 12:47
@mrmodise
Copy link
Contributor Author

mrmodise commented Oct 12, 2020

  • [[1, 2], [3, 4]]

Interesting, below are the results of these tests:

  1. [[1, 2], [3, 4]] results in [ '1', '2', '3', '4' ]
  2. [[1, 2]] results in [ '1', '2' ]
  3. [[1], [3]] results in [ '1', '3' ]
  4. [[1]] results in [ '1' ]

@bajtos @raymondfeng done the changes as requested. Thanks

bajtos
bajtos approved these changes Oct 12, 2020
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Looks pretty good now.

I am little bit concerned about handling parameters accepting an array of arrays, but I guess that's something we can fix later, if there is ever any user demand for that.

@raymondfeng can you please review the new version too?

@mrmodise mrmodise force-pushed the fix-array-query-parameter-with-single-value branch from a133736 to f71ee0e Compare October 12, 2020 14:17
@mrmodise mrmodise marked this pull request as ready for review October 12, 2020 14:19
@mrmodise mrmodise force-pushed the fix-array-query-parameter-with-single-value branch 2 times, most recently from 960b02b to 84e4345 Compare October 12, 2020 18:28
Signed-off-by: mrmodise <modisemorebodi@gmail.com>

refactor: return array data for other spec

Signed-off-by: mrmodise <modisemorebodi@gmail.com>
@mrmodise mrmodise force-pushed the fix-array-query-parameter-with-single-value branch from 84e4345 to bc2ce05 Compare October 12, 2020 20:04
@raymondfeng raymondfeng merged commit 08c4a1a into loopbackio:master Oct 13, 2020
2 checks passed
@raymondfeng
Copy link
Contributor

@mrmodise PR is landed. Thank you for the contribution with great patience. 馃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array query parameter does not work for a single value
5 participants