Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nwoolls
Copy link

@nwoolls nwoolls commented May 8, 2024

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

…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.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-components Issues pertaining to Aspire Component packages label May 8, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 8, 2024
@sebastienros sebastienros self-assigned this May 9, 2024
@sebastienros
Copy link
Member

Looking at how AddDbContextPool<TService, TImplementation> is implemented in EF Core I think we could just have the implementation for AddNpgsqlDbContext<TContextService, TContextImplementation>() and have AddNpgsqlDbContext<TContext>() invoke AddNpgsqlDbContext<TContext, TContext>().

Then we also need to do it for all other EF components (Cosmos, MySQL, Oracle, SQL Server). Can you do that?

/cc @AndriySvyryd

@nwoolls
Copy link
Author

nwoolls commented May 9, 2024

Looking at how AddDbContextPool<TService, TImplementation> is implemented in EF Core I think we could just have the implementation for AddNpgsqlDbContext<TContextService, TContextImplementation>() and have AddNpgsqlDbContext<TContext>() invoke AddNpgsqlDbContext<TContext, TContext>().

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.

@sebastienros
Copy link
Member

@nwoolls oh well, the big green blurb tricked me, sorry, we agree on the solution then ;)

@AndriySvyryd
Copy link
Member

Then we also need to do it for all other EF components (Cosmos, MySQL, Oracle, SQL Server).

This part is still missing

@sebastienros
Copy link
Member

@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.
@nwoolls nwoolls changed the title Add support for specifying DbContext service in Aspire.Npgsql.EntityFrameworkCore Add support for specifying DbContext service in Aspire.Npgsql.EntityFrameworkCore et al. May 14, 2024
@nwoolls
Copy link
Author

nwoolls commented May 14, 2024

@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");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
builder.AddMySqlDbContext<ITestDbContext, TestDbContext>("mysql");
builder.AddMySqlDbContext<ITestDbContext, TestDbContext>("mysql", optionsBuilder => optionsBuilder.UseMySql(s_serverVersion));

You need to specify the version

Copy link
Author

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))

Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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?

Copy link
Author

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-components Issues pertaining to Aspire Component packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying service (interface) and implementation when calling AddNpgsqlDbContext
4 participants