Added if_match_etag to Item Options #2705
Conversation
read me change
ItemOptions-changes 41f2acf if_match_etag changes with tests * main b1d2f9f [origin/main] Merge pull request #1 from gsa9989/read-me-change read-me-change 653b308 read me change Please enter a commit message to explain why this merge is necessary, ItemOption-changes 41f2acf if_match_etag changes with tests * main b1d2f9f [origin/main] Merge pull request #1 from gsa9989/read-me-change read-me-change 653b308 merge upstream/main
…to ItemOptions-changes
|
Thank you for your contribution @gsa9989! We will review the pull request and get back to you soon. |
github-actions
bot
added
Community Contribution
| @@ -17,6 +17,7 @@ | |||
| * Added a function `CosmosClient::with_connection_string` to enable `CosmosClient` creation via connection string. ([#2641](https://github.com/Azure/azure-sdk-for-rust/pull/2641)) | |||
| * Added support for executing limited cross-partition queries through the Gateway. See <https://learn.microsoft.com/rest/api/cosmos-db/querying-cosmosdb-resources-using-the-rest-api#queries-that-cannot-be-served-by-gateway> for more details on these limitations. ([#2577](https://github.com/Azure/azure-sdk-for-rust/pull/2577)) | |||
| * Added a preview feature (behind `preview_query_engine` feature flag) to allow the Rust SDK to integrate with an external query engine for performing cross-partition queries. ([#2577](https://github.com/Azure/azure-sdk-for-rust/pull/2577)) | |||
| * Added 'if_match_etag' to ItemOptions and necessary functions (tbd) | |||
There was a problem hiding this comment.
You should be able to put the link to this PR in there now, right?
| @@ -35,7 +35,7 @@ impl ContainerClient { | |||
| database_link: &ResourceLink, | |||
| container_id: &str, | |||
| ) -> Self { | |||
| let link = database_link | |||
| let link: ResourceLink = database_link | |||
There was a problem hiding this comment.
Yeah, this type annotation shouldn't be needed! The type of link should be apparent to the compiler from the return type of .item. You can just revert this back to how it was before :)
|
@gsa9989 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Looks good! A few suggestions, they're mostly not functional problems and just style/aesthetics stuff. The most critical functional thing to change is the way we're handling the response etag. We need to make sure we fail if the server doesn't return one.
As I expected, the tests are a bit repetitive, but I think duplicating logic is OK for now. We can refactor them when we have a better idea how we're testing all the different combinations of item options.
Overall a great first PR!
| @@ -35,7 +35,7 @@ impl ContainerClient { | |||
| database_link: &ResourceLink, | |||
| container_id: &str, | |||
| ) -> Self { | |||
| let link = database_link | |||
| let link: ResourceLink = database_link | |||
There was a problem hiding this comment.
Yeah, this type annotation shouldn't be needed! The type of link should be apparent to the compiler from the return type of .item. You can just revert this back to how it was before :)
| @@ -47,7 +47,7 @@ pub struct DeleteDatabaseOptions<'a> { | |||
| #[derive(Clone, Default)] | |||
| pub struct ItemOptions<'a> { | |||
| pub method_options: ClientMethodOptions<'a>, | |||
|
|
|||
| pub if_match_etag: Option<Etag>, | |||
There was a problem hiding this comment.
Let's add documentation comments to this, I'm trying to keep us up-to-date on that as we add features :).
I'll let you take a first stab at writing it, take a look at the comments for the other fields (method_options doesn't have one yet, but we'll address that later) and see what you think. Happy to help if you're not sure what to write! I'd include a link to this content on our docs site as well: https://learn.microsoft.com/azure/cosmos-db/nosql/database-transactions-optimistic-concurrency#optimistic-concurrency-control , since it describes the if-match header in detail.
| ## Next steps | ||
|
|
There was a problem hiding this comment.
You'll need to undo this change. Even though the Next Steps section itself is empty, it has nested sections (Provide feedback, for example).
There was a problem hiding this comment.
This section seems to be repeated. There is a next steps section right above it with information. Is that by design?
| const ETAG: HeaderName = HeaderName::from_static("etag"); | ||
| let etag = response | ||
| .headers() | ||
| .get_str(&ETAG) | ||
| .ok() | ||
| .map(|s| Etag::from(s.to_string())); |
There was a problem hiding this comment.
There's a constant in azure_core you can use, and it's already a HeaderName 🎉
I'd also change the .ok() to .expect("expected the etag to be returned"), which means you can rewrite it like this:
| const ETAG: HeaderName = HeaderName::from_static("etag"); | |
| let etag = response | |
| .headers() | |
| .get_str(&ETAG) | |
| .ok() | |
| .map(|s| Etag::from(s.to_string())); | |
| let etag: Etag = response | |
| .headers() | |
| .get_str(&azure_core::http::headers::ETAG) | |
| .expect("expected the etag to be returned") | |
| .into(); |
Using .expect means you'll unwrap the Option<String> into a String, causing a panic if the header is missing. For product code this would be bad, but in a test it's actually preferable. It's OK for test code to panic when something unexpected happens, it won't break the other tests, it'll just cause this test to fail. If for some reason the service stops returning etags, we want this test to fail, but if we're passing the Option<Etag> back in to the if_match_etag option later, then the test might continue to pass even though the behavior of the service is unexpected.
This is also a case where a type annotation (let etag: Etag) is needed because you need to tell Rust what the .into() at the end should return (any place you can write SomeType::from(some_value) you can also write the same thing like this: some_value.into(), as long as Rust can figure out what the target type is)
| &item, | ||
| Some(ItemOptions { | ||
| if_match_etag: etag, | ||
| enable_content_response_on_write: false, |
There was a problem hiding this comment.
| enable_content_response_on_write: false, |
This should be the default behavior, so let's just omit it since we're not trying to test this option in this test.
| let body = response.into_raw_body().collect_string().await?; | ||
| assert_eq!("", body); |
There was a problem hiding this comment.
Similar to the above, we're not trying to test enable_content_response_on_write behavior in this test, so let's just omit this check.
| let body = response.into_raw_body().collect_string().await?; | |
| assert_eq!("", body); |
| match response { | ||
| Ok(_) => { | ||
| return Err( | ||
| "expected a 412 Precondition Failed error when using an incorrect ETag".into(), | ||
| ); | ||
| } | ||
| Err(err) => { | ||
| assert_eq!( | ||
| Some(azure_core::http::StatusCode::PreconditionFailed), | ||
| err.http_status() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is good error handling for product code, since you're properly matching on the Result<...> returned by .replace_item. But in a test, where we know that the server is supposed to return an error and the test should fail if it does not return what we expect, we can simplify this quite a bit!
| match response { | |
| Ok(_) => { | |
| return Err( | |
| "expected a 412 Precondition Failed error when using an incorrect ETag".into(), | |
| ); | |
| } | |
| Err(err) => { | |
| assert_eq!( | |
| Some(azure_core::http::StatusCode::PreconditionFailed), | |
| err.http_status() | |
| ); | |
| } | |
| } | |
| assert_eq!( | |
| Some(azure_core::http::StatusCode::PreconditionFailed), | |
| response.expect_err("expected the server to return an error").http_status() | |
| ); |
The .expect_err method works a lot like .expect. It's just the opposite. Where .expect is used to unpack the successful return value from a Result (and panic if the Result represents an error), .expect_err unpacks the error from a Result (and panics if the Result represents a successful operation).
| } | ||
|
|
||
| #[recorded::test] | ||
| pub async fn item_upsert_if_match_etag(context: TestContext) -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
Most of the feedback from the above test applies to this one as well, so I won't repeat it all here :)
| } | ||
|
|
||
| #[recorded::test] | ||
| pub async fn item_delete_if_match_etag(context: TestContext) -> Result<(), Box<dyn Error>> { |
| } | ||
|
|
||
| #[recorded::test] | ||
| pub async fn item_patch_if_match_etag(context: TestContext) -> Result<(), Box<dyn Error>> { |
| @@ -17,6 +17,7 @@ | |||
| * Added a function `CosmosClient::with_connection_string` to enable `CosmosClient` creation via connection string. ([#2641](https://github.com/Azure/azure-sdk-for-rust/pull/2641)) | |||
| * Added support for executing limited cross-partition queries through the Gateway. See <https://learn.microsoft.com/rest/api/cosmos-db/querying-cosmosdb-resources-using-the-rest-api#queries-that-cannot-be-served-by-gateway> for more details on these limitations. ([#2577](https://github.com/Azure/azure-sdk-for-rust/pull/2577)) | |||
| * Added a preview feature (behind `preview_query_engine` feature flag) to allow the Rust SDK to integrate with an external query engine for performing cross-partition queries. ([#2577](https://github.com/Azure/azure-sdk-for-rust/pull/2577)) | |||
| * Added 'if_match_etag' to ItemOptions and necessary functions ([#2705](https://github.com/Azure/azure-sdk-for-rust/pull/2705)) | |||
There was a problem hiding this comment.
I think this should go above in the unreleased section. 0.24.0 is already released. Your change would be part of the next release.
|
|
||
| /// IfMatchEtag is used to ensure optimistic concurrency control, it helps prevent accidental overwrites and maintains data integrity. | ||
| /// | ||
| /// https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/database-transactions-optimistic-concurrency#optimistic-concurrency-control |
There was a problem hiding this comment.
Looks like your PR build is failing with the same linting error I faced from adding plain URLs in comments :P
You'll likely change this comment to address Ashley's request for documentation, so when you do, putting the URL in backticks (``) should make the lint error from the build go away. Like this:
https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/database-transactions-optimistic-concurrency#optimistic-concurrency-control
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
Added if_match_etag functionality and tests for replace item, upsert item, delete item, and patch item functions. Tests have not been recorded yet, but tests added include item_replace_if_match_etag, item_upsert_if_match_etag, item_delete_if_match_etag, and item_patch_if_match_etag. All these tests include a scenario with using if_match_etag with the correct etag and an incorrect etag.
Links
https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/database-transactions-optimistic-concurrency#optimistic-concurrency-control