From 525eff436e45137f9c4ffa422f18bab457a96500 Mon Sep 17 00:00:00 2001 From: Thomas ten Cate Date: Wed, 23 Apr 2014 17:40:15 +0200 Subject: [PATCH 1/5] Improve docs about leading and trailing slashes --- mux_test.go | 9 +++++++++ route.go | 8 ++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/mux_test.go b/mux_test.go index 292776d..95b894c 100644 --- a/mux_test.go +++ b/mux_test.go @@ -214,6 +214,15 @@ func TestPathPrefix(t *testing.T) { path: "/111", shouldMatch: true, }, + { + title: "PathPrefix route, match substring", + route: new(Route).PathPrefix("/1"), + request: newRequest("GET", "http://localhost/111/222/333"), + vars: map[string]string{}, + host: "", + path: "/1", + shouldMatch: true, + }, { title: "PathPrefix route, URL prefix in request does not match", route: new(Route).PathPrefix("/111"), diff --git a/route.go b/route.go index 7766254..6bf84ec 100644 --- a/route.go +++ b/route.go @@ -259,7 +259,8 @@ func (r *Route) Methods(methods ...string) *Route { // Path ----------------------------------------------------------------------- // Path adds a matcher for the URL path. -// It accepts a template with zero or more URL variables enclosed by {}. +// It accepts a template with zero or more URL variables enclosed by {}. The +// template must start with a "/". // Variables can define an optional regexp pattern to me matched: // // - {name} matches anything until the next slash. @@ -283,7 +284,10 @@ func (r *Route) Path(tpl string) *Route { // PathPrefix ----------------------------------------------------------------- -// PathPrefix adds a matcher for the URL path prefix. +// PathPrefix adds a matcher for the URL path prefix. Note that it does not +// treat slashes specially ("/foobar/" will be matched by the prefix "/foo") so +// in most cases you'll want to use a trailing slash here. See Route.Path() for +// details on the tpl argument. func (r *Route) PathPrefix(tpl string) *Route { r.strictSlash = false r.err = r.addRegexpMatcher(tpl, false, true) From 033224c12ed48938d1815f4851103f86abcf6201 Mon Sep 17 00:00:00 2001 From: Thomas ten Cate Date: Wed, 23 Apr 2014 18:19:14 +0200 Subject: [PATCH 2/5] Document behaviour of StrictSlash and PathPrefix better, and add tests to nail this down --- mux.go | 9 +++++++-- mux_test.go | 27 +++++++++++++++++++++++++++ route.go | 13 +++++++++---- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/mux.go b/mux.go index 711630d..fa12ffa 100644 --- a/mux.go +++ b/mux.go @@ -109,10 +109,15 @@ func (r *Router) GetRoute(name string) *Route { return r.getNamedRoutes()[name] } -// StrictSlash defines the slash behavior for new routes. +// StrictSlash defines the trailing slash behavior for new routes. The initial +// value is false. // // When true, if the route path is "/path/", accessing "/path" will redirect -// to the former and vice versa. +// to the former and vice versa. In other words, your application will always +// see the path as specified in the route. +// +// When false, if the route path is "/path", accessing "/path/" will not match +// this route and vice versa. // // Special case: when a route sets a path prefix, strict slash is // automatically set to false for that route because the redirect behavior diff --git a/mux_test.go b/mux_test.go index 95b894c..29a4ef2 100644 --- a/mux_test.go +++ b/mux_test.go @@ -151,6 +151,33 @@ func TestPath(t *testing.T) { path: "/111/222/333", shouldMatch: true, }, + { + title: "Path route, match with trailing slash in request and path", + route: new(Route).Path("/111/"), + request: newRequest("GET", "http://localhost/111/"), + vars: map[string]string{}, + host: "", + path: "/111/", + shouldMatch: true, + }, + { + title: "Path route, do not match with trailing slash in path", + route: new(Route).Path("/111/"), + request: newRequest("GET", "http://localhost/111"), + vars: map[string]string{}, + host: "", + path: "/111", + shouldMatch: false, + }, + { + title: "Path route, do not match with trailing slash in request", + route: new(Route).Path("/111"), + request: newRequest("GET", "http://localhost/111/"), + vars: map[string]string{}, + host: "", + path: "/111/", + shouldMatch: false, + }, { title: "Path route, wrong path in request in request URL", route: new(Route).Path("/111/222/333"), diff --git a/route.go b/route.go index 6bf84ec..4ce8c82 100644 --- a/route.go +++ b/route.go @@ -284,10 +284,15 @@ func (r *Route) Path(tpl string) *Route { // PathPrefix ----------------------------------------------------------------- -// PathPrefix adds a matcher for the URL path prefix. Note that it does not -// treat slashes specially ("/foobar/" will be matched by the prefix "/foo") so -// in most cases you'll want to use a trailing slash here. See Route.Path() for -// details on the tpl argument. +// PathPrefix adds a matcher for the URL path prefix. This matches if the given +// template is a prefix of the full URL path. See Route.Path() for details on +// the tpl argument. +// +// Note that it does not treat slashes specially ("/foobar/" will be matched by +// the prefix "/foo") so you may want to use a trailing slash here. +// +// Also note that the setting of Router.StrictSlash() has no effect on routes +// with a PathPrefix matcher. func (r *Route) PathPrefix(tpl string) *Route { r.strictSlash = false r.err = r.addRegexpMatcher(tpl, false, true) From bac13721298f7a2dbf554446a58826c2035e70e8 Mon Sep 17 00:00:00 2001 From: Thomas ten Cate Date: Wed, 23 Apr 2014 19:24:12 +0200 Subject: [PATCH 3/5] Convert TestStrictSlash to a table driven test --- mux_test.go | 57 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/mux_test.go b/mux_test.go index 29a4ef2..a67efee 100644 --- a/mux_test.go +++ b/mux_test.go @@ -13,13 +13,14 @@ import ( ) type routeTest struct { - title string // title of the test - route *Route // the route being tested - request *http.Request // a request to test the route - vars map[string]string // the expected vars of the match - host string // the expected host of the match - path string // the expected path of the match - shouldMatch bool // whether the request is expected to match the route at all + title string // title of the test + route *Route // the route being tested + request *http.Request // a request to test the route + vars map[string]string // the expected vars of the match + host string // the expected host of the match + path string // the expected path of the match + shouldMatch bool // whether the request is expected to match the route at all + shouldRedirect bool // whether the request should result in a redirect } func TestHost(t *testing.T) { @@ -615,26 +616,23 @@ func TestNamedRoutes(t *testing.T) { } func TestStrictSlash(t *testing.T) { - var r *Router - var req *http.Request - var route *Route - var match *RouteMatch - var matched bool - - // StrictSlash should be ignored for path prefix. - // So we register a route ending in slash but it doesn't attempt to add - // the slash for a path not ending in slash. - r = NewRouter() + r := NewRouter() r.StrictSlash(true) - route = r.NewRoute().PathPrefix("/static/") - req, _ = http.NewRequest("GET", "http://localhost/static/logo.png", nil) - match = new(RouteMatch) - matched = r.Match(req, match) - if !matched { - t.Errorf("Should match request %q -- %v", req.URL.Path, getRouteTemplate(route)) + + tests := []routeTest{ + { + title: "Ignore StrictSlash for path prefix", + route: r.NewRoute().PathPrefix("/static/"), + request: newRequest("GET", "http://localhost/static/logo.png"), + vars: map[string]string{}, + path: "/static/", + shouldMatch: true, + shouldRedirect: false, + }, } - if match.Handler != nil { - t.Errorf("Should not redirect") + + for _, test := range tests { + testRoute(t, test) } } @@ -663,6 +661,7 @@ func testRoute(t *testing.T, test routeTest) { host := test.host path := test.path url := test.host + test.path + shouldRedirect := test.shouldRedirect var match RouteMatch ok := route.Match(request, &match) @@ -700,6 +699,14 @@ func testRoute(t *testing.T, test routeTest) { return } } + if shouldRedirect && match.Handler == nil { + t.Errorf("(%v) Did not redirect", test.title) + return + } + if !shouldRedirect && match.Handler != nil { + t.Errorf("(%v) Unexpected redirect", test.title) + return + } } } From 3509745ae8a2db852c51794a6f6e9f1e533e1632 Mon Sep 17 00:00:00 2001 From: Thomas ten Cate Date: Wed, 23 Apr 2014 19:27:53 +0200 Subject: [PATCH 4/5] Add tests for all four StrictSlash cases --- mux_test.go | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/mux_test.go b/mux_test.go index a67efee..63bc491 100644 --- a/mux_test.go +++ b/mux_test.go @@ -620,11 +620,52 @@ func TestStrictSlash(t *testing.T) { r.StrictSlash(true) tests := []routeTest{ + { + title: "Redirect path without slash", + route: r.NewRoute().Path("/111/"), + request: newRequest("GET", "http://localhost/111"), + vars: map[string]string{}, + host: "", + path: "/111/", + shouldMatch: true, + shouldRedirect: true, + }, + { + title: "Do not redirect path with slash", + route: r.NewRoute().Path("/111/"), + request: newRequest("GET", "http://localhost/111/"), + vars: map[string]string{}, + host: "", + path: "/111/", + shouldMatch: true, + shouldRedirect: false, + }, + { + title: "Redirect path with slash", + route: r.NewRoute().Path("/111"), + request: newRequest("GET", "http://localhost/111/"), + vars: map[string]string{}, + host: "", + path: "/111", + shouldMatch: true, + shouldRedirect: true, + }, + { + title: "Do not redirect path without slash", + route: r.NewRoute().Path("/111"), + request: newRequest("GET", "http://localhost/111"), + vars: map[string]string{}, + host: "", + path: "/111", + shouldMatch: true, + shouldRedirect: false, + }, { title: "Ignore StrictSlash for path prefix", route: r.NewRoute().PathPrefix("/static/"), request: newRequest("GET", "http://localhost/static/logo.png"), vars: map[string]string{}, + host: "", path: "/static/", shouldMatch: true, shouldRedirect: false, From b864f07c539c7abc16652d65350d070d8e2810de Mon Sep 17 00:00:00 2001 From: Thomas ten Cate Date: Wed, 23 Apr 2014 19:53:35 +0200 Subject: [PATCH 5/5] Propagate StrictSlash to subrouters instead of rudely turning it off --- mux.go | 7 ++++--- mux_test.go | 10 ++++++++++ regexp.go | 17 ++++++++++------- route.go | 1 - 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/mux.go b/mux.go index fa12ffa..8b23c39 100644 --- a/mux.go +++ b/mux.go @@ -119,9 +119,10 @@ func (r *Router) GetRoute(name string) *Route { // When false, if the route path is "/path", accessing "/path/" will not match // this route and vice versa. // -// Special case: when a route sets a path prefix, strict slash is -// automatically set to false for that route because the redirect behavior -// can't be determined for prefixes. +// Special case: when a route sets a path prefix using the PathPrefix() method, +// strict slash is ignored for that route because the redirect behavior can't +// be determined from a prefix alone. However, any subrouters created from that +// route inherit the original StrictSlash setting. func (r *Router) StrictSlash(value bool) *Router { r.strictSlash = value return r diff --git a/mux_test.go b/mux_test.go index 63bc491..0e2e480 100644 --- a/mux_test.go +++ b/mux_test.go @@ -660,6 +660,16 @@ func TestStrictSlash(t *testing.T) { shouldMatch: true, shouldRedirect: false, }, + { + title: "Propagate StrictSlash to subrouters", + route: r.NewRoute().PathPrefix("/static/").Subrouter().Path("/images/"), + request: newRequest("GET", "http://localhost/static/images"), + vars: map[string]string{}, + host: "", + path: "/static/images/", + shouldMatch: true, + shouldRedirect: true, + }, { title: "Ignore StrictSlash for path prefix", route: r.NewRoute().PathPrefix("/static/"), diff --git a/regexp.go b/regexp.go index 4c3482b..925f268 100644 --- a/regexp.go +++ b/regexp.go @@ -98,12 +98,13 @@ func newRouteRegexp(tpl string, matchHost, matchPrefix, strictSlash bool) (*rout } // Done! return &routeRegexp{ - template: template, - matchHost: matchHost, - regexp: reg, - reverse: reverse.String(), - varsN: varsN, - varsR: varsR, + template: template, + matchHost: matchHost, + strictSlash: strictSlash, + regexp: reg, + reverse: reverse.String(), + varsN: varsN, + varsR: varsR, }, nil } @@ -114,6 +115,8 @@ type routeRegexp struct { template string // True for host match, false for path match. matchHost bool + // The strictSlash value defined on the route, but disabled if PathPrefix was used. + strictSlash bool // Expanded regexp. regexp *regexp.Regexp // Reverse template. @@ -216,7 +219,7 @@ func (v *routeRegexpGroup) setMatch(req *http.Request, m *RouteMatch, r *Route) m.Vars[v] = pathVars[k+1] } // Check if we should redirect. - if r.strictSlash { + if v.path.strictSlash { p1 := strings.HasSuffix(req.URL.Path, "/") p2 := strings.HasSuffix(v.path.template, "/") if p1 != p2 { diff --git a/route.go b/route.go index 4ce8c82..5cb2526 100644 --- a/route.go +++ b/route.go @@ -294,7 +294,6 @@ func (r *Route) Path(tpl string) *Route { // Also note that the setting of Router.StrictSlash() has no effect on routes // with a PathPrefix matcher. func (r *Route) PathPrefix(tpl string) *Route { - r.strictSlash = false r.err = r.addRegexpMatcher(tpl, false, true) return r }