Skip to content

http: add QueryUnescape in HTTPPool.ServeHTTP#91

Open
lorneli wants to merge 1 commit into
golang:masterfrom
lorneli:fix-servehttp
Open

http: add QueryUnescape in HTTPPool.ServeHTTP#91
lorneli wants to merge 1 commit into
golang:masterfrom
lorneli:fix-servehttp

Conversation

@lorneli

@lorneli lorneli commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

Use url.QueryUnescape to inverse escaped group and key in path.

@bradfitz

bradfitz commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

This is missing a bug (or description of the problem in the commit message), and is missing a test.

And does this change break behavior of existing clients?

Can this be incrementally rolled out safely?

I think if we're changing the "protocol" like this, we should make it explicit and put something in the URL from the client to indicate that the peer "server" should unescape like this, whether that something is a new path component or header or query parameter.

@lorneli

lorneli commented Nov 6, 2017

Copy link
Copy Markdown
Contributor Author

I find this change breaks behavior of existing clients. For example, a key "+myKey+" sent in raw format will be converted to " myKey ".

How about adding a query parameter http://base/group/key?escaped to specify the client used escaped path.

@bradfitz

bradfitz commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

Sounds good. Or escaped=true or v=2.

@lorneli

lorneli commented Nov 7, 2017

Copy link
Copy Markdown
Contributor Author

I add HTTPPool.parseRequest function to handle escaped=true and add corresponding test case.

Comment thread http.go Outdated

func (p *HTTPPool) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Parse request.
func (p *HTTPPool) parseRequest(r *http.Request) (string, string, bool) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use named result parameters here, otherwise (string, string, bool) is super confusing, especially without docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bradfitz Modified. Thanks for your review.

HTTPPool will inverse escaped path if query parameter contains
"escaped=true".

Add keys with char to be escaped in test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants