code review 4002041: http: support for relative URLs

http: support for relative URLs

[bradfitz, 2011-01-13 23:53:12]

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

I'd like you to review this change.

[bradfitz, 2011-01-13 23:59:49]

Pre-emptive defense about seemingly duplicating http.CanonicalPath:  I
couldn't use CanonicalPath because RFC 2396 says explicitly:

       Similarly, parsers must avoid treating "." and ".." as special when
they are not complete components of a relative path."

I have a test for that too.

Hence my "applyReferenceSegments" and its associated tests, which are
uncomfortably similar to CanonicalPath.  Perhaps we should remove
http.CanonicalPath?  It may not have any utility over ResolveReference.
 Other suggestions welcome.

On Thu, Jan 13, 2011 at 3:53 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc, adg,
>
> Message:
> Hello rsc, adg (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> http: add url.ResolveRefence(); base + relative URL => absolute URL
>
> Please review this at http://codereview.appspot.com/4002041/
>
> Affected files:
>  M src/pkg/http/url.go
>  M src/pkg/http/url_test.go
>
>
>

[rsc, 2011-01-14 15:31:15]

I think it's okay to delete CanonicalPath.
It was for this case and it clearly doesn't do its job.

IsAbsolute should be IsAbs, because of path.IsAbs.

applyReferenceSegments -> resolvePath(base, relative string) string
  (i.e. let it do the split and join, so that a future implementation
   could avoid them without changing the call sites.)

I'm not wild about the name ResolveReference - even though those
are precisely defined words in the RFC they are vague words to
most programmers - but I do not have a better suggestion, so it's fine.
Maybe func (base *URL) ParseURL(url string) (*URL, os.Error)
would be a nice convenience function to build on top of it to hit the
common case.

Russ

[bradfitz, 2011-01-14 17:31:57]

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

Please take another look.

[bradfitz, 2011-01-14 17:34:10]

All done.  PTAL.

Questions:

-- should we return an error when !base.IsAbs() ?  ParseURL returns an error
already, so that's easy.  ResolveReference doesn't return an error
currently.  It could... preference?

-- how do you want to deal with #fragments here?  It should be documented in
either case.

On Fri, Jan 14, 2011 at 7:31 AM, Russ Cox <rsc@golang.org> wrote:

> I think it's okay to delete CanonicalPath.
> It was for this case and it clearly doesn't do its job.
>
> IsAbsolute should be IsAbs, because of path.IsAbs.
>
> applyReferenceSegments -> resolvePath(base, relative string) string
>  (i.e. let it do the split and join, so that a future implementation
>   could avoid them without changing the call sites.)
>
> I'm not wild about the name ResolveReference - even though those
> are precisely defined words in the RFC they are vague words to
> most programmers - but I do not have a better suggestion, so it's fine.
> Maybe func (base *URL) ParseURL(url string) (*URL, os.Error)
> would be a nice convenience function to build on top of it to hit the
> common case.
>
> Russ
>

[rsc, 2011-01-18 19:50:08]

looks right to me.

http://codereview.appspot.com/4002041/diff/8001/src/pkg/http/url.go
File src/pkg/http/url.go (right):

http://codereview.appspot.com/4002041/diff/8001/src/pkg/http/url.go#newcode551
src/pkg/http/url.go:551: // base or reference.
// If ref is an absolute URL, then ResolveReference ignores base and returns a
copy of ref.

[rsc, 2011-01-18 19:50:40]

> -- should we return an error when !base.IsAbs() ?  ParseURL returns an error
> already, so that's easy.  ResolveReference doesn't return an error
> currently.  It could... preference?

i think as is is better.  makes base.ParseURL(url) useful.

> -- how do you want to deal with #fragments here?  It should be documented in
> either case.

probably leave them out; that's the case we care about, right?

[rsc, 2011-01-18 19:53:21]

btw, please run hg change 4002041 and edit the CL description
to fix the spelling of ResolveReference.
could even shorten to

http: support for relative URLs

if you wanted

[bradfitzgoog, 2011-01-18 23:30:34]

Done.

On Tue, Jan 18, 2011 at 11:53 AM, Russ Cox <rsc@golang.org> wrote:

> btw, please run hg change 4002041 and edit the CL description
> to fix the spelling of ResolveReference.
> could even shorten to
>
> http: support for relative URLs
>
> if you wanted
>

[bradfitz, 2011-01-18 23:31:55]

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

Please take another look.

[bradfitz, 2011-01-18 23:32:24]

http://codereview.appspot.com/4002041/diff/8001/src/pkg/http/url.go
File src/pkg/http/url.go (right):

http://codereview.appspot.com/4002041/diff/8001/src/pkg/http/url.go#newcode551
src/pkg/http/url.go:551: // base or reference.
On 2011/01/18 19:50:08, rsc wrote:
> // If ref is an absolute URL, then ResolveReference ignores base and returns a
> copy of ref.

Done.

[rsc, 2011-01-19 20:13:25]

LGTM

[rsc, 2011-01-19 20:13:44]

*** Submitted as 73c8f1823c79 ***

http: support for relative URLs

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/4002041

Committer: Russ Cox <rsc@golang.org>