code review 5630053: net/http: prevent the ServeMux from clobbering user rou...

net/http: prevent the ServeMux from clobbering user routes when its trying to be helpful

[sanjay.m, 2012-02-04 11:45:20]

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

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

[bradfitz, 2012-02-04 18:51:25]

Also, update your commit description to be shorter & more descriptive.  Perhaps:

net/http: prevent ServeMux implicit redirects from clobbering routes

http://codereview.appspot.com/5630053/diff/4001/src/pkg/net/http/serve_test.go
File src/pkg/net/http/serve_test.go (right):

http://codereview.appspot.com/5630053/diff/4001/src/pkg/net/http/serve_test.g...
src/pkg/net/http/serve_test.go:237: type tCase struct {
type "case" works instead of tCase.  or you don't have to give it a name at all:

cases := []struct {
   ...
}{
      {path: ......},
      {path: ......},
      {path: ......},
}

http://codereview.appspot.com/5630053/diff/4001/src/pkg/net/http/serve_test.g...
src/pkg/net/http/serve_test.go:240: redirect       string // set only if
shouldRedirect
might as well just combine the two fields into one then:

wantRedirect string // or empty string if we don't want a redirect

http://codereview.appspot.com/5630053/diff/4001/src/pkg/net/http/serve_test.g...
src/pkg/net/http/serve_test.go:246: {path: "/thumbs/", shouldRedirect: false},
the false values are redundant.  everything's zero by default.

[rsc, 2012-02-04 19:26:16]

I'm a little confused about why it is a good idea to treat
/foo and /foo/ as separate URLs.  Can you explain the
motivation here?

Russ

[rsc, 2012-02-04 19:30:00]

On Sat, Feb 4, 2012 at 14:26, Russ Cox <rsc@golang.org> wrote:
> I'm a little confused about why it is a good idea to treat
> /foo and /foo/ as separate URLs.  Can you explain the
> motivation here?

Also, the current registration semantics are order-independent.
This CL makes them order-dependent.  If I register /foo and then
/foo/, I get a working program, and if I register /foo/ and then /foo
I get a panic.  That's a significant regression.

If this change has to be done (and I don't think that is sure yet),
it has to be done differently.  For example, the order-independence
would be preserved if registering /foo/ did not create the map entry
for /foo ever, but the lookup for /foo would allow /foo/ to match as
a less-specific match than /foo.

This all seems very complicated.  Unless there's an excellent
reason to introduce this complication now, I'd like to defer this
to after Go 1.

Thanks.

[bradfitz, 2012-02-04 19:34:00]

On Sat, Feb 4, 2012 at 11:29 AM, Russ Cox <rsc@golang.org> wrote:

> On Sat, Feb 4, 2012 at 14:26, Russ Cox <rsc@golang.org> wrote:
> > I'm a little confused about why it is a good idea to treat
> > /foo and /foo/ as separate URLs.  Can you explain the
> > motivation here?
>
> Also, the current registration semantics are order-independent.
> This CL makes them order-dependent.  If I register /foo and then
> /foo/, I get a working program, and if I register /foo/ and then /foo
> I get a panic.  That's a significant regression.


why would it panic?

[rsc, 2012-02-04 19:36:49]

On Sat, Feb 4, 2012 at 14:33, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> why would it panic?

registering the same path twice panics.
registering /foo/ registers /foo too (so it can
redirect to /foo/).

handle("/foo/")  registers /foo/ and /foo redirect

this CL changes that behavior so that registering
/foo/ registers /foo only if there is not already
a handler for /foo.

case 1:

handle("/foo")
handle("/foo/")  registers only /foo/

case 2:

handle("/foo/")  registers /foo/ and /foo redirect
handle("/foo")  panic - /foo already registered

russ

[bradfitz, 2012-02-04 19:54:26]

On Sat, Feb 4, 2012 at 11:36 AM, Russ Cox <rsc@golang.org> wrote:

> On Sat, Feb 4, 2012 at 14:33, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > why would it panic?
>
> registering the same path twice panics.
>

I don't see that code, and doesn't seem to panic:

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

[rsc, 2012-02-04 21:55:26]

Okay, then that is definitely a bug.  You shouldn't be able to
just silently have your registrations deleted.

[kevlar, 2012-02-05 00:04:32]

for what it's worth, the current behavior causes me headaches with a
reverse proxy I have that uses prefixes to distribute queries to various
service slaves.  My users have to remember to include the trailing /,
because the automatically-generated non-/ redirect clobbers the prefix.  I
haven't yet added in the response mutation on the proxy side required to
transform redirects so they aren't broken.

basically:
I register /slave1/, /slave2/ etc and these proxy to services that define
/page1/ and /page2/ but if you go to /slave2/page1 it redirects to /page1/
which is unhelpful.  I tried to register my own handler for these, but to
no avail.

An alternate solution might be to have a RegisterOnly that doesn't do
anything special.

[dsymonds, 2012-02-05 00:14:25]

Maybe there needs to be a secondary map that holds the set of
implicitly registered paths. Then /foo/ should implicitly add a
handler for /foo if one doesn't already exist (and add an entry to
that secondary map), but then registering a duplicate path is okay iff
an entry for it doesn't exist in the secondary map. That makes the
order of registration have no effect.


Dave.

[sanjay.m, 2012-02-05 02:25:37]

As others have said, the mux, as it stands, does not panic when it would 
have clobbered old routes. In fact, its behaviour is order dependant now 
due to its redirecting behaviour.

While I'm ok with having ServeMux be silent when the user 
overwrites themselves, I'm not ok with having the Mux 
silently overwrite user-registered routes. This was the purpose of this CL.

If you do want to make it panic on duplicate registrations, then I like 
Dave's suggestion, as it would have fixed the issue I was having.

I will wait till we achieve consensus before I move further.

Sanjay

[sanjay.m, 2012-02-05 02:29:31]

Also, Brad, funnily enough, case was the name of the tCase struct, I don't 
think the grammar can handle that however :)

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

Sanjay

[rsc, 2012-02-06 17:44:25]

This is not a critical bug fix.
Let's postpone this until after Go 1.
Please file an issue or remember to ping this CL
after Go 1 is out.

Thanks.
Russ

[sanjay.m, 2012-02-07 08:53:29]

Sounds good, filed issue: golang.org/issue/2900

Sanjay

[sanjay.m, 2012-02-08 07:27:45]

*** Abandoned ***