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 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions mux.go
Original file line number Diff line number Diff line change
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,17 @@ 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
w.Header().Set("Location", url.String())
w.WriteHeader(http.StatusMovedPermanently)

Choose a reason for hiding this comment

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

Have you considered using http.Redirect here instead? Would be more consistent with what other projects are doing.
Basically the difference is that it would give a response body with the location in too and a moved permanently message.

coreydaley marked this conversation as resolved.
Show resolved Hide resolved
return
}

handler.ServeHTTP(w, req)
}

Expand Down Expand Up @@ -413,6 +406,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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