Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: net/url: add AppendQuery #63777

Open
btoews opened this issue Oct 27, 2023 · 8 comments
Open

proposal: net/url: add AppendQuery #63777

btoews opened this issue Oct 27, 2023 · 8 comments
Labels
Milestone

Comments

@btoews
Copy link

btoews commented Oct 27, 2023

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
}
@gopherbot gopherbot added this to the Proposal milestone Oct 27, 2023
@earthboundkid
Copy link
Contributor

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
Copy link
Author

btoews commented Oct 27, 2023

👍 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

btoews commented Oct 27, 2023

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
Copy link
Contributor

jba commented Oct 27, 2023

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
Copy link

icholy commented Oct 27, 2023

URL.Clone was proposed here #41733

@earthboundkid
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

6 participants