Cosmos: Fix #2824 by forcing 'enable_content_response_on_write' on for read item #2894
Conversation
analogrelay
requested review from
Pilchie,
kirankumarkolli,
tvaron3,
FabianMeiswinkel,
kundadebdatta,
nehrao1,
kushagraThapar,
heaths,
RickWinter,
ronniegeraghty and
LarryOsterman
as code owners
There was a problem hiding this comment.
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_writetotruefor 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 |
| if self.status().is_success() { | ||
| Ok(self) | ||
| } else { | ||
| RawResponse::from(self).success().await.map(Into::into) |
There was a problem hiding this comment.
This conversion chain (Response -> RawResponse -> Response) is unnecessarily complex. The error handling logic should be consistent between Response<T, F> and RawResponse without requiring conversions.
| RawResponse::from(self).success().await.map(Into::into) | |
| let http_error = HttpError::new(self).await; | |
| let status = http_error.status(); | |
| let error_kind = azure_core::error::ErrorKind::http_response( | |
| status, | |
| http_error.error_code().map(ToOwned::to_owned), | |
| ); | |
| Err(azure_core::Error::full( | |
| error_kind, | |
| http_error, | |
| format!("Request failed with status code: {}", status), | |
| )) |
Copilot uses AI. Check for mistakes.
| } | ||
| } | ||
|
|
||
| impl ResponseExt for RawResponse { |
There was a problem hiding this comment.
@chlowell this is a good idea we might consider in azure_core. Combined with #2725 we could also use something like this to wrap standard Azure errors. We'd have to implement it (foreign trait otherwise), but could also allow clients to pass in custom success codes or something, much like .NET does for their DPG clients and HTTP status code categorizations.
| @@ -105,7 +108,7 @@ impl CosmosPipeline { | |||
| req.insert_header(constants::CONTINUATION, continuation); | |||
| } | |||
|
|
|||
| let resp = pipeline.send(&ctx, &mut req).await?; | |||
| let resp = pipeline.send(&ctx, &mut req).await?.success().await?; | |||
There was a problem hiding this comment.
@analogrelay would it be better to implement this on IntoFuture<Item = Response<T, F>> such that we're not having to await twice? Granted, customers aren't having to type that themselves, but I imagine this compiles into separate poll_next calls against. If it's optimized away, fine. Just seems overkill this way.
There was a problem hiding this comment.
Ooh, that's an interesting idea, I hadn't thought of doing that. I'd definitely like to remove the await chaining, especially since we're about to go from 2 awaits down to 1. I'd hate to push it back out to 2 again
There was a problem hiding this comment.
It's possible, but I'm not a huge fan of it, so I'm going to hold off on it until we want to look at promoting this to azure_core. The main reason it doesn't feel good is that we have to do this:
impl<T, F, Fut: IntoFuture<Output = azure_core::Result<Response<T, F>>>> ResponseExt for Fut {
}This works, BUT, without min-specialization it ends up conflicting with any other attempt to implement ResponseExt. I don't want to do that unless we're sure we've got the right approach.
Maybe we need to get involved in min-specialization and try to drive it to stabilization 😅
There was a problem hiding this comment.
Yeah, if we put it in core we can implement it on RawResponse and Response<T> ourselves and not have to worry about min-specialization. But, yeah, I agree with your other sentiment about that!
analogrelay
force-pushed
the
ashleyst/fix-2824
branch
from
3691354 to
6fb7837
Compare
|
/azp run rust - pullrequest |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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_writeflag totruewhen 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 :)