From cf69537d1b4ea7405a0a969485eeb47191a51950 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Fri, 18 Dec 2020 14:12:10 +0100 Subject: [PATCH] small refactorings of test code and some methods --- proxy/pkg/proxy/policy/selector_test.go | 27 +++++ proxy/pkg/proxy/proxy.go | 37 +++--- proxy/pkg/proxy/proxy_test.go | 134 ++++++++++------------ thumbnails/pkg/thumbnail/encoding.go | 4 +- thumbnails/pkg/thumbnail/encoding_test.go | 22 ++++ 5 files changed, 127 insertions(+), 97 deletions(-) create mode 100644 thumbnails/pkg/thumbnail/encoding_test.go diff --git a/proxy/pkg/proxy/policy/selector_test.go b/proxy/pkg/proxy/policy/selector_test.go index afc26e13b..eda6b2207 100644 --- a/proxy/pkg/proxy/policy/selector_test.go +++ b/proxy/pkg/proxy/policy/selector_test.go @@ -12,6 +12,33 @@ import ( "github.com/owncloud/ocis/proxy/pkg/config" ) +func TestLoadSelector(t *testing.T) { + type test struct { + cfg *config.PolicySelector + expectedErr error + } + sCfg := &config.StaticSelectorConf{Policy: "reva"} + mcfg := &config.MigrationSelectorConf{ + AccFoundPolicy: "found", + AccNotFoundPolicy: "not_found", + UnauthenticatedPolicy: "unauth", + } + + table := []test{ + {cfg: &config.PolicySelector{Static: sCfg, Migration: mcfg}, expectedErr: ErrMultipleSelectors}, + {cfg: &config.PolicySelector{}, expectedErr: ErrSelectorConfigIncomplete}, + {cfg: &config.PolicySelector{Static: sCfg}, expectedErr: nil}, + {cfg: &config.PolicySelector{Migration: mcfg}, expectedErr: nil}, + } + + for _, test := range table { + _, err := LoadSelector(test.cfg) + if err != test.expectedErr { + t.Fail() + } + } +} + func TestStaticSelector(t *testing.T) { ctx := context.Background() req := httptest.NewRequest("GET", "https://example.org/foo", nil) diff --git a/proxy/pkg/proxy/proxy.go b/proxy/pkg/proxy/proxy.go index b4c2e7193..6b642624c 100644 --- a/proxy/pkg/proxy/proxy.go +++ b/proxy/pkg/proxy/proxy.go @@ -235,35 +235,32 @@ func (p *MultiHostReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request func (p MultiHostReverseProxy) queryRouteMatcher(endpoint string, target url.URL) bool { u, _ := url.Parse(endpoint) - if strings.HasPrefix(target.Path, u.Path) && endpoint != "/" { - query := u.Query() - if len(query) != 0 { - rQuery := target.Query() - match := true - for k := range query { - v := query.Get(k) - rv := rQuery.Get(k) - if rv != v { - match = false - break - } - } - return match + if !strings.HasPrefix(target.Path, u.Path) || endpoint == "/" { + return false + } + q := u.Query() + if len(q) == 0 { + return false + } + tq := target.Query() + for k := range q { + if q.Get(k) != tq.Get(k) { + return false } } - return false + return true } -func (p *MultiHostReverseProxy) regexRouteMatcher(endpoint string, target url.URL) bool { - matched, err := regexp.MatchString(endpoint, target.String()) +func (p *MultiHostReverseProxy) regexRouteMatcher(pattern string, target url.URL) bool { + matched, err := regexp.MatchString(pattern, target.String()) if err != nil { - p.logger.Warn().Err(err).Msgf("regex with pattern %s failed", endpoint) + p.logger.Warn().Err(err).Msgf("regex with pattern %s failed", pattern) } return matched } -func (p *MultiHostReverseProxy) prefixRouteMatcher(endpoint string, target url.URL) bool { - return strings.HasPrefix(target.Path, endpoint) && endpoint != "/" +func (p *MultiHostReverseProxy) prefixRouteMatcher(prefix string, target url.URL) bool { + return strings.HasPrefix(target.Path, prefix) && prefix != "/" } func defaultPolicies() []config.Policy { diff --git a/proxy/pkg/proxy/proxy_test.go b/proxy/pkg/proxy/proxy_test.go index 2e5e97340..8eefe3143 100644 --- a/proxy/pkg/proxy/proxy_test.go +++ b/proxy/pkg/proxy/proxy_test.go @@ -7,16 +7,27 @@ import ( "github.com/owncloud/ocis/proxy/pkg/config" ) +type matchertest struct { + endpoint, target string + matches bool +} + func TestPrefixRouteMatcher(t *testing.T) { cfg := config.New() p := NewMultiHostReverseProxy(Config(cfg)) - endpoint := "/foobar" - u, _ := url.Parse("/foobar/baz/some/url") + table := []matchertest{ + {endpoint: "/foobar", target: "/foobar/baz/some/url", matches: true}, + {endpoint: "/fobar", target: "/foobar/baz/some/url", matches: false}, + } - matched := p.prefixRouteMatcher(endpoint, *u) - if !matched { - t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + for _, test := range table { + u, _ := url.Parse(test.target) + matched := p.prefixRouteMatcher(test.endpoint, *u) + if matched != test.matches { + t.Errorf("PrefixRouteMatcher returned %t expected %t for endpoint: %s and target %s", + matched, test.matches, test.endpoint, u.String()) + } } } @@ -24,64 +35,26 @@ func TestQueryRouteMatcher(t *testing.T) { cfg := config.New() p := NewMultiHostReverseProxy(Config(cfg)) - endpoint := "/foobar?parameter=true" - u, _ := url.Parse("/foobar/baz/some/url?parameter=true") - - matched := p.queryRouteMatcher(endpoint, *u) - if !matched { - t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + table := []matchertest{ + {endpoint: "/foobar?parameter=true", target: "/foobar/baz/some/url?parameter=true", matches: true}, + {endpoint: "/foobar", target: "/foobar/baz/some/url?parameter=true", matches: false}, + {endpoint: "/foobar?parameter=false", target: "/foobar/baz/some/url?parameter=true", matches: false}, + {endpoint: "/foobar?parameter=false&other=true", target: "/foobar/baz/some/url?parameter=true", matches: false}, + { + endpoint: "/foobar?parameter=false&other=true", + target: "/foobar/baz/some/url?parameter=false&other=true", + matches: true, + }, + {endpoint: "/fobar", target: "/foobar", matches: false}, } -} -func TestQueryRouteMatcherWithoutParameters(t *testing.T) { - cfg := config.New() - p := NewMultiHostReverseProxy(Config(cfg)) - - endpoint := "/foobar" - u, _ := url.Parse("/foobar/baz/some/url?parameter=true") - - matched := p.queryRouteMatcher(endpoint, *u) - if matched { - t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) - } -} - -func TestQueryRouteMatcherWithDifferingParameters(t *testing.T) { - cfg := config.New() - p := NewMultiHostReverseProxy(Config(cfg)) - - endpoint := "/foobar?parameter=false" - u, _ := url.Parse("/foobar/baz/some/url?parameter=true") - - matched := p.queryRouteMatcher(endpoint, *u) - if matched { - t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) - } -} - -func TestQueryRouteMatcherWithMultipleDifferingParameters(t *testing.T) { - cfg := config.New() - p := NewMultiHostReverseProxy(Config(cfg)) - - endpoint := "/foobar?parameter=false&other=true" - u, _ := url.Parse("/foobar/baz/some/url?parameter=true") - - matched := p.queryRouteMatcher(endpoint, *u) - if matched { - t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) - } -} - -func TestQueryRouteMatcherWithMultipleParameters(t *testing.T) { - cfg := config.New() - p := NewMultiHostReverseProxy(Config(cfg)) - - endpoint := "/foobar?parameter=false&other=true" - u, _ := url.Parse("/foobar/baz/some/url?parameter=false&other=true") - - matched := p.queryRouteMatcher(endpoint, *u) - if !matched { - t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + for _, test := range table { + u, _ := url.Parse(test.target) + matched := p.queryRouteMatcher(test.endpoint, *u) + if matched != test.matches { + t.Errorf("QueryRouteMatcher returned %t expected %t for endpoint: %s and target %s", + matched, test.matches, test.endpoint, u.String()) + } } } @@ -89,24 +62,37 @@ func TestRegexRouteMatcher(t *testing.T) { cfg := config.New() p := NewMultiHostReverseProxy(Config(cfg)) - endpoint := ".*some\\/url.*parameter=true" - u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + table := []matchertest{ + {endpoint: ".*some\\/url.*parameter=true", target: "/foobar/baz/some/url?parameter=true", matches: true}, + {endpoint: "([\\])\\w+", target: "/foobar/baz/some/url?parameter=true", matches: false}, + } - matched := p.regexRouteMatcher(endpoint, *u) - if !matched { - t.Errorf("Endpoint %s and URL %s should match", endpoint, u.String()) + for _, test := range table { + u, _ := url.Parse(test.target) + matched := p.regexRouteMatcher(test.endpoint, *u) + if matched != test.matches { + t.Errorf("RegexRouteMatcher returned %t expected %t for endpoint: %s and target %s", + matched, test.matches, test.endpoint, u.String()) + } } } -func TestRegexRouteMatcherWithInvalidPattern(t *testing.T) { - cfg := config.New() - p := NewMultiHostReverseProxy(Config(cfg)) +func TestSingleJoiningSlash(t *testing.T) { + type test struct { + a, b, result string + } - endpoint := "([\\])\\w+" - u, _ := url.Parse("/foobar/baz/some/url?parameter=true") + table := []test{ + {a: "a", b: "b", result: "a/b"}, + {a: "a/", b: "b", result: "a/b"}, + {a: "a", b: "/b", result: "a/b"}, + {a: "a/", b: "/b", result: "a/b"}, + } - matched := p.regexRouteMatcher(endpoint, *u) - if matched { - t.Errorf("Endpoint %s and URL %s should not match", endpoint, u.String()) + for _, test := range table { + p := singleJoiningSlash(test.a, test.b) + if p != test.result { + t.Errorf("SingleJoiningSlash got %s expected %s", p, test.result) + } } } diff --git a/thumbnails/pkg/thumbnail/encoding.go b/thumbnails/pkg/thumbnail/encoding.go index d8d2d358b..b253ca3b5 100644 --- a/thumbnails/pkg/thumbnail/encoding.go +++ b/thumbnails/pkg/thumbnail/encoding.go @@ -60,9 +60,7 @@ func EncoderForType(fileType string) Encoder { switch strings.ToLower(fileType) { case "png": return PngEncoder{} - case "jpg": - fallthrough - case "jpeg": + case "jpg", "jpeg": return JpegEncoder{} default: return nil diff --git a/thumbnails/pkg/thumbnail/encoding_test.go b/thumbnails/pkg/thumbnail/encoding_test.go new file mode 100644 index 000000000..1dfbef60a --- /dev/null +++ b/thumbnails/pkg/thumbnail/encoding_test.go @@ -0,0 +1,22 @@ +package thumbnail + +import "testing" + +func TestEncoderForType(t *testing.T) { + table := map[string]Encoder{ + "jpg": JpegEncoder{}, + "JPG": JpegEncoder{}, + "jpeg": JpegEncoder{}, + "JPEG": JpegEncoder{}, + "png": PngEncoder{}, + "PNG": PngEncoder{}, + "invalid": nil, + } + + for k, v := range table { + e := EncoderForType(k) + if e != v { + t.Fail() + } + } +}