-
Notifications
You must be signed in to change notification settings - Fork 715
Add Cosmos database builder pattern for child containers #8266
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a builder pattern for Azure Cosmos DB that enables registering multiple containers against a single database with shared client configuration. Key changes include the creation of the CosmosDatabaseBuilder class with a chainable API, extension methods in AspireMicrosoftAzureCosmosExtensions for configuring the database builder, and updated tests and playground samples to validate and demonstrate its usage.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Components/Aspire.Microsoft.Azure.Cosmos/CosmosDatabaseBuilder.cs | Implements the new builder pattern to register containers using a shared CosmosClient. |
| src/Components/Aspire.Microsoft.Azure.Cosmos/AspireMicrosoftAzureCosmosExtensions.cs | Adds extension methods including WithAzureCosmosDatabase and an overload for AddContainer. |
| tests/Aspire.Microsoft.Azure.Cosmos.Tests/AspireMicrosoftAzureCosmosExtensionsTests.cs | Provides tests for multiple container registrations and custom client options. |
| playground/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs | Updates sample usage to chain container registration. |
| playground/CosmosEndToEnd/CosmosEndToEnd.AppHost/Program.cs | Updates project integration to reference multiple containers via the new builder API. |
| var entries = db.AddContainer("entries", "/id"); | ||
| var users = db.AddContainer("users", "/id"); | ||
|
|
Copilot
AI
Mar 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AddContainer method is being invoked with a partition key parameter that is not supported by the current implementation. Please either adjust the method to accept the extra parameter or revise the usage to only pass a container name.
| var entries = db.AddContainer("entries", "/id"); | |
| var users = db.AddContainer("users", "/id"); | |
| var entries = db.AddContainer("entries"); | |
| var users = db.AddContainer("users"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting obsevation, Copilot! We're introducing an AddContainer API in the client integration that might cause people to be confused by the AddContainer API that we use in the hosting side.
I like the name AddContainer though and would like to avoid dismabiguating between the two with a different name. Maybe WithContainer? But I like the way With CosmosDatabase Add Container reads from a natural language perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way we've been disambiguating between the hosting APIs and the client APIs is that the client APIs always end in Client. e.g. AddAzureCosmosClient.
Maybe we should follow that same naming pattern in the new APIs we are introducing here. I should have caught this in the previous PR.
| builder.AddAzureCosmosDatabase("db"); | ||
| builder.AddAzureCosmosContainer("entries"); | ||
| builder.WithAzureCosmosDatabase("db") | ||
| .AddContainer("entries") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be called WithKeyedContainer.
- We use "With" when you return the same builder, "Add" when you return the new thing being added.
- Adding "Keyed" makes it obvious that the service will be keyed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the root method should be AddAzureCosmosDatabase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the root method should be AddAzureCosmosDatabase.
I like the prefix With for this API because it gives a better clue that you're registering containers within the context of a specific database.
Also, with:
"Add" when you return the new thing being added.
The WithAzureCosmosDatabase doesn't register any services currently so using the Add prefix would violate this convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that we'd have:
builder.AddAzureCosmosDatabase("db")
.WithKeyedContainer("entries")
.WithKeyedContainer("users");And it would register 3 services:
- The unkeyed Cosmos
Database. - Two keyed Cosmos
Containerservices.
And if I want to register keyed Databases, I would use:
builder.AddKeyedAzureCosmosDatabase("db")
.WithKeyedContainer("entries")
.WithKeyedContainer("users");There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it would register 3 services:
The unkeyed Cosmos Database.
Two keyed Cosmos Container services.
I find this a little confusing given that our convention in the other types has been that they only register the service that they are associated with. For example, currently when you call AddKeyedAzureCosmosDatabase, you don't also get a CosmosClient registered as well.
src/Components/Aspire.Microsoft.Azure.Cosmos/AspireMicrosoftAzureCosmosExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Microsoft.Azure.Cosmos/AspireMicrosoftAzureCosmosExtensions.cs
Outdated
Show resolved
Hide resolved
427df7a to
7ab3cc7
Compare
|
@davidfowl @eerhardt Ping for review on latest API. |
|
These APIs look great. |
| string name, | ||
| Action<MicrosoftAzureCosmosSettings>? configureSettings = null, | ||
| Action<CosmosClientOptions>? configureClientOptions = null) | ||
| public static void AddKeyedAzureCosmosContainer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need/want these direct "AddContainer" APIs? Or is it good enough to just have the new AddCosmosDatabase + builder APIs?
| .WithReference(db).WaitFor(db) | ||
| .WithReference(container).WaitFor(container); | ||
| .WithReference(entries).WaitFor(entries) | ||
| .WithReference(users).WaitFor(users); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You really only need to .WithReference(db).WaitFor(db);. The ApiService doesn't use the entries and users references, so we can remove them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, it doesn't end up being helpful outside of validating the way connection strings are emitted in the manifest. We can make this more interesting by changing the resource and database names so that we can validate the behavior of the resolution logic:
| return _client.GetContainer(settings.DatabaseName, connectionInfo?.ContainerName ?? name); |
7ab3cc7 to
dc5dae9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
This PR introduces new APIs to the Azure Cosmos integration to support interacting with multiple containers against the same database. Our goal is to reuse client configuration for multiple containers. Our current set of APIs makes it possible to reuse client information if there is always one database -> one container.
This falls apart when there are multiple containers that are uniquely identified by their service key. In this case, we can assume that all containers share the same database but that might not always be the case.
The
WithCosmosDatabaseBuilderAPI introduced here makes it possible to chain the database->container relationships:In application code, these services can be resolved as follows: