[Storage] get_account_info all clients, set/get_tags for BlobClient, set_properties for BlobServiceClient
#2795
Conversation
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
vincenttran-msft
requested review from
heaths,
RickWinter,
LarryOsterman and
jalauzon-msft
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds new functionality to the Azure Storage Blob client library, implementing get_account_info for all clients, set/get_tags and start_copy_from_url for BlobClient, and set_properties for BlobServiceClient. These additions align with the Azure REST API specifications and provide essential blob management capabilities.
- Adds account information retrieval across all client types (BlobClient, BlobContainerClient, BlobServiceClient)
- Implements blob tagging functionality with set/get operations for BlobClient
- Adds blob copy functionality and service properties management
Reviewed Changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/storage/azure_storage_blob_test/src/lib.rs |
Adds utility function for comparing blob tags in tests |
sdk/storage/azure_storage_blob/src/parsers.rs |
Implements blob tags serialization helper function |
sdk/storage/azure_storage_blob/src/models/mod.rs |
Exports new model types for account info, tags, and copy operations |
sdk/storage/azure_storage_blob/src/clients/blob_service_client.rs |
Adds set_properties and get_account_info methods |
sdk/storage/azure_storage_blob/src/clients/blob_container_client.rs |
Adds get_account_info method |
sdk/storage/azure_storage_blob/src/clients/blob_client.rs |
Adds start_copy_from_url, set_tags, get_tags, and get_account_info methods |
sdk/storage/azure_storage_blob/tests/ |
Comprehensive test coverage for all new functionality |
Comments suppressed due to low confidence (3)
sdk/storage/azure_storage_blob_test/src/lib.rs:124
- [nitpick] The function name
test_blob_tag_equalitydoesn't follow the naming convention for utility functions. Consider renaming tocompare_blob_tagsorblob_tags_equalsince it's not a test function itself but a helper for tests.
pub fn test_blob_tag_equality(tags1: BlobTags, tags2: BlobTags) -> bool {
sdk/storage/azure_storage_blob_test/src/lib.rs:125
- The variable name
count_mapis misleading since it stores key-value pairs from tags, not counts. Consider renaming totag_maporfirst_tags_mapto better reflect its purpose.
let mut count_map = HashMap::new();
sdk/storage/azure_storage_blob/src/parsers.rs:50
- Variable names
kandvare too short and unclear. Consider using more descriptive names likekeyandvalueto improve readability.
for (k, v) in tags.into_iter() {
vincenttran-msft
changed the title
get_account_info all clients, set/get_tags and start_copy_from_url for BlobClient, set_properties for BlobServiceClientget_account_info all clients, set/get_tags for BlobClient, set_properties for BlobServiceClient
|
Also, I believe I'm stumbling upon a pretty fun issue 😄 <Tags>
<TagSet>
<Tag>
<Key>hello</Key>
<Value>world</Value>
</Tag>
<Tag>
<Key>ferris</Key>
<Value>crab</Value>
</Tag>
</TagSet>
</Tags>",Scenario 2: <Tags>
<TagSet>
<Tag>
<Key>ferris</Key>
<Value>crab</Value>
</Tag>
<Tag>
<Key>hello</Key>
<Value>world</Value>
</Tag>
</TagSet>
</Tags>",I am wondering if this is something we can resolve at the test proxy level (i.e. sort alphabetically before comparing) at least in areas where order shouldn't matter. |
For this issue, @heaths Heath do you have any examples of using a I tried the following: let recording = ctx.recording();
let no_body_match: Matcher = CustomDefaultMatcher {
compare_bodies: Some(false),
..Default::default()
}
.into();
recording.set_matcher(no_body_match).await?;But this yields an error (issue goes away if I don't set a custom matcher):
|
|
For the matcher, I don't recommend not comparing bodies. What's the point of a client test then? We don't need to test the service since you're presumably already done that. A solution is much simpler: #[cfg(test)]
let mut tags = std::collections::BTreeMap::new();
#[cfg(not(test))]
let mut tags = std::collections::HashMap::new();As long as you stick to methods that exist on both - which were designed to be mostly compatible - it shouldn't matter. Use fully-qualified names and you don't have to worry about conditioning the imports as well. |
| /// Converts a `BlobTags` struct into `HashMap<String, String>`. | ||
| impl TryFrom<BlobTags> for HashMap<String, String> { | ||
| type Error = &'static str; | ||
|
|
||
| fn try_from(blob_tags: BlobTags) -> Result<Self, Self::Error> { | ||
| let mut map = HashMap::new(); | ||
|
|
||
| if let Some(tags) = blob_tags.blob_tag_set { | ||
| for tag in tags { | ||
| match (tag.key, tag.value) { | ||
| (Some(k), Some(v)) => { | ||
| map.insert(k, v); | ||
| } | ||
| _ => return Err("BlobTag missing key or value"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(map) | ||
| } | ||
| } | ||
|
|
||
| /// Converts a `HashMap<String, String>` into a `BlobTags` struct. | ||
| impl From<HashMap<String, String>> for BlobTags { | ||
| fn from(tags: HashMap<String, String>) -> Self { | ||
| let sorted_tags: BTreeMap<_, _> = tags.into_iter().collect(); | ||
| let blob_tags = sorted_tags | ||
| .into_iter() | ||
| .map(|(k, v)| BlobTag { | ||
| key: Some(k), | ||
| value: Some(v), | ||
| }) | ||
| .collect(); | ||
| BlobTags { | ||
| blob_tag_set: Some(blob_tags), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Added a TryFrom and From to be used for get_tags and set_tags respectively.
| // Assert | ||
| let response_tags = blob_client.get_tags(None).await?.into_body().await?; | ||
| assert_eq!(blob_tags, response_tags); | ||
| let map: HashMap<String, String> = response_tags.try_into()?; | ||
| assert_eq!(blob_tags, map); |
There was a problem hiding this comment.
Usage example- so 1 extra step try_into() to get the user-friendly HashMap<String, String> format.
As title states.
.tsp: Azure/azure-rest-api-specs#35921BlobClientSetTagsResult
• ̶ ̶D̶a̶t̶e̶
BlobClientGetAccountInfoResult
• Date
• Account_kind
• Is_hns_enabled
• Sku_name
BlobContainerClientGetAccountInfoResult
• ̶ ̶D̶a̶t̶e̶
• Account_kind
• Is_hns_enabled
• Sku_name
BlobServiceClientGetAccountInfoResult
• ̶ ̶D̶a̶t̶e̶
• Account_kind
• Is_hns_enabled
• Sku_name
This PR also moves the assets file.