GetWorkflowStateAsync should always return WorkflowState#1847
Conversation
…so the `WorkflowState.Exists` properly reflects the presence of the value (or not). Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1847 +/- ##
==========================================
- Coverage 63.19% 63.18% -0.02%
==========================================
Files 330 330
Lines 9534 9534
Branches 1110 1110
==========================================
- Hits 6025 6024 -1
Misses 3198 3198
- Partials 311 312 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…re's an unexpected response from the runtime Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
|
Love it! As always @WhitWaldo, thank you for your vigilance as the keeper of the Dapr dotnet scrolls! ✊ |
| catch (RpcException ex) when (ex.StatusCode == StatusCode.NotFound) | ||
| { | ||
| logger.LogGetWorkflowMetadataInstanceNotFound(ex, instanceId); | ||
| return null; |
There was a problem hiding this comment.
@barakbbn If there are other issues like this you're experiencing, please don't hesitate to file an issue about it so I can address them too!
There was a problem hiding this comment.
@barakbbn -- Echoing what Whit said: We would absolutely love to have you (or anyone reading this) open tickets about any pain points or inconsistencies you think exist in the SDK! 🙏
… always populate now Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Description
Ensuring that GetWorkflowStateAsync always returns a
WorkflowStateso theWorkflowState.Existsproperly reflects the presence of the value (or not). This was erroneously returning null if the metadata wasn't available when this is, in fact, a supported value in theWorkflowStateobject, so I'm applying this fix. The signature doesn't change, just the behavior, so this shouldn't be a dramatic breaking change.Thank you to @atrauzzi for the helpful discussion in which this was pointed out.
Also, after a very long discussion with @atrauzzi about the merits of logging at the SDK level, he's won me over and I'm also removing the error that was previously logged when using
GetWorkflowStateAsyncwhen the workflow was returned as not existing. He's right in his assertion that an error was too severe for an operation that completed successfully. The error remains if the runtime returns an unexpected value (null or some other response shape), but will no longer log if it's just a matter of a workflow instance not existing in the runtime.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: