Tweak Update-with-Start error reporting #1746
Conversation
| @@ -1800,7 +1800,7 @@ func (w *workflowClientInterceptor) UpdateWithStartWorkflow( | |||
| } | |||
| handle, err := w.updateHandleFromResponse(ctx, updateReq.WaitPolicy.LifecycleStage, updateResp) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("%w: %w", errInvalidWithStartWorkflowOperation, err) | |||
| return nil, err | |||
There was a problem hiding this comment.
Wrapping with errInvalidWithStartWorkflowOperation ("invalid WithStartWorkflowOperation") is inappropriate IMO, since it's not related to the start operation.
| return nil, startErr | ||
|
|
||
| // this should never happen | ||
| return nil, errors.New(multiErr.Error()) |
There was a problem hiding this comment.
This would only happen if the server sent back [nil, nil]; which would be a bug. But in case it does, this is the fallback.
There was a problem hiding this comment.
Should we be honest and return a message like "This should never happen, please report a bug at ..."?
There was a problem hiding this comment.
Oh, I only saw this comment now. I'm not sure; is that something we do?
This scenario would mean the server decided to create a MultiOps error, but messed up the inner errors. So the server's intention is unambiguous; but there aren't enough details sent back to the user. Would it be reasonable to assume that a user will be confused and ask for better errors?
| @@ -1167,32 +1217,6 @@ func (s *workflowRunSuite) TestExecuteWorkflowWithUpdate_ServerResponseCountMism | |||
| s.ErrorContains(err, "invalid server response: 0 instead of 2 operation results") | |||
| } | |||
|
|
|||
| func (s *workflowRunSuite) TestExecuteWorkflowWithUpdate_ServerErrorResponseCountMismatch() { | |||
There was a problem hiding this comment.
Moved into the above.
| } | ||
| case *workflowservice.ExecuteMultiOperationRequest_Operation_UpdateWorkflow: | ||
| if !errors.As(opErr, &abortedErr) { | ||
| startErr = fmt.Errorf("%w: %w", errInvalidWithStartWorkflowOperation, opErr) |
There was a problem hiding this comment.
"invalid WithStartWorkflowOperation" isn't quite accurate anyway. It could have just "failed" despite the operation itself being valid.
What was changed
Tweak error reporting of Update-with-Start.
Why?
Communicate errors better to user.
Checklist
Closes
How was this tested: