net/url: empty query value is dropped on Encode()

[carl-mastrangelo]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9

https://play.golang.org/p/FWtvu_9XEM

The Values.Encode function adds an unnecessary = to the end of kv pairs, even if the original value was empty. When using flag style url query values, this is kind of annoying because it is supposed to be either present or absent.

It would be nice to interpret a url.Values with nil value slice as a flag, and skip the = for encoding.

[gopherbot]

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

[carl-mastrangelo]

To clarify, there are two related issues going on: ParseQuery and Encode don't round-trip the string. I don't think this is technically possible (we don't know if = should be kept), but it is possible to pick a better lossy translation.

Trying to work around this behavior by explicitly setting the value to nil:

v := make(url.Values)
v["prev"] = nil

doesn't produce any output.

[bradfitz]

We probably can't change the v["prev"] = nil behavior to start generating output. That would probably break enough people. A lot of code & tests have accumulated over 5 years depending on every quirk of our implementations.

It would be nice to interpret a url.Values with nil value slice as a flag, and skip the = for encoding.

Using nil or even a sentinel []string value would be unable to represent ?foo&foo&foo: https://play.golang.org/p/nO_OlSCiCE

You'd almost need a sentinel string value, but strings in Go are compared by value, which means we'd need to make up some magic value like:

package url

// EmptyValue is blah blah no equals sign.
const EmptyValue = "\x00\x00"

But then that precludes any user from actually using that value.

We did something like this for https://golang.org/pkg/net/http/#TrailerPrefix but TrailerPrefix worked because a valid value can't contain a colon, so we have a safe value to use.

With URL parameter values, all values are unacceptable.

[carl-mastrangelo]

Perhaps an empty sentinel array? It would be hacky no doubt. Something like:

var sentinel [0]string

func EmptyValue() []string {
  return sentinel[:]
}

Then, in Values.Encode, it could check if the address of slice[0] matches the address of sentinel. It wouldn't be able to catch the multiple 'foo&foo&foo' case, but since this PR treats it as a flag, just setting it once is the important case. Thoughts?

[bradfitz]

Again, that doesn't help with https://play.golang.org/p/nO_OlSCiCE

[carl-mastrangelo]

Depending on what level of Hacks are passible, I could imagine creating special strings with a prefix, and then slicing them to put in url.Values, When encoding, reflect and unsafe could be used to look for the sentinel prefix to know if its a special empty:

https://play.golang.org/p/hbj9XZEDEl

[bradfitz]

Sorry, we will not stoop to that level.

[carl-mastrangelo]

I don't think that's stooping. If there is a valid technical reason for disallowing it that would be an acceptable answer. The core of the problem is url.URL is publicly defined to be a map. Without defining a new type, or special parameters, the only remaining solution is to modify behavior. That is what the original CL does.

That said, supported foo&foo&foo is not a primary goal of the CL. Only that specific by more common problem needs a solution.

[bradfitz]

I don't think that's stooping.

You may not, but I do, and so would most of the Go team. We avoid unsafe except in very low-level places. The net/url package is not low-level and we don't want to make it be low-level by using unsafe.

That said, supported foo&foo&foo is not a primary goal of the CL. Only that specific by more common problem needs a solution.

It seems weird to support x=y&foo but not foo&foo. I'd prefer to be consistent, even if that meant the current foo=&foo= behavior.

[carl-mastrangelo]

Ok, expanding the scope of solutions. Please rank these in the most acceptable order:

1. Declare that this issue is not a real problem, ignore it (The "ostrich" algorithm)
2. Fork net/url, put it in x/net/url, which causes package proliferation
3. Overload Encode to accept a func(p string) bool, that decides if it should be empty or not.
4. Overload Encode to accept a func(ps []string) []bool, to allow deciding particular empties (like foo&foo=&foo
5. Add a BetterValues, which is a [][2]*string that can accurately describe a query.
6. Modify Encode to interpret nil or empty slices as an indicator that it should not be present
7. Modify URL to have a ForceEmptyParam which changes how it encodes. There is precedent for adding such booleans to URL.
8. Other?

3, 4, and 5 are API creep, 1 is not a real solution, because one of the other will take its place.

[bradfitz]

Commentary:

1. This hasn't been a problem for 5 years. I'm a little surprised it suddenly is. Which applications require ?foo without an = sign?
2. We've stopped expanding x/*. See proposal: x: decide policy for sub-repositories #17244.
3. We can't overload Encode. That's an API change.
4. We can't overload Encode. That's an API change.
5. Gross.
6. Ideal (not changing API), if we figure out which way causes the least breakage in the community.
7. Maybe. But force which? All? And that only helps (*URL).String, not https://golang.org/pkg/net/url/#Values.Encode, which is probably okay.
8. Put it on github.com/carl-mastrangelo/carlurl

[carl-mastrangelo]

1. Eliding the = makes for shorter urls. This is a good thing. I'm actually kind of surprised this isn't possible today or even tested for, given how many other good API decisions were made in net/.

You can see why I picked option 6 CL, because it was the best.

8. is not feasible. By making my own, I force it to be on my API surface, just like Values is on URL, which is itself in http and elsewhere.

Looking at the Go 1 Compatibility promise, this appears to be undefined behavior that I am proposing to change. If some very small number of tests break, can we proceed with the original change?

[bradfitz]

We'll discuss at a @golang/proposal-review meeting.

Maybe @dsnet could test the change inside Google (a pretty large codebase) and run all those tests and see how much breaks.