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

Add new & improved glob matcher #457

Merged
merged 10 commits into from
Mar 11, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
11 changes: 2 additions & 9 deletions route/matcher.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package route

import (
"log"
"path"
"strings"
)

Expand All @@ -21,12 +19,7 @@ func prefixMatcher(uri string, r *Route) bool {
return strings.HasPrefix(uri, r.Path)
}

// globMatcher matches path to the routes' path using globbing.
// globMatcher matches path to the routes' path using gobwas/glob.
func globMatcher(uri string, r *Route) bool {
var hasMatch, err = path.Match(r.Path, uri)
if err != nil {
log.Printf("[ERROR] Glob matching error %s for path %s route %s", err, uri, r.Path)
return false
}
return hasMatch
return r.Matcher.Match(uri)
}
29 changes: 19 additions & 10 deletions route/matcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package route

import (
"testing"

"github.com/gobwas/glob"
)

func TestPrefixMatcher(t *testing.T) {
Expand Down Expand Up @@ -32,18 +34,21 @@ func TestGlobMatcher(t *testing.T) {
route *Route
}{
// happy flows
{uri: "/foo", matches: true, route: &Route{Path: "/foo"}},
{uri: "/fool", matches: true, route: &Route{Path: "/foo?"}},
{uri: "/fool", matches: true, route: &Route{Path: "/foo*"}},
{uri: "/fools", matches: true, route: &Route{Path: "/foo*"}},
{uri: "/fools", matches: true, route: &Route{Path: "/foo*"}},
{uri: "/foo/x/bar", matches: true, route: &Route{Path: "/foo/*/bar"}},
{uri: "/foo", matches: true, route: getRoute("/foo")},
{uri: "/fool", matches: true, route: getRoute("/foo?")},
{uri: "/fool", matches: true, route: getRoute("/foo*")},
{uri: "/fools", matches: true, route: getRoute("/foo*")},
{uri: "/fools", matches: true, route: getRoute("/foo*")},
{uri: "/foo/x/bar", matches: true, route: getRoute("/foo/*/bar")},
{uri: "/foo/x/y/z/w/bar", matches: true, route: getRoute("/foo/**")},
{uri: "/foo/x/y/z/w/bar", matches: true, route: getRoute("/foo/**/bar")},

// error flows
{uri: "/fo", matches: false, route: &Route{Path: "/foo"}},
{uri: "/fools", matches: false, route: &Route{Path: "/foo"}},
{uri: "/fo", matches: false, route: &Route{Path: "/foo*"}},
{uri: "/fools", matches: false, route: &Route{Path: "/foo.*"}},
{uri: "/fo", matches: false, route: getRoute("/foo")},
{uri: "/fools", matches: false, route: getRoute("/foo")},
{uri: "/fo", matches: false, route: getRoute("/foo*")},
{uri: "/fools", matches: false, route: getRoute("/foo.*")},
{uri: "/foo/x/y/z/w/baz", matches: false, route: getRoute("/foo/**/bar")},
}

for _, tt := range tests {
Expand All @@ -54,3 +59,7 @@ func TestGlobMatcher(t *testing.T) {
})
}
}

func getRoute(path string) *Route {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if you compile the glob matcher in the test since this keeps the changeset smaller, e.g.

for _, tt := range tests {
    t.Run(tt.uri, func(t *testing.T) {
        tt.route.Matcher = glob.MustCompile(tt.route.path)
        ...
    })
}

return &Route{Path: path, Matcher: glob.MustCompile(path)}
}
4 changes: 4 additions & 0 deletions route/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strconv"
"strings"

"github.com/gobwas/glob"
"github.com/fabiolb/fabio/metrics"
)

Expand Down Expand Up @@ -36,6 +37,9 @@ type Route struct {
// total contains the total number of requests for this route.
// Used by the RRPicker
total uint64

// Matcher represents compiled pattern.
Matcher glob.Glob
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Glob is a better name than Matcher.

}

func (r *Route) addTarget(service string, targetURL *url.URL, fixedWeight float64, tags []string, opts map[string]string) {
Expand Down
3 changes: 2 additions & 1 deletion route/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/fabiolb/fabio/metrics"
"github.com/ryanuber/go-glob"
glob2 "github.com/gobwas/glob"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please name this glob and also remove the github.com/ryanuber/go-glob library in a separate commit.

)

var errInvalidPrefix = errors.New("route: prefix must not be empty")
Expand Down Expand Up @@ -153,7 +154,7 @@ func (t Table) addRoute(d *RouteDef) error {
switch {
// add new host
case t[host] == nil:
r := &Route{Host: host, Path: path}
r := &Route{Host: host, Path: path, Matcher: glob2.MustCompile(path)}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this is an invalid pattern? Does it panic? You probably need to catch the error and return it.

r.addTarget(d.Service, targetURL, d.Weight, d.Tags, d.Opts)
t[host] = Routes{r}

Expand Down
21 changes: 21 additions & 0 deletions vendor/github.com/gobwas/glob/LICENSE

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions vendor/github.com/gobwas/glob/bench.sh

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading