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

GH-5 add a query directive to pull values from querystring params #6

Merged
merged 2 commits into from
Oct 10, 2021

Conversation

davidalpert
Copy link
Contributor

No description provided.

the 'query' directive parses values out of the
'request.URL.Query()' values in the same way
as the 'form' directive parses values out of
the form body
@davidalpert davidalpert mentioned this pull request Aug 28, 2021
@ggicci
Copy link
Owner

ggicci commented Oct 3, 2021

Thanks a lot for this PR. Could you please confirm that if the form and query directives work the same? If so, I think we can make query as an alias of form.

@davidalpert
Copy link
Contributor Author

I was surprised to learn that the unit test I added still passed after using the form directive.

I am going to confirm that this works also in the web project I experienced the initial challenge, then will update this pr to demonstrate that the form tag works to parse querystring values.

@ggicci
Copy link
Owner

ggicci commented Oct 6, 2021

I think the name form may have caused the misunderstanding. In Go world, if we use net/http package to get query parameters, the corresponding method is Request.FormValue. That's why I named the directive asform.

Now through your case I think that maybe other people have the same problem like yours. Thus the name query should be more intuitive.

If the name was the root cause leading to the misunderstanding. I propose to make an alias of the original form directive. Which can be called query.

@ggicci
Copy link
Owner

ggicci commented Oct 9, 2021

I've reviewed this PR these days and have a conclusion here:

The form directive extracts values from Request.Form which contains the parsed form data, including both the URL field's query parameters and the PATCH, POST, or PUT form data. It's populated by Request.ParseForm() and the most important is:

Request body parameters take precedence over URL query string values in r.Form.

Though it looks like the proposed query directive can be replaced with the form directive to archieve the same function, it still has subtle differences:

The query directive ONLY treats the values from the URL query string as the valid input. Thus the following request may cause unexpected behaviour when using the form directive:

package main

import (
	"fmt"
	"net/http"
	"net/http/httptest"
	"net/url"
	"strings"
)

func PrintInput(rw http.ResponseWriter, r *http.Request) {
	r.ParseForm()
	fmt.Printf("ContentType: %#v\n", r.Header.Get("Content-Type"))
	fmt.Printf("Form: %#v\n", r.Form)
	fmt.Printf("Query: %#v\n", r.URL.Query())
}

func main() {
	postForm := url.Values{}
	postForm.Add("is_member", "false")
	r2 := httptest.NewRequest(
		"POST",
		"http://example.com/users?page=5&page_size=20&is_member=true",
		strings.NewReader(postForm.Encode()),
	)

	r2.Header.Add("Content-Type", "application/x-www-form-urlencoded")
	w2 := httptest.NewRecorder()
	PrintInput(w2, r2)
}

Output:

ContentType: "application/x-www-form-urlencoded"
Form: url.Values{"is_member":[]string{"false", "true"}, "page":[]string{"5"}, "page_size":[]string{"20"}}
                                        ^^^^^ (x)
Query: url.Values{"is_member":[]string{"true"}, "page":[]string{"5"}, "page_size":[]string{"20"}}
                                         ^^^^ (√)

Finally, I would like to merge this pull request and give some detail explanations in the documentation. @davidalpert Thanks again :)

@@ -60,11 +60,12 @@ func ListUsers(rw http.ResponseWriter, r *http.Request) {

## Features

- [x] Builtin directive `form` to decode a field from HTTP query (URL params), i.e. `http.Request.Form`
- [x] Builtin directive `form` to decode a field from HTTP form values, i.e. `http.Request.Form`
- [x] Builtin directive `query` to decode a field from HTTP querystring parameters, i.e. `http.Request.URL.Query()`
Copy link
Owner

@ggicci ggicci Oct 9, 2021

Choose a reason for hiding this comment

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

I think query is used more frequently than the other directives. It's better to be documented at the first place. And we can tell something about the difference between form and query or simply link to the conversation of this PR.

@davidalpert
Copy link
Contributor Author

Thank you for considering this PR and explaining that Request.ParseForm() already merges Request.Url.Query() into r.Form and the nuance about precedence (where form body values will override querystring values). Perhaps we can update the documentation to make it extra clear that the form directive already includes querystring values.

By having the query directive in this PR pull from r.URL.Query() instead of the merged r.Form collection, I understand that it would bind to values from the querystring but ignore any values posted in the form.

Documentation can be tricky to get right while iterating through a pull request. Would you like to update the readme directly in this branch? Feel free to take this PR, adjust it as you like, and merge it when you feel it is complete.

@ggicci ggicci merged commit d92379b into ggicci:main Oct 10, 2021
@davidalpert
Copy link
Contributor Author

davidalpert commented Oct 17, 2021

@ggicci would you be willing to add a hacktoberfest-accepted label to this PR?

I am participating in Digital Ocean's Hacktoberfest this year and that label will allow this PR to be recognized as a Hacktoberfest contribution.

@ggicci
Copy link
Owner

ggicci commented Oct 18, 2021

sure

@ggicci ggicci added the hacktoberfest-accepted https://hacktoberfest.digitalocean.com/ label Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants