From 46920a1f1ae3d27e0fb087f1f5b45d13d48df9d0 Mon Sep 17 00:00:00 2001 From: Alex Shtin Date: Fri, 26 Mar 2021 14:05:46 -0700 Subject: [PATCH] Fix reserved search attribute name validation logic (#1398) --- common/searchattribute/defs.go | 115 ++++++++++++------ common/searchattribute/validator.go | 4 +- common/searchattribute/validator_test.go | 2 +- config/dynamicconfig/development_es.yaml | 2 - .../v6/visibility/index_template.json | 8 +- .../v7/visibility/index_template.json | 8 +- service/frontend/adminHandler.go | 2 +- 7 files changed, 83 insertions(+), 58 deletions(-) diff --git a/common/searchattribute/defs.go b/common/searchattribute/defs.go index b8fab7b3072..d1cc32f0598 100644 --- a/common/searchattribute/defs.go +++ b/common/searchattribute/defs.go @@ -38,34 +38,73 @@ const ( ExecutionTime = "ExecutionTime" CloseTime = "CloseTime" ExecutionStatus = "ExecutionStatus" - - // TODO (alex): move these 3 to non-indexed. - HistoryLength = "HistoryLength" - TaskQueue = "TaskQueue" - Encoding = "Encoding" + TaskQueue = "TaskQueue" // Valid non-indexed fields on ES. + HistoryLength = "HistoryLength" + Encoding = "Encoding" Memo = "Memo" VisibilityTaskKey = "VisibilityTaskKey" - // Attr is prefix of custom search attributes. + // Attr is prefix for search attributes. Attr = "Attr" - // Custom search attributes. - CustomStringField = "CustomStringField" - CustomKeywordField = "CustomKeywordField" - CustomIntField = "CustomIntField" - CustomDoubleField = "CustomDoubleField" - CustomBoolField = "CustomBoolField" - CustomDatetimeField = "CustomDatetimeField" + // System search attributes. TemporalChangeVersion = "TemporalChangeVersion" BinaryChecksums = "BinaryChecksums" CustomNamespace = "CustomNamespace" Operator = "Operator" + + // Default custom search attributes. + CustomStringField = "CustomStringField" + CustomKeywordField = "CustomKeywordField" + CustomIntField = "CustomIntField" + CustomDoubleField = "CustomDoubleField" + CustomBoolField = "CustomBoolField" + CustomDatetimeField = "CustomDatetimeField" ) var ( - // systemTypeMap is internal system search attributes. - systemTypeMap = map[string]enumspb.IndexedValueType{ + // reservedFields are internal field names that can't be used as search attribute names. + reservedFields = map[string]struct{}{ + NamespaceID: {}, + WorkflowID: {}, + RunID: {}, + WorkflowType: {}, + StartTime: {}, + ExecutionTime: {}, + CloseTime: {}, + ExecutionStatus: {}, + TaskQueue: {}, + + HistoryLength: {}, + Encoding: {}, + Memo: {}, + VisibilityTaskKey: {}, + } + + // systemSearchAttributes are internal system search attributes which don't require Attr prefix. + systemSearchAttributes = map[string]struct{}{ + NamespaceID: {}, + WorkflowID: {}, + RunID: {}, + WorkflowType: {}, + StartTime: {}, + ExecutionTime: {}, + CloseTime: {}, + ExecutionStatus: {}, + TaskQueue: {}, + } + + // builtInSearchAttributes are built-in internal search attributes. + builtInSearchAttributes = map[string]struct{}{ + TemporalChangeVersion: {}, + BinaryChecksums: {}, + CustomNamespace: {}, + Operator: {}, + } + + // defaultTypeMap defines default search attributes and their types. + defaultTypeMap = map[string]interface{}{ NamespaceID: enumspb.INDEXED_VALUE_TYPE_KEYWORD, WorkflowID: enumspb.INDEXED_VALUE_TYPE_KEYWORD, RunID: enumspb.INDEXED_VALUE_TYPE_KEYWORD, @@ -74,41 +113,41 @@ var ( ExecutionTime: enumspb.INDEXED_VALUE_TYPE_INT, CloseTime: enumspb.INDEXED_VALUE_TYPE_INT, ExecutionStatus: enumspb.INDEXED_VALUE_TYPE_INT, - HistoryLength: enumspb.INDEXED_VALUE_TYPE_INT, TaskQueue: enumspb.INDEXED_VALUE_TYPE_KEYWORD, - Encoding: enumspb.INDEXED_VALUE_TYPE_KEYWORD, - } - // defaultTypeMap defines all search attributes. - defaultTypeMap = newDefaultTypeMap() -) + CustomStringField: enumspb.INDEXED_VALUE_TYPE_STRING, + CustomKeywordField: enumspb.INDEXED_VALUE_TYPE_KEYWORD, + CustomIntField: enumspb.INDEXED_VALUE_TYPE_INT, + CustomDoubleField: enumspb.INDEXED_VALUE_TYPE_DOUBLE, + CustomBoolField: enumspb.INDEXED_VALUE_TYPE_BOOL, + CustomDatetimeField: enumspb.INDEXED_VALUE_TYPE_DATETIME, -func newDefaultTypeMap() map[string]interface{} { - result := map[string]interface{}{ - CustomStringField: enumspb.INDEXED_VALUE_TYPE_STRING, - CustomKeywordField: enumspb.INDEXED_VALUE_TYPE_KEYWORD, - CustomIntField: enumspb.INDEXED_VALUE_TYPE_INT, - CustomDoubleField: enumspb.INDEXED_VALUE_TYPE_DOUBLE, - CustomBoolField: enumspb.INDEXED_VALUE_TYPE_BOOL, - CustomDatetimeField: enumspb.INDEXED_VALUE_TYPE_DATETIME, TemporalChangeVersion: enumspb.INDEXED_VALUE_TYPE_KEYWORD, BinaryChecksums: enumspb.INDEXED_VALUE_TYPE_KEYWORD, CustomNamespace: enumspb.INDEXED_VALUE_TYPE_KEYWORD, Operator: enumspb.INDEXED_VALUE_TYPE_KEYWORD, } - for k, v := range systemTypeMap { - result[k] = v - } - return result +) + +// IsReservedField return true if field name is system reserved. +func IsReservedField(fieldName string) bool { + _, ok := reservedFields[fieldName] + return ok } -// GetDefaultTypeMap return default valid search attributes. -func GetDefaultTypeMap() map[string]interface{} { - return defaultTypeMap +// IsBuiltIn return true if search attribute is system built-in. +func IsBuiltIn(saName string) bool { + _, ok := builtInSearchAttributes[saName] + return ok } -// IsSystem return true if search attribute is system. +// IsSystem return true if search attribute is system and doesn't require Attr prefix. func IsSystem(saName string) bool { - _, ok := systemTypeMap[saName] + _, ok := systemSearchAttributes[saName] return ok } + +// GetDefaultTypeMap return default valid search attributes. +func GetDefaultTypeMap() map[string]interface{} { + return defaultTypeMap +} diff --git a/common/searchattribute/validator.go b/common/searchattribute/validator.go index 7054566a23f..c4dd767ab2c 100644 --- a/common/searchattribute/validator.go +++ b/common/searchattribute/validator.go @@ -106,8 +106,8 @@ func (v *Validator) Validate(searchAttributes *commonpb.SearchAttributes, namesp } // verify: saName is not system reserved - if IsSystem(saName) { - return serviceerror.NewInvalidArgument(fmt.Sprintf("%s is read-only Temporal reserved search attribute", saName)) + if IsReservedField(saName) { + return serviceerror.NewInvalidArgument(fmt.Sprintf("%s is Temporal reserved field name", saName)) } // verify: size of single value <= limit dataSize := len(saPayload.GetData()) diff --git a/common/searchattribute/validator_test.go b/common/searchattribute/validator_test.go index d8f6f2cff1d..0b4a89a0202 100644 --- a/common/searchattribute/validator_test.go +++ b/common/searchattribute/validator_test.go @@ -110,7 +110,7 @@ func (s *searchAttributesValidatorSuite) TestValidateSearchAttributes() { } attr.IndexedFields = fields err = validator.Validate(attr, namespace) - s.Equal("StartTime is read-only Temporal reserved search attribute", err.Error()) + s.Equal("StartTime is Temporal reserved field name", err.Error()) fields = map[string]*commonpb.Payload{ "CustomKeywordField": payload.EncodeString("123456"), diff --git a/config/dynamicconfig/development_es.yaml b/config/dynamicconfig/development_es.yaml index a085c193806..a49b92c924d 100644 --- a/config/dynamicconfig/development_es.yaml +++ b/config/dynamicconfig/development_es.yaml @@ -44,9 +44,7 @@ frontend.validSearchAttributes: ExecutionTime: "Int" CloseTime: "Int" ExecutionStatus: "Int" - HistoryLength: "Int" TaskQueue: "Keyword" - Encoding: "Keyword" CustomStringField: "String" CustomKeywordField: "Keyword" CustomIntField: "Int" diff --git a/schema/elasticsearch/v6/visibility/index_template.json b/schema/elasticsearch/v6/visibility/index_template.json index dce927683cc..bbb69b989a7 100644 --- a/schema/elasticsearch/v6/visibility/index_template.json +++ b/schema/elasticsearch/v6/visibility/index_template.json @@ -37,16 +37,10 @@ "ExecutionStatus": { "type": "long" }, - - "HistoryLength": { - "type": "long" - }, - "Encoding": { - "type": "keyword" - }, "TaskQueue": { "type": "keyword" }, + "Attr": { "properties": { "TemporalChangeVersion": { "type": "keyword" }, diff --git a/schema/elasticsearch/v7/visibility/index_template.json b/schema/elasticsearch/v7/visibility/index_template.json index 039db9926e0..73d18e7d9c3 100644 --- a/schema/elasticsearch/v7/visibility/index_template.json +++ b/schema/elasticsearch/v7/visibility/index_template.json @@ -37,16 +37,10 @@ "ExecutionStatus": { "type": "long" }, - - "HistoryLength": { - "type": "long" - }, - "Encoding": { - "type": "keyword" - }, "TaskQueue": { "type": "keyword" }, + "Attr": { "properties": { "TemporalChangeVersion": { diff --git a/service/frontend/adminHandler.go b/service/frontend/adminHandler.go index 2448f906211..6243b81dea1 100644 --- a/service/frontend/adminHandler.go +++ b/service/frontend/adminHandler.go @@ -162,7 +162,7 @@ func (adh *AdminHandler) AddSearchAttribute(ctx context.Context, request *admins currentValidAttr, _ := adh.params.DynamicConfig.GetMapValue( dynamicconfig.ValidSearchAttributes, nil, searchattribute.GetDefaultTypeMap()) for k, v := range searchAttr { - if searchattribute.IsSystem(k) { + if searchattribute.IsReservedField(k) || searchattribute.IsBuiltIn(k) { return nil, adh.error(serviceerror.NewInvalidArgument(fmt.Sprintf(errKeyIsReservedBySystemMessage, k)), scope) } if _, exist := currentValidAttr[k]; exist {