code review 3910042: http: fix scheme-relative URL parsing; add ParseRequestURL

http: fix scheme-relative URL parsing; add ParseRequestURL

Also adds some tests for Issue 900 which was the reason
the current URL parsing is broken.  (the previous fix
was wrong)

[brad_danga_com, 2011-01-10 19:01:09]

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

I'd like you to review this change.

[adg, 2011-01-11 02:44:56]

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

http://codereview.appspot.com/3910042/diff/2001/src/pkg/http/url.go#newcode536
src/pkg/http/url.go:536: func cleanURLForHTTPRequest(url *URL) {
I'm a little uncomfortable with the naming and location of this function. At the
very least it should have a TODO comment, and perhaps it should be in request.go
instead.

The comment doesn't really explain what it does, only what it is for. Maybe
that's why I find it confusing.

http://codereview.appspot.com/3910042/diff/2001/src/pkg/http/url_test.go
File src/pkg/http/url_test.go (right):

http://codereview.appspot.com/3910042/diff/2001/src/pkg/http/url_test.go#newc...
src/pkg/http/url_test.go:571: t.Errorf("Expected authority of 'foo'; got %q",
url.RawAuthority)
s/foo/user@foo/

http://codereview.appspot.com/3910042/diff/2001/src/pkg/http/url_test.go#newc...
src/pkg/http/url_test.go:578: t.Errorf("Expected blank authority.")
got %q ?

If not, use Error instead of Errorf

[rsc, 2011-01-11 16:08:21]

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

http://codereview.appspot.com/3910042/diff/2001/src/pkg/http/url.go#newcode531
src/pkg/http/url.go:531: // cleanURLForRequest cleans URLs as parsed from
ReadRequest.
I agree with adg that this code should be in ReadRequest,
maybe even inlined.  The comment that would help me would be:

// ParseURL interprets //foo as a scheme-relative URL, but
// in an HTTP request //foo is just a path.  Convert.

[brad_danga_com, 2011-01-11 20:08:14]

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

Please take another look.

[rsc, 2011-01-11 20:18:11]

Sorry for the runaround but I think you
need to tell ParseURL to do the right thing.
I was looking just now at whether prefix should
be urlUnescaped before adding it to the beginning
of url.Path.  The answer is that it should, except
that if there were escaped characters then ParseURL
rejected the URL as having an invalid authority.

That is, I believe GET //%66%6f%6f should end up
asking for //foo, but I think ParseURL will just reject it
because %66%6f%6f is a bogus authority.

I suggest renaming ParseURL to parseURL and
adding a bool flag saying whether to assume the
HTTP behavior.

Then ParseURL is parseURL(url, false) and
ParseRequestURL is parseURL(url, true).

Russ

[dangabrad, 2011-01-11 20:24:37]

Yeah, sounds good.  I was talking with adg about going that route too.  Will
update.

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

> Sorry for the runaround but I think you
> need to tell ParseURL to do the right thing.
> I was looking just now at whether prefix should
> be urlUnescaped before adding it to the beginning
> of url.Path.  The answer is that it should, except
> that if there were escaped characters then ParseURL
> rejected the URL as having an invalid authority.
>
> That is, I believe GET //%66%6f%6f should end up
> asking for //foo, but I think ParseURL will just reject it
> because %66%6f%6f is a bogus authority.
>
> I suggest renaming ParseURL to parseURL and
> adding a bool flag saying whether to assume the
> HTTP behavior.
>
> Then ParseURL is parseURL(url, false) and
> ParseRequestURL is parseURL(url, true).
>
> Russ
>

[brad_danga_com, 2011-01-11 22:58:30]

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

Please take another look.

[brad_danga_com, 2011-01-11 23:07:05]

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

Please take another look.

[adg, 2011-01-13 05:16:27]

LGTM

Much better. Leaving for rsc.

http://codereview.appspot.com/3910042/diff/19001/src/pkg/http/url_test.go
File src/pkg/http/url_test.go (right):

http://codereview.appspot.com/3910042/diff/19001/src/pkg/http/url_test.go#new...
src/pkg/http/url_test.go:341: tests := []struct {
The convention is to make test cases global variables (in this case,
ParseRequestURLTests).

[brad_danga_com, 2011-01-13 20:11:48]

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

Please take another look.

[bradfitzgoog, 2011-01-13 20:12:08]

PTAL

http://codereview.appspot.com/3910042/diff/19001/src/pkg/http/url_test.go
File src/pkg/http/url_test.go (right):

http://codereview.appspot.com/3910042/diff/19001/src/pkg/http/url_test.go#new...
src/pkg/http/url_test.go:341: tests := []struct {
On 2011/01/13 05:16:27, adg wrote:
> The convention is to make test cases global variables (in this case,
> ParseRequestURLTests).

Done.

[rsc, 2011-01-13 20:53:52]

lgtm; please make tiny comment changes below

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

http://codereview.appspot.com/3910042/diff/30001/src/pkg/http/url.go#newcode393
src/pkg/http/url.go:393: // ParseRequestURL parses rawurl into a URL structure,
assuming the rawurl
// ParseRequestURL parses rawurl into a URL structure.  It assumes that
// rawurl was received from an HTTP request, so the rawurl is interpreted
// only as an absolute URI or an absolute path.
// The string rawurl ...

http://codereview.appspot.com/3910042/diff/30001/src/pkg/http/url.go#newcode397
src/pkg/http/url.go:397: // The rawurl may be absolute or path-absolute, not
path- or scheme-relative.
delete (moved up into earlier content)

[brad_danga_com, 2011-01-13 21:12:52]

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

Please take another look.

[adg, 2011-01-13 22:10:38]

*** Submitted as http://code.google.com/p/go/source/detail?r=2df40d9601d2 ***

http: fix scheme-relative URL parsing; add ParseRequestURL

Also adds some tests for Issue 900 which was the reason
the current URL parsing is broken.  (the previous fix
was wrong)

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

Committer: Andrew Gerrand <adg@golang.org>