Refactor ValidIDLength (cadence-workflow#5526)

* add unit tests for ValidIDLength * rename ValidIDLength to IsValidIDLength * extract warnIDLengthExceedsLimit from IsValidIDLength * add unit tests covering warnIDLengthExceedsLimit * fix mr conflicts * fix imports * refactor IsValidIDLength and TestIsValidIDLength
freespace8 · Dec 22, 2023 · a31e217 · a31e217
1 parent 72407a9
commit a31e217
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 63 deletions.
diff --git 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
@@ -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
@@ -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(),