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

Added ctx.QueryMulti() #288

Closed
wants to merge 2 commits into from
Closed

Added ctx.QueryMulti() #288

wants to merge 2 commits into from

Conversation

moneszarrugh
Copy link
Contributor

QueryMulti returns all values for the query string parameter in the url.

QueryMulti returns all values for the query string parameter in the url.
@welcome
Copy link

welcome bot commented Apr 23, 2020

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you want to chat with us, join ons on Telegram https://t.me/gofiber

ctx.go Outdated
// QueryMulti returns all values for the query string parameter in the url.
func(ctx *Ctx) QueryMulti(key string) (values []string) {
valuesBytes := ctx.Fasthttp.QueryArgs().PeekMulti(key)
values := make([]string, len(valuesBytes))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@moneszarrugh moneszarrugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@thomasvvugt
Copy link
Contributor

Hi @monzarrugh !

Thanks for opening this Pull Request. I think it's a nice addition and I do see use cases of having multiple query parameters sharing the same key. However, I was unable to find any official documentation of having duplicate key entries in query strings.

Commonly seen when having the same identifier and data type in query string, is adding the model name, i.e. /logs?user_id=1&server_id=2.

Because this pull request does not share a default or common use case, I would suggest using the pointer to FastHTTP's RequestCtx ctx.Fasthttp.QueryArgs().PeekMulti(key), rather than implementing this function into Fiber itself.

Let us know if you have any questions!

Copy link
Member

@renanbastos93 renanbastos93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems good to me

But I believe we must create unit tests in this case and added basic documents about it and smash both pull requests.

@moneszarrugh
Copy link
Contributor Author

moneszarrugh commented Apr 23, 2020

Hi @monzarrugh !

Thanks for opening this Pull Request. I think it's a nice addition and I do see use cases of having multiple query parameters sharing the same key. However, I was unable to find any official documentation of having duplicate key entries in query strings.

Commonly seen when having the same identifier and data type in query string, is adding the model name, i.e. /logs?user_id=1&server_id=2.

Because this pull request does not share a default or common use case, I would suggest using the pointer to FastHTTP's RequestCtx ctx.Fasthttp.QueryArgs().PeekMulti(key), rather than implementing this function into Fiber itself.

Let us know if you have any questions!

I added it because I found the need for it for my own use actually.

The use case would be if you would implement filtering by multiple values for the same key (categories) for example.
/products?cat=shoes?cat=shirts?cat=bags

@Fenny
Copy link
Member

Fenny commented Apr 24, 2020

Hi @monzarrugh and welcome, thank you for trying to improve fiber by making a PR that contains a possible new feature for fiber.

I do agree with @thomasvvugt, RFC 3986 has a section 3.4. Query, which refers to key=value pairs, but nothing is said on how to interpret order and duplicate fields and so on.

This means you may do what you like but at the same time duplicate keys have no standard when it comes to the order of the values (first-given, last-given, array-of-all,), in addition to the server keeping the order of the query string, there is also the question about the browser sending them in DOM (or some other fixed) order.

I did some research on how other HTTP back-end's parse duplicate queries and found this report from OWASP that analyzed HTTP Parameter Pollution attacks. Page 9 clearly shows that many frameworks are inconsistent when it comes to parsing duplicate queries. Some only use the first or last occurrence.

For your use case you could also use one query key and divide the values with , for example, this way you have a consistent result on both the server and client side. /products?cat=shoes,shirts,bags.

Since it's use-case depended and we want to keep Fiber's API as clean as possible and inline with expressjs. I think that this would not have a valid purpose in this framework.

If you are stuck with /products?cat=shoes?cat=shirts?cat=bags my advice would be to use the ctx.Fasthttp.QueryArgs().PeekMulti("cat") or create a custom function within your application using this endpoint.

I hope this answered your question, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants