code review 5213043: http: remove Request.RawURL

http: remove Request.RawURL

Its purpose is not only undocumented, it's also unknown (to me
and Russ, at least) and leads to complexity, bugs and
confusion.

[bradfitz, 2011-10-06 23:09:51]

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

I'd like you to review this change to
https://go.googlecode.com/hg

[adg, 2011-10-06 23:22:52]

LGTM

Can it.

[bradfitz, 2011-10-06 23:26:54]

While I appreciate the quick LGTM, I'm a little scared of this CL and would
like a paranoid review.

That said: I'm more scared of the old situation where every time I touched a
*Request, I had to deal with 4 cases...

1) req.URL == nil, req,RawURL == ""  (easy to reject)
2) req.URL != nil, req,RawURL != ""  (uh, which?)
3) req.URL != nil, req,RawURL == "" (easy, URL wins)
4) req.URL == nil, req,RawURL != ""  (wonder what the hell the caller meant,
since it wasn't usually documented)

With the new way, I can ignore URL == nil is an error, and all I have to
worry about is req.URL.RawPath if documented to mean something, otherwise
just using req.URL.String() or req.URL.Path as before.


On Thu, Oct 6, 2011 at 4:22 PM, <adg@golang.org> wrote:

> LGTM
>
> Can it.
>
>
http://codereview.appspot.com/**5213043/<http://codereview.appspot.com/5213043/>
>

[adg, 2011-10-06 23:30:12]

On 6 October 2011 16:26, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> While I appreciate the quick LGTM, I'm a little scared of this CL and would
> like a paranoid review.
>
> That said: I'm more scared of the old situation where every time I touched
> a *Request, I had to deal with 4 cases...
>
> 1) req.URL == nil, req,RawURL == ""  (easy to reject)
> 2) req.URL != nil, req,RawURL != ""  (uh, which?)
> 3) req.URL != nil, req,RawURL == "" (easy, URL wins)
> 4) req.URL == nil, req,RawURL != ""  (wonder what the hell the caller
> meant, since it wasn't usually documented)
>

1 and 4 are just an invalid requests. If the URL can't be parsed then how
can you serve it? Otherwise, trust req.URL.

[bradfitz, 2011-10-06 23:35:28]

On Thu, Oct 6, 2011 at 4:29 PM, Andrew Gerrand <adg@golang.org> wrote:

>
>
> On 6 October 2011 16:26, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>> While I appreciate the quick LGTM, I'm a little scared of this CL and
>> would like a paranoid review.
>>
>> That said: I'm more scared of the old situation where every time I touched
>> a *Request, I had to deal with 4 cases...
>>
>> 1) req.URL == nil, req,RawURL == ""  (easy to reject)
>> 2) req.URL != nil, req,RawURL != ""  (uh, which?)
>> 3) req.URL != nil, req,RawURL == "" (easy, URL wins)
>> 4) req.URL == nil, req,RawURL != ""  (wonder what the hell the caller
>> meant, since it wasn't usually documented)
>>
>
> 1 and 4 are just an invalid requests. If the URL can't be parsed then how
> can you serve it? Otherwise, trust req.URL.
>

One would think. But no.  4) was still valid in some places, like RoundTrip.
 So there were still 3 cases to deal with, only one of which was sanely
defined.

[rsc, 2011-10-07 01:45:30]

I'd like to leave this open for a few days and see if people weigh in.

Russ

[bradfitz, 2011-10-12 17:57:07]

Ping.

Can we kill this now?

Does it need a gofix module?  If so, what should it do?  Writing a gofix
rule implies that we know the purpose of this field.  If we knew the purpose
of it, we wouldn't be removing it.


On Thu, Oct 6, 2011 at 6:45 PM, Russ Cox <rsc@golang.org> wrote:

> I'd like to leave this open for a few days and see if people weigh in.
>
> Russ
>
>

[rsc, 2011-10-12 18:14:07]

LGTM

Don't bother with a gofix.

[bradfitz, 2011-10-12 18:48:27]

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

http: remove Request.RawURL

Its purpose is not only undocumented, it's also unknown (to me
and Russ, at least) and leads to complexity, bugs and
confusion.

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