update based on reviews

Author: salonichf5

internal/mode/static/nginx/config/servers.go

@@ -721,10 +721,6 @@ func createRouteMatch(match dataplane.Match, redirectPath string) routeMatch {
 // Query Parameters are case-sensitive so case is preserved.
 // The match type is optional and defaults to "Exact".
 func createQueryParamKeyValString(p dataplane.HTTPQueryParamMatch) string {
-	// this condition check is added for unit tests
-	if p.Type == "" {
-		p.Type = dataplane.MatchTypeExact
-	}
 	return p.Name + "=" + string(p.Type) + "=" + p.Value
 }
 
@@ -736,10 +732,6 @@ func createQueryParamKeyValString(p dataplane.HTTPQueryParamMatch) string {
 // We preserve the case of the name here because NGINX allows us to look up the header names in a case-insensitive
 // manner.
 func createHeaderKeyValString(h dataplane.HTTPHeaderMatch) string {
-	// this condition check is added for unit tests
-	if h.Type == "" {
-		h.Type = dataplane.MatchTypeExact
-	}
 	return h.Name + HeaderMatchSeparator + string(h.Type) + HeaderMatchSeparator + h.Value
 }
 

internal/mode/static/nginx/config/servers_test.go

@@ -589,6 +589,20 @@ func TestExecuteForDefaultServers(t *testing.T) {
 	}
 }
 
+func getHeaderMatchType(matchType string) dataplane.MatchType {
+	if len(matchType) == 0 {
+		return dataplane.MatchTypeExact
+	}
+	return dataplane.MatchType(matchType)
+}
+
+func getQueryParamMatchType(matchType string) dataplane.MatchType {
+	if len(matchType) == 0 {
+		return dataplane.MatchTypeExact
+	}
+	return dataplane.MatchType(matchType)
+}
+
 func TestCreateServers(t *testing.T) {
 	t.Parallel()
 	const (
@@ -710,25 +724,30 @@ func TestCreateServers(t *testing.T) {
 							{
 								Name:  "Version",
 								Value: "V1",
+								Type:  getHeaderMatchType(""),
 							},
 							{
 								Name:  "test",
 								Value: "foo",
+								Type:  getHeaderMatchType(""),
 							},
 							{
 								Name:  "my-header",
 								Value: "my-value",
+								Type:  getHeaderMatchType(""),
 							},
 						},
 						QueryParams: []dataplane.HTTPQueryParamMatch{
 							{
 								// query names and values should not be normalized to lowercase
 								Name:  "GrEat",
 								Value: "EXAMPLE",
+								Type:  getQueryParamMatchType(""),
 							},
 							{
 								Name:  "test",
 								Value: "foo=bar",
+								Type:  getQueryParamMatchType(""),
 							},
 						},
 					},
@@ -797,6 +816,7 @@ func TestCreateServers(t *testing.T) {
 							{
 								Name:  "redirect",
 								Value: "this",
+								Type:  getHeaderMatchType(""),
 							},
 						},
 					},
@@ -839,6 +859,7 @@ func TestCreateServers(t *testing.T) {
 							{
 								Name:  "rewrite",
 								Value: "this",
+								Type:  getHeaderMatchType(""),
 							},
 						},
 					},
@@ -878,6 +899,7 @@ func TestCreateServers(t *testing.T) {
 							{
 								Name:  "filter",
 								Value: "this",
+								Type:  getHeaderMatchType(""),
 							},
 						},
 					},
@@ -2702,14 +2724,17 @@ func TestCreateRouteMatch(t *testing.T) {
 		{
 			Name:  "header-1",
 			Value: "val-1",
+			Type:  getHeaderMatchType(""),
 		},
 		{
 			Name:  "header-2",
 			Value: "val-2",
+			Type:  getHeaderMatchType(""),
 		},
 		{
 			Name:  "header-3",
 			Value: "val-3",
+			Type:  getHeaderMatchType(""),
 		},
 	}
 
@@ -2725,14 +2750,17 @@ func TestCreateRouteMatch(t *testing.T) {
 		{
 			Name:  "arg1",
 			Value: "val1",
+			Type:  getQueryParamMatchType(""),
 		},
 		{
 			Name:  "arg2",
 			Value: "val2=another-val",
+			Type:  getQueryParamMatchType(""),
 		},
 		{
 			Name:  "arg3",
 			Value: "==val3",
+			Type:  getQueryParamMatchType(""),
 		},
 	}
 
@@ -2856,31 +2884,49 @@ func TestCreateRouteMatch(t *testing.T) {
 
 func TestCreateQueryParamKeyValString(t *testing.T) {
 	t.Parallel()
-	g := NewWithT(t)
 
-	expected := "key=Exact=value"
-
-	result := createQueryParamKeyValString(
-		dataplane.HTTPQueryParamMatch{
-			Name:  "key",
-			Value: "value",
-			Type:  dataplane.MatchTypeExact,
+	tests := []struct {
+		msg      string
+		input    dataplane.HTTPQueryParamMatch
+		expected string
+	}{
+		{
+			msg: "Exact match",
+			input: dataplane.HTTPQueryParamMatch{
+				Name:  "key",
+				Value: "value",
+				Type:  getQueryParamMatchType(string(dataplane.MatchTypeExact)),
+			},
+			expected: "key=Exact=value",
 		},
-	)
-
-	g.Expect(result).To(Equal(expected))
-
-	expected = "KeY=RegularExpression=vaLUe-[a-z]=="
-
-	result = createQueryParamKeyValString(
-		dataplane.HTTPQueryParamMatch{
-			Name:  "KeY",
-			Value: "vaLUe-[a-z]==",
-			Type:  dataplane.MatchTypeRegularExpression,
+		{
+			msg: "RegularExpression match",
+			input: dataplane.HTTPQueryParamMatch{
+				Name:  "KeY",
+				Value: "vaLUe-[a-z]==",
+				Type:  getQueryParamMatchType(string(dataplane.MatchTypeRegularExpression)),
+			},
+			expected: "KeY=RegularExpression=vaLUe-[a-z]==",
 		},
-	)
+		{
+			msg: "empty match type",
+			input: dataplane.HTTPQueryParamMatch{
+				Name:  "keY",
+				Value: "vaLUe==",
+				Type:  getQueryParamMatchType(""),
+			},
+			expected: "keY=Exact=vaLUe==",
+		},
+	}
 
-	g.Expect(result).To(Equal(expected))
+	for _, tc := range tests {
+		t.Run(tc.msg, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+			result := createQueryParamKeyValString(tc.input)
+			g.Expect(result).To(Equal(tc.expected))
+		})
+	}
 }
 
 func TestCreateHeaderKeyValString(t *testing.T) {
@@ -2893,7 +2939,7 @@ func TestCreateHeaderKeyValString(t *testing.T) {
 		dataplane.HTTPHeaderMatch{
 			Name:  "kEy",
 			Value: "vALUe",
-			Type:  dataplane.MatchTypeExact,
+			Type:  getHeaderMatchType(string(dataplane.MatchTypeExact)),
 		},
 	)
 
@@ -2905,7 +2951,7 @@ func TestCreateHeaderKeyValString(t *testing.T) {
 		dataplane.HTTPHeaderMatch{
 			Name:  "kEy",
 			Value: "vALUe-[0-9]",
-			Type:  dataplane.MatchTypeRegularExpression,
+			Type:  getHeaderMatchType(string(dataplane.MatchTypeRegularExpression)),
 		},
 	)
 

internal/mode/static/nginx/modules/src/httpmatches.js

@@ -210,9 +210,9 @@ function paramsMatch(requestParams, params) {
 		const idx = p.indexOf('=', firstIdx + 1);
 
 		// Three possible error cases for improperly constructed query parameter match:
-		// (1) if the index is -1, then there are no second occurence of "=" in the string (e.g. "Exactvalue")
+		// (1) if the index is -1, then there are no second occurence of "=" in the string (e.g. "key=Exactvalue")
 		// (2) if the index is 0, then there is no value in the string and has only one "=" (e.g. "key=Exact").
-		// (3) if the index is equal to length -1, then there is no key and type in the string (e.g. "=Exact=value").
+		// (3) if the index is equal to length -1, then there is no key and type in the string (e.g. "=value").
 		if (idx === -1 || idx === 0 || idx === p.length - 1) {
 			throw Error(`invalid query parameter: ${p}`);
 		}

internal/mode/static/state/dataplane/types.go

@@ -210,15 +210,17 @@ type HTTPURLRewriteFilter struct {
 // PathModifierType is the type of the PathModifier in a redirect or rewrite rule.
 type PathModifierType string
 
-// MatchType is the type of match in a MatchRule for headers and query parameters.
-type MatchType string
-
 const (
 	// ReplaceFullPath indicates that we replace the full path.
 	ReplaceFullPath PathModifierType = "ReplaceFullPath"
 	// ReplacePrefixMatch indicates that we replace a prefix match.
 	ReplacePrefixMatch PathModifierType = "ReplacePrefixMatch"
+)
 
+// MatchType is the type of match in a MatchRule for headers and query parameters.
+type MatchType string
+
+const (
 	// MatchTypeExact indicates that the match type is exact.
 	MatchTypeExact MatchType = "Exact"