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

feat: support peek multi value for query args #684

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

a631807682
Copy link
Member

@a631807682 a631807682 commented Mar 22, 2023

What type of PR is this?

feat

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.

(Optional) Translate the PR title into Chinese.

支持获取多个值的query参数

(Optional) More detail description for this PR(en: English/zh: Chinese).

en:
ctx.Query cannot get multiple values of a from a=1&a=2&b=1.
QueryArray is provided in gin to get multiple values, but rfc3986 does not define the implementation of the server, and many frameworks are inconsistent when parsing repeated queries , some only use the first or last occurrence.
So I'm not sure it needs to be implemented in RequestContext, but I hope there is still a way to get it.

 ctx.QueryArgs().PeekMulti("a")

zh(optional):
ctx.Query 无法从 a=1&a=2&b=1 获取 a 的多个值。
gin中提供了QueryArray获取多个值,但是 rfc3986 没有定义服务端的实现,很多框架在解析的时候不一致,有些只使用第一个值或最后一个值。
所以我不确定它是否需要在RequestContext中实现,但我希望有办法获到它。

 ctx.QueryArgs().PeekMulti("a")

Which issue(s) this PR fixes:

@a631807682 a631807682 requested review from a team as code owners March 22, 2023 08:45
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f7a873f) 81.82% compared to head (1b70abe) 81.83%.

❗ Current head 1b70abe differs from pull request most recent head 9f510f1. Consider uploading reports for the commit 9f510f1 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #684      +/-   ##
===========================================
+ Coverage    81.82%   81.83%   +0.01%     
===========================================
  Files           98       98              
  Lines         9926     9932       +6     
===========================================
+ Hits          8122     8128       +6     
  Misses        1314     1314              
  Partials       490      490              
Files Coverage Δ
pkg/protocol/args.go 83.10% <100.00%> (+0.47%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@li-jin-gou
Copy link
Member

refer to https://github.com/cloudwego/hertz/pull/569/files and do you have a real world example of this use?

@li-jin-gou li-jin-gou self-assigned this Mar 22, 2023
@li-jin-gou li-jin-gou added the enhancement New feature or request label Mar 22, 2023
@a631807682
Copy link
Member Author

refer to #569 (files) and do you have a real world example of this use?

Do you mean I need to describe this scenario in a test like https://github.com/cloudwego/hertz/pull/569/files#diff-6b0ef71f167bc3e1767a55944dc08d8cfb025629780b5cfcceb1cbe21e668e21R577 ?
Or do I need to add relevant code samples in the PR's description?

@li-jin-gou
Copy link
Member

refer to #569 (files) and do you have a real world example of this use?

Do you mean I need to describe this scenario in a test like https://github.com/cloudwego/hertz/pull/569/files#diff-6b0ef71f167bc3e1767a55944dc08d8cfb025629780b5cfcceb1cbe21e668e21R577 ? Or do I need to add relevant code samples in the PR's description?

Reference code implementation

@a631807682
Copy link
Member Author

refer to #569 (files) and do you have a real world example of this use?

Do you mean I need to describe this scenario in a test like #569 (files) ? Or do I need to add relevant code samples in the PR's description?

Reference code implementation

Sorry, I still don't get your point, can you tell me what aspect of PR implementation I need to refer to?

  1. Is it about api design?
    We need to provide this api on RequestContext? There are two different considerations here, and the current implementation is the first one, please let me know if there is any problem.
    Added ctx.QueryMulti() gofiber/fiber#288 (comment)
    Implement QueryArray and PostFormArray methods gin-gonic/gin#570
  2. Are there performance concerns?
    QueryArgs will only be parsed once, this method is not much different from GetQuery
  3. Did not describe the real usage scenario?
    Do I need to add tests for multiple parameter requests to describe it?

The above is all my guesswork, I'd like some clear advice to let me know what I should do. 🙏

@li-jin-gou
Copy link
Member

refer to #569 (files) and do you have a real world example of this use?

Do you mean I need to describe this scenario in a test like #569 (files) ? Or do I need to add relevant code samples in the PR's description?

Reference code implementation

Sorry, I still don't get your point, can you tell me what aspect of PR implementation I need to refer to?

  1. Is it about api design?
    We need to provide this api on RequestContext? There are two different considerations here, and the current implementation is the first one, please let me know if there is any problem.
    Added ctx.QueryMulti() gofiber/fiber#288 (comment)
    Implement QueryArray and PostFormArray methods gin-gonic/gin#570
  2. Are there performance concerns?
    QueryArgs will only be parsed once, this method is not much different from GetQuery
  3. Did not describe the real usage scenario?
    Do I need to add tests for multiple parameter requests to describe it?

The above is all my guesswork, I'd like some clear advice to let me know what I should do. 🙏

ok

pkg/protocol/args.go Outdated Show resolved Hide resolved
pkg/protocol/args.go Outdated Show resolved Hide resolved
@li-jin-gou
Copy link
Member

users needs to use this PR and I will CR it today

@li-jin-gou li-jin-gou merged commit e02c162 into cloudwego:develop Oct 19, 2023
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

5 participants