Azure / azure-sdk-for-cpp

This repository is for active development of the Azure SDK for C++. For consumers of the SDK we recommend visiting our versioned developer docs at https://azure.github.io/azure-sdk-for-cpp.
MIT License
181 stars 126 forks source link

`BlobLeaseClient` is missing a `GetUrl` getter #5326

Open felipecrv opened 9 months ago

felipecrv commented 9 months ago

Is your feature request related to a problem? Please describe.

I need to know the URL of the BlobLeaseClient to produce good error message from my utility functions.

For convenience, a direct link to the header: https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/blob_lease_client.hpp

Describe the solution you'd like

BlobLeaseClient should have a GetUrl getter.

blob_lease_client.hpp:

  std::string BlobLeaseClient::GetUrl() const
  {
    if (m_blobClient.HasValue())
    {
      return m_blobClient->GetUrl();
    }
    else if (m_blobContainerClient.HasValue())
    {
      return m_blobContainerClient->GetUrl();
    }
    else
    {
      AZURE_UNREACHABLE_CODE();
    }
  }

blob_lease_client.cpp:

    /**
     * @brief Get's the primary URL endpoint of the blob or container.
     *
     * @return A URL of a blob or container.
     */
    std::string GetUrl() const;

Describe alternatives you've considered

Passing a string as parameter everywhere in my code.

Information Checklist Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

Jinming-Hu commented 9 months ago

Lease client is a convenient method for customers to do lease operations, unlike BlobClient, BlobContainerClient or BlobServiceClient, lease client doesn't have a corresponding real resource at server side. I think maybe we shouldn't have GetUrl for it.

@LarryOsterman What do you think?

LarryOsterman commented 9 months ago

The C++ guidelines require that service clients support a GetUrl method. The same does not apply to non-service clients.

How is a LeaseClient instantiated? With a connection string/url? Or is it instantiated with a BlobClient? Or from a BlobClient?

If it's constructed with a connection string or URL it makes sense for there to be a GetUrl method. If it's constructed from or with a BlobClient, it doesn't make sense for there to be a GetUrl method.

One thing you might consider is having an ostream inserter for LeaseClient which generates a meaningful string for diagnostics. That would resolve the customer concern without having to add a method which doesn't necessarily make sense. You can think of an ostream inserter as being logically similar to the C# "ToString()" method.

felipecrv commented 9 months ago

At the moment I can't generate a meaningful string through the the public interface of BlobLeaseClient because there isn't a single public member function that can tell me anything about the resource the BlobLeaseClient targets.

GetUrl might be a bad name for it, but I believe there should be a way to ask for a string that identifies the resource receiving the lease operations that BlobLeaseClient can trigger.

felipecrv commented 9 months ago

These are the private data members of BlobLeaseClient

  private:
    Azure::Nullable<BlobClient> m_blobClient;
    Azure::Nullable<BlobContainerClient> m_blobContainerClient;
    std::mutex m_mutex;
    std::string m_leaseId;

It can be instantiated by passing either BlobClient or BlobContainerClient because blobs and containers can be leased. Once you give a client to the BlobLeaseClient you can never recover information about them anymore, you would have to keep a copy of the client and not loose track of the client/lease-client pairing.

RickWinter commented 8 months ago

@Jinming-Hu We should expose at a minimum the LeaseId. I'm mixed on whether it makes sense to expose the URL of the targeted object. Other language SDKs do expose both of these values. see: https://learn.microsoft.com/en-us/dotnet/api/azure.storage.blobs.specialized.blobleaseclient?view=azure-dotnet

felipecrv commented 8 months ago

@RickWinter there is GetLeaseId already. It's only the target of the lease that is missing.

Screenshot 2024-02-27 at 18 56 48
Jinming-Hu commented 8 months ago

I'll leave this decision to @LarryOsterman

LarryOsterman commented 8 months ago

3 of the 4 P0 languages (.Net, Java, JavaScript) have a Uri property, Python does not. Go doesn't appear to have a BlobLeaseClient object.

I don't feel strongly about this, to be honest, but my take is that we should follow the pattern of the majority of the P0 SDK clients here.

Jinming-Hu commented 8 months ago

Hi @microzchang, can you add GetUrl method for all lease clients?

microzchang commented 8 months ago

Will add it.