Add container client options - #4992
Conversation
Add the required optional options argument to DatabaseClient::container_client and update all call sites so the API can evolve without another signature change. Document the breaking migration and preserve existing behavior by passing None. Fixes Azure#4617 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds a future-proof options parameter to container client construction and migrates existing callers.
Changes:
- Public API: adds and exports
ContainerClientOptions. - Requires
DatabaseClient::container_clientcallers to pass optional options. - Updates call sites and documents the breaking change.
Show a summary per file
| File | Description |
|---|---|
sdk/cosmos/azure_data_cosmos/src/clients/database_client.rs |
Adds the options parameter. |
sdk/cosmos/azure_data_cosmos/src/options/container.rs |
Defines the options type. |
sdk/cosmos/azure_data_cosmos/src/options/mod.rs |
Exports the options type. |
sdk/cosmos/azure_data_cosmos/CHANGELOG.md |
Documents migration. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/batch.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/create.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/delete.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/metadata.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/query.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/read.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/replace.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/examples/cosmos/upsert.rs |
Migrates example. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_aad.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_batch.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_containers.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_fault_injection.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_items.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_patch.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_query_features.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_response_metadata.rs |
Migrates tests. |
sdk/cosmos/azure_data_cosmos/tests/framework/test_client.rs |
Migrates test helpers. |
sdk/cosmos/azure_data_cosmos/tests/framework/test_data.rs |
Migrates test data helper. |
sdk/cosmos/azure_data_cosmos/tests/gateway_v2_tests/gateway_v2_e2e.rs |
Migrates gateway tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs |
Migrates emulator tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/change_feed.rs |
Migrates emulator tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/cosmos_hpk_split.rs |
Migrates emulator tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/dtx_sdk_validation.rs |
Migrates validation tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/end_to_end.rs |
Migrates end-to-end tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/handler_propagation.rs |
Migrates handler tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/hpk.rs |
Migrates HPK tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/partition_key_equality.rs |
Migrates partition tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/partition_range_drain.rs |
Migrates drain test. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/query_comparison.rs |
Migrates query tests. |
sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/user_agent.rs |
Migrates user-agent tests. |
sdk/cosmos/azure_data_cosmos/tests/multi_write_tests/cosmos_multi_write_fault_injection.rs |
Migrates fault tests. |
sdk/cosmos/azure_data_cosmos/tests/multi_write_tests/cosmos_multi_write_retry_policies.rs |
Migrates retry tests. |
sdk/cosmos/azure_data_cosmos_perf/src/setup.rs |
Migrates performance setup. |
sdk/cosmos/azure_data_cosmos_perf/tests/binary_sampled_testdata.rs |
Migrates performance test. |
sdk/cosmos/azure_data_cosmos_observability_harness/src/workload.rs |
Migrates observability workload. |
samples/cosmos_read_item_native_tls/src/main.rs |
Migrates native TLS sample. |
Review details
Suppressed comments (2)
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_fault_injection.rs:1590
- This added argument pushes the line past rustfmt's default width, so the repository's required
cargo fmt --checkwill rewrite it. Wrap the method chain as shown.
let fault_container_client = fault_db_client.container_client(&container_id, None).await?;
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_fault_injection.rs:1477
- This added argument pushes the line past rustfmt's default width, so the repository's required
cargo fmt --checkwill rewrite it. Wrap the method chain as shown.
let fault_container_client = fault_db_client.container_client(&container_id, None).await?;
- Files reviewed: 40/40 changed files
- Comments generated: 3
- Review effort level: Balanced
Carry ContainerClientOptions operation settings through container metadata resolution and document the updated API example. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add the required optional options argument to DatabaseClient::container_client and update all call sites so the API can evolve without another signature change. Document the breaking migration and preserve existing behavior by passing None. Fixes Azure#4617 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Carry ContainerClientOptions operation settings through container metadata resolution and document the updated API example. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…lay-cosmos-container-client-options
1fbce67 to
5bc87aa
Compare
Resolve conflicts while preserving container client options and upstream Cosmos API updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve Cosmos API conflicts while preserving container client options. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| resolved.database_rid(), | ||
| db_rid.as_str() | ||
| )) | ||
| .with_status(azure_data_cosmos_driver::error::CosmosStatus::CLIENT_INVALID_RESOURCE_ID) |
There was a problem hiding this comment.
Nit: I seem to see a bunch of reformatting from one line to multiple and back, is there something we can do to reduce noise here (like configuring a longer max line length) or something?
| /// This method eagerly resolves immutable container metadata (resource ID and partition key | ||
| /// definition) from the service, so the returned client is ready for immediate use without | ||
| /// per-operation cache lookups. | ||
| /// | ||
| /// The container's addressing mode must match this database's: a name-addressed | ||
| /// database accepts only name-addressed containers, and a RID-addressed database | ||
| /// accepts only [`ResourceId`](crate::ResourceId)-addressed containers. | ||
| /// This method eagerly resolves immutable container metadata before returning the client. | ||
| /// | ||
| /// # Arguments | ||
| /// * `container` - The name or RID of the container. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if the container does not exist, the metadata cannot be | ||
| /// resolved, or the addressing mode does not match this database's. | ||
| /// * `options` - Optional parameters for creating the client. |
There was a problem hiding this comment.
Is this a mis-merge, or did you remove the other content on purpose?
| { | ||
| Ok(response) => { | ||
| let created = response.into_model()?; | ||
| return db_client.container_client(&created.id, None).await; |
There was a problem hiding this comment.
A bunch of these don't seem to remove the old call?
Simon Moreno (simorenoh)
left a comment
There was a problem hiding this comment.
some small comments, seems like some merge issue might have missed some bits here and there - locally copilot also called out some tests went through un-updated in tests/in_memory_emulator_tests/order_by.rs, maybe other files are also missing the options propagation.
| } | ||
| assert_eq!(vec![properties.id.clone()], ids); | ||
|
|
||
| let container_client = db_client.container_client(&properties.id, None).await?; |
There was a problem hiding this comment.
is this just a duplicate line from the one below?
| (ResourceIdentity::Rid(db_rid), ResourceIdentity::Rid(container_rid)) => { | ||
| let resolved = context | ||
| .driver | ||
| .resolve_container_by_rid(container_rid.as_str()) |
There was a problem hiding this comment.
does the rid method also need the options.operation passed in?
|
|
||
| ### Breaking Changes | ||
|
|
||
| - `DatabaseClient::container_client` now requires a second argument of type `Option<ContainerClientOptions>`; pass `None` to retain the previous behavior. |
There was a problem hiding this comment.
nit: could you link the pr
| (ResourceIdentity::Rid(db_rid), ResourceIdentity::Rid(container_rid)) => { | ||
| let resolved = context | ||
| .driver | ||
| .resolve_container_by_rid(container_rid.as_str()) |
There was a problem hiding this comment.
we should add the options in the rid path as well
|
Definitely looks like some merge issues. I'll just pick this up when I'm back, it's a fairly minor change. Alternatively, anyone else is welcome to grab it and finish it off. |
DatabaseClient::container_clientneeds an options argument so future client construction settings can be added without another signature change.This adds
OperationOptionsto the publicContainerClientOptionstype and carries those settings through container metadata resolution into the driver. Existing callers passNone, preserving behavior, and the README example documents the required argument.The driver API now accepts operation options for container resolution while retaining default options for its existing direct callers.
Fixes: #4617