diff --git a/common/definition/indexedKeys.go b/common/definition/indexedKeys.go index 50a6c3fd309..d9f993fb9d0 100644 --- a/common/definition/indexedKeys.go +++ b/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 diff --git a/common/elasticsearch/validator/queryValidator_test.go b/common/elasticsearch/validator/queryValidator_test.go index d3d0c39344d..68fd433faa0 100644 --- a/common/elasticsearch/validator/queryValidator_test.go +++ b/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) diff --git a/common/visibility/validate_search_attribute_key.go b/common/visibility/validate_search_attribute_key.go new file mode 100644 index 00000000000..8a438e083d8 --- /dev/null +++ b/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) +} diff --git a/common/visibility/validate_search_attribute_key_test.go b/common/visibility/validate_search_attribute_key_test.go new file mode 100644 index 00000000000..cab1e0d5072 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/service/history/task/transfer_active_task_executor_test.go b/service/history/task/transfer_active_task_executor_test.go index f9a3f8496d7..ac691bb30af 100644 --- a/service/history/task/transfer_active_task_executor_test.go +++ b/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, } } diff --git a/service/history/task/transfer_standby_task_executor_test.go b/service/history/task/transfer_standby_task_executor_test.go index 1891a08505a..8e39f100f3e 100644 --- a/service/history/task/transfer_standby_task_executor_test.go +++ b/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{}{}, } } diff --git a/service/history/task/transfer_task_executor_base.go b/service/history/task/transfer_task_executor_base.go index 002e7407a4b..7b10ceebc12 100644 --- a/service/history/task/transfer_task_executor_base.go +++ b/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 } diff --git a/service/history/testing/workflow_util.go b/service/history/testing/workflow_util.go index 21210119722..016082c7a53 100644 --- a/service/history/testing/workflow_util.go +++ b/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()}, diff --git a/tools/cli/admin_cluster_commands.go b/tools/cli/admin_cluster_commands.go index 0f5c1fb106e..bc653c1be7d 100644 --- a/tools/cli/admin_cluster_commands.go +++ b/tools/cli/admin_cluster_commands.go @@ -23,7 +23,6 @@ package cli import ( "encoding/json" "fmt" - "regexp" "github.com/fatih/color" "github.com/pborman/uuid" @@ -31,17 +30,17 @@ import ( "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 } diff --git a/tools/cli/admin_cluster_commands_test.go b/tools/cli/admin_cluster_commands_test.go index c0ece6f8437..884e7fbdb8c 100644 --- a/tools/cli/admin_cluster_commands_test.go +++ b/tools/cli/admin_cluster_commands_test.go @@ -33,6 +33,7 @@ import ( "github.com/uber/cadence/client/frontend" "github.com/uber/cadence/common" "github.com/uber/cadence/common/types" + "github.com/uber/cadence/common/visibility" ) func TestAdminAddSearchAttribute_isValueTypeValid(t *testing.T) { @@ -122,12 +123,12 @@ func TestAdminFailover(t *testing.T) { } func TestValidSearchAttributeKey(t *testing.T) { - assert.NoError(t, validateSearchAttributeKey("city")) - assert.NoError(t, validateSearchAttributeKey("cityId")) - assert.NoError(t, validateSearchAttributeKey("paymentProfileUUID")) - assert.NoError(t, validateSearchAttributeKey("job_type")) - assert.NoError(t, validateSearchAttributeKey("Header.ctx-tenancy")) + assert.NoError(t, visibility.ValidateSearchAttributeKey("city")) + assert.NoError(t, visibility.ValidateSearchAttributeKey("cityId")) + assert.NoError(t, visibility.ValidateSearchAttributeKey("paymentProfileUUID")) + assert.NoError(t, visibility.ValidateSearchAttributeKey("job_type")) - assert.Error(t, validateSearchAttributeKey("9lives")) - assert.Error(t, validateSearchAttributeKey("tax%")) + assert.Error(t, visibility.ValidateSearchAttributeKey("payments-biling-invoices-TransactionUUID")) + assert.Error(t, visibility.ValidateSearchAttributeKey("9lives")) + assert.Error(t, visibility.ValidateSearchAttributeKey("tax%")) }