💥 Fix how serialization context is applied in workflows [MINOR COMPAT BREAK] #525
Conversation
In Python I'm adding a test that demonstrates that every key in the context on the outbound side is available for decryption in the corresponding context on the inbound side, for a kitchen-sink-ish workflow. I'm not suggesting that's necessary to add here. But seeing as this PR does fix a genuine issue, it could be good to have a test that proves that an outbound Nexus payload now lacks any context and hence context cannot be used to encrypt in an un-decryptable way. |
|
👍 I adjusted the .NET kichen-sink-ish converter context test to check that Nexus is contextless (the first contextless use of a converter in workflows, so had to adjust a bit). |
| } | ||
| break; | ||
| case WorkflowActivationJob.VariantOneofCase.ResolveNexusOperation: | ||
| // TODO(cretz): Support Nexus serialization context |
There was a problem hiding this comment.
Nit: add issue link for Nexus serialization context
There was a problem hiding this comment.
Yeah, arguably this shouldn't be a TODO, more of a "here is where you'd add it". We do not support Nexus serialization context at this time in SDKs and not sure when we will (it's not really a bug, we may never).
| break; | ||
| case WorkflowActivationJob.VariantOneofCase.ResolveNexusOperationStart: | ||
| if (job.ResolveNexusOperationStart.Failed != null) | ||
| // TODO(cretz): Support Nexus serialization context |
There was a problem hiding this comment.
Nit: add issue link for Nexus serialization context
3 participants
What was changed
Previously, we applied context-based codecs/converters to everything a workflow does, and then if a workflow also did anything needing more context (e.g. activity or child workflow calls), we applied a context on top of that. Per #523, this was an incorrect approach. We need to apply contexts only in serialization/deserialization situations where the exact same context can be applied on the other side with deserialization/serialization.
There are no test adjustments for this PR because the test coverage from #446 still applies, this just affects the edge case of stacking the codecs/converters.
This is a 💥 breaking change technically because in situations where we used a specific context on top of outer-workflow context before (e.g. working with activity or child), we only apply the specific context now instead. This will only affect users with custom codecs/converters that support serialization contexts and built an expectation on the workflow-context one to be used for everything in a workflow.
Checklist