Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow SkipClean to be evaluated on each route independently. #463

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 19 additions & 20 deletions mux.go
Expand Up @@ -116,6 +116,8 @@ func copyRouteConf(r routeConf) routeConf {
c.matchers = append(c.matchers, m)
}

c.skipClean = r.skipClean

return c
}

Expand Down Expand Up @@ -173,26 +175,6 @@ func (r *Router) Match(req *http.Request, match *RouteMatch) bool {
// When there is a match, the route variables can be retrieved calling
// mux.Vars(request).
func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
if !r.skipClean {
path := req.URL.Path
if r.useEncodedPath {
path = req.URL.EscapedPath()
}
// Clean path to canonical form and redirect.
if p := cleanPath(path); p != path {

// Added 3 lines (Philip Schlump) - It was dropping the query string and #whatever from query.
// This matches with fix in go 1.2 r.c. 4 for same problem. Go Issue:
// http://code.google.com/p/go/issues/detail?id=5252
url := *req.URL
url.Path = p
p = url.String()

w.Header().Set("Location", p)
w.WriteHeader(http.StatusMovedPermanently)
return
}
}
var match RouteMatch
var handler http.Handler
if r.Match(req, &match) {
Expand All @@ -209,6 +191,16 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
handler = http.NotFoundHandler()
}

if match.OnlyMatchedCleanPath {
// Added 3 lines (Philip Schlump) - It was dropping the query string and #whatever from query.
// This matches with fix in go 1.2 r.c. 4 for same problem. Go Issue:
// http://code.google.com/p/go/issues/detail?id=5252
url := *req.URL
url.Path = match.CleanPath
http.Redirect(w, req, url.String(), http.StatusMovedPermanently)
return
}

handler.ServeHTTP(w, req)
}

Expand Down Expand Up @@ -413,6 +405,13 @@ type RouteMatch struct {
Handler http.Handler
Vars map[string]string

// Cleaned version of the path being matched.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are exported, can you provide clearer guidance for a user trying to understand either:

  • What their value is?
  • Should I be setting this?
  • What the default is?

CleanPath string

// true if a valid route match occurs, but only on the cleaned version
// of a URL.
OnlyMatchedCleanPath bool

// MatchErr is set to appropriate matching error
// It is set to ErrMethodMismatch if there is a mismatch in
// the request method and route method
Expand Down
34 changes: 34 additions & 0 deletions mux_test.go
Expand Up @@ -1995,6 +1995,40 @@ func TestSkipClean(t *testing.T) {
}
}

func TestSkipCleanSubrouter(t *testing.T) {
func1 := func(w http.ResponseWriter, r *http.Request) {}
func2 := func(w http.ResponseWriter, r *http.Request) {}

r := NewRouter().SkipClean(true)
r.HandleFunc("/api/", func1).Name("func2")
subRouter := r.PathPrefix("/v0").Subrouter().SkipClean(false)
subRouter.HandleFunc("/action/do/", func2).Name("func2")

req, _ := http.NewRequest("GET", "http://localhost/api/?abc=def", nil)
res := NewRecorder()
r.ServeHTTP(res, req)

if len(res.HeaderMap["Location"]) != 0 {
t.Errorf("Req 1: Shouldn't redirect since route is already clean")
}

req, _ = http.NewRequest("GET", "http://localhost//api/?abc=def", nil)
res = NewRecorder()
r.ServeHTTP(res, req)

if len(res.HeaderMap["Location"]) != 0 {
t.Errorf("Req 2: Shouldn't redirect since skip clean is disabled")
}

req, _ = http.NewRequest("GET", "http://localhost/v0/action//do/?ghi=jkl", nil)
res = NewRecorder()
r.ServeHTTP(res, req)

if len(res.HeaderMap["Location"]) == 0 {
t.Errorf("Req 3: Should redirect since skip clean is enabled for subroute")
}
}

// https://plus.google.com/101022900381697718949/posts/eWy6DjFJ6uW
func TestSubrouterHeader(t *testing.T) {
expected := "func1 response"
Expand Down
19 changes: 18 additions & 1 deletion regexp.go
Expand Up @@ -17,6 +17,7 @@ import (
type routeRegexpOptions struct {
strictSlash bool
useEncodedPath bool
skipClean bool
}

type regexpType int
Expand Down Expand Up @@ -170,7 +171,23 @@ func (r *routeRegexp) Match(req *http.Request, match *RouteMatch) bool {
if r.options.useEncodedPath {
path = req.URL.EscapedPath()
}
return r.regexp.MatchString(path)
if !r.options.skipClean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this code-path more clearly - e.g. "why".

// Only clean the path when needed, and cache the result for later use.
if match.CleanPath == "" {
// Clean path to canonical form for testing purposes.
match.CleanPath = cleanPath(path)
}
path = match.CleanPath
}
isMatch := r.regexp.MatchString(path)
if isMatch {
if !r.options.skipClean && req.URL.Path != match.CleanPath {
match.OnlyMatchedCleanPath = true
} else {
match.OnlyMatchedCleanPath = false
}
}
return isMatch
}

return r.regexp.MatchString(getHost(req))
Expand Down
1 change: 1 addition & 0 deletions route.go
Expand Up @@ -186,6 +186,7 @@ func (r *Route) addRegexpMatcher(tpl string, typ regexpType) error {
rr, err := newRouteRegexp(tpl, typ, routeRegexpOptions{
strictSlash: r.strictSlash,
useEncodedPath: r.useEncodedPath,
skipClean: r.skipClean,
})
if err != nil {
return err
Expand Down