Merge pull request gorilla#51 from ttencate/master

Make PathPrefix more polite towards StrictSlash setting, and add better tests for the latter

Author: kisielk

mux.go

@@ -109,14 +109,20 @@ 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.
 //
-// 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.
+// 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 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

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) {
@@ -151,6 +152,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"),
@@ -214,6 +242,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"),
@@ -579,26 +616,74 @@ 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:          "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:          "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/"),
+			request:        newRequest("GET", "http://localhost/static/logo.png"),
+			vars:           map[string]string{},
+			host:           "",
+			path:           "/static/",
+			shouldMatch:    true,
+			shouldRedirect: false,
+		},
 	}
-	if match.Handler != nil {
-		t.Errorf("Should not redirect")
+
+	for _, test := range tests {
+		testRoute(t, test)
 	}
 }
 
@@ -627,6 +712,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)
@@ -664,6 +750,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
+		}
 	}
 }
 

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 {

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,9 +284,16 @@ 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. 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)
 	return r
 }