Use url.QueryEscape instead of url.PathEscape to escape query string

[pankona]

Fixes #1122.

• Quit replacing white space with "+"
• Use url.QueryEscape instead of url.PathEscape to escape query string

[gmlewis]

Thank you, @pankona.

Can you please take a look at #966 and #963 for me?
I obviously didn't fix the original problem correctly since it is still an issue.
Yet the original code used this (see https://github.com/google/go-github/blame/fbd7fb9c7f3d27aa691e8421690e00be916f8d8d/github/search.go):

import (
	qs "github.com/google/go-querystring/query"
)
...
	params, err := qs.Values(opt)
	if err != nil {
		return nil, err
	}
	params.Set("q", parameters.Query)
	if parameters.RepositoryID != nil {
		params.Set("repository_id", strconv.FormatInt(*parameters.RepositoryID, 10))
	}
	u := fmt.Sprintf("search/%s?%s", searchType, params.Encode())

and I'm wondering if the correct fix is to make the changes in the upstream go-querystring/query package instead?

Thoughts?

[pankona]

@gmlewis

I've checked #966 and #963 .
I've pushed a new commit 6bc0bca . I think go-querystring has no problem. Could you please check current patch is as you expected?

[gmlewis]

Thank you, @pankona! I'll have to take time and look at this tonight... gotta run... but in a nutshell, was #966 just broken? I'm curious how this current PR differs from the original, but don't have time right now to look... I just know there was a problem in #963 that needed fixing and hope that this addresses that issue as well.

[pankona]

@gmlewis

I've checked snippet on #963 (as follows)

rs, _, err := client.Search.Repositories(context.Background(),
		"language:golang+stars:>=200+pushed:>=2018-01-01",
		&github.SearchOptions{Sort: "stars", Order: "desc",
				ListOptions: github.ListOptions{Page: 2, PerPage: 100}})

This query string includes "+" (instead of white space). I think this is NOT acceptable query string.

• "language:golang+stars:>=200+pushed:>=2018-01-01" doesn't work with GitHub's search input form
• According to GitHub's search API documentation, there're some examples that include "+" instead of white space, I think the white spaces are already escaped.
• I think the query string written above should be "language:golang stars:>=200 pushed:>=2018-01-01"

[gmlewis]

This query string includes "+" (instead of white space). I think this is NOT acceptable query string.

...although it worked when using curl... does your recommended change also work against the GitHub v3 API? If so, then I'm happy to incorporate your solution.

But are you saying here that the original code was correct?

[pankona]

@gmlewis

But are you saying here that the original code was correct?

Yes. I think original code was correct.

In my understanding, escaping of query string is responsibility of go-github itself, but not of library user. Thus a query string that includes "+" instead of white space may return unexpected result.

curl

• Here's curl command using "+" instead of white spaces (got OK)

curl -X GET "https://api.github.com/search/repositories?order=desc&page=2&per_page=100&q=language%3Agolang+stars%3A%3E%3D200+pushed%3A%3E%3D2018-01-01&sort=stars"

• Here's curl command using white spaces directly (got error)

curl -X GET "https://api.github.com/search/repositories?order=desc&page=2&per_page=100&q=language%3Agolang stars%3A%3E%3D200 pushed%3A%3E%3D2018-01-01&sort=stars"

In case of using curl, escaping of query string is responsibility of user. Thus former case goes okay.

[gmlewis]

Thank you for the analysis, @pankona! It is greatly appreciated!

@gauntface - do you agree?

TL;DR: #966 was the wrong fix for #963 - the query in #963 was malformed. This PR goes back to the original implementation.

[gauntface]

Ok, my understanding from everything is that this is the correct change.

Forcing users to not do this:

client.Search.Repositories(ctx, "language:golang+stars:>=200+pushed:>=2018-01-01", ...)

And instead do this:

client.Search.Repositories(ctx, "language:golang stars:>=200 pushed:>=2018-01-01", ...)

I like this approach just from the perspective of being close to the input you would provide to the Github.com search box.

Regarding this PR, @pankona could you add a comment to the search() method calling out the example input of " " vs "+" and highlight that we expect " ".

[pankona]

@gauntface
Thank you for checking!

comment

okay, I will.

[gmlewis]

One tiny nit, then LGTM.
Sorry I missed that before.
Thank you, @pankona!

Awaiting second LGTM before merge.

[gmlewis]

nit: please remove leading * as we typically do not put these for pointers in Godocs.

[pankona]

Thanks! fixed on 63ab8d9

[gmlewis]

Great! Thank you, @pankona!

One more quick request since @gauntface hasn't reviewed yet, please...

Could you please add language:c++ to line 118 in search_test.go just to make your comment super-clear in the test results for how it will look?

[pankona]

@gmlewis

understood, I will!

[pankona]

@gmlewis

At b94a2b5, I introduced c++ in unit tests to make escape result more clear. How about it?

[gmlewis]

Perfect, @pankona ! Thank you!

Awaiting @gauntface LGTM before merging.

[pankona]

Gently ping to @gauntface :)

[gauntface]

Sorry for the delayed LGTM. This looks great. Thank you so much for the change 🎉

[pankona]

Thank you @gauntface for your confirmation!

@gmlewis

If there's no concern for merging, could you please proceed?

[gmlewis]

Thank you, @gauntface!
Merging.