Skip to content

Conversation

@bgrainger
Copy link
Contributor

@bgrainger bgrainger commented Mar 8, 2024

Fixes #2690

This reflects connection string validation changes from PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1819.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-integrations Issues pertaining to Aspire Integrations packages label Mar 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 Mar 8, 2024
@bgrainger bgrainger marked this pull request as draft March 8, 2024 03:08
@bgrainger bgrainger force-pushed the pomelo-v8.0.1 branch 2 times, most recently from 12957de to 7682561 Compare March 8, 2024 21:00
This reflects connection string validation changes from PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1819.

The connection string options required by Pomelo are not set by default for a MySqlDataSource, but are required when using that MySqlDataSource with Pomelo.EntityFrameworkCore.MySql.
@bgrainger bgrainger marked this pull request as ready for review March 9, 2024 00:16
@RussKie RussKie merged commit cb4e3b8 into dotnet:main Mar 11, 2024
Comment on lines +23 to +24
AllowUserVariables = true,
UseAffectedRows = false,
Copy link
Member

Choose a reason for hiding this comment

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

Should we be setting these by default in the Aspire.Pomelo.EntityFrameworkCore.MySql package? Why force every user to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only needed when you register a MySqlDataSource in DI (which is what these tests do). If you use Pomelo via a connection string (the default), it adjusts the connection string as necessary (which creates a new connection pool under the hood).

Most users should not need to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Pomelo's integration with MySqlDataSource has some rough edges (that I ran into when building a WIP version of the AspireEFMySqlExtensions that used MySqlDataSource). I don't have a good suggestion for improving that, yet.

Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm: because these tests use both Aspire.MySqlConnector and Aspire.Pomelo.EntityFrameworkCore.MySql this is required? So anyone who does the same would need to set these? (I agree it is not that common, just trying to make sure I understand.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically it's because these tests use builder.AddMySqlDataSource and AddMySqlDataContext, and these two extension methods don't play well together due to requirements in the underlying Pomelo provider.

I would like this interaction to be better, but not sure how common it would be in the real world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because these tests use both Aspire.MySqlConnector and Aspire.Pomelo.EntityFrameworkCore.MySql this is required?

Since AddMySqlDataSource is the only Aspire.MySqlConnector extension method, and AddMySqlDataContext is the only Aspire.Pomelo.EntityFrameworkCore.MySql method, then your original statement was correct and my earlier response added nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started a WIP change to make Aspire.Pomelo.EntityFrameworkCore.MySql use a MySqlDataSource but ran into this exact problem. I think this is a point of friction that ideally would be addressed.

One thought I had is that UseAffectedRows=false, which is a connection-level setting, is the default. So most MySqlDataSource objects that a user creates would have the right value for that setting.

AllowUserVariables simply controls a runtime check for whether unmapped parameters have been used in the CommandText of a MySqlCommand. In theory, there's no reason why this check couldn't be suppressed on a command-by-command basis (while using the same underlying MySqlDataSource/connection pool). Pomelo could maybe tap into IDbCommandInterceptor to change this setting on a MySqlCommand (if support were implemented in MySqlConnector).

@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-integrations Issues pertaining to Aspire Integrations 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.

Failing EndToEnd test for Pomelo.EntityFrameworkCore.MySql with connection string .. must contain "AllowUserVariables=True;UseAffectedRows=False"

3 participants