Skip to content

Commit

Permalink
refactor routeRegexp, particularily newRouteRegexp. (#328)
Browse files Browse the repository at this point in the history 
The existing options matchPrefix, matchHost, and matchQueries are
mutually exclusive so there's no point in having a separate boolean
argument for each one. It's clearer if there's a single type variable.

strictSlash and useEncodedPath were also being passed as naked bools so
I've wrapped these in a struct called routeRegexpOptions for more clarity
at the call site.
  • Loading branch information
@kisielk
kisielk authored Jan 5, 2018
1 parent 5ab525f commit 512169e
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 45 deletions.
2 changes: 1 addition & 1 deletion mux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func (r *Route) GoString() string {
}

func (r *routeRegexp) GoString() string {
return fmt.Sprintf("&routeRegexp{template: %q, matchHost: %t, matchQuery: %t, strictSlash: %t, regexp: regexp.MustCompile(%q), reverse: %q, varsN: %v, varsR: %v", r.template, r.matchHost, r.matchQuery, r.strictSlash, r.regexp.String(), r.reverse, r.varsN, r.varsR)
return fmt.Sprintf("&routeRegexp{template: %q, regexpType: %v, options: %v, regexp: regexp.MustCompile(%q), reverse: %q, varsN: %v, varsR: %v", r.template, r.regexpType, r.options, r.regexp.String(), r.reverse, r.varsN, r.varsR)
}

type routeTest struct {
Expand Down
2 changes: 1 addition & 1 deletion old_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ func TestNewRegexp(t *testing.T) {
}

for pattern, paths := range tests {
p, _ = newRouteRegexp(pattern, false, false, false, false, false)
p, _ = newRouteRegexp(pattern, regexpTypePath, routeRegexpOptions{})
for path, result := range paths {
matches = p.regexp.FindStringSubmatch(path)
if result == nil {
Expand Down
74 changes: 40 additions & 34 deletions regexp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,20 @@ import (
"strings"
)

type routeRegexpOptions struct {
strictSlash bool
useEncodedPath bool
}

type regexpType int

const (
regexpTypePath regexpType = 0
regexpTypeHost regexpType = 1
regexpTypePrefix regexpType = 2
regexpTypeQuery regexpType = 3
)

// newRouteRegexp parses a route template and returns a routeRegexp,
// used to match a host, a path or a query string.
//
Expand All @@ -24,7 +38,7 @@ import (
// Previously we accepted only Python-like identifiers for variable
// names ([a-zA-Z_][a-zA-Z0-9_]*), but currently the only restriction is that
// name and pattern can't be empty, and names can't contain a colon.
func newRouteRegexp(tpl string, matchHost, matchPrefix, matchQuery, strictSlash, useEncodedPath bool) (*routeRegexp, error) {
func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*routeRegexp, error) {
// Check if it is well-formed.
idxs, errBraces := braceIndices(tpl)
if errBraces != nil {
Expand All @@ -34,19 +48,18 @@ func newRouteRegexp(tpl string, matchHost, matchPrefix, matchQuery, strictSlash,
template := tpl
// Now let's parse it.
defaultPattern := "[^/]+"
if matchQuery {
if typ == regexpTypeQuery {
defaultPattern = ".*"
} else if matchHost {
} else if typ == regexpTypeHost {
defaultPattern = "[^.]+"
matchPrefix = false
}
// Only match strict slash if not matching
if matchPrefix || matchHost || matchQuery {
strictSlash = false
if typ != regexpTypePath {
options.strictSlash = false
}
// Set a flag for strictSlash.
endSlash := false
if strictSlash && strings.HasSuffix(tpl, "/") {
if options.strictSlash && strings.HasSuffix(tpl, "/") {
tpl = tpl[:len(tpl)-1]
endSlash = true
}
Expand Down Expand Up @@ -88,16 +101,16 @@ func newRouteRegexp(tpl string, matchHost, matchPrefix, matchQuery, strictSlash,
// Add the remaining.
raw := tpl[end:]
pattern.WriteString(regexp.QuoteMeta(raw))
if strictSlash {
if options.strictSlash {
pattern.WriteString("[/]?")
}
if matchQuery {
if typ == regexpTypeQuery {
// Add the default pattern if the query value is empty
if queryVal := strings.SplitN(template, "=", 2)[1]; queryVal == "" {
pattern.WriteString(defaultPattern)
}
}
if !matchPrefix {
if typ != regexpTypePrefix {
pattern.WriteByte('$')
}
reverse.WriteString(raw)
Expand All @@ -118,15 +131,13 @@ func newRouteRegexp(tpl string, matchHost, matchPrefix, matchQuery, strictSlash,

// Done!
return &routeRegexp{
template: template,
matchHost: matchHost,
matchQuery: matchQuery,
strictSlash: strictSlash,
useEncodedPath: useEncodedPath,
regexp: reg,
reverse: reverse.String(),
varsN: varsN,
varsR: varsR,
template: template,
regexpType: typ,
options: options,
regexp: reg,
reverse: reverse.String(),
varsN: varsN,
varsR: varsR,
}, nil
}

Expand All @@ -135,15 +146,10 @@ func newRouteRegexp(tpl string, matchHost, matchPrefix, matchQuery, strictSlash,
type routeRegexp struct {
// The unmodified template.
template string
// True for host match, false for path or query string match.
matchHost bool
// True for query string match, false for path and host match.
matchQuery bool
// The strictSlash value defined on the route, but disabled if PathPrefix was used.
strictSlash bool
// Determines whether to use encoded req.URL.EnscapedPath() or unencoded
// req.URL.Path for path matching
useEncodedPath bool
// The type of match
regexpType regexpType
// Options for matching
options routeRegexpOptions
// Expanded regexp.
regexp *regexp.Regexp
// Reverse template.
Expand All @@ -156,12 +162,12 @@ type routeRegexp struct {

// Match matches the regexp against the URL host or path.
func (r *routeRegexp) Match(req *http.Request, match *RouteMatch) bool {
if !r.matchHost {
if r.matchQuery {
if r.regexpType != regexpTypeHost {
if r.regexpType == regexpTypeQuery {
return r.matchQueryString(req)
}
path := req.URL.Path
if r.useEncodedPath {
if r.options.useEncodedPath {
path = req.URL.EscapedPath()
}
return r.regexp.MatchString(path)
Expand All @@ -178,7 +184,7 @@ func (r *routeRegexp) url(values map[string]string) (string, error) {
if !ok {
return "", fmt.Errorf("mux: missing route variable %q", v)
}
if r.matchQuery {
if r.regexpType == regexpTypeQuery {
value = url.QueryEscape(value)
}
urlValues[k] = value
Expand All @@ -203,7 +209,7 @@ func (r *routeRegexp) url(values map[string]string) (string, error) {
// For a URL with foo=bar&baz=ding, we return only the relevant key
// value pair for the routeRegexp.
func (r *routeRegexp) getURLQuery(req *http.Request) string {
if !r.matchQuery {
if r.regexpType != regexpTypeQuery {
return ""
}
templateKey := strings.SplitN(r.template, "=", 2)[0]
Expand Down Expand Up @@ -280,7 +286,7 @@ func (v *routeRegexpGroup) setMatch(req *http.Request, m *RouteMatch, r *Route)
if len(matches) > 0 {
extractVars(path, matches, v.path.varsN, m.Vars)
// Check if we should redirect.
if v.path.strictSlash {
if v.path.options.strictSlash {
p1 := strings.HasSuffix(path, "/")
p2 := strings.HasSuffix(v.path.template, "/")
if p1 != p2 {
Expand Down
21 changes: 12 additions & 9 deletions route.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,20 +171,23 @@ func (r *Route) addMatcher(m matcher) *Route {
}

// addRegexpMatcher adds a host or path matcher and builder to a route.
func (r *Route) addRegexpMatcher(tpl string, matchHost, matchPrefix, matchQuery bool) error {
func (r *Route) addRegexpMatcher(tpl string, typ regexpType) error {
if r.err != nil {
return r.err
}
r.regexp = r.getRegexpGroup()
if !matchHost && !matchQuery {
if typ == regexpTypePath || typ == regexpTypePrefix {
if len(tpl) > 0 && tpl[0] != '/' {
return fmt.Errorf("mux: path must start with a slash, got %q", tpl)
}
if r.regexp.path != nil {
tpl = strings.TrimRight(r.regexp.path.template, "/") + tpl
}
}
rr, err := newRouteRegexp(tpl, matchHost, matchPrefix, matchQuery, r.strictSlash, r.useEncodedPath)
rr, err := newRouteRegexp(tpl, typ, routeRegexpOptions{
strictSlash: r.strictSlash,
useEncodedPath: r.useEncodedPath,
})
if err != nil {
return err
}
Expand All @@ -193,7 +196,7 @@ func (r *Route) addRegexpMatcher(tpl string, matchHost, matchPrefix, matchQuery
return err
}
}
if matchHost {
if typ == regexpTypeHost {
if r.regexp.path != nil {
if err = uniqueVars(rr.varsN, r.regexp.path.varsN); err != nil {
return err
Expand All @@ -206,7 +209,7 @@ func (r *Route) addRegexpMatcher(tpl string, matchHost, matchPrefix, matchQuery
return err
}
}
if matchQuery {
if typ == regexpTypeQuery {
r.regexp.queries = append(r.regexp.queries, rr)
} else {
r.regexp.path = rr
Expand Down Expand Up @@ -289,7 +292,7 @@ func (r *Route) HeadersRegexp(pairs ...string) *Route {
// Variable names must be unique in a given route. They can be retrieved
// calling mux.Vars(request).
func (r *Route) Host(tpl string) *Route {
r.err = r.addRegexpMatcher(tpl, true, false, false)
r.err = r.addRegexpMatcher(tpl, regexpTypeHost)
return r
}

Expand Down Expand Up @@ -349,7 +352,7 @@ func (r *Route) Methods(methods ...string) *Route {
// Variable names must be unique in a given route. They can be retrieved
// calling mux.Vars(request).
func (r *Route) Path(tpl string) *Route {
r.err = r.addRegexpMatcher(tpl, false, false, false)
r.err = r.addRegexpMatcher(tpl, regexpTypePath)
return r
}

Expand All @@ -365,7 +368,7 @@ 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.err = r.addRegexpMatcher(tpl, false, true, false)
r.err = r.addRegexpMatcher(tpl, regexpTypePrefix)
return r
}

Expand Down Expand Up @@ -396,7 +399,7 @@ func (r *Route) Queries(pairs ...string) *Route {
return nil
}
for i := 0; i < length; i += 2 {
if r.err = r.addRegexpMatcher(pairs[i]+"="+pairs[i+1], false, false, true); r.err != nil {
if r.err = r.addRegexpMatcher(pairs[i]+"="+pairs[i+1], regexpTypeQuery); r.err != nil {
return r
}
}
Expand Down

0 comments on commit 512169e

Please sign in to comment.