Consolidation of Postgres resources. #2097
Conversation
dotnet-issue-labeler
bot
added
the
area-integrations
mitchdenny
added
area-app-model
| @@ -75,7 +75,7 @@ public static partial class AspireEFPostgreSqlExtensions | |||
| } | |||
|
|
|||
| builder.UseLoggerFactory(null); // a workaround for https://github.com/npgsql/efcore.pg/issues/2821 | |||
| }); | |||
| }, serviceKey: typeof(TContext)); | |||
There was a problem hiding this comment.
This is a breaking change. If someone is relying on getting a NpgsqlDataSource out of DI after calling AddNpgsqlDbContext, their code no longer works because it is now keyed.
| // 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 => | ||
| dbContextOptionsBuilder.UseNpgsql(connection, builder => |
There was a problem hiding this comment.
@roji - What are the differences in semantics here? Previously the Npgsql code would get the NpgsqlDataSource from DI and use it. But now this is providing a single NpgsqlConnection.
Should this instead be grabbing the NpgsqlDataSource from DI and passing it here?
There was a problem hiding this comment.
Maybe we should scrap the idea of registering an NpgsqlDataSource in DI, and instead just build one here and passing it to UseNpgsql. Thoughts?
There was a problem hiding this comment.
I looked back in private source control history, and it looks like we added the call to services.AddNpgsqlDataSource with a commit titled:
use Data Source as this is the recommended way (perf)
Would we still get the same perf benefits if we didn't register the NpgsqlDataSource in DI and instead just built one here and passed it in?
cc @adamsitnik
There was a problem hiding this comment.
I'll share what I know, but please wait for @roji for the ultimate answer.
The call to builder.Services.AddNpgsqlDataSource registers a NpgsqlDataSource factory and also a NpgsqlConnection factory that will internally resolve NpgsqlDataSource.
What I don't know is how creating an EF DB Context from NpgsqlConnection is different than from NpgsqlDataSource, namely how is it going to work when there will be a need for having more than one NpgsqlConnection
There was a problem hiding this comment.
I've pushed a change to the way we register the Npgsql EF component. Now we are just passing in the connection string. There isn't a need for us to register an NpgsqlDataSource in DI. We are now following the exact code from https://www.npgsql.org/efcore/index.html#additional-configuration-for-aspnet-core-applications. @roji can you take a look?
If the user wants to also register an NpgsqlDataSource, they can do it themselves once #1403 is completed (cc @sebastienros).
There was a problem hiding this comment.
@eerhardt so i revert this change?
There was a problem hiding this comment.
Oh you did it on this branch. Cool thanks for that.
…ction string to UseNpgsql
| @@ -130,7 +111,12 @@ private static void WritePostgresDatabaseToManifest(ManifestPublishingContext co | |||
| context.Writer.WriteString("parent", postgresDatabase.Parent.Name); | |||
| } | |||
|
|
|||
| private static void WritePostgresContainerResourceToManifest(ManifestPublishingContext context, PostgresContainerResource resource) | |||
| public static IResourceBuilder<PostgresServerResource> PublishAsContainer(this IResourceBuilder<PostgresServerResource> builder) | |||
mitchdenny
deleted the
mitchdenny/consolidation-postgres-resource-types
branch
* Consolidation of Postgres resources. * Fix up test cases. * Don't register an NpgsqlDataSource in DI anymore. Just pass the connection string to UseNpgsql * Add XML doc comments. --------- Co-authored-by: Eric Erhardt <[email protected]>
Follow up to dotnet#2097
Fixes #2096
@eerhardt could you take a look at this. I fixed a bug that we have in the Postgres EF Aspire component (linked above) but I want to get your input on the solution.
The main purpose for this PR is getting rid of
PostgresContainerResourceand changingPostgresServiceResourceto derive fromContainerResourceand introducing aPublishAsContainer()method to preserve the ability to deploy Postgres as acontainer.v0resource.Also consolidates
PostgresServerResourceandPostgresContainerResourceas part of the P4 API changes.Still some tests failing which I'm working through.
Microsoft Reviewers: Open in CodeFlow