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 SDK based repository #263

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

richard-einfinity
Copy link
Contributor

Cosmos SDK based repository with configuration helpers.

@adrianhall adrianhall requested a review from Copilot February 9, 2025 00:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 37 changed files in this pull request and generated 4 comments.

Files not reviewed (15)
  • Datasync.Toolkit.sln: Language not supported
  • Directory.Packages.props: Language not supported
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Properties/launchSettings.json: Language not supported
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Sample.Datasync.Server.SingleContainer.csproj: Language not supported
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Sample.Datasync.Server.SingleContainer.http: Language not supported
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/appsettings.Development.json: Language not supported
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/appsettings.json: Language not supported
  • src/CommunityToolkit.Datasync.Server.CosmosDb/CommunityToolkit.Datasync.Server.CosmosDb.csproj: Language not supported
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Controllers/TodoItemController.cs: Evaluated as low risk
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Controllers/TodoListController.cs: Evaluated as low risk
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Models/TodoList.cs: Evaluated as low risk
  • samples/datasync-server/datasync-server-cosmosdb/Sample.Datasync.Server.SingleContainer/Program.cs: Evaluated as low risk
  • src/CommunityToolkit.Datasync.Server.CosmosDb/Configuration/CosmosContainerOptions.cs: Evaluated as low risk
  • src/CommunityToolkit.Datasync.Server.CosmosDb/Configuration/CosmosService.cs: Evaluated as low risk
  • src/CommunityToolkit.Datasync.Server.CosmosDb/Configuration/CosmosSingleTableOptions.cs: Evaluated as low risk

@adrianhall
Copy link
Collaborator

This seems overly complex for what is basically a repository over a cosmos container (using the Azure CosmosDb SDK) with a method for getting the partition key. I’ll review this more fully during the working week, but we should probably have a chat about design here. Maybe a quick design doc on what you are trying to achieve, how it’s done, etc.

@richard-einfinity
Copy link
Contributor Author

Hi @adrianhall,

Yeah, I don't think I've got it quite right at the moment.

you're right the basics here are the CosmosTableRepository implementation and the accompanying ICosmosTableOptions interface. The table options provide the entity specific methods for deducing the partition keys etc. I've provided a few base implementations for what I think are common scenarios. I think this bit is quite straightforward, and could be submitted on their own.

With the rest my goal was trying to provide a more streamlined configuration and development experience. By basically providing a fluent interface for saying here's my database, then in that I have these containers and in these containers I have these entities. The extensions then provide things like the datasync composite indexes through a default indexing policy. And a helper service primarily for the development environment which helps create and tear down the database. I think this has become a bit unwieldy and I think I need to restructure it again, but I think compared to what I was doing in my api project before this the configuration is a lot more streamlined.

In the initial sample the interfaces are being used to setup a database and a single container for both the entities using the entity name as the partition key. I probably need to align the json serializer options to the data sync defaults.

@adrianhall
Copy link
Collaborator

I suspect you should separate out the “configuring Cosmos” bit from the table repository. Let’s assume you provide an ICosmosContainer implementation which you say holds TEntity, then an ICosmosRepositoryOptions (or whatever) that tells the repository how to get the partition.

So you would have three classes and an interface

Public interface ICosmosRepositoryOptions {}

Public abstract class CosmosTableData : ITableData {}

Public class CosmosRepository<TEntity>(ICosmosContainer, ICosmosRepositoryOptions) : IRepository<TEntity>
  Where TEntity : CosmosTableData
{}

Public class CosmosRepositoryOptions<TEntity> : ICosmosRepositoryOptions where TEntity : CosmosTableData
{}

I suspect that the setup would be something like this:

Public class TodoItemsController : TableController<TodoItem>
{
  Public TodoItemsController(ICosmosDatabase database)
  {
    ICosmosContainer container = database.GetContainer(“todoitems”);
    CosmosRepositoryOptions<TodoItem> options = new()
    {
        PartitionKeyFinder = (TodoItem entity) => entity.PartitionKey;
    }
   Repository = new CosmosRepository<TodoItem>(container, options);
}

It’s reasonable to add an extension method (e.g.ICosmosContainer.ConfigureDatasyncIndices()) - that should be enough to get everything done. Anything additional would be setting options on individual operations, I think, since the account, database, and container are configured within the context of the application setup.

What additional are you thinking of?

@richard-einfinity
Copy link
Contributor Author

I've moved the essential classes to the root of the project. These classes basically do what you describe. There's base implementations of ITableData and ICosmosTableOptions interface.

The rest of the stuff in the configuration folder is for convenience really, they handle database and container creation in development and register the repositories and provided options in the DI container. In production the creation is ignored if the database and containers exist.

We have 2 scenarios where we need to calculate the Partition key.

When we have the entity (Create, Replace)
When we don't have the entity (Read, Delete)

When we have the entity it's quite straight forward, when we don't have the entity we need to get it from the key, maybe through packing, which is my current approach. But ultimately the implementor will need to decide. So I've provided an interface for both.

The rest of the properties databaseId and containerId are quite straight forward and are just used for referencing the Container from within the repository.

The shouldUpdateTimestamp controls whether the repository updates the timestamp, default to true but can be set to false if you have trigger defined on the container.

The nature of cosmos is that multiple entities can live in the same container. By default EF does this under the hood unless you specify a container per entity via the model builder. So there's a method on the options to filter the dataset before returning the IQueryable primarily used to select the right entity from the container. Defaults to just true for single entity containers and a filter on the entity name for shared containers.

There are some base implementations of the table options. One for single entity per container and one for multiple entities per container with some default.

The suggested extension method on the cosmos container would be ignore as container properties such as partition keys and indexing policies can only be set when the container is created.

Hopefully that make a bit of sense

@adrianhall
Copy link
Collaborator

Makes sense. My experience is that infra folks like to configure containers via bicep/terraform, so I don't see a lot of code configuration.

I would split up this PR into two - one for basic support (the four or five top level classes and interfaces) and a second one for the support stuff.

Introduced new projects and configurations to support Cosmos DB integration in a .NET web application. This includes:

- Adding new project references and updating solution configurations.
- Setting up necessary services, controllers, models, and configuration files.
- Implementing a Cosmos DB repository with options, extensions, and tests.
- Adding license information to several files.
- Creating a test project to ensure repository functionality and correctness.
@richard-einfinity
Copy link
Contributor Author

I've pulled the support classes into a separate branch.

I'm thinking maybe I need some tests around partition key parsing. Shared containers. And perhaps that sample project isn't in the right place.

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.

2 participants