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

Support multiple values for query params #1130

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sanjayatn
Copy link

@sanjayatn sanjayatn commented Apr 24, 2023

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
Copy link
Member

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

@kiplingw
Copy link
Member

And rebase too.

@sanjayatn
Copy link
Author

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

@sanjayatn sanjayatn force-pushed the sanjaya/mutliple-query-params branch from 64ad95e to d268fdc Compare April 26, 2023 16:51
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2023

Codecov Report

Patch coverage: 26.66% and project coverage change: -0.12 ⚠️

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

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

📣 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
@@            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 Coverage Δ
include/pistache/http.h 86.48% <ø> (ø)
src/common/http.cc 73.57% <26.66%> (-1.09%) ⬇️

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

@sanjayatn
Copy link
Author

@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
Copy link
Member

kiplingw commented May 3, 2023

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

@sanjayatn
Copy link
Author

@kiplingw thanks!

@Tachi107
Copy link
Member

Tachi107 commented Jul 3, 2023

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!

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.

5 participants