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 URL.Clone method #41733

Closed
icholy opened this issue Oct 1, 2020 · 18 comments
Closed

proposal: net/url: add URL.Clone method #41733

icholy opened this issue Oct 1, 2020 · 18 comments

Comments

@icholy
Copy link

icholy commented Oct 1, 2020

Copying a url is often done by shallow copying the struct. This can be confusing because it's not clear if the Userinfo should also be copied.

u, _ := url.Parse("http://foo")

// shallow copy the struct
u2 := *u

Another option is to format/re-parse the url. But this adds unnecessary overhead.

u2, _ := url.Parse(u.String())

Given how often url cloning comes up, I think it warrants adding a helper.

@icholy
Copy link
Author

icholy commented Oct 1, 2020

Related #38351

@andybons andybons changed the title net/url: add URL.Clone method proposal: net/url: add URL.Clone method Oct 1, 2020
@gopherbot gopherbot added this to the Proposal milestone Oct 1, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 14, 2020
@ianlancetaylor
Copy link
Contributor

What new API do you suggest?

@ianlancetaylor
Copy link
Contributor

That is, what is the exact definition of the new Clone method?

@icholy
Copy link
Author

icholy commented Oct 14, 2020

@ianlancetaylor

// Clone returns a copy of u.
func (u *URL) Clone() *URL {
    u2 := *u
    return &u2
}

@rsc
Copy link
Contributor

rsc commented Oct 14, 2020

I'm confused about this. This operation would apply to many structs where you want to make a change to one field to produce a new copy. This ends up being a common idiom in Go. Why is URL special? Why does it merit a special method?

You wrote:

Given how often url cloning comes up, I think it warrants adding a helper.

But honestly I have not seen it come up often at all, which would warrant not adding a helper.

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 14, 2020
@icholy
Copy link
Author

icholy commented Oct 14, 2020

@rsc

Why is URL special? Why does it merit a special method?

The Userinfo field makes it unclear if the u2 := *u approach is correct.

But honestly I have not seen it come up often at all, which would warrant not adding a helper.

I have no empirical evidence to support my statement. However, I like to use this type of pattern:

// will usually come from configuration
base, _ := url.Parse("http://foo/bar")

func DoReq() error {
  u := base.Clone()
  u.Path = path.Join(u.Path, "my/route")

  // use the url to make a request
  // ...
}

@earthboundkid
Copy link
Contributor

I also end up making modifications to a base URL (e.g. here). Maybe it's enough just to document that URLs can be shallow copied safely?

@rsc
Copy link
Contributor

rsc commented Oct 16, 2020

@carlmjohnson "Can be shallow copied safely" is true of many types, not just URLs. But also, URLs can be mutated safely too, if you own the URL. Just like most types.

@earthboundkid
Copy link
Contributor

@carlmjohnson "Can be shallow copied safely" is true of many types, not just URLs.

Yes but as @icholy said originally,

This can be confusing because it's not clear if the Userinfo should also be copied.

You have to dig into the documentation for UserInfo to see that it’s immutable.

@rsc
Copy link
Contributor

rsc commented Oct 20, 2020

You have to dig into the documentation for UserInfo to see that it’s immutable.

But the reason to make a shallow copy is to get a local copy you can mutate. If you are not going to modify the UserInfo in the copy then you don't need to dig into the documentation.

@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

It seems like there are two concerns with this proposal:

  1. How often is this really needed? If not much, then we shouldn't add new API.
  2. When it is needed, why isn't doing a u2 := *u, modify u2 the answer that people should reach for? Why is a new method warranted? (Are there uses other than preparing to modify a URL?)

@ainar-g
Copy link
Contributor

ainar-g commented Oct 21, 2020

Regarding the first question, I've definitely seen the “base URL clone plus path” pattern described above in the wild. As for the second, perhaps this is a documentation issue, and we just need to add an example of doing that to the docs?

@icholy
Copy link
Author

icholy commented Oct 22, 2020

I'd be happy with an example or a note in the documentation.

@rsc
Copy link
Contributor

rsc commented Oct 28, 2020

I still don't understand what exactly is worth calling out in URL's documentation.
It is a general property of data that if you want to make a copy before mutating you can do u2 := *u; mutate u2.

And I still don't understand how often this operation is needed on URLs.
@ainar-g has seen it, but that establishes existence not frequency.

@rsc
Copy link
Contributor

rsc commented Oct 28, 2020

Put another way, if we decide to add net.URL.Clone, what other types will we need to add Clone to?
It kind of seems like we'd need to add it to almost any type in the standard library.
Is there something special about URL that I am missing?

@earthboundkid
Copy link
Contributor

earthboundkid commented Oct 29, 2020

Speaking for myself, I’m happy not to have a Clone() method, but just noting that even though UserInfo is a pointer, it’s immutable so you don’t have to worry about it being mutated by someone else holding the original url.URL.

@wader
Copy link

wader commented Oct 29, 2020

// will usually come from configuration
base, _ := url.Parse("http://foo/bar")

func DoReq() error {
  u := base.Clone()
  u.Path = path.Join(u.Path, "my/route")

  // use the url to make a request
  // ...
}

For me in most these cases what i actually want is to resolve a new URL. So in this case if I understand your problem correctly I would do

base, _ := url.Parse("http://foo/bar/")
u := base.ResolveReference(&url.URL{Path: "my/route"})

Maybe a bit cumbersome but I like that it makes it very explicit what is going on.

@icholy
Copy link
Author

icholy commented Oct 29, 2020

@wader I don't like ResolveReference because it requires the base url to have a trailing slash. https://play.golang.org/p/oka6MJ1674i

@rsc IMO the ambiguity around UserInfo is the "special" thing about it. However, I agree that the Clone method doesn't have a precedent here. I'm closing this issue in favour of #38351

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants