-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
π FEATURE: add queries function #2475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add benchmark
Sure, I'll add it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the body of the method still needs to be corrected
encoded strings should not be taken into account, because the control characters are therefore invalid
Sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a question to @ReneWerner87 I suppose: but why not use c.app.getString
here? Is there a specific reason? It is way slower than using c.app.getString
.
I ran a benchmark with string(...)
and c.app.getString
both 100 times, and the results are these:
with string(...)
it takes an average of 1,006 ns/op
, uses 418 B/op
, and 11 allocs/op
See Queries.xlsx
with c.app.getString
it takes an average of 725 ns/op
, uses 336 B/op
, and 2 allocs/op
See QueriesPreAllocOpt.xlsx
This (string(...)
) is also in GetReqHeaders
, and GetRespHeaders
, no other occurance of string(...)
in ctx.go, and tests seem to pass just fine with c.app.getString
as well, so is there a specific reason that string(...)
was used there?
Co-authored-by: cmd777 <83428931+cmd777@users.noreply.github.com>
@leonklingele I revert two suggestion commits because Linter gets an error. |
Linter failed because of timeout, not because of the code. |
Good to know; thanks! |
|
As for your first question, I don't think it's needed. |
We should think about the first point, because if we release it first, a change would be a breaking change. I assume that the users also assume that the array values are split and stored in a deep structure, because otherwise you would have to do it yourself. |
You are right, but the output of this function is a c.Request().URI().SetQueryString("list=1,2,3")
queries := c.Queries()
queries["list"] // 3 Do you have any suggestions about the output of the queries function? |
Returning map[string][]string seems better to me |
Or map[string]interface{} |
If that |
And what happens for these examples? GET /api/posts?filters.author.name=John&filters.category.name=Technology GET /api/orders?filters[customer][name]=Alice&filters[status]=pending |
Some more interesting examples which we should consider GET /api/search?filters[category]=electronics&filters[tags][]=apple&filters[tags][]=orange&filters[tags][]=banana GET /api/search?tags=apple,orange,banana GET /api/search?filters[tags]=apple,orange,banana&filters[category][name]=fruits GET /api/search?filters.tags=apple,orange,banana&filters.category.name=fruits GET /api/search?filters.tags.0=apple&filters.tags.1=orange&filters.tags.2=banana&filters.category.name=fruits GET /api/search?filters[tags][0]=apple&filters[tags][1]=orange&filters[tags][2]=banana&filters[category][name]=fruits Can you include these examples in the test and document the expectations |
Sure, I add this test case and documents. |
@ReneWerner87 shouldn't we make output of function to |
Not really, I thought about it again and thought that it is ok, as long as we describe it well in the documentation. |
* π FEATURE: add queries method * π DOCS: add documents for queries method. * π©Ή Fix: fix wrap error returned from Queries function * π¨ tests: add url encoded tests * π₯ feature: add url encoded support for queries * π©Ή Fix: fix wrap error returned from Queries function * β»οΈ Refactor: change error message of url.QueryUnescape * β»οΈ Refactor: refactor of mapping key and value queries * π¨ Test: Validate to fail parse queries * π¨ Test: Add benchmark test for Queries * π©Ή Fix: remove parsing for encoded urls * β»οΈ Refactor: change string function to c.app.getString fucntion Co-authored-by: cmd777 <83428931+cmd777@users.noreply.github.com> * β»οΈ Refactor: change name of benchamark function ctx queries Co-authored-by: leonklingele <git@leonklingele.de> * β»οΈ Refactor: remove empty lines Co-authored-by: leonklingele <git@leonklingele.de> * Revert "β»οΈ Refactor: change string function to c.app.getString fucntion" This reverts commit 28febf9. * π Docs: add documents for queries method * π¨ Tests: add more tests for queries function * β»οΈ Refactor: change string function to c.app.getString function * π¨ Tests: add more test for queries function * π Docs: add more documents to queries function --------- Co-authored-by: cmd777 <83428931+cmd777@users.noreply.github.com> Co-authored-by: leonklingele <git@leonklingele.de>
This PR adds a new function called
Queries()
to theCtx
struct. This function returns a map of query parameters and their values.