Base path changes when it shouldn't - url.path = base.path only copies pointer value

[jimsmart]

Hi,

First of all: great package, and nicely written, thanks! :)

My use case is needing a URL parser that preserves an empty fragment # during parsing, if present at the end of the URL — and the Go stdlib url package does not do this, it discards it, and URLs do not round-trip back to a string in the manner I require. And your implementation here doesn't do this, it strictly honours the input values.

— But I have found a bug...!

If I do this:

	baseURL := "http://example.org/ns/foo"
	p := url.NewParser()
	base, _ := p.Parse(baseURL)
	u, _ := base.Parse("bar#")

It causes the base URL to change its path. Which is not intended behaviour.

A deeper exploration of this, plus a test (at the bottom of the code) that you can probably copy paste into an existing test file with minimal edits, can be found here: https://go.dev/play/p/Vr-NlSqzZcd

I've taken a look at the code, and understand where and why this is happening — you have a few of these throughout:

	url.path = base.path // TODO: Ensure copy????

...which, based upon the comment there, you already realise it might not do quite do what is intended.

Here, because URL.path is defined as *path, the value of path itself is not copied in this assignment — instead, the pointer value of url.path is set to the same pointer value as base.path, and because they are pointers, as a result both url.path and base.path then point to the same instance of the original path struct instance.

What is needed here is a copy of the original path struct, and it needs to be a deep copy, not a shallow one, assuming the internal p might get updated at any time.

I think the cleanest fix here, is perhaps to add the following method to path:

func (p *path) DeepCopy() *path {
	return &path{p: append([]string{}, p.p...), opaque: p.opaque}
}

— Which ensures that a new path instance is returned, and that that new instance also has its own copy of the internal p []string slice also.

And then to do this whenever a path needs copying:

	url.path = base.path.DeepCopy() // Ensures copy!

But perhaps you have your own solution. Or maybe I am missing something here.

And perhaps you'd prefer to call the method Copy instead of DeepCopy

Happy to make this change, and submit a pull request, if that is preferable to you?

Thanks for your time, and attention.
Regards,
/Jim

[jimsmart]

Looking at the rest of the Url struct, I see searchParams *SearchParams

This too will behave in the same way as the above, when copied via a simple assignment — both pointers will point to the same instance of SearchParams.

— So perhaps when this is copied it also should make a (deep) copy of the SearchParams instance that is being pointed too?

[jimsmart]

Re my initial comment:

func (p *path) DeepCopy() *path {

This could be deepCopy instead, because it probably does not need to be public at all.