Add Enrich EF API #2125

sebastienros · 2024-02-07T22:37:29Z

Microsoft Reviewers: Open in CodeFlow

# Conflicts: # src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

sebastienros · 2024-02-07T22:50:15Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/ConfigurationSchema.json

                      "type": "string",
                      "description": "Gets or sets the connection string of the PostgreSQL database to connect to."
                    },
-                    "DbContextPooling": {


Pooling is enabled by default in AddNpgsql. Use Enrich otherwise.

sebastienros · 2024-02-07T22:51:13Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/ConfigurationSchema.json

                      "description": "Gets or sets a boolean value that indicates whether the OpenTelemetry metrics are enabled or not.",
                      "default": true
                    },
+                    "Retry": {


bool instead of int to simplify the usage. Use a custom call in the options builder to define a custom value.

sebastienros · 2024-02-07T22:53:24Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/TestDbContext.cs

 namespace Aspire.Components.Common.Tests;

-public class TestDbContext : DbContext
+public interface ITestDbContext


To test we can register and Enrich a DbContext with distinct service and implementation.

eerhardt · 2024-02-08T00:02:04Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

        configureSettings?.Invoke(settings);

-        if (settings.DbContextPooling)
+        builder.Services.AddNpgsqlDataSource(settings.ConnectionString ?? string.Empty, builder =>


You are overwriting my changes in ae11632. Can you revert this so it doesn't add back NpgsqlDataSource into DI?

eerhardt · 2024-02-08T00:04:38Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

+
+        // We don't provide the connection string, it's going to use the pre-registered DataSource.
+        // We don't register logger factory, because there is no need to: https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontextoptionsbuilder.useloggerfactory?view=efcore-7.0#remarks
+        dbContextOptionsBuilder.UseNpgsql(builder =>


I think you always need to call UseNpgsql whether settings.Retry is true or false.

Right, mistake when I refactored the code to reuse the same method for both Enrich and AddNpgsql. And with your changes I might have to split them.

It also means we might be missing a test that should have caught that for AddNpgsql.

It also means we might be missing a test that should have caught that for AddNpgsql.

Good call. #2136

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

eerhardt · 2024-02-08T00:08:12Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/AspireEFPostgreSqlExtensionsTests.cs

    }
+
+    [Fact]
+    public void EnrichCanConfigureDbContextOptions()


(nit) Why are these tests here, and not in EnrichNpgsqlTests?

The class was named AspireEFPostgreSqlExtensionsTests.cs which is where they are. Didn't think we would do a test class for a specific method.

Didn't realize you meant the one that just got created, moved them.

eerhardt · 2024-02-08T00:09:20Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/ConformanceTests.cs


    protected override void SetupConnectionInformationIsDelayValidated()
    {
        throw new SkipTestException("Need to skip this test until https://github.com/npgsql/efcore.pg/issues/2891 is fixed.");


We should be able to remove this now that we have builder.EnableServiceProviderCaching(false); in ConfigureDbContextOptionsBuilderForTesting.

That works, but I had to add a similar thing for Enrich instead since it doesn't care about the connection string.

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

davidfowl · 2024-02-08T01:59:56Z

Add a playground sample or modify one with the new apai

eerhardt · 2024-02-08T21:46:45Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/AspireEFPostgreSqlExtensionsTests.cs

 #pragma warning restore EF1001 // Internal EF Core API usage.
    }
+
+    public class WorkaroundToReadProtectedField : NpgsqlRetryingExecutionStrategy


(nit) maybe we can extract this to a common class to share it.

eerhardt · 2024-02-08T22:00:05Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/EnrichNpgsqlTests.cs

+
+        builder.EnrichNpgsqlDbContext<TestDbContext>();
+
+        // The service descriptor should not be affected with MaxRetryCount set to 0


What does with MaxRetryCount set to 0 mean here?

Forgot to change the text, will fix. The idea is that if the setting is false (0 previously) then we should not have replaced the service descriptor.

sebastienros · 2024-02-08T22:56:09Z

@davidfowl example added in README and in the Postgres playground.

eerhardt · 2024-02-08T23:56:47Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/EnrichNpgsqlTests.cs

+    internal static void ConfigureDbContextOptionsBuilderForTesting(DbContextOptionsBuilder builder)
+    {
+        // Don't cache the service provider in testing.
+        // Works around https://github.com/npgsql/efcore.pg/issues/2891, which is errantly caches connection strings across DI containers.
+        builder.EnableServiceProviderCaching(false);
+    }


Suggested change

internal static void ConfigureDbContextOptionsBuilderForTesting(DbContextOptionsBuilder builder)

{

// Don't cache the service provider in testing.

// Works around https://github.com/npgsql/efcore.pg/issues/2891, which is errantly caches connection strings across DI containers.

builder.EnableServiceProviderCaching(false);

}

Can we reuse the one from AspireEFPostgreSqlExtensionsTests

eerhardt

LGTM. Thanks for the great work here.

eerhardt · 2024-02-08T23:59:04Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/ConfigurationTests.cs

        => Assert.True(new NpgsqlEntityFrameworkCorePostgreSQLSettings().Metrics);

-    [Fact]
-    public void MaxRetryCountIsSameAsInTheDefaultNpgsqlPolicy()


Although I don't think these are super valuable tests, following the current example we should have one that ensures Retry is true by default.

eerhardt · 2024-02-09T00:01:03Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/AspireEFPostgreSqlExtensionsTests.cs

        Assert.Equal(ConnectionString, actualConnectionString);

        // ensure the max retry count from config was respected
        Assert.NotNull(extension.ExecutionStrategyFactory);


The comment above this line needs to be updated.

eerhardt · 2024-02-09T00:03:28Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/ConfigurationTests.cs

-        Assert.Equal(new WorkaroundToReadProtectedField(dbContext).RetryCount, new NpgsqlEntityFrameworkCorePostgreSQLSettings().MaxRetryCount);
-    }
-
    public class WorkaroundToReadProtectedField : NpgsqlRetryingExecutionStrategy


This can be replaced by the common one now. Or just removed since it isn't used anymore.

eerhardt · 2024-02-09T00:06:25Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

+        NpgsqlEntityFrameworkCorePostgreSQLSettings settings = new();
+        var typeSpecificSectionName = $"{DefaultConfigSectionName}:{typeof(TContext).Name}";
+        var typeSpecificConfigurationSection = builder.Configuration.GetSection(typeSpecificSectionName);
+        if (typeSpecificConfigurationSection.Exists()) // https://github.com/dotnet/runtime/issues/91380


Can we extract this into a common method?

eerhardt · 2024-02-09T00:07:42Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

+                return;
+            }
+
+            // Resolving DbContext<TContextService> will resolve DbContextOptions<TContextImplementation>.


I'm not sure I understand the significance of this line. What exactly does it mean?

It's a reminder that we seek the service descriptor for DbContextOptions<TestDbContext> (referenced as TContextImplementation in EF APIs) because it is the db context options that is resolved even when we register a db context with an interface (TContextService in EF APIs)

eerhardt · 2024-02-09T00:09:58Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/EnrichNpgsqlTests.cs

+        // ensure the command timeout was respected
+        Assert.Equal(123, extension.CommandTimeout);


Do we need the command timeout 123 stuff in these lower tests? Is it necessary?

eerhardt · 2024-02-09T00:29:00Z

tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/EnrichNpgsqlTests.cs

+        // ensure the command timeout was respected
+        Assert.Equal(123, extension.CommandTimeout);
+
+        // ensure the max retry count from config was respected


Suggested change

// ensure the max retry count from config was respected

// ensure the Retry from config was respected

Repeated elsewhere.

eerhardt · 2024-02-09T00:31:59Z

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/README.md

 # Aspire.Npgsql.EntityFrameworkCore.PostgreSQL library

-Registers [EntityFrameworkCore](https://learn.microsoft.com/ef/core/) [DbContext](https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontext) in the DI container for connecting PostgreSQL®* database. Enables connection pooling, health check, logging and telemetry.
+Registers [EntityFrameworkCore](https://learn.microsoft.com/ef/core/) [DbContext](https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontext) in the DI container for connecting PostgreSQL®* database. Enables connection pooling, connection retries, health check, logging and telemetry.


Suggested change

Registers [EntityFrameworkCore](https://learn.microsoft.com/ef/core/) [DbContext](https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontext) in the DI container for connecting PostgreSQL®* database. Enables connection pooling, connection retries, health check, logging and telemetry.

Registers [EntityFrameworkCore](https://learn.microsoft.com/ef/core/) [DbContext](https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontext) in the DI container for connecting PostgreSQL®* database. Enables connection pooling, retries, health check, logging and telemetry.

Retries in general, right?

davidfowl · 2024-02-09T00:54:08Z

Did we update one of the playground samples?

sebastienros · 2024-02-09T00:54:55Z

@davidfowl yes

ghost added the area-codeflow for labeling automated codeflow. intentionally a different color! label Feb 7, 2024

sebastienros added 2 commits February 7, 2024 14:42

Add Enrich EF API

07758f5

Merge remote-tracking branch 'origin/main' into sebros/efapi2

a76069d

# Conflicts: # src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs

sebastienros force-pushed the sebros/efapi2 branch from b7695d8 to a76069d Compare February 7, 2024 22:47

Formatting

9deb56f

sebastienros commented Feb 7, 2024

View reviewed changes

sebastienros requested review from AndriySvyryd, eerhardt and roji February 7, 2024 22:53

eerhardt reviewed Feb 8, 2024

View reviewed changes

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs Outdated Show resolved Hide resolved

eerhardt reviewed Feb 8, 2024

View reviewed changes

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs Outdated Show resolved Hide resolved

eerhardt reviewed Feb 8, 2024

View reviewed changes

PR feedback

36005a9

AndriySvyryd reviewed Feb 8, 2024

View reviewed changes

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs Outdated Show resolved Hide resolved

AndriySvyryd reviewed Feb 8, 2024

View reviewed changes

src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs Outdated Show resolved Hide resolved

AndriySvyryd approved these changes Feb 8, 2024

View reviewed changes

sebastienros added 2 commits February 7, 2024 16:52

Simplify service descriptor hijacking

3f8cdf8

Fix variable name

5ba7fb0

sebastienros added 2 commits February 8, 2024 09:48

Test error message when DbContext is missing

f7771cb

Update README

fa41aa0

eerhardt reviewed Feb 8, 2024

View reviewed changes

sebastienros added 2 commits February 8, 2024 14:17

Extract WorkaroundToReadProtectedField

d3ffcd9

Add usage in playground

3a30cc1

sebastienros marked this pull request as ready for review February 8, 2024 23:01

eerhardt reviewed Feb 8, 2024

View reviewed changes

eerhardt approved these changes Feb 9, 2024

View reviewed changes

Feedback

7c6f96e

sebastienros enabled auto-merge (squash) February 9, 2024 01:52

sebastienros merged commit dc8fa33 into main Feb 9, 2024

sebastienros deleted the sebros/efapi2 branch February 9, 2024 02:07

This was referenced Feb 12, 2024

Add ability to configure NpgsqlDataSource #1276

Closed

Improve ServerVersion.AutoDetect in AddMySqlDbContext #1866

Closed

eerhardt mentioned this pull request Mar 11, 2024

Database context factory registration for EF core database components #2763

Closed

eerhardt mentioned this pull request Apr 15, 2024

Add support for pooled DbContext factory #3356

Closed

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


		builder.EnrichNpgsqlDbContext<TestDbContext>();

		// The service descriptor should not be affected with MaxRetryCount set to 0

		// ensure the command timeout was respected
		Assert.Equal(123, extension.CommandTimeout);

	// ensure the max retry count from config was respected
	// ensure the Retry from config was respected

	Registers [EntityFrameworkCore](https://learn.microsoft.com/ef/core/) [DbContext](https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontext) in the DI container for connecting PostgreSQL®* database. Enables connection pooling, connection retries, health check, logging and telemetry.
	Registers [EntityFrameworkCore](https://learn.microsoft.com/ef/core/) [DbContext](https://learn.microsoft.com/dotnet/api/microsoft.entityframeworkcore.dbcontext) in the DI container for connecting PostgreSQL®* database. Enables connection pooling, retries, health check, logging and telemetry.

Add Enrich EF API #2125

Add Enrich EF API #2125

Uh oh!

Conversation

sebastienros commented Feb 7, 2024 • edited by dotnet-policy-service bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienros commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Feb 9, 2024

Uh oh!

sebastienros commented Feb 9, 2024

sebastienros commented Feb 7, 2024 •

edited by dotnet-policy-service bot

Loading