proposal: net/url: add AppendQuery

[btoews]

It's pretty easy to accidentally mutate a shared URL struct when trying to create a URL with query parameters:

var redirectURL, _ = url.Parse("https://example.com")

func main() {
	http.ListenAndServe("", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		u := redirectURL
		q := u.Query()

		// opaque process that might fail
		token, err := getRedirectToken()
		if err != nil {
			q.Set("error", err.Error())
		} else {
			q.Set("token", token)
		}

		u.RawQuery = q.Encode()

		http.Redirect(w, r, u.String(), http.StatusFound)
	}))
}

APIs to help with constructing query strings and adding them to a URL — a la (*URL) JoinPath(elems ...string) *URL — could prevent this:

func (u *URL) AppendQuery(values Values) *URL {
	q := u.Query()	
	for k, v := range values {
		q[k] = append(q[k], v...)
	}

	ret := *u
	ret.RawQuery = q.Encode()

	return &ret
}

[earthboundkid]

This is a pretty common need, but should it be append or set?

// WithValues returns a copy of u with the keys and values in vv set on its query string.
func (u *URL) WithValues(vv Values) *URL {
	q := u.Query()	
	for k, v := range vv {
		q[k] = v
	}

	ret := *u
	ret.RawQuery = q.Encode()

	return &ret
}

I think the main argument against adding this is that people need different variations on this in different circumstances, so the easiest thing is for people to just keep doing it by hand each time. But probably setting Values covers the 80% case, so it might be worth it…

[btoews]

👍 I agree that that's the more common use case. I was thinking append would be more versatile, but versatility isn't necessarily a good thing.

[FiloSottile]

Kinda surprised we don't have a Clone() method either.

I like WithValues because it implies returning a copy.

Are there other parts of the URL we should give the same treatment to? For example, WithPath?

/cc @golang/security since the example shows a pretty scary security footgun. It's not a vulnerability in the standard library but we can probably make safer APIs.

[FiloSottile]

By the way, the race detector would catch this if there was a concurrent test. Those are not easy to write but often catch bugs especially with the race detector.

Moreover, I wonder if @dominikh's staticcheck could try to flag any global write in an http.Handler, which is by nature concurrent. (Maybe it's too hard to ignore mutex-guarded values?)

[btoews]

Are there other parts of the URL we should give the same treatment to? For example, WithPath?

Any part of the URL could probably benefit from this. There's already JoinPath, but having a helper to replace the entire path would be reasonable too.

[jba]

I have no particular dog here, but I would like to point out a couple of things.

This is the URL type:

type URL struct {
	Scheme      string
	Opaque      string    // encoded opaque data
	User        *Userinfo // username and password information
	Host        string    // host or host:port (see Hostname and Port methods)
	Path        string    // path (relative paths may omit leading slash)
	RawPath     string    // encoded path hint (see EscapedPath method)
	OmitHost    bool      // do not emit empty host (authority)
	ForceQuery  bool      // append a query ('?') even if RawQuery is empty
	RawQuery    string    // encoded query values, without '?'
	Fragment    string    // fragment for references, without '#'
	RawFragment string    // encoded fragment hint (see EscapedFragment method)
}

1. Since Userinfo is immutable, simply copying a URL suffices to clone it (to make a copy that shares no writeable memory). Of course that might not always be true, so a Clone method might still be worthwhile. But it isn't needed now.
2. Since every field is public, there doesn't seem to be much point in providing safe ways to modify one or two of them. Maybe it's better to start over with an immutable type.

[icholy]

URL.Clone was proposed here #41733

[earthboundkid]

URL already has JoinPath. I don’t think modifying host or scheme is common enough to need a helper. Modifying query parameters is a frequent need though. Maybe call it JoinValue so it rhymes with JoinPath?