Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Fix comma separated list in query parameters #186

Closed lmazuel closed 4 years ago

lmazuel commented 4 years ago

Fix https://github.com/Azure/azure-sdk-for-python/issues/6370

codecov-io commented 4 years ago

Codecov Report

Merging #186 into master will decrease coverage by 0.82%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
- Coverage   87.89%   87.06%   -0.83%     
==========================================
  Files          25       25              
  Lines        2593     2597       +4     
==========================================
- Hits         2279     2261      -18     
- Misses        314      336      +22
Impacted Files Coverage Δ
msrest/serialization.py 89.88% <100%> (-1.25%) :arrow_down:
msrest/paging.py 87.71% <0%> (-3.51%) :arrow_down:
msrest/exceptions.py 95.94% <0%> (-2.71%) :arrow_down:
msrest/pipeline/universal.py 93.13% <0%> (-1.97%) :arrow_down:
msrest/pipeline/__init__.py 89.6% <0%> (-1.61%) :arrow_down:
msrest/universal_http/__init__.py 74.09% <0%> (-1.04%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4a0a44a...2fcb2a9. Read the comment docs.

jhgoodwin commented 3 years ago

Did anyone happen to notice that while this passes tests and seems like an obvious fix, the resulting query parameter is encoded incorrectly? array/list query parameters are expected to be like this: https://myhost.com/api/myendpoint?friend=Robert+Downey%2C+Jr.&friend=Michael+Jordan I validated this using URLSearchParams in a developer console of a browser window:

z = new URLSearchParams()
URLSearchParams {  }

z.append('friend', 'Robert Downey, Jr.')
undefined
z.append('friend', 'Michael Jordan')
undefined
z.getAll('friend')
Array [ "Robert Downey, Jr.", "Michael Jordan" ]
z.toString()
"friend=Robert+Downey%2C+Jr.&friend=Michael+Jordan"

Additionally, note that putting a comma in the query string results in the same behavior as the encoded version:

x = new URLSearchParams('friend=Robert+Downey,+Jr.&friend=Michael+Jordan')
URLSearchParams {  }

x.getAll('friend')
Array [ "Robert Downey, Jr.", "Michael Jordan" ]
jhgoodwin commented 3 years ago

A coworker pointed out that this may be relevant to the implementation of this feature: https://swagger.io/docs/specification/serialization/

(Or for 2.0 spec: https://swagger.io/docs/specification/2-0/describing-parameters/#array )

jhgoodwin commented 3 years ago

Looking closely, apparently 2.0 spec and 3.0 spec have different defaults, too. One is csv, the other is form, respectively. Fun times.