net/url: url.RawQuery = url.Query().Encode() can change the URL

[tombergan]

In the following code:

u, _ := url.Parse("http://www.example.com/path?foo")
fmt.Println(u)
u.RawQuery = u.Query().Encode()
fmt.Println(u)

The third line actually changes the URL. The output is:

http://www.example.com/path?foo
http://www.example.com/path?foo=

A strict reading of RFC 3986 Section 6 suggests that these are two different URLs even after applying the (optional) syntax-based normalizations described in Section 6.2.2. I know of some servers that consider these URLs different. For example, compare the following links:

http://www.vcfed.org/forum/forumdisplay.php?23-DEC
http://www.vcfed.org/forum/forumdisplay.php?23-DEC=

I consider this a bug, but I doubt we could fix this bug in a backwards-compatible way. At least, I think we should update the documentation to make it clear that url.Query().Encode() can change query param "foo" to "foo=", which may be considered a different URL by some servers.

[bradfitz]

Well, they do parse as different URLs:

https://play.golang.org/p/7xC_QxjGyI

But maybe the (*URL).Query() method could return different values?

[tombergan]

Additionally, url.Query().Encode() will percent-encode query parameters that were not previously encoded, which some servers don't like. One example:

http://www.dayonepatch.com/index.php?/topic/135060-data-saver-and-d1p/
http://www.dayonepatch.com/index.php?%2Ftopic%2F135060-data-saver-and-d1p%2F=

This may be fixable by adding url.Value.RawEncode()? Or again, this may just be something to document more clearly.

[bradfitz]

Maybe we should back up. What is the problem?

This bug is bordering on "Doctor, it hurts if I do this..."

That is, don't modify the RawQuery if the RawQuery was correct to begin with.

What is your use case?

[tombergan]

The use case was adding/removing query params from a URL. We needed to do this without changing any of the existing query params. We're fixing this a different way, so I don't need a code change in the library, but I think the methods should document the behavior more clearly, particularly in the first example, which I think is surprising.

[bradfitz]

Gotcha, thanks.

[quentinmit]

I don't think there's anything to do here. url.Query() unfortunately only works for key=value query strings; isindex-style query strings have to be implemented using RawQuery.

[tombergan]

I agree that a code change isn't needed, but I think the godoc could be clarified. URL.Query says that it "parses RawQuery" but doesn't say anything about formatting expectations. Something like:

// Query parses RawQuery and returns the corresponding values. It assumes
// RawQuery is a list of key=value pairs separated by '&' or ';'. If RawQuery
// has a different form, the return value is undefined.
func (u *URL) Query() Values

[gopherbot]

CL https://golang.org/cl/31331 mentions this issue.