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

[Cosmos] Remove options builders and (most) impl parameters #1880

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

analogrelay
Copy link
Member

This realizes some recent changes to guidelines regarding options and service method parameters. Specifically:

  • Options types should now be PODs (plain old data) with public fields and avoid using builders unless absolutely necessary.
  • Avoid the use of impl Into and impl AsRef parameters

The options changes were easy and deleted a bunch of unnecessary boilerplate, my favorite! Avoiding the use of impls led to the following exceptions (which I mentioned when we were chatting about the guidelines earlier but will detail again here):

  • impl Into<Query> is used for APIs that accept a query. Cosmos queries are made up of a query text, and a parameter. Other SDKs, such as the Go SDK, encode the parameters in the options, but I don't feel like that's an accurate modelling. Query parameters appear in the body, and having to merge a parameter with an option to create the body doesn't feel right. We want to support calling container_client.query_items("SELECT * FROM c", ...) and container_client.query_items(Query::from("SELECT * FROM c WHERE c.id = @id").with_parameter("@id", id), ...), so we use impl Into<Query> here, and implement From<&'static str> and From<String> on Query.

  • impl Into<PartitionKey> is used on APIs for item CRUD, which require a partition key be specified. Cosmos allows multiple types as partition keys (strings, numbers, booleans) and allows up to 3 levels of hierarchical keys. We want to be able to call container_client.read_item(item_id, "partition1", ...), and container_client.read_item(item_id, ("parent_partition", 42, "grandchild_partition"), ...). So, we accept impl Into<PartitionKey> and have several implementations (in sdk/data/cosmos/azure_data_cosmos/partition_key.rs) of From for PartitionKey

  • Finally, impl Into<QueryPartitionStrategy> is used on APIs that query items. Cosmos supports single-partition queries, where the partition key must be specified independent of the query, and cross-partition queries (where no partition key is specified). The Cosmos Rust SDK does not support cross-partition queries at this time but may add that support later. So, we use QueryPartitionStrategy as an enum that currently only has a single variant (QueryPartitionStrategy::SinglePartition(PartitionKey)) and have impl<T: Into<PartitionKey>> From<T> for QueryPartitionStrategy which allows the API to accept the same arguments as the CRUD APIs, and a future QueryPartitionStrategy::CrossPartition marker to indicate a cross-partition query.

I left all those cases above alone, though we can absolutely continue discussions there.

cc @heaths @JeffreyRichter @RickWinter (as FYI for the exceptions pointed out above, though I'm sure we'll do some more formal API reviewing later)

@analogrelay analogrelay force-pushed the ashleyst/yeet-impls-and-builders branch from 32d1fd0 to 3f86b01 Compare October 30, 2024 16:05
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

A change or two I'd like to see, but otherwise nothing blocking.

sdk/cosmos/azure_data_cosmos/examples/cosmos/main.rs Outdated Show resolved Hide resolved
@analogrelay
Copy link
Member Author

Drafting while I add method_options to our options

Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

I know it's draft right now, but this looks fine to me in it's current state (in case I don't have a chance to re-review before going on vacation).

@analogrelay
Copy link
Member Author

analogrelay commented Oct 30, 2024

Re-un-drafting. I've added a method_options: azure_core::ClientMethodOptions field to every method options type. Since that type carries an optional Context, I've also changed all our methods to use that Context as the base context we add our ResourceLink value to.

@analogrelay analogrelay merged commit 9bc2dae into Azure:main Oct 31, 2024
26 checks passed
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