Fix Argument Order in MergeTags() when Scheduling Orchestration in Entity Dispatcher#1080
Fix Argument Order in MergeTags() when Scheduling Orchestration in Entity Dispatcher#1080
Conversation
|
I would like to add a unit test for the change I made but I didn't find a appropriate file or test class for EntityDispatcher. So I only did the E2E test. Let me know if you guys have any idea about the testing. Thanks! |
davidmrdavid
left a comment
davidmrdavid
left a comment
There was a problem hiding this comment.
Left a question:
Agreed that it would be good to have a test. Not immediately sure if there's entity tests at this level, but if they don't exist, then we should create them, though I wouldn't block this PR for that (since we have an active case)
| Tags = OrchestrationTags.MergeTags( | ||
| runtimeState.Tags, | ||
| new Dictionary<string, string>() { { OrchestrationTags.FireAndForget, "" } }), | ||
| new Dictionary<string, string>() { { OrchestrationTags.FireAndForget, "" } }, | ||
| runtimeState.Tags), | ||
| OrchestrationInstance = destination, |
There was a problem hiding this comment.
can you please clarify why the order of tags matters? Ideally we'd document that in the code as well, to prevent regressions :)
There was a problem hiding this comment.
Thanks for reviewing!
It's because the logic in MergeTags() In summary, the FireAndForget value from newTags is intended to overwrite the one in existingTags. However, in our case, newTags include a key-value pair of FireAndForget: true, while the existing tag is null(which is the default). Due to incorrect ordering, the existing tag was mistakenly treated as new tags, causing the FireAndForget value to be overwritten as null. That's how we caused the issue.
There was a problem hiding this comment.
Got it. Good catch, thanks! Left an edit request based on that here.
davidmrdavid
left a comment
davidmrdavid
left a comment
There was a problem hiding this comment.
Good catch, this was a subtle bug.
Can you please also add a handler in the Entity event processor so that, if a SubOrchestration response message (or any message outside the expected set) is received, we throw an exception? That would have also helped us catch this error faster!
| Tags = OrchestrationTags.MergeTags( | ||
| runtimeState.Tags, | ||
| new Dictionary<string, string>() { { OrchestrationTags.FireAndForget, "" } }), | ||
| new Dictionary<string, string>() { { OrchestrationTags.FireAndForget, "" } }, | ||
| runtimeState.Tags), | ||
| OrchestrationInstance = destination, |
There was a problem hiding this comment.
Got it. Good catch, thanks! Left an edit request based on that here.
davidmrdavid
left a comment
davidmrdavid
left a comment
There was a problem hiding this comment.
Left some thoughts
| Assert.IsInstanceOfType(resultEvent, typeof(ExecutionStartedEvent)); | ||
| var executionStartedEvent = (ExecutionStartedEvent)resultEvent; | ||
|
|
||
| // The resulting event should contain a fire and forget tag |
There was a problem hiding this comment.
This should be contain not not contain . Only when there is a fire and forget tag then the orchestration won't send back a OrchestrationCompleted event.
davidmrdavid
left a comment
davidmrdavid
left a comment
There was a problem hiding this comment.
LGTM, thanks for your hard work here.
3 participants
This PR fixes a bug where tags were incorrectly passed when entities scheduling orchestrations, causing entities to wrongly receive a
SubOrchestrationInstanceCompletedmessage. By correcting the tag passing, this update ensures the intended fire-and-forget behavior of orchestration scheduling.Add error handling in TaskEntityDispatcher. If there is any unexpected events received by the dispatcher, throw error indicating that this is not expected and should be an issue.
Add unit test for TaskEntityDispatcher.