Fix execution order of read_before_serialization and modify_before_serialization
#3798
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| modify_before_serialization(ctx, runtime_components, cfg); | ||
| read_before_serialization(ctx, runtime_components, cfg); |
There was a problem hiding this comment.
This seems like it could be a breaking change if someone was relying on the read values not yet having been modified? I do think we should make the change since correctness > backwards compatibility IMO, but maybe it could use a new BehaviorVersion? That also kind of seems like overkill for a change this small, but we should probably discuss
There was a problem hiding this comment.
When we discussed yesterday we weren't going to pursue a new BehaviorVersion. One of the main arguments (I have anyway) against it is it isn't "valid" to have the SDK work both ways, hence it isn't really opt-in behavior difference we would want to support. I think we treat it as a bug and make the change (perhaps bumping minor versions where appropriate).
There was a problem hiding this comment.
Admittedly, whether or not we introduce a new BehaviorVersion is a calculated risk. I code searched GitHub and internal and didn't find code that depended on the prior order of read_before_serialization -> modify_before_serialization. I believe it's safe to make this change without introducing a new BehaviorVersion.
perhaps bumping minor versions where appropriate
Yeah, we can bump a minor version of aws-smithy-runtime in this PR.
There was a problem hiding this comment.
The nature of the fix is the same as #3392 (a new BehaviorVersion wasn't introduced there either).
IMHO, if the design is correct, but the implementation didn't align with it, we should first consider not introducing a new BehaviorVersion. If the design is incorrect/sub-optimal, the implementation aligns with it, and we later need to update both design & implementation, then we should first consider introducing a new BehaviorVersion.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
Fixes the execution order of modify_before_serialization and read_before_serialization in the orchestrator.
Description
The fix ensures that the execution order of these interceptor methods aligns with our orchestrator design. While customers may see a behavior change as a result, we don't treat this as a new
BehaviorVersionsince it is intended as a bug fix.Testing
Existing tests in CI
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.