Fix interceptor contract inconsistency for start_update_with_start_workflow#1588
Open
brucearctor wants to merge 3 commits into
Open
Fix interceptor contract inconsistency for start_update_with_start_workflow#1588brucearctor wants to merge 3 commits into
brucearctor wants to merge 3 commits into
Conversation
brucearctor
force-pushed
the
fix/interceptor-rpc-metadata-1582
branch
2 times, most recently
from
572c339 to
91e26f1
Compare
…rkflow Add top-level rpc_metadata and rpc_timeout fields to StartWorkflowUpdateWithStartInput, making it consistent with every other OutboundInterceptor input dataclass. Previously this composite input lacked these fields, forcing interceptors to special-case it. Also fix the _ClientImpl to actually pass rpc_metadata and rpc_timeout to the execute_multi_operation gRPC call, which were previously silently dropped. Add a test verifying that rpc_metadata set by an interceptor on StartWorkflowUpdateWithStartInput is forwarded to the gRPC call. Fixes temporalio#1582
brucearctor
force-pushed
the
fix/interceptor-rpc-metadata-1582
branch
from
91e26f1 to
a66bda4
Compare
Contributor
|
I think we should also remove the rpc metadata from the child interceptor objects, since they aren't actually used when it gets to the underlying multi_operation RPC. This will be a breaking change for a tiny number of folks we'll have to call out in release notes. If you'd like to address that, you can, otherwise I will end up making the change. |
Remove rpc_metadata and rpc_timeout fields from UpdateWithStartUpdateWorkflowInput and UpdateWithStartStartWorkflowInput. These fields were never forwarded to the underlying execute_multi_operation gRPC call — only the top-level StartWorkflowUpdateWithStartInput fields are authoritative. Also remove the corresponding parameters from WithStartWorkflowOperation since they only served to populate the (now-removed) child input fields. This is a breaking change for interceptors that accessed rpc_metadata or rpc_timeout on the child input objects.
Contributor
Author
Was 2cfe041 what you had in mind @tconley1428 ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1582.
StartWorkflowUpdateWithStartInputwas the onlyOutboundInterceptorinput dataclass missing top-levelrpc_metadataandrpc_timeoutfields, breaking interceptors that generically set metadata on every outbound call.Additionally,
_ClientImpl._start_workflow_update_with_start()intemporalio/client/_impl.pycalledexecute_multi_operation()without passingmetadata=ortimeout=, silently dropping any RPC metadata.Changes
temporalio/client/_interceptor.pyrpc_metadata: Mapping[str, str | bytes]andrpc_timeout: timedelta | Nonefields toStartWorkflowUpdateWithStartInputtemporalio/client/_client.pyClient._start_update_with_start()temporalio/client/_impl.pyrpc_metadataandrpc_timeoutthrough_start_workflow_update_with_start()to theexecute_multi_operation()gRPC callretry=Trueto theexecute_multi_operation()call to match every other gRPC call site in this file (this was a pre-existing omission, not part of [Bug] Interceptor contract inconsistency for OutboundInterceptor.start_update_with_start_workflow #1582)tests/worker/test_update_with_start.pytest_update_with_start_rpc_metadata_and_timeout_forwardedwhich verifies:rpc_metadatavia the new top-level fieldrpc_metadatais preservedrpc_timeoutis forwarded to the gRPC callBackward Compatibility
StartWorkflowUpdateWithStartInputis only constructed internally (inClient._start_update_with_start), not by external callersrpc_metadataon this method — they manipulateheaders. No changes needed to themrpc_metadatafields for introspectionTesting
tests/worker/test_update_with_start.pypasspyright: 0 errors, 0 warningsruff+pydocstyle: clean