Add mongodb container and connection support #986

ailtonguitar · 2023-11-21T19:15:00Z

Adds MongoDB resources to connection and container

ailtonguitar · 2023-11-21T19:22:45Z

@ailtonguitar please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@dotnet-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"
Contributor License Agreement

@dotnet-policy-service agree company="Microsoft"

src/Components/Aspire.MongoDB.Driver/Aspire.MongoDB.Driver.csproj

src/Components/Aspire.MongoDB.Driver/AspireMongoDbDriverExtensions.cs

davidfowl · 2023-11-24T08:19:35Z

src/Components/Aspire.MongoDB.Driver/ConfigurationSchema.json

+        "MongoDB": {
+          "type": "object",
+          "properties": {
+            "ConnectionString": {


Is this the only setting worth exposing? There must be more 😄

Hi @davidfowl !
I'll add more settings here.
Also, I've tried to add healthcheck support but the package AspNetCore.HealthChecks.MongoDb is not restored by the configured package sources. Is there any problem to use this package here?

The package will need to be added to our internal mirror.

I've implemented a custom health check for that but if you prefer I can change it to use the package when it available.

Please use https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/tree/master/src/HealthChecks.MongoDb instead of implementing a custom health check.

Hi @eerhardt I've already tried to use this health check but the package is not available.

Please see the previous comments.
#986 (comment)

Ah, sorry I missed that. The package needs to be mirrored to our trusted feed. I'm doing that now. It should be done in a few minutes.

I've changed to use the package, please review.

tests/Aspire.MongoDB.Driver.Tests/AspireMongoDbDriverExtensionsTests.cs

src/Components/Aspire.MongoDB.Driver/README.md

eerhardt · 2023-11-27T20:52:53Z

src/Components/Aspire.MongoDB.Driver/ConfigurationSchema.json

+        "MongoDB": {
+          "type": "object",
+          "properties": {
+            "ConnectionString": {


Please use https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/tree/master/src/HealthChecks.MongoDb instead of implementing a custom health check.

eerhardt · 2023-11-27T20:53:20Z

tests/Aspire.MongoDB.Driver.Tests/Aspire.MongoDB.Driver.Tests.csproj

+
+  <PropertyGroup>
+    <TargetFramework>$(NetCurrent)</TargetFramework>
+    <NoWarn>CS8002;CS1591</NoWarn>


Why are these being suppressed?

Because the official MongoDB.Driver package doesn't have a strongly name and it causes compilation errors.

https://jira.mongodb.org/browse/CSHARP-142

I've updated with the suggestion, can we close this one?

@jkotas @dsplaisted - are there any drawbacks/limitations/issues in .NET 8+ with using weak named assemblies?

One that @ericstj told me was:

CoreCLR will load any version of a weak named assembly. So even if you reference v2, if v1 happens to be lying around the runtime will load it and you'll get crazy errors later.

Our guidance https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming is that people consider strong naming their libraries. Although adding one is a binary breaking change.

CoreCLR will load any version of a weak named assembly. So even if you reference v2, if v1 happens to be lying around the runtime will load it and you'll get crazy errors later.

I didn't know that was the case, I'm kind of surprised by that.

Agree with concerns about Aspire being the canary here. Feels more like it should be a more centrally coordinated change across the suite of .NET products than something we just start doing. Suppressing the warning (given it's just a compiler-level thing) seems fine for us.

Feels more like it should be a more centrally coordinated change across the suite of .NET products than something we just start doing

I do not see what you would like to centrally coordinate. The strong name cannot be changed for anything existing, it would be breaking. The strong-name free world can be only enjoyed by brand new components, like Aspire.

If you do not want the Aspire to be first, it is ok. If the new components are not willing to do the jump, we will be stuck with strong naming forever for everything.

cc @davidfowl

Moving from ASP.NET Core 7 to ASP.NET Core 8 is a breaking change anyway though. Are you suggesting we wouldn't stop strong-naming those assemblies ever? We don't guarantee roll-forward across TFMs for apps works without recompilation.

That said, if the overall guidance is that new libraries targeting only modern .NET should not strong name their assemblies (and we update the docs to make that clearer), then I'm more inclined to try it in Aspire.

We don't guarantee roll-forward across TFMs for apps works without recompilation.

This applies to apps only, it does not apply to nuget packages. We do guarantee compatibility of public surface, so that packages targeting .NET 7 have a good chance of working on .NET 8.

Are you suggesting we wouldn't stop strong-naming those assemblies ever?

Never say never :-) .NET SDK and other tools do strong name matches in number of places. Removing strong name on existing assembly would break those tools when package targeting .NET 7 is used in .NET 8 app. We have not done analysis of what it would take to get all tools fixed.

we update the docs to make that clearer

dotnet/docs#38537

src/Components/Aspire.MongoDB.Driver/MongoDbHealthCheckSettings.cs

src/Components/Aspire.MongoDB.Driver/AspireMongoDbDriverExtensions.cs

eerhardt · 2023-12-01T20:39:13Z

src/Components/Aspire.MongoDB.Driver/MongoDBSettings.cs

+    /// <summary>
+    /// Gets or sets a integer value that indicates the MongoDB health check timeout in milliseconds.
+    /// </summary>
+    public int? HealthCheckTimeout { get; set; }


We don't have this is our other database components, for example Npgsql. I wonder if we should.

Thoughts, @roji @ajcvickers @AndriySvyryd?

src/Components/Aspire.MongoDB.Driver/AspireMongoDBDriverExtensions.cs

tests/Aspire.MongoDB.Driver.Tests/AspireMongoDBDriverExtensionsTests.cs

eerhardt · 2023-12-01T21:09:56Z

src/Components/Aspire.MongoDB.Driver/AspireMongoDBDriverExtensions.cs

+    private static void AddHealthCheck(
+        this IHostApplicationBuilder builder,
+        string connectionString,
+        string healthCheckName,
+        int? timeout)
+    {
+        builder.TryAddHealthCheck(
+            healthCheckName,
+            healthCheck => healthCheck.AddMongoDb(
+                connectionString,
+                healthCheckName,
+                null,
+                null,
+                timeout > 0 ? TimeSpan.FromMilliseconds(timeout.Value) : null));
+    }


Is this worth having a separate method to just call 1 method? Why not inline this code up where this method is called?

I've changed the place where the check if the healthcheck is enable is done, so now it makes sense to have a method to encapsulate all the logic related to the healthcheck feature.
Please, check gain.

eerhardt · 2023-12-01T22:04:05Z

tests/Aspire.MongoDB.Driver.Tests/ConformanceTests.cs

+    protected override void SetHealthCheck(MongoDBSettings options, bool enabled)
+    {
+        options.HealthCheckEnabled = enabled;
+        options.HealthCheckTimeout = 10;


Should we have a better default if we feel the need to set this everywhere?

I don't know if it can be applied everywhere, the idea here is to set a very short timeout to mongo health checkt and and avoid the tests to wait the default mongoclient timeout (30000).
But what is your idea?

tests/Aspire.MongoDB.Driver.Tests/ConformanceTests.cs

sebastienros · 2023-12-01T23:25:32Z

@ailtonguitar I pushed some changes. Please try to avoid force push if you can, I know it's your fork and you should do what pleases you with it, but I am afraid that is I do some changes they will be lost if you don't pay attention. It might also be nicer to understand the changes that you made overtime. And don't worry since we squash the commits before merging to main.

sebastienros · 2023-12-02T01:29:38Z

@ailtonguitar While trying your PR I think I realize there is a flaw and a mismatch with the concept of "database". I believe you followed the pattern defined by the PostgreSQL component, and added an AddDatabase extension method on the host in the same way. Then from an app we call builder.AddMongoDBClient("mydatabase") to inject a IMongoClient instance for this named resource.

The issue with this model is that the current authentication only works with the "admin" database. If you try it you will see that passing anything else than "admin" as the database name will fail auth because the environment variables used are only defining a user for this db.

Once I used "admin" and resolved the client for it, I could then call GetDatabase("anythingelse").

So right now it looks like the argument in AddDatabase should not be used "in" the connection string, but only for the name of the connection string, and force "admin" in the actual connection string. When it's deployed it will matter to use something else than /admin in the connection string, but at that point the environment will provide the actual one.

src/Components/Aspire.MongoDB.Driver/AspireMongoDBDriverExtensions.cs

DamianEdwards · 2023-12-02T17:35:39Z

@sebastienros I'm not sure that's right. I'm pretty sure this behavior is the same as all the other database hosting extensions. See #1170 for a report of the same behavior. The app is responsible for ensuring the specified database name exists so that the login can succeed. In the case of EF Core, it actually detects this state when you call DbContext.Database.MigrateAsync and falls back to connecting to the default database depending on the provider so that it can then create the requested database.

…erExtensions

…sTests.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

eerhardt

I think this is getting pretty close to a mergeable state. @mitchdenny - are you good with the Hosting changes?

src/Aspire.Hosting/MongoDB/MongoDBDatabaseResource.cs

src/Components/Aspire.MongoDB.Driver/README.md

tests/Aspire.MongoDB.Driver.Tests/ConformanceTests.cs

tests/testproject/TestProject.IntegrationServiceA/TestProject.IntegrationServiceA.csproj

src/Components/Aspire.MongoDB.Driver/AspireMongoDBDriverExtensions.cs

eerhardt · 2023-12-04T17:23:34Z

src/Components/Aspire.MongoDB.Driver/AspireMongoDBDriverExtensions.cs

+        builder.TryAddHealthCheck(
+            healthCheckName,
+            healthCheck => healthCheck.AddMongoDb(
+                settings.ConnectionString,


Ideally this should use the same IMongoClient instance that is registered in DI. See https://github.com/dotnet/aspire/blob/main/src/Components/README.md#health-checks.

Consider whether the Health Check should reuse the same client object registered in DI by the component or not. Reusing the same client object has the advantages of getting the same configuration, logging, etc during the health check.

The AspNetCore.HealthChecks.MongoDb does not support to pass an IMongoClient instance.
It is only possible to pass the connection string or the MongoClientSettings

I've created a new PR to add support for receiving an IMongoClient instance when configuring the healthcheck

Xabaril/AspNetCore.Diagnostics.HealthChecks#2121

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

…roject.IntegrationServiceA.csproj Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

sebastienros · 2023-12-04T23:27:28Z

I created a PR that will create the database in the docker container automatically, and uses admin in the connection string as this is the authentication database. Without it the connection tries to authenticate using the account and database define in configuration but it doesn't work.

ailtonguitar#1

I didn't want to push this change directly in case someone has other suggestions.

Update: PR closed, not required anymore

eerhardt

I think this looks good for an initial implementation.

Thanks @ailtonguitar for this great contribution.

If my only question needs a code change, we can follow up with a separate PR for it.

eerhardt · 2023-12-06T00:06:15Z

src/Components/Aspire.MongoDB.Driver/MongoDBSettings.cs

+    /// <summary>
+    /// Gets or sets a integer value that indicates the MongoDB health check timeout in milliseconds.
+    /// </summary>
+    public int? HealthCheckTimeout { get; set; }


This may be a silly question, but if this isn't set, the health check never times out, right? How do we know if it is not healthy if the check never times out?

I think that it will use the defined mongoclient timeout or the default value (30s).

eerhardt · 2023-12-06T00:09:06Z

@ailtonguitar - are you OK with the changes @sebastienros made?

ailtonguitar · 2023-12-06T01:19:51Z

@ailtonguitar - are you OK with the changes @sebastienros made?]

Hi @eerhardt ! Yes, fine by me!

dotnet-issue-labeler bot added the area-components Issues pertaining to Aspire Component packages label Nov 21, 2023

ailtonguitar closed this Nov 21, 2023

ailtonguitar reopened this Nov 21, 2023

DamianEdwards reviewed Nov 21, 2023

View reviewed changes

src/Components/Aspire.MongoDB.Driver/Aspire.MongoDB.Driver.csproj Outdated Show resolved Hide resolved

DamianEdwards reviewed Nov 21, 2023

View reviewed changes

src/Components/Aspire.MongoDB.Driver/AspireMongoDbDriverExtensions.cs Outdated Show resolved Hide resolved

ailtonguitar force-pushed the features/add-mongodb-support branch 4 times, most recently from 4d9b309 to 850b587 Compare November 22, 2023 10:30

ailtonguitar changed the title ~~WIP: Features/add mongodb support~~ Add mongodb container and connection support Nov 22, 2023

ailtonguitar changed the title ~~Add mongodb container and connection support~~ [WIP] Add mongodb container and connection support Nov 22, 2023

ailtonguitar force-pushed the features/add-mongodb-support branch 2 times, most recently from 5f7fd8a to b1be7c1 Compare November 22, 2023 12:06

ailtonguitar changed the title ~~[WIP] Add mongodb container and connection support~~ Add mongodb container and connection support Nov 22, 2023

ailtonguitar force-pushed the features/add-mongodb-support branch from b1be7c1 to c43b591 Compare November 22, 2023 12:19

brmagadutra approved these changes Nov 22, 2023

View reviewed changes

BLage approved these changes Nov 22, 2023

View reviewed changes

ailtonguitar force-pushed the features/add-mongodb-support branch 3 times, most recently from 22c77d9 to 40d81ee Compare November 23, 2023 17:23

davidfowl reviewed Nov 24, 2023

View reviewed changes

davidfowl added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 24, 2023

ailtonguitar force-pushed the features/add-mongodb-support branch 5 times, most recently from a0b42c3 to 74b9c9a Compare November 27, 2023 18:12

eerhardt self-assigned this Nov 27, 2023

eerhardt reviewed Nov 27, 2023

View reviewed changes

feat: add mongodb container and connection support

fc7fc08

ailtonguitar force-pushed the features/add-mongodb-support branch from 8cf20b3 to fc7fc08 Compare December 1, 2023 14:02

Move package reference to correct section

bc3d5bf

eerhardt reviewed Dec 1, 2023

View reviewed changes

Use standard HealthChecks property name

4a08605

sebastienros reviewed Dec 2, 2023

View reviewed changes

src/Components/Aspire.MongoDB.Driver/AspireMongoDBDriverExtensions.cs Show resolved Hide resolved

ailtonguitar and others added 3 commits December 4, 2023 10:22

refactor: change methods to void and small fixes in AspireMongoDBDriv…

49835c2

…erExtensions

Update tests/Aspire.MongoDB.Driver.Tests/AspireMongoDBDriverExtension…

5d6461e

…sTests.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

refactor: mongodb conformancetests refactoring

9ce8ef2

eerhardt reviewed Dec 4, 2023

View reviewed changes

ailtonguitar and others added 8 commits December 4, 2023 14:39

refactor: mongodb connectionString builder

89cecae

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

chore: update mongodb readme

b880d73

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

refactor: refactoring on AspireMongoDBDriverExtensions.cs

b551170

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

refactor: refactoring on AspireMongoDBDriverExtensions.cs

c7adf69

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

chore: Update tests/testproject/TestProject.IntegrationServiceA/TestP…

c027e51

…roject.IntegrationServiceA.csproj Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

fix: mongodb conformanceTests to check if the server is available

e405fde

fix: remove unnecessary usings and spaces

efc6886

Merge branch 'main' into features/add-mongodb-support

4a5eecc

sebastienros added 2 commits December 5, 2023 14:19

Remove authentication altogether

0f52a59

Fix unit tests

fbcc1ad

eerhardt approved these changes Dec 6, 2023

View reviewed changes

eerhardt merged commit 55dd83a into dotnet:main Dec 6, 2023
8 checks passed

eerhardt mentioned this pull request Jan 23, 2024

Aspire: Support mongodb.server.v0, mongodb.database.v0 Azure/azure-dev#3220

Closed

github-actions bot locked and limited conversation to collaborators Apr 28, 2024

Add mongodb container and connection support #986

Add mongodb container and connection support #986

Conversation

ailtonguitar commented Nov 21, 2023 • edited

ailtonguitar commented Nov 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailtonguitar Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailtonguitar Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Dec 1, 2023

sebastienros commented Dec 2, 2023

DamianEdwards commented Dec 2, 2023

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailtonguitar Dec 5, 2023 • edited

Choose a reason for hiding this comment

sebastienros commented Dec 4, 2023 • edited

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailtonguitar Dec 6, 2023 • edited

Choose a reason for hiding this comment

eerhardt commented Dec 6, 2023

ailtonguitar commented Dec 6, 2023 • edited

ailtonguitar commented Nov 21, 2023 •

edited

ailtonguitar commented Nov 21, 2023 •

edited

ailtonguitar Nov 28, 2023 •

edited

ailtonguitar Dec 4, 2023 •

edited

ailtonguitar Dec 5, 2023 •

edited

sebastienros commented Dec 4, 2023 •

edited

ailtonguitar Dec 6, 2023 •

edited

ailtonguitar commented Dec 6, 2023 •

edited