Skip to content
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

Simplify client usage for CosmosDB #676

Closed
wants to merge 7 commits into from

Conversation

yoshuawuyts
Copy link
Contributor

This PR is an attempt to streamline the usage of Cosmos in Rust. I noticed that compared to for example the mongodb driver our API can at times feel heavy to use. In order to improve our user experience, this PR does two things:

  1. It removes the requirement for users to call .clone on generated clients.
  2. It removes the _client suffix from most operations.

I'm rather happy with how this experiment turned out. Though I expect we might want to talk it over in a sync meeting first, so I'm marking this as a "Draft" until we've had a chance to talk this over. Thanks!

Example

// before
let res = client
    .clone()                                      // we need to `clone` here
    .into_database_client(database_name.clone())  // because this consumes `self`
    .create_collection("panzadoro", "/id")
    .into_future()
    .await?;

// after
let res = client                             // no more `clone` required
    .database(database_name.clone())         // no more `into_` or `_client`
    .create_collection("panzadoro", "/id")
    .into_future()
    .await?;

Prior Art

I felt reasonably confident that dropping the .clone requirement by users was the right way to go. This is a typical Rust API design issue. I was less sure about dropping the _client suffixes, so I checked our other SDKs before trying this out. Aside from Rust I'm most comfortable reading JavaScript and Golang, and neither of their respective Cosmos SDKs seems to have the _client convention, making Rust the odd one out.

// JavaScript
// source: https://docs.microsoft.com/en-us/azure/cosmos-db/sql/sql-api-sdk-node

container.items.query('SELECT * from c').fetchAll()
container.items.query('SELECT * from c').fetchNext()
for await(const { result: item } in client.databases.readAll().getAsyncIterator()) {
    console.log(item.id)
}
// Golang
// source: https://docs.microsoft.com/en-us/azure/cosmos-db/mongodb/create-mongodb-go

func create(desc string) {
    c := connect()
    ctx := context.Background()
    defer c.Disconnect(ctx)

    todoCollection := c.Database(database).Collection(collection)
    r, err := todoCollection.InsertOne(ctx, Todo{Description: desc, Status: statusPending})
    if err != nil {
        log.Fatalf("failed to add todo %v", err)
    }

Future Opportunities

We're almost done with the IntoFuture conversions, and I'm going to be filing a stabilization PR for this in Rust soon too. No guarantees, though I'm feeling optimistic. Once that comes through our earlier example would be streamlined even further:

// with `IntoFuture` stabilized, we no longer need to call `into_future` by hand
let create_collection_response = client
    .database(database_name.clone())
    .create_collection("panzadoro", "/id")
    .await?;

We can probably streamline this even further by removing the clone requirement on database_name and other APIs where we currently require it. And with in #591 we're discussing ways we could potentially streamline the Cosmos SDK client construction even more.

I think now that we're rounding out some of our existing refactors we can start thinking of ways we can streamline the APIs, and I hope this PR feels like a step in the right direction!

@yoshuawuyts yoshuawuyts force-pushed the simplify-clients branch 3 times, most recently from 1cfae56 to ba6a895 Compare March 8, 2022 13:47
@yoshuawuyts yoshuawuyts marked this pull request as ready for review March 8, 2022 17:02
@heaths
Copy link
Member

heaths commented Mar 8, 2022

For a good overview of clients and sub-clients, see https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-subclients. @annelo-msft drove this effort and while the guidelines above are specific to .NET, in concept they are fairly cross-language.

@johnbatty
Copy link
Contributor

@yoshuawuyts In case you aren't aware, the Rust SDK storage crates do currently have sub-clients that are similar to Heath's link above.

Example from the storage_blobs example put_block_blob_00.rs:

    let blob_client =
        StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key)
            .as_container_client(&container)
            .as_blob_client(&blob_name);

Another example from list_blobs_00.rs:

    let http_client = azure_core::new_http_client();
    let storage_client =
        StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key)
            .as_storage_client();
    let blob_service = storage_client.as_blob_service_client();
    ...
    let iv = blob_service.list_containers().execute().await?;

When I was working on splitting up the storage crates, I did play with a similar change to what you have done here to make the names less verbose by dropping the as_... and _client prefix/suffix, resulting in something like this:

    let blob =
        StorageAccountClient::new_access_key(http_client.clone(), &account, &master_key)
            .container(&container)
            .blob(&blob_name);

However, the following code that uses the blob client looks like this...

    let res = blob_client
        .put_block_blob(data.clone())
        .content_type("text/plain")
        .hash(&hash)
        .execute()
        .await?;

In the end I (slightly reluctantly) decided that this might be less clear if blob_client was called simply blob, as users might think that blob is a chunk of data whereas it is actually a client. I did look as some of the Python SDK examples to see how this was handled, and they used a mixture of styles in their example - sometimes calling the blob client blob_client, and other times simply blob.

Personally I do find the shorter names more "Rusty", and would be happy to switch to the more concise naming.

@heaths
Copy link
Member

heaths commented Mar 8, 2022

this might be less clear if blob_client was called simply blob

This is why I'm leaning toward keeping the _client suffix. As we also discussed in the meeting, sometimes you have similarly-named methods that return either a resource - like a blob - or a client to act on the resource - like a BlobClient - and making that clear in the function name sounds like a slightly better alternative.

/cc @annelo-msft @tg-msft for their thoughts.

Certainly where we can reduce noise / unnecessities like @yoshuawuyts is also suggestion is great.

@tg-msft
Copy link
Member

tg-msft commented Mar 8, 2022

We've found tremendous benefit in mirroring the resource hierarchy names in our navigation methods. A lot of customers struggle to make the distinction between simply walking down the resource hierarchy locally vs. doing something on the service that modifies those resources (i.e., am I getting a new container client or creating a container?). Consistent use of Client or the sub-client name creates a pattern to make the difference obvious. I've seen users completely lost at the beginning of a UX Study start to pick that up by the end. Even the Go folks, who appear allergic to pressing their keyboards, are using blobSvc.NewContainerClient(name) to walk down the hierarchy.

@rylev rylev mentioned this pull request Mar 11, 2022
@rylev
Copy link
Contributor

rylev commented Mar 11, 2022

Closing in favor of #684

@rylev rylev closed this Mar 11, 2022
@yoshuawuyts yoshuawuyts deleted the simplify-clients branch March 11, 2022 11:15
bmc-msft pushed a commit to bmc-msft/azure-sdk-for-rust that referenced this pull request Jun 23, 2022
Per discussion in Azure#676, this renames the various as_X_client to X_client
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.

5 participants