-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,6 +116,8 @@ func copyRouteConf(r routeConf) routeConf { | |
c.matchers = append(c.matchers, m) | ||
} | ||
|
||
c.skipClean = r.skipClean | ||
|
||
return c | ||
} | ||
|
||
|
@@ -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) { | ||
|
@@ -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) | ||
coreydaley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return | ||
} | ||
|
||
handler.ServeHTTP(w, req) | ||
} | ||
|
||
|
@@ -413,6 +406,13 @@ type RouteMatch struct { | |
Handler http.Handler | ||
Vars map[string]string | ||
|
||
// Cleaned version of the path being matched. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import ( | |
type routeRegexpOptions struct { | ||
strictSlash bool | ||
useEncodedPath bool | ||
skipClean bool | ||
} | ||
|
||
type regexpType int | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
There was a problem hiding this comment.
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.