pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 688 forks source link

Support multiple values for query params #1130

Open sanjayatn opened 1 year ago

sanjayatn commented 1 year ago

Support Multiple Values for Query Params. In the wikipedia article for https://en.wikipedia.org/wiki/Query_string it says "While there is no definitive standard, most web frameworks allow multiple values to be associated with a single field (e.g. field1=value1&field1=value2&field2=value3). Many frameworks support this even though it is not the standard. Currently Pistache supports multiple values separated by a comma by default.

This PR adds two enhancements:

  1. Transform the multiple value through duplicate param names into a list of values separated by a , to allow the rest of the framework to convert values to std::vector
  2. Provide all the param/value pairs received with the request.
  3. Add Query update method to replace a value in the Query collection.
Tachi107 commented 1 year ago

Hi @sanjayatn, thanks for your patch! Could you please add more information about it (comments etc.)?

kiplingw commented 1 year ago

And rebase too.

sanjayatn commented 1 year ago

@Tachi107 @kiplingw I didn't realized you can see this branch but I love to submit it as a proper PR! Coming up today!

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 26.66% and project coverage change: -0.12 :warning:

Comparison is base (a68ad09) 78.48% compared to head (aa6c74a) 78.36%.

:exclamation: Current head aa6c74a differs from pull request most recent head d268fdc. Consider uploading reports for the commit d268fdc to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1130 +/- ## ========================================== - Coverage 78.48% 78.36% -0.12% ========================================== Files 53 53 Lines 6892 6907 +15 ========================================== + Hits 5409 5413 +4 - Misses 1483 1494 +11 ``` | [Impacted Files](https://codecov.io/gh/pistacheio/pistache/pull/1130?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [include/pistache/http.h](https://codecov.io/gh/pistacheio/pistache/pull/1130?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-aW5jbHVkZS9waXN0YWNoZS9odHRwLmg=) | `86.48% <ø> (ø)` | | | [src/common/http.cc](https://codecov.io/gh/pistacheio/pistache/pull/1130?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9odHRwLmNj) | `73.57% <26.66%> (-1.09%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

sanjayatn commented 1 year ago

@Tachi107 @kiplingw I see two things failing in the checks. Which one should focus on? If you can give me 2 sentence summary of what abidiff / abi step's goal, then I have better idea to fix it.

kiplingw commented 1 year ago

Hey @sanjayatn. For commitlint you can find what it's complaining about here. For abidiff my guess is you may have missed this.

sanjayatn commented 1 year ago

@kiplingw thanks!

Tachi107 commented 1 year ago

Hi @sanjayatn, thanks for the added description and sorry for the delayed response.

I'm personally not a fan of multiple query parameters because, as you say, the behaviour is not standardized, and differences in how different software components handle them can be a source of security vulnerabilities, so much that this class of vulnerabilities got a name: "HTTP parameter pollution".

That being said, aligning to what others do is probably fine, as long as the others do sensible stuff.

Could you please add a couple of test cases showing this new behaviour? Thanks!