code review 7203059: net/url: better path resolution

net/url: better path resolution

This includes a simplified resolvePath function and tests for all normal and abnormal path resolution examples described in RFC 3986, sections 5.4.1 and 5.4.2 [1]. Some of those examples failed before (see http://play.golang.org/p/F0ApSaXniv).

Also, parsing a reference "//foo" now works as expected. It was treated as an absolute path with very weird results (see http://play.golang.org/p/089b-_xoNe).

During path resolution, all dot segments are removed as described by the RFC.

A few existing tests had to be changed because they expected the wrong output.

Fixes issue 4700.

Fixes issue 4706.

[1] http://tools.ietf.org/html/rfc3986#section-5.4.1

[rsc, 2013-01-31 17:11:43]

This CL was not created using the standard hg code review tools. Please do that.

[rodrigo.moraes, 2013-01-31 17:38:21]

Hello rsc@golang.org, adg@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

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

[rodrigo.moraes, 2013-01-31 17:58:32]

On 2013/01/31 17:11:43, rsc wrote:
> This CL was not created using the standard hg code review tools. Please do
that.

Now it is, sorry. I initially created it using the standard tool, but was
confused about how to send new patch sets and ended using the codereview default
script.

[adg, 2013-02-04 01:33:40]

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

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:598: return base
return ""
(just to be really explicit)

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:604: for k, v := range src {
s/k/i/
(it's an index, not a key)

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go
File src/pkg/net/url/url_test.go (right):

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:633: t.Errorf("Didn't expect an error from Parse
wrapper parsing an empty string.")
t.Errorf("unexpected error parsing empty string: %v", err)

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:684: t.Errorf("Didn't expect an error from Parse
wrapper parsing an empty string.")
ditto

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:746: t.Fatalf("Expected URL to parse: %q, got error:
%v", url, err)
"Parse(%q) returned error: %v", url, err

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:751: t.Errorf("Error resolving %q + %q: %v", url,
test.ref, err)
"Base: %q Parse(%q) return error: %v", url, test.ref, err)

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:756: t.Errorf("Resolving %q + %q != %q; got %q",
url, test.ref, test.expected, urlStr)
t.Errorf("%q + %q == %q, want  %q", url, test.ref, urlStr, test.expected)

[rsc, 2013-02-04 04:45:40]

Please add tests for the cases I asked about below, assuming we can figure out
what the correct answers are. Thanks.

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

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:328: // Special case: self-reference
It seems like if you delete this (lines 328-331) then the code you've introduced
below will handle it fine.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:334: if u == "" {
It would be nice to eliminate this special case too. It seems like you could
change parse. See next comment.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:365: if rawurl == "" {
I think you can change this to

if rawurl == "" && viaRequest {

and then the special cases above disappear.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:587: if ref != "" {
I think it would help comprehension here to say:

var full string
if ...
    full = 

instead of reassigning base to be something more than the base.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:603: size := -1
size == len(dst)-1, so you might as well use that instead of maintaining two
variables in parallel. The last logic is really just making sure that there's a
final slash when the last element was '.' or '..'. Instead of checking on every
element, check once at the end. If you apply both those simplifications you get:

var dst []string
src := strings.Split(base, "/")
for _, elem := range src {
    switch elem {
    case ".":
        // drop
    case "..":
        if len(dst) > 0 {
            dst = dst[:len(dst)-1]
        }
    default:
        dst = append(dst, elem)
    }
}
if last := src[len(src)-1]; last == "." || last == ".." {
    // Add final slash to the joined path.
    dst = append(dst, "/")
}

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:650: if url.Scheme != "" || url.Host != "" || url.User !=
nil {
It would be nice not to have to track what changes and what doesn't through this
code. When you're using url to refer to the original unmodified content of ref,
please write ref.

if ref.Scheme != "" || ref.Host != "" || ref.User != nil {
    if ref.Scheme == "" {
        url.Scheme = u.Scheme
    }
    url.Path = resolvePath(ref.Path, "")
    return &url
}

and so on throughout the function.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:663: if url.Fragment == "" {
It seems like this say that 

Parse("/abc#foo").ResolveReference("?query") = "/abc?query#foo".
That seems wrong to me. Should this if be inside the previous one?

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:667: if url.Opaque != "" {
I don't understand how this can happen, but assuming it can, should this be
above the previous if? It seems to say that

Parse("/abc?x=y#foo").ResolveReference("OPAQUE") = "OPAQUE?x=y#foo".

except that I have no idea what actual URL to substitute for "OPAQUE"

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go
File src/pkg/net/url/url_test.go (right):

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:631: _, err := base.Parse("")
mustParse("")

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:682: _, err := base.Parse("")
mustParse("")

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:689: type rfcTest struct {
Please move tables to top level. You don't need the type at all.

var rfc3986Tests = []struct{
    ref string
    expected string
}{
    ...
}

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:746: t.Fatalf("Expected URL to parse: %q, got error:
%v", url, err)
On 2013/02/04 01:33:40, adg wrote:
> "Parse(%q) returned error: %v", url, err

or even just

Parse(%q): %v

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:751: t.Errorf("Error resolving %q + %q: %v", url,
test.ref, err)
On 2013/02/04 01:33:40, adg wrote:
> "Base: %q Parse(%q) return error: %v", url, test.ref, err)

or even

URL(%q).Parse(%q): %v

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:756: t.Errorf("Resolving %q + %q != %q; got %q",
url, test.ref, test.expected, urlStr)
On 2013/02/04 01:33:40, adg wrote:
> t.Errorf("%q + %q == %q, want  %q", url, test.ref, urlStr, test.expected)

URL(%q).Parse(%q) = %q, want %q

To someone not familiar with this test (i.e. someone introducing a compiler bug)
the error looks like it is just adding two strings, which is not what is going
on here.

[rodrigo.moraes, 2013-02-04 11:12:26]

Hello rsc@golang.org, adg@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rodrigo.moraes, 2013-02-04 11:12:53]

Applied all suggestions. Please take another look.

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

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:328: // Special case: self-reference
Handled by parse() as suggested.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:334: if u == "" {
Ditto.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:365: if rawurl == "" {
Done.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:587: if ref != "" {
Done.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:598: return base
Done.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:603: size := -1
Perfectly clean. Thanks.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:604: for k, v := range src {
No longer relevant. See previous comment.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:650: if url.Scheme != "" || url.Host != "" || url.User !=
nil {
Agreed. I made the difference clear.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:663: if url.Fragment == "" {
You are right. Missed this one; added a test now.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url.go#newc...
src/pkg/net/url/url.go:667: if url.Opaque != "" {
Moved Opaque checking above Path checking. Opaque is now tested for all
ResolveReference tests.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go
File src/pkg/net/url/url_test.go (right):

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:631: _, err := base.Parse("")
I removed these tests because they are covered by TestResolveReference.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:633: t.Errorf("Didn't expect an error from Parse
wrapper parsing an empty string.")
Removed these tests. See previous comment.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:682: _, err := base.Parse("")
Ditto.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:684: t.Errorf("Didn't expect an error from Parse
wrapper parsing an empty string.")
Ditto.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:689: type rfcTest struct {
I merged this table with resolveReferenceTests.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:746: t.Fatalf("Expected URL to parse: %q, got error:
%v", url, err)
Moved the whole table to be tested by TestResolveReference, so this function no
longer exists.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:751: t.Errorf("Error resolving %q + %q: %v", url,
test.ref, err)
Ditto.

https://codereview.appspot.com/7203059/diff/14001/src/pkg/net/url/url_test.go...
src/pkg/net/url/url_test.go:756: t.Errorf("Resolving %q + %q != %q; got %q",
url, test.ref, test.expected, urlStr)
Ditto.

[rodrigo.moraes, 2013-02-04 12:56:43]

Two notes:

- It seems that TestResolveReferenceOpaque isn't testing anything different that
is not covered by TestResolveReference. Can I remove it?

- Should I update the docs from ResolveReference to point to RFC 3986 instead of
2396?

[rodrigo.moraes, 2013-02-04 17:22:33]

Hello rsc@golang.org, adg@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-03-11 19:02:42]

LGTM

Sorry for the delay.

[rsc, 2013-03-11 19:03:21]

*** Submitted as https://code.google.com/p/go/source/detail?r=c890576b82a1 ***

net/url: better path resolution

This includes a simplified resolvePath function and tests for all normal and
abnormal path resolution examples described in RFC 3986, sections 5.4.1 and
5.4.2 [1]. Some of those examples failed before (see
http://play.golang.org/p/F0ApSaXniv).

Also, parsing a reference "//foo" now works as expected. It was treated as an
absolute path with very weird results (see http://play.golang.org/p/089b-_xoNe).

During path resolution, all dot segments are removed as described by the RFC.

A few existing tests had to be changed because they expected the wrong output.

Fixes issue 4700.

Fixes issue 4706.

[1] http://tools.ietf.org/html/rfc3986#section-5.4.1

R=rsc, adg, bradfitz
CC=golang-dev
https://codereview.appspot.com/7203059

Committer: Russ Cox <rsc@golang.org>