Skip to content

Conversation

@richard-einfinity
Copy link
Contributor

PR for CosmosDB repository support.

Add Cosmos Specific Repository with support for hierarchical partition keys.
Add Cosmos Specific Samples with bicep deployment files.
Add Cosmos Repository Test projects with additional Packed key specific tests.

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.
- Removed `Sample.Datasync.Server.SingleContainer` project from `Datasync.Toolkit.sln`.
- Added `CommunityToolkit.Datasync.Server.CosmosDb.Test` to the solution.
- Created new solution file `Datasync.Server.CosmosDb.SingleContainer.sln` with relevant projects.
- Updated `main.bicep` to include licensing and resource deployment parameters.
- Added `main.parameters.json` for Bicep deployment parameters.
- Created `resources.bicep` to define CosmosDB and App Service resources.
- Updated `Sample.Datasync.Server.SingleContainer.csproj` with necessary project references.
- Updated `main.bicep` to include new parameters for `accountName`, `databaseName`, and `containerName`, modifying existing parameters to use these values.
- Adjusted composite indexes in `resources.bicep` for better data organization.
- Enhanced `PackedKeyRepository_Tests.cs` with setup code and new test cases for error handling of malformed IDs.
- Introduced `PackedKeyOptions.cs` to manage ID generation and partition key handling for Cosmos DB entities.
Modified the `partitionKey` property in the Bicep file, changing the path from `'/myPartitionKey'` to `'/entity'`. This change reflects an updated partitioning strategy for the database container.
@richard-einfinity richard-einfinity marked this pull request as ready for review February 19, 2025 22:47
@adrianhall adrianhall self-assigned this Feb 20, 2025
Copy link
Contributor

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 30 out of 30 changed files in this pull request and generated 3 comments.

Copy link
Collaborator

@adrianhall adrianhall left a comment

Choose a reason for hiding this comment

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

Aside from the two comments, there are also code style warnings being emitted - please fix those.

builder.Services.AddSingleton(cosmosClient);
builder.Services.AddSingleton<ICosmosTableOptions<TodoItem>>(new CosmosSharedTableOptions<TodoItem>("TodoDb", "TodoContainer"));
builder.Services.AddSingleton<ICosmosTableOptions<TodoList>>(new CosmosSharedTableOptions<TodoList>("TodoDb", "TodoContainer"));
builder.Services.AddSingleton(typeof(IRepository<>), typeof(CosmosTableRepository<>));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there significant overhead with creating a repository per request? Why not just create the repository within the table controller?

Ditto for the table options - why are they done here and not in the table controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is be correct use of DI. I like my config in one place not throughout the code, If I was going to swap out the implementation I'm doing it then in one place. I know this is complicated slightly by the provider specific ITableData implementation in this project. I've opted for Singleton registration in this case as there's no per request dependencies in the Repository or the Options, the CosmosClient is also singleton which is recommended.

Changing the registration to scoped would move these to be resolved per request.

<PackageReference Include="Microsoft.Azure.Cosmos" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
<PackageReference Include="Newtonsoft.Json" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is Newtonsoft required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Adrian, this is a dependency of the Cosmos Sdk even though I've elected to use System.Text.Json I believe Newtonsoft is still required under the hood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've attempted to remove the dependency and disable the check but the program crashes on the instantiation of the CosmosClient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’ll open a bug with the Cosmos folks on Monday. Newtonsoft should be defined as a transitive dependency in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @adrianhall, there are already multiple threads on this issue. We can mark the library to not required the dependency directly but then it would be up to the consumer to install it in their api project, it would generate an error at build time. We would still need it as a dependency then for the sample project. It's a bit of a mess, they seem to be one foot in one foot out.

@adrianhall adrianhall linked an issue Feb 20, 2025 that may be closed by this pull request
Updated the declarations for `builder` and `app` to use explicit types (`WebApplicationBuilder` and `WebApplication`, respectively) instead of `var`. This change improves code clarity and readability.
@adrianhall
Copy link
Collaborator

After deploying the server (I copied and edited the azure.yml from the test service), I get the following when doing a request:

    <title> HTTP Error 500.30 - ASP.NET Core app failed to start </title>

What I did:

  1. Change the long name in the .http file to @BaseURL to make it easier.
  2. Copy the azure.yml from the existing sample to the new sample
  3. Run "azd up" to deploy everything.
  4. Open the solution
  5. Run the request for creating an item.

Copying the CosmosDB connection string into the secrets.json ConnectionStrings:DefaultConnection results in a working system. Diagnosis - you aren't setting the ConnectionStrings__DefaultConnection to the right thing in the bicep code. This is the problem with working with App Service. the connection strings are not named what you expect them to be.

Suggest you do the following:

  1. Add an azure.yml file and actually deploy the sample to Azure to ensure it is working.

  2. Change the connectionString reader in Program.cs (line 17) of the sample to

    string connectionString = builder.Configuration.GetConnectionString("DefaultConnection")
        ?? builder.Configuration["DOCDBCONNSTR_DefaultConnection"]
        ?? throw new ApplicationException("DefaultConnection is not set");

There is a potential problem with the output of the library. Here is an example:

{"title":"Second item","isComplete":false,"id":"2","deleted":false,"updatedAt":"2025-02-21T21:21:05.056Z","version":"IjAwMDBjZGNjLTAwMDAtMDIwMC0wMDAwLTY3YjhlZTQxMDAwMCI=","_etag":"\u00220000cdcc-0000-0200-0000-67b8ee410000\u0022"}

While the version and ETag header match, the _etag doesn't match the version. Maybe it should? Why are you creating a version when the service provides one for you? The service doesn't provide an UpdatedAt property, but we should reuse the ETag rather than storing a version.

@richard-einfinity
Copy link
Contributor Author

Hi @adrianhall

I'll take another look at the deployment as suggested. I had a little trouble with the azd tool, it's my first foray into bicep, I was expecting it to expand variables in the main.parameters.json file when passed, but it didn't. so ended up passing parameters on the command line. Ended up using az deploy to test. Perhaps the azure.yml file is the piece I was missing, I'll give it another go.

Version is the Base64Encoded value of the _etag. The base CosmosTableData is converting the _etag to bytes and the serializer is converting it to Base64.

Ideally, I don't want to be exposing the _etag and I don't really want Version saved in the store since it's derived from the cosmos generated etag... however as the model is using the same serializer attributes for both if I add an ignore attribute to the ETag its never retrieved from the store and if I ignore the Version properties it not serialized out via the api. I'm thinking it's one of those things we need to live with for now.

Renamed connection string resource to 'appsettings' and updated its properties. Added HTTP logging settings for the app service. Enhanced `azure.yaml` with schema reference, project metadata, and defined a deployment workflow. Configured backend service with project details and hosting environment.
@richard-einfinity
Copy link
Contributor Author

@adrianhall yaml file done the trick. Bicep file updated. azd up now works nicely.

@adrianhall
Copy link
Collaborator

Nice - and I agree with your sentiment on the _etag / version. It’s something we will live with.

@adrianhall adrianhall merged commit f680cc4 into CommunityToolkit:main Feb 28, 2025
5 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.

Cosmos DB Expansion

2 participants