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

net/url: Parse+String does not round trip magnet URLs #20054

Closed
rsc opened this issue Apr 20, 2017 · 9 comments
Closed

net/url: Parse+String does not round trip magnet URLs #20054

rsc opened this issue Apr 20, 2017 · 9 comments
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Apr 20, 2017

I received a private email with a report that Go inserts // in magnet URLs.
That appears to mean https://en.wikipedia.org/wiki/Magnet_URI_scheme.
Round-tripping the example from that wikipedia page:

package main

import (
	"fmt"
	"net/url"
)

func main() {
	fmt.Println(url.Parse(`magnet:?xt=urn:btih:c12fe1c06bba254a9dc9f519b335aa7c1367a88a&dn`))
}

prints

magnet://?xt=urn:btih:c12fe1c06bba254a9dc9f519b335aa7c1367a88a&dn <nil>

https://play.golang.org/p/OQKMvDyTD-

It does seem like the leading // should not be there. However, I don't know how much this matters, nor how invasive this would be to fix, nor whether the // insertion is correct for any other schemes (in particular, http and https).

/cc @bradfitz

@rsc rsc added this to the Go1.9Maybe milestone Apr 20, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 20, 2017

We don't special case mailto: so it'd be nice to not special case magnet:.

But perhaps we could tweak our heuristics for URL.String's Opaque-vs-:// forms.

@rsc
Copy link
Contributor Author

@rsc rsc commented Apr 20, 2017

Hmm, so mailto:rsc?subject=hi round-trips but mailto:?subject=hi turns into mailto://?subject=hi.

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 20, 2017

Part of the problem is that we parse the Opaque field out as the empty string. When we try to round trip, we notice that the Opaque field is empty and assume it's a hierarchical URL, which it is not. Perhaps, we should add a ForceOpaque bool field?

@rsc
Copy link
Contributor Author

@rsc rsc commented Apr 20, 2017

I would rather not add a new bool.

If u.User == nil && u.Host=="" && u.Path=="", can we print that part as an empty string instead of as //?

@johnnyluo
Copy link
Contributor

@johnnyluo johnnyluo commented Jul 15, 2017

I am interested to work on this one, can someone assign it to me? so other people won't waste time on this

@dsnet dsnet assigned dsnet and unassigned dsnet Jul 15, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 16, 2017

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

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jul 19, 2017

Moving this to Go1.10 as per the CL update and given how deep we are in the freeze Go1.9.

@odeke-em odeke-em added this to the Go1.10 milestone Jul 19, 2017
@odeke-em odeke-em removed this from the Go1.9Maybe milestone Jul 19, 2017
@johnnyluo
Copy link
Contributor

@johnnyluo johnnyluo commented Aug 29, 2017

@dsnet @odeke-em could you guys please help to run trybot on CL https://golang.org/cl/49050, the previous trybot failed , however it is nothing to do with my change.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 29, 2017

Thanks for the ping @johnnyluo, I've ran the trybot.

@golang golang locked and limited conversation to collaborators Aug 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants