Skip to content

Commit 0589e03

Browse files
davidporter-id-audavidporter-id-au
authored
feat: fix/enforce/address mappers nil handling (#7434)
<!-- Describe what has changed in this PR --> **What changed?** This: - Adds some changes to make mappers nilsafe - enforces nilaway in lint <!-- Tell your future self why have you made these changes --> **Why?** <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> **How did you test it?** <!-- Assuming the worst case, what can be broken when deploying this change to production? --> **Potential risks** This is making a few mappers return nil to values which would previously of paniced. This is probably not the right solution and they probably should be changed to return an error. However, thats a larger set of changes because the risks seem fairly minimal (register domain specifically) <!-- Is it notable for release? e.g. schema updates, configuration or data migration required? If so, please mention it, and also update CHANGELOG.md --> **Release notes** <!-- Is there any documentation updates should be made for config, https://cadenceworkflow.io/docs/operation-guide/setup/ ? If so, please open an PR in https://github.com/cadence-workflow/cadence-docs --> **Documentation Changes** --------- Signed-off-by: David Porter <[email protected]>
1 parent b2826a8 commit 0589e03

File tree

4 files changed

+96
-51
lines changed

4 files changed

+96
-51
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,7 @@ $(BUILD)/code-lint: $(LINT_SRC) $(BIN)/revive $(BIN)/nilaway | $(BUILD)
406406
exit 1; \
407407
fi
408408
$Q echo "nilaway check..."
409-
$Q GOTOOLCHAIN=go1.24.0 $(BIN)/nilaway github.com/uber/cadence/common/types/mapper/... || true # todo (remove the || true) to block builds which break this
409+
$Q GOTOOLCHAIN=go1.24.0 $(BIN)/nilaway -test=false github.com/uber/cadence/common/types/mapper/...
410410
$Q touch $@
411411

412412
$(BUILD)/goversion-lint: go.work Dockerfile docker/github_actions/Dockerfile${DOCKERFILE_SUFFIX}

‎common/types/mapper/proto/api.go‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1194,10 +1194,15 @@ func ToFailoverInfo(t *apiv1.FailoverInfo) *types.FailoverInfo {
11941194
if t == nil {
11951195
return nil
11961196
}
1197+
startTs := timeToUnixNano(t.GetFailoverStartTimestamp())
1198+
expireTs := timeToUnixNano(t.GetFailoverExpireTimestamp())
1199+
if startTs == nil || expireTs == nil {
1200+
return nil
1201+
}
11971202
return &types.FailoverInfo{
11981203
FailoverVersion: t.GetFailoverVersion(),
1199-
FailoverStartTimestamp: *timeToUnixNano(t.GetFailoverStartTimestamp()),
1200-
FailoverExpireTimestamp: *timeToUnixNano(t.GetFailoverExpireTimestamp()),
1204+
FailoverStartTimestamp: *startTs,
1205+
FailoverExpireTimestamp: *expireTs,
12011206
CompletedShardCount: t.GetCompletedShardCount(),
12021207
PendingShards: t.GetPendingShards(),
12031208
}
@@ -2610,11 +2615,15 @@ func ToRegisterDomainRequest(t *apiv1.RegisterDomainRequest) *types.RegisterDoma
26102615
if t == nil {
26112616
return nil
26122617
}
2618+
days := durationToDays(t.WorkflowExecutionRetentionPeriod)
2619+
if days == nil {
2620+
days = common.Int32Ptr(3) // default to 3 days if not set
2621+
}
26132622
return &types.RegisterDomainRequest{
26142623
Name: t.Name,
26152624
Description: t.Description,
26162625
OwnerEmail: t.OwnerEmail,
2617-
WorkflowExecutionRetentionPeriodInDays: *durationToDays(t.WorkflowExecutionRetentionPeriod),
2626+
WorkflowExecutionRetentionPeriodInDays: *days,
26182627
EmitMetric: common.BoolPtr(true), // this is a legacy field that doesn't exist in proto and probably can be removed
26192628
Clusters: ToClusterReplicationConfigurationArray(t.Clusters),
26202629
ActiveClusterName: t.ActiveClusterName,
@@ -4102,13 +4111,19 @@ func ToTaskListStatus(t *apiv1.TaskListStatus) *types.TaskListStatus {
41024111
}
41034112

41044113
func FromIsolationGroupMetrics(t *types.IsolationGroupMetrics) *apiv1.IsolationGroupMetrics {
4114+
if t == nil {
4115+
return nil
4116+
}
41054117
return &apiv1.IsolationGroupMetrics{
41064118
NewTasksPerSecond: t.NewTasksPerSecond,
41074119
PollerCount: t.PollerCount,
41084120
}
41094121
}
41104122

41114123
func ToIsolationGroupMetrics(t *apiv1.IsolationGroupMetrics) *types.IsolationGroupMetrics {
4124+
if t == nil {
4125+
return nil
4126+
}
41124127
return &types.IsolationGroupMetrics{
41134128
NewTasksPerSecond: t.NewTasksPerSecond,
41144129
PollerCount: t.PollerCount,

‎common/types/mapper/proto/errors.go‎

Lines changed: 74 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -112,24 +112,33 @@ func ToError(err error) error {
112112
case yarpcerrors.CodeNotFound:
113113
switch details := getErrorDetails(err).(type) {
114114
case *apiv1.EntityNotExistsError:
115+
if details != nil {
116+
return &types.EntityNotExistsError{
117+
Message: status.Message(),
118+
CurrentCluster: details.CurrentCluster,
119+
ActiveCluster: details.ActiveCluster,
120+
ActiveClusters: details.ActiveClusters,
121+
}
122+
}
115123
return &types.EntityNotExistsError{
116-
Message: status.Message(),
117-
CurrentCluster: details.CurrentCluster,
118-
ActiveCluster: details.ActiveCluster,
119-
ActiveClusters: details.ActiveClusters,
124+
Message: status.Message(),
120125
}
121126
case *apiv1.WorkflowExecutionAlreadyCompletedError:
122127
return &types.WorkflowExecutionAlreadyCompletedError{
123128
Message: status.Message(),
124129
}
125130
case *sharddistributorv1.NamespaceNotFoundError:
126-
return &types.NamespaceNotFoundError{
127-
Namespace: details.Namespace,
131+
if details != nil {
132+
return &types.NamespaceNotFoundError{
133+
Namespace: details.Namespace,
134+
}
128135
}
129136
case *sharddistributorv1.ShardNotFoundError:
130-
return &types.ShardNotFoundError{
131-
Namespace: details.Namespace,
132-
ShardKey: details.ShardKey,
137+
if details != nil {
138+
return &types.ShardNotFoundError{
139+
Namespace: details.Namespace,
140+
ShardKey: details.ShardKey,
141+
}
133142
}
134143
}
135144
case yarpcerrors.CodeInvalidArgument:
@@ -146,31 +155,39 @@ func ToError(err error) error {
146155
case yarpcerrors.CodeAborted:
147156
switch details := getErrorDetails(err).(type) {
148157
case *sharedv1.ShardOwnershipLostError:
149-
return &types.ShardOwnershipLostError{
150-
Message: status.Message(),
151-
Owner: details.Owner,
158+
if details != nil {
159+
return &types.ShardOwnershipLostError{
160+
Message: status.Message(),
161+
Owner: details.Owner,
162+
}
152163
}
153164
case *sharedv1.TaskListNotOwnedByHostError:
154-
return &cadence_errors.TaskListNotOwnedByHostError{
155-
OwnedByIdentity: details.OwnedByIdentity,
156-
MyIdentity: details.MyIdentity,
157-
TasklistName: details.TaskListName,
165+
if details != nil {
166+
return &cadence_errors.TaskListNotOwnedByHostError{
167+
OwnedByIdentity: details.OwnedByIdentity,
168+
MyIdentity: details.MyIdentity,
169+
TasklistName: details.TaskListName,
170+
}
158171
}
159172
case *sharedv1.CurrentBranchChangedError:
160-
return &types.CurrentBranchChangedError{
161-
Message: status.Message(),
162-
CurrentBranchToken: details.CurrentBranchToken,
173+
if details != nil {
174+
return &types.CurrentBranchChangedError{
175+
Message: status.Message(),
176+
CurrentBranchToken: details.CurrentBranchToken,
177+
}
163178
}
164179
case *sharedv1.RetryTaskV2Error:
165-
return &types.RetryTaskV2Error{
166-
Message: status.Message(),
167-
DomainID: details.DomainId,
168-
WorkflowID: ToWorkflowID(details.WorkflowExecution),
169-
RunID: ToRunID(details.WorkflowExecution),
170-
StartEventID: ToEventID(details.StartEvent),
171-
StartEventVersion: ToEventVersion(details.StartEvent),
172-
EndEventID: ToEventID(details.EndEvent),
173-
EndEventVersion: ToEventVersion(details.EndEvent),
180+
if details != nil {
181+
return &types.RetryTaskV2Error{
182+
Message: status.Message(),
183+
DomainID: details.DomainId,
184+
WorkflowID: ToWorkflowID(details.WorkflowExecution),
185+
RunID: ToRunID(details.WorkflowExecution),
186+
StartEventID: ToEventID(details.StartEvent),
187+
StartEventVersion: ToEventVersion(details.StartEvent),
188+
EndEventID: ToEventID(details.EndEvent),
189+
EndEventVersion: ToEventVersion(details.EndEvent),
190+
}
174191
}
175192
}
176193
case yarpcerrors.CodeAlreadyExists:
@@ -188,10 +205,12 @@ func ToError(err error) error {
188205
Message: status.Message(),
189206
}
190207
case *apiv1.WorkflowExecutionAlreadyStartedError:
191-
return &types.WorkflowExecutionAlreadyStartedError{
192-
Message: status.Message(),
193-
StartRequestID: details.StartRequestId,
194-
RunID: details.RunId,
208+
if details != nil {
209+
return &types.WorkflowExecutionAlreadyStartedError{
210+
Message: status.Message(),
211+
StartRequestID: details.StartRequestId,
212+
RunID: details.RunId,
213+
}
195214
}
196215
}
197216
case yarpcerrors.CodeDataLoss:
@@ -201,22 +220,28 @@ func ToError(err error) error {
201220
case yarpcerrors.CodeFailedPrecondition:
202221
switch details := getErrorDetails(err).(type) {
203222
case *apiv1.ClientVersionNotSupportedError:
204-
return &types.ClientVersionNotSupportedError{
205-
FeatureVersion: details.FeatureVersion,
206-
ClientImpl: details.ClientImpl,
207-
SupportedVersions: details.SupportedVersions,
223+
if details != nil {
224+
return &types.ClientVersionNotSupportedError{
225+
FeatureVersion: details.FeatureVersion,
226+
ClientImpl: details.ClientImpl,
227+
SupportedVersions: details.SupportedVersions,
228+
}
208229
}
209230
case *apiv1.FeatureNotEnabledError:
210-
return &types.FeatureNotEnabledError{
211-
FeatureFlag: details.FeatureFlag,
231+
if details != nil {
232+
return &types.FeatureNotEnabledError{
233+
FeatureFlag: details.FeatureFlag,
234+
}
212235
}
213236
case *apiv1.DomainNotActiveError:
214-
return &types.DomainNotActiveError{
215-
Message: status.Message(),
216-
DomainName: details.Domain,
217-
CurrentCluster: details.CurrentCluster,
218-
ActiveCluster: details.ActiveCluster,
219-
ActiveClusters: details.ActiveClusters,
237+
if details != nil {
238+
return &types.DomainNotActiveError{
239+
Message: status.Message(),
240+
DomainName: details.Domain,
241+
CurrentCluster: details.CurrentCluster,
242+
ActiveCluster: details.ActiveCluster,
243+
ActiveClusters: details.ActiveClusters,
244+
}
220245
}
221246
}
222247
case yarpcerrors.CodeResourceExhausted:
@@ -226,9 +251,11 @@ func ToError(err error) error {
226251
Message: status.Message(),
227252
}
228253
case *apiv1.ServiceBusyError:
229-
return &types.ServiceBusyError{
230-
Message: status.Message(),
231-
Reason: details.Reason,
254+
if details != nil {
255+
return &types.ServiceBusyError{
256+
Message: status.Message(),
257+
Reason: details.Reason,
258+
}
232259
}
233260
}
234261
case yarpcerrors.CodeUnavailable:

‎common/types/mapper/thrift/shared.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6118,6 +6118,9 @@ func ToTaskListStatus(t *shared.TaskListStatus) *types.TaskListStatus {
61186118
}
61196119

61206120
func FromIsolationGroupMetrics(t *types.IsolationGroupMetrics) *shared.IsolationGroupMetrics {
6121+
if t == nil {
6122+
return nil
6123+
}
61216124
return &shared.IsolationGroupMetrics{
61226125
NewTasksPerSecond: &t.NewTasksPerSecond,
61236126
PollerCount: &t.PollerCount,

0 commit comments

Comments
 (0)