Decouples eskip.Template from filters.FilterContext (#1668)

The change #1528 unified template placeholder resolution across filters but introduced coupling between `eskip.Template` and `filters.FilterContext` This change decouples them by introducing `TemplateContext` so that it is possible to use template placeholders with non-filter context. Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Jan 11, 2021 · a45b4f2 · a45b4f2
1 parent cbc94b1
commit a45b4f2
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 83 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v0.12
+v0.13
diff --git a/eskip/template.go b/eskip/template.go
@@ -5,10 +5,9 @@
 package eskip
 
 import (
+	"net/http"
 	"regexp"
 	"strings"
-
-	"github.com/zalando/skipper/filters"
 )
 
 var placeholderRegexp = regexp.MustCompile(`\$\{([^{}]+)\}`)
@@ -22,6 +21,14 @@ type Template struct {
 	placeholders []string
 }
 
+type TemplateContext interface {
+	PathParam(string) string
+
+	Request() *http.Request
+
+	Response() *http.Response
+}
+
 // New parses a template string and returns a reusable *Template object.
 // The template string can contain named placeholders of the format:
 //
@@ -48,20 +55,10 @@ func (t *Template) Apply(get TemplateGetter) string {
 	return result
 }
 
-// ApplyRequestContext evaluates the template using a filter context and request attributes to resolve the
+// ApplyContext evaluates the template using template context to resolve the
 // placeholders. Returns true if all placeholders resolved to non-empty values.
-func (t *Template) ApplyRequestContext(ctx filters.FilterContext) (string, bool) {
-	return t.apply(contextGetter(ctx, false))
-}
-
-// ApplyResponseContext evaluates the template using a filter context, request and response attributes to resolve the
-// placeholders. Returns true if all placeholders resolved to non-empty values.
-func (t *Template) ApplyResponseContext(ctx filters.FilterContext) (string, bool) {
-	return t.apply(contextGetter(ctx, true))
-}
-
-func contextGetter(ctx filters.FilterContext, response bool) func(key string) string {
-	return func(key string) string {
+func (t *Template) ApplyContext(ctx TemplateContext) (string, bool) {
+	return t.apply(func(key string) string {
 		if h := strings.TrimPrefix(key, "request.header."); h != key {
 			return ctx.Request().Header.Get(h)
 		}
@@ -77,13 +74,13 @@ func contextGetter(ctx filters.FilterContext, response bool) func(key string) st
 		if key == "request.path" {
 			return ctx.Request().URL.Path
 		}
-		if response {
+		if ctx.Response() != nil {
 			if h := strings.TrimPrefix(key, "response.header."); h != key {
 				return ctx.Response().Header.Get(h)
 			}
 		}
 		return ctx.PathParam(key)
-	}
+	})
 }
 
 // apply evaluates the template using a TemplateGetter function to resolve the

diff --git a/eskip/template_test.go b/eskip/template_test.go
@@ -64,7 +64,7 @@ func TestTemplateGetter(t *testing.T) {
 	}})
 }
 
-func TestTemplateApplyRequestResponseContext(t *testing.T) {
+func TestTemplateApplyContext(t *testing.T) {
 	parseUrl := func(s string) *url.URL {
 		u, err := url.Parse(s)
 		if err != nil {
@@ -74,13 +74,11 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 	}
 
 	for _, ti := range []struct {
-		name           string
-		template       string
-		context        *filtertest.Context
-		requestExpect  string
-		requestOk      bool
-		responseExpect string
-		responseOk     bool
+		name     string
+		template string
+		context  *filtertest.Context
+		expected string
+		ok       bool
 	}{{
 		"path params",
 		"hello ${p1} ${p2}",
@@ -92,16 +90,12 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello path params",
 		true,
-		"hello path params",
-		true,
 	}, {
 		"all missing",
 		"hello ${p1} ${p2}",
 		&filtertest.Context{},
 		"hello  ",
 		false,
-		"hello  ",
-		false,
 	}, {
 		"some missing",
 		"hello ${p1} ${missing}",
@@ -112,8 +106,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello X ",
 		false,
-		"hello X ",
-		false,
 	}, {
 		"request header",
 		"hello ${request.header.X-Foo}",
@@ -126,8 +118,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello foo",
 		true,
-		"hello foo",
-		true,
 	}, {
 		"missing request header",
 		"hello ${request.header.X-Foo}",
@@ -136,22 +126,24 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello ",
 		false,
-		"hello ",
-		false,
 	}, {
 		"response header",
 		"hello ${response.header.X-Foo}",
 		&filtertest.Context{
 			FResponse: &http.Response{
 				Header: http.Header{
-					"X-Foo": []string{"foo"},
+					"X-Foo": []string{"bar"},
 				},
 			},
 		},
-		"hello ",
-		false, // response headers are not available in request context
-		"hello foo",
+		"hello bar",
 		true,
+	}, {
+		"response header when response is absent",
+		"hello ${response.header.X-Foo}",
+		&filtertest.Context{},
+		"hello ",
+		false,
 	}, {
 		"missing response header",
 		"hello ${response.header.X-Foo}",
@@ -160,8 +152,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello ",
 		false,
-		"hello ",
-		false,
 	}, {
 		"request and response headers (lowercase)",
 		"hello ${request.header.x-foo} ${response.header.x-foo}",
@@ -177,10 +167,20 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 				},
 			},
 		},
-		"hello bar ",
-		false, // response headers are not available in request context
 		"hello bar baz",
 		true,
+	}, {
+		"request and response headers when response is absent",
+		"hello ${request.header.x-foo} ${response.header.x-foo}",
+		&filtertest.Context{
+			FRequest: &http.Request{
+				Header: http.Header{
+					"X-Foo": []string{"bar"},
+				},
+			},
+		},
+		"hello bar ",
+		false,
 	}, {
 		"request query",
 		"hello ${request.query.q-Q} ${request.query.P_p}",
@@ -191,8 +191,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello foo bar",
 		true,
-		"hello foo bar",
-		true,
 	}, {
 		"missing request query",
 		"hello ${request.query.missing}",
@@ -203,8 +201,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello ",
 		false,
-		"hello ",
-		false,
 	}, {
 		"request cookie",
 		"hello ${request.cookie.foo} ${request.cookie.x}",
@@ -217,8 +213,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello bar y",
 		true,
-		"hello bar y",
-		true,
 	}, {
 		"missing request cookie",
 		"hello ${request.cookie.missing}",
@@ -231,8 +225,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello ",
 		false,
-		"hello ",
-		false,
 	}, {
 		"request path",
 		"hello ${request.path}",
@@ -243,8 +235,6 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello /foo/bar",
 		true,
-		"hello /foo/bar",
-		true,
 	}, {
 		"request path is empty",
 		"hello ${request.path}",
@@ -255,8 +245,7 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 		},
 		"hello ",
 		false, // path is empty
-		"hello ",
-		false,
+
 	}, {
 		"all in one",
 		"Hello ${name} ${request.header.name} ${request.query.name} ${request.cookie.name} ${response.header.name}",
@@ -277,22 +266,15 @@ func TestTemplateApplyRequestResponseContext(t *testing.T) {
 				},
 			},
 		},
-		"Hello one two three four ",
-		false, // response headers are not available in request context
 		"Hello one two three four five",
 		true,
 	},
 	} {
 		t.Run(ti.name, func(t *testing.T) {
 			template := NewTemplate(ti.template)
-			result, ok := template.ApplyRequestContext(ti.context)
-			if result != ti.requestExpect || ok != ti.requestOk {
-				t.Errorf("Apply request context result mismatch: '%s' (%v) != '%s' (%v)", result, ok, ti.requestExpect, ti.requestOk)
-			}
-
-			result, ok = template.ApplyResponseContext(ti.context)
-			if result != ti.responseExpect || ok != ti.responseOk {
-				t.Errorf("Apply response context result mismatch: '%s' (%v) != '%s' (%v)", result, ok, ti.responseExpect, ti.responseOk)
+			result, ok := template.ApplyContext(ti.context)
+			if result != ti.expected || ok != ti.ok {
+				t.Errorf("Apply context result mismatch: '%s' (%v) != '%s' (%v)", result, ok, ti.expected, ti.ok)
 			}
 		})
 	}

diff --git a/filters/builtin/headerfilter.go b/filters/builtin/headerfilter.go
@@ -90,7 +90,7 @@ func NewResponseHeader() filters.Spec {
 
 // Returns a filter specification that is used to set headers for requests.
 // Instances expect two parameters: the header name and the header value template,
-// see eskip.Template.ApplyRequestContext
+// see eskip.Template.ApplyContext
 // Name: "setRequestHeader".
 //
 // If the header name is 'Host', the filter uses the `SetOutgoingHost()`
@@ -102,7 +102,7 @@ func NewSetRequestHeader() filters.Spec {
 
 // Returns a filter specification that is used to append headers for requests.
 // Instances expect two parameters: the header name and the header value template,
-// see eskip.Template.ApplyRequestContext
+// see eskip.Template.ApplyContext
 // Name: "appendRequestHeader".
 //
 // If the header name is 'Host', the filter uses the `SetOutgoingHost()`
@@ -121,15 +121,15 @@ func NewDropRequestHeader() filters.Spec {
 
 // Returns a filter specification that is used to set headers for responses.
 // Instances expect two parameters: the header name and the header value template,
-// see eskip.Template.ApplyResponseContext
+// see eskip.Template.ApplyContext
 // Name: "setResponseHeader".
 func NewSetResponseHeader() filters.Spec {
 	return &headerFilter{typ: setResponseHeader}
 }
 
 // Returns a filter specification that is used to append headers for responses.
 // Instances expect two parameters: the header name and the header value template,
-// see eskip.Template.ApplyResponseContext
+// see eskip.Template.ApplyContext
 // Name: "appendResponseHeader".
 func NewAppendResponseHeader() filters.Spec {
 	return &headerFilter{typ: appendResponseHeader}
@@ -260,15 +260,15 @@ func (f *headerFilter) Request(ctx filters.FilterContext) {
 	header := ctx.Request().Header
 	switch f.typ {
 	case setRequestHeader:
-		value, ok := f.template.ApplyRequestContext(ctx)
+		value, ok := f.template.ApplyContext(ctx)
 		if ok {
 			header.Set(f.key, value)
 			if strings.ToLower(f.key) == "host" {
 				ctx.SetOutgoingHost(value)
 			}
 		}
 	case appendRequestHeader:
-		value, ok := f.template.ApplyRequestContext(ctx)
+		value, ok := f.template.ApplyContext(ctx)
 		if ok {
 			header.Add(f.key, value)
 			if strings.ToLower(f.key) == "host" {
@@ -301,12 +301,12 @@ func (f *headerFilter) Response(ctx filters.FilterContext) {
 	header := ctx.Response().Header
 	switch f.typ {
 	case setResponseHeader:
-		value, ok := f.template.ApplyResponseContext(ctx)
+		value, ok := f.template.ApplyContext(ctx)
 		if ok {
 			header.Set(f.key, value)
 		}
 	case appendResponseHeader:
-		value, ok := f.template.ApplyResponseContext(ctx)
+		value, ok := f.template.ApplyContext(ctx)
 		if ok {
 			header.Add(f.key, value)
 		}

diff --git a/filters/builtin/path.go b/filters/builtin/path.go
@@ -31,7 +31,7 @@ func NewModPath() filters.Spec { return &modPath{behavior: regexpReplace} }
 // the request path.
 //
 // Instances expect one parameter: the new path to be set, or the path
-// template to be evaluated, see eskip.Template.ApplyRequestContext
+// template to be evaluated, see eskip.Template.ApplyContext
 //
 // Name: "setPath".
 func NewSetPath() filters.Spec { return &modPath{behavior: fullReplace} }
@@ -105,7 +105,7 @@ func (f *modPath) Request(ctx filters.FilterContext) {
 	case regexpReplace:
 		req.URL.Path = f.rx.ReplaceAllString(req.URL.Path, f.replacement)
 	case fullReplace:
-		req.URL.Path, _ = f.template.ApplyRequestContext(ctx)
+		req.URL.Path, _ = f.template.ApplyContext(ctx)
 	default:
 		panic("unspecified behavior")
 	}