code review 6931047: net/url: properly parse file URL with host omitted

net/url: parse file URL with host omitted

Modified both url.Parse and url.String to explicitely account for differences in the file:// scheme
Fixes issue 4189

[erikstmartin, 2012-12-11 20:40:53]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-12-11 22:04:12]

Subject could be more clear:

properly parse file URL with host omitted
On Dec 11, 2012 4:53 PM, <alakriti@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
>       net/url: properly parse url with file scheme and host omitted
>
> Modified both url.Parse and url.String to explicitely account for
> differences in the file:// scheme
> Fixes issue 4189
>
> Please review this at
https://codereview.appspot.**com/6931047/<https://codereview.appspot.com/6931...
>
> Affected files:
>   M src/pkg/net/url/url.go
>   M src/pkg/net/url/url_test.go
>
>
> Index: src/pkg/net/url/url.go
> ==============================**==============================**=======
> --- a/src/pkg/net/url/url.go
> +++ b/src/pkg/net/url/url.go
> @@ -386,7 +386,8 @@
>                 }
>         }
>
> -       if (url.Scheme != "" || !viaRequest) && strings.HasPrefix(rest,
> "//") && !strings.HasPrefix(rest, "///") {
> +       // file scheme can have an empty host, which would appear as ///,
> so it's ignored
> +       if (url.Scheme != "" || !viaRequest) && strings.HasPrefix(rest,
> "//") && (!strings.HasPrefix(rest, "///") || url.Scheme == "file") {
>                 var authority string
>                 authority, rest = split(rest[2:], '/', false)
>                 url.User, url.Host, err = parseAuthority(authority)
> @@ -442,7 +443,8 @@
>         if u.Opaque != "" {
>                 result += u.Opaque
>         } else {
> -               if u.Host != "" || u.User != nil {
> +               // file scheme can have an empty host
> +               if u.Host != "" || u.User != nil || u.Scheme == "file" {
>                         result += "//"
>                         if u := u.User; u != nil {
>                                 result += u.String() + "@"
> Index: src/pkg/net/url/url_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/url/url_test.go
> +++ b/src/pkg/net/url/url_test.go
> @@ -242,6 +242,26 @@
>                 },
>                
"http://www.google.com/?q=go+**language#foo&bar<http://www.google.com/?q=go+language#foo&bar>
> ",
>         },
> +       // file scheme with host
> +       {
> +               "file://localhost/foo",
> +               &URL{
> +                       Scheme: "file",
> +                       Host:   "localhost",
> +                       Path:   "/foo",
> +               },
> +               "file://localhost/foo",
> +       },
> +       // file scheme with host omitted
> +       {
> +               "file:///foo/bar",
> +               &URL{
> +                       Scheme: "file",
> +                       Host:   "",
> +                       Path:   "/foo/bar",
> +               },
> +               "file:///foo/bar",
> +       },
>  }
>
>  // more useful string for debugging than fmt's struct printer
>
>
>

[erikstmartin, 2012-12-12 01:35:54]

I've updated the description as requested. PTAL

Erik

On 2012/12/11 22:04:12, bradfitz wrote:
> Subject could be more clear:
> 
> properly parse file URL with host omitted
> On Dec 11, 2012 4:53 PM, <mailto:alakriti@gmail.com> wrote:
> 
> > Reviewers: http://golang-dev_googlegroups.com,
> >
> > Message:
> > Hello mailto:golang-dev@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go
> >
> >
> > Description:
> >       net/url: properly parse url with file scheme and host omitted
> >
> > Modified both url.Parse and url.String to explicitely account for
> > differences in the file:// scheme
> > Fixes issue 4189
> >
> > Please review this at
>
https://codereview.appspot.**com/6931047/%3Chttps://codereview.appspot.com/69...>
> >
> > Affected files:
> >   M src/pkg/net/url/url.go
> >   M src/pkg/net/url/url_test.go
> >
> >
> > Index: src/pkg/net/url/url.go
> > ==============================**==============================**=======
> > --- a/src/pkg/net/url/url.go
> > +++ b/src/pkg/net/url/url.go
> > @@ -386,7 +386,8 @@
> >                 }
> >         }
> >
> > -       if (url.Scheme != "" || !viaRequest) && strings.HasPrefix(rest,
> > "//") && !strings.HasPrefix(rest, "///") {
> > +       // file scheme can have an empty host, which would appear as ///,
> > so it's ignored
> > +       if (url.Scheme != "" || !viaRequest) && strings.HasPrefix(rest,
> > "//") && (!strings.HasPrefix(rest, "///") || url.Scheme == "file") {
> >                 var authority string
> >                 authority, rest = split(rest[2:], '/', false)
> >                 url.User, url.Host, err = parseAuthority(authority)
> > @@ -442,7 +443,8 @@
> >         if u.Opaque != "" {
> >                 result += u.Opaque
> >         } else {
> > -               if u.Host != "" || u.User != nil {
> > +               // file scheme can have an empty host
> > +               if u.Host != "" || u.User != nil || u.Scheme == "file" {
> >                         result += "//"
> >                         if u := u.User; u != nil {
> >                                 result += u.String() + "@"
> > Index: src/pkg/net/url/url_test.go
> > ==============================**==============================**=======
> > --- a/src/pkg/net/url/url_test.go
> > +++ b/src/pkg/net/url/url_test.go
> > @@ -242,6 +242,26 @@
> >                 },
> >                
>
"http://www.google.com/?q=go+**language#foo&bar<http://www.google.com/?q=go+language#foo&bar>
> > ",
> >         },
> > +       // file scheme with host
> > +       {
> > +               "file://localhost/foo",
> > +               &URL{
> > +                       Scheme: "file",
> > +                       Host:   "localhost",
> > +                       Path:   "/foo",
> > +               },
> > +               "file://localhost/foo",
> > +       },
> > +       // file scheme with host omitted
> > +       {
> > +               "file:///foo/bar",
> > +               &URL{
> > +                       Scheme: "file",
> > +                       Host:   "",
> > +                       Path:   "/foo/bar",
> > +               },
> > +               "file:///foo/bar",
> > +       },
> >  }
> >
> >  // more useful string for debugging than fmt's struct printer
> >
> >
> >

[erikstmartin, 2012-12-12 01:38:09]

PTAL. I've modified the description as requested.

Thanks,
Erik

[rsc, 2012-12-12 03:58:16]

s/properly // too

[erikstmartin, 2012-12-12 04:00:22]

On 2012/12/12 03:58:16, rsc wrote:
> s/properly // too

updated. PTAL

[rsc, 2012-12-12 04:24:16]

Thanks for putting up with the subject bikeshedding.
I think the fix might be too narrowly applied.

https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go
File src/pkg/net/url/url.go (right):

https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go#newco...
src/pkg/net/url/url.go:390: if (url.Scheme != "" || !viaRequest) &&
strings.HasPrefix(rest, "//") && (!strings.HasPrefix(rest, "///") || url.Scheme
== "file") {
I understand that the CL is trying to make sure file:///foo ends up with Path ==
"/foo" and not Path == "///foo".

However, why is file a special case? We still turn https:///foo into Path ==
"///foo. What's different about file? 

I don't understand why this isn't a more general fix, like removing the ///
clause entirely.

https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go#newco...
src/pkg/net/url/url.go:446: // file scheme can have an empty host
Is file:/tmp a valid URL? That's what this code used to produce. Unless it's
invalid, we should preserve the existing behavior. I would support
|| strings.HasPrefix(u.Path, "//") instead of || u.Scheme == "file".

[erikstmartin, 2012-12-12 04:32:30]

No worries, i'll pickup the formatting etc. of these types of things you guys
like along the way.

I considered that it might be too specific as well, doing a check for file, but
it turns out that each scheme can define it's own idea of whether an empty
host/authority is acceptable, or defaulted.

https://code.google.com/p/go/issues/detail?id=4271 is a prime example, for http
and https schemes a missing host is considered invalid. I plan on submitting a
patch to account for that case as well once a decision has been made if
url.Parse should return an error, or if the caller is responsible for checking
for an empty host.

On 2012/12/12 04:24:16, rsc wrote:
> Thanks for putting up with the subject bikeshedding.
> I think the fix might be too narrowly applied.
> 
> https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go
> File src/pkg/net/url/url.go (right):
> 
>
https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go#newco...
> src/pkg/net/url/url.go:390: if (url.Scheme != "" || !viaRequest) &&
> strings.HasPrefix(rest, "//") && (!strings.HasPrefix(rest, "///") ||
url.Scheme
> == "file") {
> I understand that the CL is trying to make sure file:///foo ends up with Path
==
> "/foo" and not Path == "///foo".
> 
> However, why is file a special case? We still turn https:///foo into Path ==
> "///foo. What's different about file? 
> 
> I don't understand why this isn't a more general fix, like removing the ///
> clause entirely.
> 
>
https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go#newco...
> src/pkg/net/url/url.go:446: // file scheme can have an empty host
> Is file:/tmp a valid URL? That's what this code used to produce. Unless it's
> invalid, we should preserve the existing behavior. I would support
> || strings.HasPrefix(u.Path, "//") instead of || u.Scheme == "file".

[erikstmartin, 2012-12-12 14:00:26]

Does this raise a bigger question between this issue and the other of whether or
url.Parse should be ok with ///, and it's up to the caller to determine whether
the empty host/authority is valid for that particular scheme? do we put clauses
in for specific schemes, or do we parse everything we can, then return that
along with possibly an error specific to the scheme that they could choose to
ignore?

I'm happy to modify this patch to go with popular opinion.

On 2012/12/12 04:32:30, erikstmartin wrote:
> No worries, i'll pickup the formatting etc. of these types of things you guys
> like along the way.
> 
> I considered that it might be too specific as well, doing a check for file,
but
> it turns out that each scheme can define it's own idea of whether an empty
> host/authority is acceptable, or defaulted.
> 
> https://code.google.com/p/go/issues/detail?id=4271 is a prime example, for
http
> and https schemes a missing host is considered invalid. I plan on submitting a
> patch to account for that case as well once a decision has been made if
> url.Parse should return an error, or if the caller is responsible for checking
> for an empty host.
> 
> On 2012/12/12 04:24:16, rsc wrote:
> > Thanks for putting up with the subject bikeshedding.
> > I think the fix might be too narrowly applied.
> > 
> > https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go
> > File src/pkg/net/url/url.go (right):
> > 
> >
>
https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go#newco...
> > src/pkg/net/url/url.go:390: if (url.Scheme != "" || !viaRequest) &&
> > strings.HasPrefix(rest, "//") && (!strings.HasPrefix(rest, "///") ||
> url.Scheme
> > == "file") {
> > I understand that the CL is trying to make sure file:///foo ends up with
Path
> ==
> > "/foo" and not Path == "///foo".
> > 
> > However, why is file a special case? We still turn https:///foo into Path ==
> > "///foo. What's different about file? 
> > 
> > I don't understand why this isn't a more general fix, like removing the ///
> > clause entirely.
> > 
> >
>
https://codereview.appspot.com/6931047/diff/3003/src/pkg/net/url/url.go#newco...
> > src/pkg/net/url/url.go:446: // file scheme can have an empty host
> > Is file:/tmp a valid URL? That's what this code used to produce. Unless it's
> > invalid, we should preserve the existing behavior. I would support
> > || strings.HasPrefix(u.Path, "//") instead of || u.Scheme == "file".

[bradfitz, 2012-12-12 21:29:36]

On Wed, Dec 12, 2012 at 6:00 AM, <alakriti@gmail.com> wrote:

> Does this raise a bigger question between this issue and the other of
> whether or url.Parse should be ok with ///, and it's up to the caller to
> determine whether the empty host/authority is valid for that particular
> scheme? do we put clauses in for specific schemes, or do we parse
> everything we can, then return that along with possibly an error
> specific to the scheme that they could choose to ignore?
>

Go 1 already allowed successful parses of http URLs without Hosts:

     http://play.golang.org/p/fur6PZdZXu

url.Parse("http:///path") -> &url.URL{Scheme:"http", Opaque:"",
User:(*url.Userinfo)(nil), Host:"", Path:"///path", RawQuery:"",
Fragment:""}

So I think it's fine to keep doing that.  Let's just fix the Path to be
correct for all schemes (file, http(s), etc).

I've made the error message for HTTP readable in
http://code.google.com/p/go/source/detail?r=61e3278a earlier today.

[erikstmartin, 2012-12-19 15:24:27]

On 2012/12/12 21:29:36, bradfitz wrote:
> On Wed, Dec 12, 2012 at 6:00 AM, <mailto:alakriti@gmail.com> wrote:
> 
> > Does this raise a bigger question between this issue and the other of
> > whether or url.Parse should be ok with ///, and it's up to the caller to
> > determine whether the empty host/authority is valid for that particular
> > scheme? do we put clauses in for specific schemes, or do we parse
> > everything we can, then return that along with possibly an error
> > specific to the scheme that they could choose to ignore?
> >
> 
> Go 1 already allowed successful parses of http URLs without Hosts:
> 
>      http://play.golang.org/p/fur6PZdZXu
> 
> url.Parse("http:///path") -> &url.URL{Scheme:"http", Opaque:"",
> User:(*url.Userinfo)(nil), Host:"", Path:"///path", RawQuery:"",
> Fragment:""}
> 
> So I think it's fine to keep doing that.  Let's just fix the Path to be
> correct for all schemes (file, http(s), etc).
> 
> I've made the error message for HTTP readable in
> http://code.google.com/p/go/source/detail?r=61e3278a earlier today.

Ok, so the approach we're looking for here is to just allow the /// for all
schemes and leave host empty, and put it on the implementor to determine whether
the empty host is invalid or not?

[bradfitz, 2012-12-19 16:47:15]

On Wed, Dec 19, 2012 at 7:24 AM, <alakriti@gmail.com> wrote:

> On 2012/12/12 21:29:36, bradfitz wrote:
>
>  On Wed, Dec 12, 2012 at 6:00 AM, <mailto:alakriti@gmail.com> wrote:
>>
>
>  > Does this raise a bigger question between this issue and the other
>>
> of
>
>> > whether or url.Parse should be ok with ///, and it's up to the
>>
> caller to
>
>> > determine whether the empty host/authority is valid for that
>>
> particular
>
>> > scheme? do we put clauses in for specific schemes, or do we parse
>> > everything we can, then return that along with possibly an error
>> > specific to the scheme that they could choose to ignore?
>> >
>>
>
>  Go 1 already allowed successful parses of http URLs without Hosts:
>>
>
>       
http://play.golang.org/p/**fur6PZdZXu<http://play.golang.org/p/fur6PZdZXu>
>>
>
>  url.Parse("http:///path") -> &url.URL{Scheme:"http", Opaque:"",
>> User:(*url.Userinfo)(nil), Host:"", Path:"///path", RawQuery:"",
>> Fragment:""}
>>
>
>  So I think it's fine to keep doing that.  Let's just fix the Path to
>>
> be
>
>> correct for all schemes (file, http(s), etc).
>>
>
>  I've made the error message for HTTP readable in
>>
http://code.google.com/p/go/**source/detail?r=61e3278a<http://code.google.com...
today.
>>
>
> Ok, so the approach we're looking for here is to just allow the /// for
> all schemes and leave host empty, and put it on the implementor to
> determine whether the empty host is invalid or not?
>

Yes.

Unless you find some case where we were stricter in Go 1 for some scheme,
in which case we should remain strict for that scheme.

But if it's just the case that we allowed some input in Go 1 but returned
something broken, we can be more correct now.

[adg, 2013-01-17 23:28:49]

Work continues here: https://codereview.appspot.com/7135051

[gobot, 2013-12-20 16:21:44]

Replacing golang-dev with golang-codereviews.

[bradfitz, 2013-12-26 20:44:20]

R=close

Looks like this has been fixed otherwise in the meantime.