Skip to content

Create azure_storage_blobs crate from azure_storage::blob #499

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

Merged
merged 13 commits into from
Dec 20, 2021

Conversation

johnbatty
Copy link
Contributor

@johnbatty johnbatty commented Nov 7, 2021

As per: #483

This is work in progress. I've created the new crate, and the main code builds but there is some cleanup to do. I have moved the blob examples and the unit tests work.

I had similar issues as I did when creating the azure_messaging_queues crate (#493).

  • A circular reference between the new azure_storage_blobs crate and azure_storage, as StorageClient implemented a list_containers method. I resolved in a similar way as I did for azure_messaging_queues - created a new service client BaseBlobService (name based on the equivalent object in the Python SDK) in azure_storage_blobs that is just a wrapper around StorageClient, then moved the list_containers method to that.
    • I'm open to suggestions about the BaseBlobService name. It is a "client", and all the other clients have Client in the name. However I opted for matching the Python SDK naming.
  • Various private or pub(crate) modules/methods in azure_storage that had to be made pub to allow access from the new crate.

@johnbatty johnbatty changed the title WIP: Create azure_storage_blobs crate from azure_storage::blob Create azure_storage_blobs crate from azure_storage::blob Nov 7, 2021
@johnbatty johnbatty marked this pull request as ready for review November 7, 2021 22:40
@thovoll
Copy link
Contributor

thovoll commented Nov 7, 2021

Thanks @johnbatty, appreciate your efforts!

@thovoll
Copy link
Contributor

thovoll commented Nov 7, 2021

  • I'm open to suggestions about the BaseBlobService name. It is a "client", and all the other clients have Client in the name. However I opted for matching the Python SDK naming.

I can't find the BaseBlobService in the Python SDK, but I did find BlobServiceClient which matches .Net and Java.

@johnbatty
Copy link
Contributor Author

@thovoll Ah - thanks for the BlobServiceClient tip - I'll change the name to match that. I didn't pay enough attention to my search, and ended up looking at the "Azure SDK for Python Legacy", which I presume is an outdated release!
https://docs.microsoft.com/en-us/python/api/azure-storage-blob/azure.storage.blob.baseblobservice.baseblobservice?view=azure-python-previous&viewFallbackFrom=azure-python

@thovoll
Copy link
Contributor

thovoll commented Nov 8, 2021

Anytime @johnbatty, I am only slowly getting better at navigating the legacy, new, track 1, track 2, version 11, version 12, etc, etc jungle :) There's also the generated code to be aware of.

@cataggar
Copy link
Member

I'm looking forward to these being split up. There is a conflict.

@cataggar cataggar requested a review from MindFlavor November 10, 2021 17:01
@cataggar cataggar mentioned this pull request Nov 10, 2021
4 tasks
@johnbatty
Copy link
Contributor Author

johnbatty commented Dec 19, 2021

I've noticed that there are some inconsistencies in the examples for the storage APIs. I'd like to make them more consistent, so would like to get agreement on the style before I do so.

A typical example that uses storage does this (from identity/examples/device_code_flow.rs):

    let http_client = azure_core::new_http_client();
    let storage_account_client = StorageAccountClient::new_bearer_token(
        http_client.clone(),
        &storage_account_name,
        authorization.access_token().secret() as &str,
    );
    let blob_service_client = storage_account_client.as_blob_service_client();
    let containers = blob_service_client.list_containers().execute().await?;

Another example (from storage_blobs/examples/blob_00.rs):

    let http_client = azure_core::new_http_client();
    let storage_account_client =
        StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key);
    let storage_client = storage_account_client.as_storage_client();
    let blob = storage_client
        .as_container_client(&container)
        .as_blob_client(&blob);

    let response = blob
        .get()
        .range(Range::new(0, 1024))
        .execute()
        .await?;

Issues:

  • A _client suffix is usually used on most, but not all, variables. Should we do this in all cases?
    • e.g. in the second example blob is a client - should it be called blob_client?
    • I'm inclined to use blob_client, as although it is more verbose, it is more consistent and matches this Python SDK example
  • In most cases we need to create a series of clients, e.g. storage_account_client => storage_client => container_client => blob_client. If we don't need the intermediate clients, should we chain these calls together to remove the intermediate variables. e.g. in the second example we could change it to:
    let storage_client = StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key)
                              .as_storage_client();
    Or to take it one stage further:
    let blob_client = StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key)
                               .as_storage_client()
                               .as_container_client(&container)
                               .as_blob_client(&blob);
    To simplify this further I could implement as_container_client(...) for StorageAccountClient to avoid having to include as_storage_client():
    let blob_client = StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key)
                               .as_container_client(&container)
                               .as_blob_client(&blob);
    I'm inclined to take this approach - removing the intermediate variables where possible.
    EDIT: I've updated the PR to use this approach.

Any thoughts/comments appreciated.

Aside: It is worth noting that the Python SDK has an alternative approach where a BlobClient class can be directly instantiated - but think that is an approach to be discussed outside the scope of this PR (aha! - see #490)

# Create the client object using the storage URL and the credential
blob_client = BlobClient(storage_url,
   container_name="blob-container-01", blob_name="sample-blob.txt", credential=credential)

@johnbatty
Copy link
Contributor Author

johnbatty commented Dec 19, 2021

@MindFlavor @cataggar I have rebased onto the latest main, and fixed up the remaining issues, so please could you review. Apologies for taking so long to get this fixed up!

Notes:

  • There are some references to azure_storage from the new azure_storage_blobs crate, e.g.
    • azure_storage::response_from_headers macro
      • This is currently only used by the storage_blobs code, so can presumably be moved to the new crate.
    • azure_storage::core::clients::StorageClient::blob_url_with_segments(...)
    • azure_storage::core::parsing_xml (had to change to be pub rather than pub(crate))
    • azure_storage::core::util (had to change to be pub rather than pub(crate))
    • azure_storage::core::{copy_id_from_headers, CopyId}
    • azure_storage::xml::read_xml
    • azure_storage::{headers::consistency_from_headers, ConsistencyCRC64, ConsistencyMD5}
    • azure_storage::headers::content_md5_from_headers
    • azure_storage::core::clients::{AsStorageClient, StorageAccountClient, StorageClient, StorageCredentials}
    • azure_storage::core::StoredAccessPolicyList
    • azure_storage::core::shared_access_signature::{...}
      • I think this is probably specific to Storage Blobs, so can probably be moved into the new azure_storage_blobs crate

I propose that we (I) split out the remaining storage crates and we then review what is left in azure_storage/azure_storage::core and decide whether we can and/or should move some or all of that function into the new individual crates (possibly replicating code where necessary).

Copy link
Member

@cataggar cataggar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Thanks for making these easy to review.

@cataggar cataggar merged commit 08d5000 into Azure:main Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants