-
Notifications
You must be signed in to change notification settings - Fork 316
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
Add support for specifying DbContext service in Aspire.Npgsql.EntityFrameworkCore et al. #4115
base: main
Are you sure you want to change the base?
Add support for specifying DbContext service in Aspire.Npgsql.EntityFrameworkCore et al. #4115
Conversation
…rameworkCore This update introduces the ability to register a DbContext with a specified context service interface in the Aspire.Npgsql.EntityFrameworkCore. Extension and test methods have been added to support this functionality.
src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
Show resolved
Hide resolved
Looking at how Then we also need to do it for all other EF components (Cosmos, MySQL, Oracle, SQL Server). Can you do that? /cc @AndriySvyryd |
If I understand your comment that is what this PR does. |
@nwoolls oh well, the big green blurb tricked me, sorry, we agree on the solution then ;) |
This part is still missing |
@AndriySvyryd good point, that's why I had not provided my approval yet, we should merge when we have the other providers implemented. |
New code was added to allow the specification of a context service (interface) when registering a DbContext. This provides the ability to resolve the context from a container when using different services such as Azure SQL, MS SQL Server, Oracle Database and Cosmos DB. Corresponding unit tests were penned to validate the functionality across the respective databases.
@AndriySvyryd @sebastienros I've added the suggested changes but there's a single test failure that is not failing locally for me. Any guidance would be appreciated. The tests / implementations are all nearly identical, but only one is failing and only in CI. |
new KeyValuePair<string, string?>("ConnectionStrings:mysql", ConnectionString), | ||
]); | ||
|
||
builder.AddMySqlDbContext<ITestDbContext, TestDbContext>("mysql"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
builder.AddMySqlDbContext<ITestDbContext, TestDbContext>("mysql"); | |
builder.AddMySqlDbContext<ITestDbContext, TestDbContext>("mysql", optionsBuilder => optionsBuilder.UseMySql(s_serverVersion)); |
You need to specify the version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndriySvyryd I've added that code to the test but it's still failing in CI. I do see some of the other tests (but not all) specify version 8.2.0 via:
new MySqlServerVersion(new Version(8, 2, 0))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can try it, but now I am not sure that it'll work. That test shouldn't be connecting to the database at all, you might need to debug it to see why that's happening. It might be passing locally just because it successfully connects to the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I'm not sure where to go from here. I only needed this for Postgres, and all of the tests for the other DB providers work.
If I stop Docker, I get 14 test failures for MySQL with:
Docker is either not running or misconfigured. Please ensure that Docker is running and that the endpoint is properly configured. You can customize your configuration using either the environment variables or the ~/.testcontainers.properties file. For more information, visit:
https://dotnet.testcontainers.org/custom_configuration/
If I start Docker, the tests all pass locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading the failed actions correctly, these same tests succeed on Linux. This is only failing on Windows, which may explain why I cannot reproduce it. I am not on Windows, nor do I have access to a Windows device. @AndriySvyryd @sebastienros are you able to run this failing test on a Windows machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took one more stab based on existing tests. All passing now.
This commit modifies the AddMySqlDbContext method call in AspireEFMySqlExtensionsTests by adding an additional argument for configuring DbContextOptions. The new configuration now makes use of UseMySql with a specified server version.
The MySql DbContext configuration in the tests has been simplified. The server version setting has been moved to the in-memory collection for configuration, and the redundant configuration option building has been removed.
@sebastienros @AndriySvyryd - I thought the design with #1403 was to not add all these APIs to Aspire. And instead, if the "simple" API doesn't work for you, drop to using the EF API directly to register your DbContext, and then call Is this no longer the case? |
@eerhardt It depends on where we define the line for the "simple" API, I don't have a strong opinion one way or the other. Perhaps @DamianEdwards can weigh in? |
@eerhardt I'm clearly a bit biased having introduced this, but I don't think this is equivalent to the requests in #1403, nor do I think this is particularly pushing in that direction. This only introduces the ability to specify the context service in addition to the implementation. Without that, how do we unit test Aspire applications that have database contexts? As soon as someone wants to inject an interface instead of a concrete class, they have to drop back to using the |
We were concerned that there will always be pressure and expectation to add more overloads and/or methods to the Aspire EF components to support all the possible ways one can configure EF Core. Each seems reasonable in isolation but the approach isn't tenable on the whole IMO. I think we should just stick with what we landed with in #1403 for now. |
@DamianEdwards agreed on that, but this isn't about introducing overloads to configure EF core. This is simply allowing someone to specify both the interface and implementation with the DI container, something I would think is extremely common with DI in .NET. |
@nwoolls while I appreciate this space is full of many opinions, my reading of https://learn.microsoft.com/ef/core/testing/ is that the EF Core team themselves discourage mocking or At this point I'm not convinced that these methods belong in the Aspire components. They're easy enough to add as extensions in your own projects or as a separate package (perhaps even in aspirant). |
In this specific case I am not sure that this is the case. The one we currently expose only takes one generic type, the one added takes two. I am not disagreeing with the other arguments though. |
@sebastienros is it not as simple as? public static class DbContextSqlServerExtensions
{
public static void AddSqlServerDbContext<TContextService, TContextImplementation>(this IHostApplicationBuilder builder, string name)
where TContextImplementation : DbContext, TContextService
where TContextService : class
{
var connectionString = builder.Configuration.GetConnectionString(name)
?? throw new InvalidOperationException("Connection string not found");
builder.Services.AddDbContextPool<TContextService, TContextImplementation>(options =>
options.UseSqlServer(connectionString));
builder.EnrichSqlServerDbContext<TContextImplementation>();
}
} |
@DamianEdwards yes, perfect, was afraid Enrich would do something different but I checked the doc and it should be equivalent. |
This update introduces the ability to register a DbContext with a specified context service interface in the Aspire.Npgsql.EntityFrameworkCore. Extension and test methods have been added to support this functionality.
Resolves #4114.
Microsoft Reviewers: Open in CodeFlow