-
Notifications
You must be signed in to change notification settings - Fork 310
Cosmos: Fix #2824 by forcing 'enable_content_response_on_write' on for read item #2894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes Cosmos Database issue #2824 by ensuring read operations return content, and addresses test failures after HTTP behavior changes in #2834. The fix prevents read operations from returning empty responses when the enable_content_response_on_write
flag is disabled.
Key changes:
- Added error handling for non-2xx HTTP status codes in the Cosmos pipeline
- Force
enable_content_response_on_write
totrue
for read item operations - Updated test code to properly handle HTTP error responses
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
sdk/typespec/typespec_client_core/src/http/response.rs | Added success() method to validate HTTP status codes and convert errors |
sdk/cosmos/azure_data_cosmos/src/pipeline/mod.rs | Implemented error handling for non-2xx responses in pipeline operations |
sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs | Fixed read_item to force content response flag to true |
sdk/cosmos/azure_data_cosmos/tests/framework/test_data/mod.rs | Updated test to use new error handling pattern |
sdk/cosmos/azure_data_cosmos/tests/framework/test_account.rs | Updated test to use new error handling pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts, but nothing blocking. Creative!
3691354
to
6fb7837
Compare
/azp run rust - pullrequest |
Azure Pipelines successfully started running 1 pipeline(s). |
6fb7837
to
7906642
Compare
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
7aa4e71
to
c6a2367
Compare
This PR has two changes in one, but I think it's reasonable:
Ok
. We now convert non-2xx status codes to errors in the Cosmos Pipelineenable_content_response_on_write
flag totrue
when reading an item.I needed to do 1 in order for the tests to properly start failing on errors after #2834, and then I went in to fix the bug. Otherwise a pretty straightforward change :)