[Storage] get_blob_properties for BlobClient
#2014
Conversation
vincenttran-msft
requested review from
heaths,
RickWinter,
JeffreyRichter,
LarryOsterman and
ronniegeraghty
as code owners
vincenttran-msft
changed the title
get_blob_properties initial implementationget_blob_properties for BlobClient
sdk/storage/azure_storage_blob/src/pipeline/storage_headers_policy.rs
Outdated
Show resolved
Hide resolved
sdk/storage/azure_storage_blob/src/pipeline/storage_headers_policy.rs
Outdated
Show resolved
Hide resolved
|
Hi @heaths Heath, I have documented the remaining 4 feedback items into my notes, and will address them in the following PR. I have commented out the testcases since they currently rely on static resources and have a hard dependency on some NYI functionality. With that in mind- please let me know if there is any other showstoppers in this specific PR. Thanks! |
There was a problem hiding this comment.
Don't start your module with test_ (the file name). It's already in tests/ and this is redundant. blob_client.rs is perfect.
| // use azure_core_test::recorded; | ||
| // use azure_identity::DefaultAzureCredentialBuilder; | ||
| // use azure_storage_blob::{BlobBlobClientGetPropertiesOptions, BlobClient, BlobClientOptions}; | ||
| // use std::{env, error::Error}; | ||
|
|
||
| // #[recorded::test(live)] | ||
| // async fn test_get_blob_properties() -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
Why is all this commented out? If you're not ready to record, fine. But you've already marked them as live-only tests anyway so that's not a concern. We still want to build them to make sure they work.
| pub endpoint: Url, | ||
| pub container_name: String, | ||
| pub blob_name: String, |
There was a problem hiding this comment.
Clients shouldn't have public members. We only do that for plain ol' data objects (models). We have thread-safety guarantees for clients and making them mutable violates that. You can add accessors, which is already covered in our guidelines for endpoint.
| client: GeneratedBlobClient, | ||
| } | ||
|
|
||
| impl BlobClient { |
There was a problem hiding this comment.
In other languages, I thought you can only get a BlobClient from a ContainerClient or something i.e., these aren't creatable. If so, they must be exported from both the crate root (re-exported) and from the clients module in which all clients - creatable or gettable - are exported.
| pub(crate) use blob_client::BlobClient as GeneratedBlobClient; | ||
|
|
||
| pub use blob_client::BlobClient; | ||
| pub use crate::generated::clients::blob_blob_client::BlobBlobClientGetPropertiesOptions; | ||
| pub use crate::generated::clients::blob_client::BlobClientOptions; |
There was a problem hiding this comment.
Why export them from the root for the crate? Just have the modules within the crate import crate::generated::clients::BlobClient. Re-exporting types only for the crate like this is going to lead to a lot of confusion, I think. It's not typical.
| /// Properties of an Azure Storage blob. | ||
| /// | ||
| #[derive(Clone, Default, Debug)] | ||
| pub struct BlobProperties { |
There was a problem hiding this comment.
Model properties must all be Option<T>.
| } | ||
|
|
||
| impl BlobProperties { | ||
| pub(crate) fn build_from_response_headers(response_headers: &Headers) -> BlobProperties { |
| }; | ||
| for (key, value) in response_headers.iter() { | ||
| match key.as_str() { | ||
| "content-length" => properties.content_length = String::from(value.as_str()), |
There was a problem hiding this comment.
Some, like this, aren't strings. Why are you parsing them as such? You should use our header traits and even define a header. We also have a bunch of these already defined as HeaderNames that you should use for consistency.
| "content-length" => properties.content_length = String::from(value.as_str()), | ||
| "content-md5" => properties.content_md5 = String::from(value.as_str()), | ||
| "content-type" => properties.content_type = String::from(value.as_str()), | ||
| "etag" => properties.etag = String::from(value.as_str()), |
There was a problem hiding this comment.
Use our existing Etag header type.
This PR contains the following:
BlobClientto supportget_blob_propertiesget_blob_propertiesand simple test cases (get properties with existing blob, container not exists case)