diff --git a/common/util.go b/common/util.go index bcd4d3f16cf..258631b2878 100644 --- a/common/util.go +++ b/common/util.go @@ -224,8 +224,8 @@ func CreateReplicationServiceBusyRetryPolicy() backoff.RetryPolicy { return policy } -// ValidIDLength checks if id is valid according to its length -func ValidIDLength( +// IsValidIDLength checks if id is valid according to its length +func IsValidIDLength( id string, scope metrics.Scope, warnLimit int, @@ -235,20 +235,14 @@ func ValidIDLength( logger log.Logger, idTypeViolationTag tag.Tag, ) bool { - idLength := len(id) - valid := idLength <= errorLimit - if idLength > warnLimit { - if scope != nil { - scope.IncCounter(metricsCounter) - } - if logger != nil { - logger.Warn("ID length exceeds limit.", - tag.WorkflowDomainName(domainName), - tag.Name(id), - idTypeViolationTag) - } + if len(id) > warnLimit { + scope.IncCounter(metricsCounter) + logger.Warn("ID length exceeds limit.", + tag.WorkflowDomainName(domainName), + tag.Name(id), + idTypeViolationTag) } - return valid + return len(id) <= errorLimit } // CheckDecisionResultLimit checks if decision result count exceeds limits. diff --git a/common/util_test.go b/common/util_test.go index 60e6639eaec..913da29a927 100644 --- a/common/util_test.go +++ b/common/util_test.go @@ -37,7 +37,9 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/yarpc/yarpcerrors" + "github.com/uber/cadence/common/log" "github.com/uber/cadence/common/log/tag" + "github.com/uber/cadence/common/metrics" "github.com/uber/cadence/common/types" ) @@ -424,13 +426,55 @@ func TestAwaitWaitGroup(t *testing.T) { }) } -func TestValidIDLength(t *testing.T) { +func TestIsValidIDLength(t *testing.T) { + var ( + // test setup + scope = metrics.NoopScope(0) + + // arguments + metricCounter = 0 + idTypeViolationTag = tag.ClusterName("idTypeViolationTag") + domainName = "domain_name" + id = "12345" + ) + + mockWarnCall := func(logger *log.MockLogger) { + logger.On( + "Warn", + "ID length exceeds limit.", + []tag.Tag{ + tag.WorkflowDomainName(domainName), + tag.Name(id), + idTypeViolationTag, + }, + ).Once() + } + t.Run("valid id length, no warnings", func(t *testing.T) { - got := ValidIDLength("12345", nil, 7, 10, 0, "", nil, tag.Tag{}) + logger := new(log.MockLogger) + got := IsValidIDLength(id, scope, 7, 10, metricCounter, domainName, logger, idTypeViolationTag) require.True(t, got, "expected true, because id length is 5 and it's less than error limit 10") }) + + t.Run("valid id length, with warnings", func(t *testing.T) { + logger := new(log.MockLogger) + mockWarnCall(logger) + + got := IsValidIDLength(id, scope, 4, 10, metricCounter, domainName, logger, idTypeViolationTag) + require.True(t, got, "expected true, because id length is 5 and it's less than error limit 10") + + // logger should be called once + logger.AssertExpectations(t) + }) + t.Run("non valid id length", func(t *testing.T) { - got := ValidIDLength("12345", nil, 1, 4, 0, "", nil, tag.Tag{}) + logger := new(log.MockLogger) + mockWarnCall(logger) + + got := IsValidIDLength(id, scope, 1, 4, metricCounter, domainName, logger, idTypeViolationTag) require.False(t, got, "expected false, because id length is 5 and it's more than error limit 4") + + // logger should be called once + logger.AssertExpectations(t) }) } diff --git a/service/frontend/workflowHandler.go b/service/frontend/workflowHandler.go index 89a3de74b70..a7b76c5e52b 100644 --- a/service/frontend/workflowHandler.go +++ b/service/frontend/workflowHandler.go @@ -557,7 +557,7 @@ func (wh *WorkflowHandler) PollForActivityTask( } idLengthWarnLimit := wh.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( domainName, scope, idLengthWarnLimit, @@ -573,7 +573,7 @@ func (wh *WorkflowHandler) PollForActivityTask( return nil, wh.error(err, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( pollRequest.GetIdentity(), scope, idLengthWarnLimit, @@ -680,7 +680,7 @@ func (wh *WorkflowHandler) PollForDecisionTask( } idLengthWarnLimit := wh.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( domainName, scope, idLengthWarnLimit, @@ -692,7 +692,7 @@ func (wh *WorkflowHandler) PollForDecisionTask( return nil, wh.error(errDomainTooLong, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( pollRequest.GetIdentity(), scope, idLengthWarnLimit, @@ -1135,7 +1135,7 @@ func (wh *WorkflowHandler) RespondActivityTaskCompleted( RunID: taskToken.RunID, }) - if !common.ValidIDLength( + if !common.IsValidIDLength( completeRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1242,7 +1242,7 @@ func (wh *WorkflowHandler) RespondActivityTaskCompletedByID( return wh.error(errActivityIDNotSet, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( completeRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1369,7 +1369,7 @@ func (wh *WorkflowHandler) RespondActivityTaskFailed( RunID: taskToken.RunID, }) - if !common.ValidIDLength( + if !common.IsValidIDLength( failedRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1464,7 +1464,7 @@ func (wh *WorkflowHandler) RespondActivityTaskFailedByID( return wh.error(errActivityIDNotSet, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( failedRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1582,7 +1582,7 @@ func (wh *WorkflowHandler) RespondActivityTaskCanceled( RunID: taskToken.RunID, }) - if !common.ValidIDLength( + if !common.IsValidIDLength( cancelRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1689,7 +1689,7 @@ func (wh *WorkflowHandler) RespondActivityTaskCanceledByID( return wh.error(errActivityIDNotSet, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( cancelRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1816,7 +1816,7 @@ func (wh *WorkflowHandler) RespondDecisionTaskCompleted( RunID: taskToken.RunID, }) - if !common.ValidIDLength( + if !common.IsValidIDLength( completeRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -1926,7 +1926,7 @@ func (wh *WorkflowHandler) RespondDecisionTaskFailed( RunID: taskToken.RunID, }) - if !common.ValidIDLength( + if !common.IsValidIDLength( failedRequest.GetIdentity(), scope, wh.config.MaxIDLengthWarnLimit(), @@ -2107,7 +2107,7 @@ func (wh *WorkflowHandler) StartWorkflowExecution( } idLengthWarnLimit := wh.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( domainName, scope, idLengthWarnLimit, @@ -2119,7 +2119,7 @@ func (wh *WorkflowHandler) StartWorkflowExecution( return nil, wh.error(errDomainTooLong, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( startRequest.GetWorkflowID(), scope, idLengthWarnLimit, @@ -2139,7 +2139,7 @@ func (wh *WorkflowHandler) StartWorkflowExecution( "Received StartWorkflowExecution. WorkflowID", tag.WorkflowID(startRequest.GetWorkflowID())) - if !common.ValidIDLength( + if !common.IsValidIDLength( startRequest.WorkflowType.GetName(), scope, idLengthWarnLimit, @@ -2194,7 +2194,7 @@ func (wh *WorkflowHandler) StartWorkflowExecution( } } - if !common.ValidIDLength( + if !common.IsValidIDLength( startRequest.GetRequestID(), scope, idLengthWarnLimit, @@ -2569,7 +2569,7 @@ func (wh *WorkflowHandler) SignalWorkflowExecution( } idLengthWarnLimit := wh.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( domainName, scope, idLengthWarnLimit, @@ -2585,7 +2585,7 @@ func (wh *WorkflowHandler) SignalWorkflowExecution( return wh.error(errSignalNameNotSet, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( signalRequest.GetSignalName(), scope, idLengthWarnLimit, @@ -2597,7 +2597,7 @@ func (wh *WorkflowHandler) SignalWorkflowExecution( return wh.error(errSignalNameTooLong, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( signalRequest.GetRequestID(), scope, idLengthWarnLimit, @@ -2691,7 +2691,7 @@ func (wh *WorkflowHandler) SignalWithStartWorkflowExecution( } idLengthWarnLimit := wh.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( domainName, scope, idLengthWarnLimit, @@ -2703,7 +2703,7 @@ func (wh *WorkflowHandler) SignalWithStartWorkflowExecution( return nil, wh.error(errDomainTooLong, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( signalWithStartRequest.GetWorkflowID(), scope, idLengthWarnLimit, @@ -2719,7 +2719,7 @@ func (wh *WorkflowHandler) SignalWithStartWorkflowExecution( return nil, wh.error(errSignalNameNotSet, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( signalWithStartRequest.GetSignalName(), scope, idLengthWarnLimit, @@ -2735,7 +2735,7 @@ func (wh *WorkflowHandler) SignalWithStartWorkflowExecution( return nil, wh.error(errWorkflowTypeNotSet, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( signalWithStartRequest.WorkflowType.GetName(), scope, idLengthWarnLimit, @@ -2751,7 +2751,7 @@ func (wh *WorkflowHandler) SignalWithStartWorkflowExecution( return nil, wh.error(err, scope, tags...) } - if !common.ValidIDLength( + if !common.IsValidIDLength( signalWithStartRequest.GetRequestID(), scope, idLengthWarnLimit, @@ -4228,7 +4228,7 @@ func (wh *WorkflowHandler) validateTaskList(t *types.TaskList, scope metrics.Sco return errTaskListNotSet } - if !common.ValidIDLength( + if !common.IsValidIDLength( t.GetName(), scope, wh.config.MaxIDLengthWarnLimit(), diff --git a/service/history/decision/checker.go b/service/history/decision/checker.go index a4c023ffafc..fc149afe490 100644 --- a/service/history/decision/checker.go +++ b/service/history/decision/checker.go @@ -225,7 +225,7 @@ func (v *attrValidator) validateActivityScheduleAttributes( } idLengthWarnLimit := v.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetActivityID(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -237,7 +237,7 @@ func (v *attrValidator) validateActivityScheduleAttributes( return &types.BadRequestError{Message: "ActivityID exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetActivityType().GetName(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -249,7 +249,7 @@ func (v *attrValidator) validateActivityScheduleAttributes( return &types.BadRequestError{Message: "ActivityType exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetDomain(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -366,7 +366,7 @@ func (v *attrValidator) validateTimerScheduleAttributes( if attributes.GetTimerID() == "" { return &types.BadRequestError{Message: "TimerId is not set on decision."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetTimerID(), v.metricsClient.Scope(metricsScope), v.config.MaxIDLengthWarnLimit(), @@ -398,7 +398,7 @@ func (v *attrValidator) validateActivityCancelAttributes( return &types.BadRequestError{Message: "ActivityId is not set on decision."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetActivityID(), v.metricsClient.Scope(metricsScope), v.config.MaxIDLengthWarnLimit(), @@ -424,7 +424,7 @@ func (v *attrValidator) validateTimerCancelAttributes( if attributes.GetTimerID() == "" { return &types.BadRequestError{Message: "TimerId is not set on decision."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetTimerID(), v.metricsClient.Scope(metricsScope), v.config.MaxIDLengthWarnLimit(), @@ -450,7 +450,7 @@ func (v *attrValidator) validateRecordMarkerAttributes( if attributes.GetMarkerName() == "" { return &types.BadRequestError{Message: "MarkerName is not set on decision."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetMarkerName(), v.metricsClient.Scope(metricsScope), v.config.MaxIDLengthWarnLimit(), @@ -520,7 +520,7 @@ func (v *attrValidator) validateCancelExternalWorkflowExecutionAttributes( } idLengthWarnLimit := v.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetDomain(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -532,7 +532,7 @@ func (v *attrValidator) validateCancelExternalWorkflowExecutionAttributes( return &types.BadRequestError{Message: "Domain exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetWorkflowID(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -576,7 +576,7 @@ func (v *attrValidator) validateSignalExternalWorkflowExecutionAttributes( } idLengthWarnLimit := v.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetDomain(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -588,7 +588,7 @@ func (v *attrValidator) validateSignalExternalWorkflowExecutionAttributes( return &types.BadRequestError{Message: "Domain exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.Execution.GetWorkflowID(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -647,7 +647,7 @@ func (v *attrValidator) validateContinueAsNewWorkflowExecutionAttributes( attributes.WorkflowType = &types.WorkflowType{Name: executionInfo.WorkflowTypeName} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.WorkflowType.GetName(), v.metricsClient.Scope(metricsScope), v.config.MaxIDLengthWarnLimit(), @@ -716,7 +716,7 @@ func (v *attrValidator) validateStartChildExecutionAttributes( } idLengthWarnLimit := v.config.MaxIDLengthWarnLimit() - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetDomain(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -727,7 +727,7 @@ func (v *attrValidator) validateStartChildExecutionAttributes( tag.IDTypeDomainName) { return &types.BadRequestError{Message: "Domain exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.GetWorkflowID(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -739,7 +739,7 @@ func (v *attrValidator) validateStartChildExecutionAttributes( return &types.BadRequestError{Message: "WorkflowId exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( attributes.WorkflowType.GetName(), v.metricsClient.Scope(metricsScope), idLengthWarnLimit, @@ -801,7 +801,7 @@ func (v *attrValidator) validatedTaskList( } name := taskList.GetName() - if !common.ValidIDLength( + if !common.IsValidIDLength( name, v.metricsClient.Scope(metricsScope), v.config.MaxIDLengthWarnLimit(), diff --git a/service/history/historyEngine.go b/service/history/historyEngine.go index 7d0f08ac90f..7e31fa7e482 100644 --- a/service/history/historyEngine.go +++ b/service/history/historyEngine.go @@ -3199,7 +3199,7 @@ func (e *historyEngineImpl) validateStartWorkflowExecutionRequest( return &types.BadRequestError{Message: "Missing WorkflowType."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( request.GetDomain(), e.metricsClient.Scope(metricsScope), e.config.MaxIDLengthWarnLimit(), @@ -3211,7 +3211,7 @@ func (e *historyEngineImpl) validateStartWorkflowExecutionRequest( return &types.BadRequestError{Message: "Domain exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( request.GetWorkflowID(), e.metricsClient.Scope(metricsScope), e.config.MaxIDLengthWarnLimit(), @@ -3222,7 +3222,7 @@ func (e *historyEngineImpl) validateStartWorkflowExecutionRequest( tag.IDTypeWorkflowID) { return &types.BadRequestError{Message: "WorkflowId exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( request.TaskList.GetName(), e.metricsClient.Scope(metricsScope), e.config.MaxIDLengthWarnLimit(), @@ -3233,7 +3233,7 @@ func (e *historyEngineImpl) validateStartWorkflowExecutionRequest( tag.IDTypeTaskListName) { return &types.BadRequestError{Message: "TaskList exceeds length limit."} } - if !common.ValidIDLength( + if !common.IsValidIDLength( request.WorkflowType.GetName(), e.metricsClient.Scope(metricsScope), e.config.MaxIDLengthWarnLimit(),