Fix bug to query header search attributes correctly in visibility (ca…

…dence-workflow#6163)

What changed?

Change the format of header search attributes in visibility and added sanitization
Revert https://github.com/uber/cadence/pull/6144/files
Why?

search attributes cannot contain . and this assumption exists in multiple places in the code
sqlparser library does not allow - in the column name in where clause.
How did you test it?

unit test

Potential risks

This feature only exists in pre-releases and internally it's not rolled out. Additionally, this feature is also behind a feature flag so it's very safe.

common/definition/indexedKeys.go

@@ -58,7 +58,7 @@ const (
 	// Attr is prefix of custom search attributes
 	Attr = "Attr"
 	// HeaderFormat is the format of context headers in search attributes
-	HeaderFormat = "Header.%s"
+	HeaderFormat = "Header_%s"
 )
 
 // defaultIndexedKeys defines all searchable keys

common/elasticsearch/validator/queryValidator_test.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/uber/cadence/common/definition"
 	"github.com/uber/cadence/common/dynamicconfig"
+	"github.com/uber/cadence/common/types"
 )
 
 func TestValidateQuery(t *testing.T) {
@@ -35,6 +36,7 @@ func TestValidateQuery(t *testing.T) {
 		query     string
 		validated string
 		err       string
+		dcValid   map[string]interface{}
 	}{
 		{
 			msg:       "empty query",
@@ -126,11 +128,54 @@ func TestValidateQuery(t *testing.T) {
 			query: "WorkflowID = 'wid' union select * from dummy",
 			err:   "Invalid select query.",
 		},
+		{
+			msg:       "valid custom search attribute",
+			query:     "CustomStringField = 'value'",
+			validated: "`Attr.CustomStringField` = 'value'",
+		},
+		{
+			msg:       "custom search attribute can contain underscore",
+			query:     "Custom_Field = 'value'",
+			validated: "`Attr.Custom_Field` = 'value'",
+			dcValid: map[string]interface{}{
+				"Custom_Field": types.IndexedValueTypeString,
+			},
+		},
+		{
+			msg:       "custom search attribute can contain number",
+			query:     "Custom_0 = 'value'",
+			validated: "`Attr.Custom_0` = 'value'",
+			dcValid: map[string]interface{}{
+				"Custom_0": types.IndexedValueTypeString,
+			},
+		},
+		{
+			msg:   "customg search attribute cannot contain dot",
+			query: "Custom.Field = 'value'",
+			err:   "invalid search attribute \"Field\"",
+			dcValid: map[string]interface{}{
+				"Custom.Field": types.IndexedValueTypeString,
+			},
+		},
+		{
+			msg:   "custom search attribute cannot contain dash",
+			query: "Custom-Field = 'value'",
+			err:   "invalid comparison expression",
+			dcValid: map[string]interface{}{
+				"Custom-Field": types.IndexedValueTypeString,
+			},
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.msg, func(t *testing.T) {
-			validSearchAttr := dynamicconfig.GetMapPropertyFn(definition.GetDefaultIndexedKeys())
+			validSearchAttr := func(opts ...dynamicconfig.FilterOption) map[string]interface{} {
+				valid := definition.GetDefaultIndexedKeys()
+				for k, v := range tt.dcValid {
+					valid[k] = v
+				}
+				return valid
+			}
 			validateSearchAttr := dynamicconfig.GetBoolPropertyFn(true)
 			qv := NewQueryValidator(validSearchAttr, validateSearchAttr)
 			validated, err := qv.ValidateQuery(tt.query)

common/visibility/validate_search_attribute_key.go

@@ -0,0 +1,52 @@
+// The MIT License (MIT)
+
+// Copyright (c) 2017-2020 Uber Technologies Inc.
+
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+package visibility
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+)
+
+var (
+	validSearchAttributeKey = regexp.MustCompile(`^[a-zA-Z][a-zA-Z_0-9]*$`)
+	nonAlphanumericRegex    = regexp.MustCompile(`[^a-zA-Z0-9]+`)
+)
+
+// ValidateSearchAttributeKey checks if the search attribute key has valid format
+func ValidateSearchAttributeKey(name string) error {
+	if !validSearchAttributeKey.MatchString(name) {
+		return fmt.Errorf("has to be contain alphanumeric and start with a letter")
+	}
+	return nil
+}
+
+// SanitizeSearchAttributeKey try to sanitize the search attribute key
+func SanitizeSearchAttributeKey(name string) (string, error) {
+	sanitized := nonAlphanumericRegex.ReplaceAllString(name, "_")
+	// remove leading and trailing underscores
+	sanitized = strings.Trim(sanitized, "_")
+	// remove leading numbers
+	sanitized = strings.TrimLeft(sanitized, "0123456789")
+	return sanitized, ValidateSearchAttributeKey(sanitized)
+}

common/visibility/validate_search_attribute_key_test.go

@@ -0,0 +1,120 @@
+// The MIT License (MIT)
+
+// Copyright (c) 2017-2020 Uber Technologies Inc.
+
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+package visibility
+
+import (
+	"testing"
+)
+
+func TestValidateSearchAttributeKey(t *testing.T) {
+	type args struct {
+		name string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "pure character",
+			args: args{name: "CustomStringField"},
+		},
+		{
+			name: "alphanumeric",
+			args: args{name: "CustomStringField1"},
+		},
+		{
+			name:    "start with number",
+			args:    args{name: "1CustomStringField"},
+			wantErr: true,
+		},
+		{
+			name:    "contain special character",
+			args:    args{name: "CustomStringField!"},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := ValidateSearchAttributeKey(tt.args.name); (err != nil) != tt.wantErr {
+				t.Errorf("ValidateSearchAttributeKey() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestSanitizeSearchAttributeKey(t *testing.T) {
+	type args struct {
+		name string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    string
+		wantErr bool
+	}{
+		{
+			name: "pure character",
+			args: args{name: "CustomStringField"},
+			want: "CustomStringField",
+		},
+		{
+			name: "alphanumeric",
+			args: args{name: "CustomStringField1"},
+			want: "CustomStringField1",
+		},
+		{
+			name: "start with number",
+			args: args{name: "1CustomStringField"},
+			want: "CustomStringField",
+		},
+		{
+			name: "contain special character",
+			args: args{name: "CustomStringField!"},
+			want: "CustomStringField",
+		},
+		{
+			name: "contain special character in the middle",
+			args: args{name: "CustomString-Field"},
+			want: "CustomString_Field",
+		},
+		{
+			name:    "all numbers",
+			args:    args{name: "1234567890"},
+			want:    "",
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := SanitizeSearchAttributeKey(tt.args.name)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("SanitizeSearchAttributeKey() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("SanitizeSearchAttributeKey() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

service/history/task/transfer_active_task_executor_test.go

@@ -1543,7 +1543,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessRecordWorkflowStartedTaskWi
 	s.mockShard.GetConfig().EnableContextHeaderInVisibility = func(domain string) bool { return true }
 	s.mockShard.GetConfig().ValidSearchAttributes = func(opts ...dc.FilterOption) map[string]interface{} {
 		return map[string]interface{}{
-			"Header.contextKey": struct{}{},
+			"Header_context_key": struct{}{},
 		}
 	}
 
@@ -1626,7 +1626,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessUpsertWorkflowSearchAttribu
 	s.mockShard.GetConfig().EnableContextHeaderInVisibility = func(domain string) bool { return true }
 	s.mockShard.GetConfig().ValidSearchAttributes = func(opts ...dc.FilterOption) map[string]interface{} {
 		return map[string]interface{}{
-			"Header.contextKey": struct{}{},
+			"Header_context_key": struct{}{},
 		}
 	}
 
@@ -1782,7 +1782,7 @@ func createRecordWorkflowExecutionStartedRequest(
 			t.Fatal(err)
 		}
 		searchAttributes = map[string][]byte{
-			"Header.contextKey": contextValueJSONString,
+			"Header_context_key": contextValueJSONString,
 		}
 	}
 	return &persistence.RecordWorkflowExecutionStartedRequest{
@@ -1937,7 +1937,7 @@ func createUpsertWorkflowSearchAttributesRequest(
 			t.Fatal(err)
 		}
 		searchAttributes = map[string][]byte{
-			"Header.contextKey": contextValueJSONString,
+			"Header_context_key": contextValueJSONString,
 		}
 	}
 

service/history/task/transfer_standby_task_executor_test.go

@@ -815,7 +815,7 @@ func (s *transferStandbyTaskExecutorSuite) TestProcessRecordWorkflowStartedTaskW
 	}
 	s.mockShard.GetConfig().ValidSearchAttributes = func(opts ...dc.FilterOption) map[string]interface{} {
 		return map[string]interface{}{
-			"Header.contextKey": struct{}{},
+			"Header_context_key": struct{}{},
 		}
 	}
 
@@ -906,7 +906,7 @@ func (s *transferStandbyTaskExecutorSuite) TestProcessUpsertWorkflowSearchAttrib
 	}
 	s.mockShard.GetConfig().ValidSearchAttributes = func(opts ...dc.FilterOption) map[string]interface{} {
 		return map[string]interface{}{
-			"Header.contextKey": struct{}{},
+			"Header_context_key": struct{}{},
 		}
 	}
 

service/history/task/transfer_task_executor_base.go

@@ -36,6 +36,7 @@ import (
 	"github.com/uber/cadence/common/persistence"
 	"github.com/uber/cadence/common/service"
 	"github.com/uber/cadence/common/types"
+	"github.com/uber/cadence/common/visibility"
 	"github.com/uber/cadence/service/history/config"
 	"github.com/uber/cadence/service/history/execution"
 	"github.com/uber/cadence/service/history/shard"
@@ -402,7 +403,12 @@ func getWorkflowMemo(
 
 func appendContextHeaderToSearchAttributes(attr, context map[string][]byte, allowedKeys map[string]interface{}) (map[string][]byte, error) {
 	for k, v := range context {
-		key := fmt.Sprintf(definition.HeaderFormat, k)
+		unsanitizedKey := fmt.Sprintf(definition.HeaderFormat, k)
+		key, err := visibility.SanitizeSearchAttributeKey(unsanitizedKey)
+		if err != nil {
+			return nil, fmt.Errorf("fail to sanitize context key %s: %w", key, err)
+		}
+
 		if _, ok := attr[key]; ok { // skip if key already exists
 			continue
 		}

service/history/testing/workflow_util.go

@@ -65,8 +65,8 @@ func StartWorkflow(
 				ExecutionStartToCloseTimeoutSeconds: common.Int32Ptr(2),
 				TaskStartToCloseTimeoutSeconds:      common.Int32Ptr(1),
 				Header: &types.Header{Fields: map[string][]byte{
-					"contextKey":        []byte("contextValue"),
-					"invalidContextKey": []byte("invalidContextValue"),
+					"context-key":         []byte("contextValue"),
+					"invalid-context-key": []byte("invalidContextValue"),
 				}},
 			},
 			PartitionConfig: map[string]string{"userid": uuid.New()},

tools/cli/admin_cluster_commands.go

@@ -23,25 +23,24 @@ package cli
 import (
 	"encoding/json"
 	"fmt"
-	"regexp"
 
 	"github.com/fatih/color"
 	"github.com/pborman/uuid"
 	"github.com/urfave/cli"
 
 	"github.com/uber/cadence/common"
 	"github.com/uber/cadence/common/types"
+	"github.com/uber/cadence/common/visibility"
 	"github.com/uber/cadence/service/worker/failovermanager"
 )
 
 // An indirection for the prompt function so that it can be mocked in the unit tests
 var promptFn = prompt
-var validSearchAttributeKey = regexp.MustCompile(`^[a-zA-Z][a-zA-Z_.-0-9]*$`)
 
 // AdminAddSearchAttribute to whitelist search attribute
 func AdminAddSearchAttribute(c *cli.Context) {
 	key := getRequiredOption(c, FlagSearchAttributesKey)
-	if err := validateSearchAttributeKey(key); err != nil {
+	if err := visibility.ValidateSearchAttributeKey(key); err != nil {
 		ErrorAndExit("Invalid search-attribute key.", err)
 	}
 
@@ -158,13 +157,6 @@ func intValTypeToString(valType int) string {
 	}
 }
 
-func validateSearchAttributeKey(name string) error {
-	if !validSearchAttributeKey.MatchString(name) {
-		return fmt.Errorf("has to be contain alphanumeric and start with a letter")
-	}
-	return nil
-}
-
 func isValueTypeValid(valType int) bool {
 	return valType >= 0 && valType <= 5
 }