Merge pull request #1142 from owncloud/small-refactoring

small refactorings of test code and some methods

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)

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 {

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)
+		}
 	}
 }

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

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()
+		}
+	}
+}