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 ADO.NET Provider support MySqlConector 0.x and 1.x. #6831

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Add ADO.NET Provider support MySqlConector 0.x and 1.x. #6831

merged 1 commit into from
Dec 16, 2020

Conversation

buzzers
Copy link
Contributor

@buzzers buzzers commented Nov 13, 2020

MySqlConnector 1.x introduces two breaking changes, which prevents Orleans from using the new version of the driver. This modification will enable the factory class loader to support multiple assemblies and class names to be compatible with versions 0.x and 1.x.

@dnfadmin
Copy link

dnfadmin commented Nov 13, 2020

CLA assistant check
All CLA requirements met.

@veikkoeeva
Copy link
Contributor

veikkoeeva commented Nov 15, 2020

I tested this pull request using the following libraries, pertinently snipped from ´Tester.AdoNet.csproj´

<ItemGroup>
    <PackageReference Include="Microsoft.Data.SqlClient" Version="1.1.0" />
    <PackageReference Include="MySqlConnector" Version="0.61.0" />    
    <PackageReference Include="MySql.Data" Version="$(MySqlDataVersion)" />
  </ItemGroup>

and then updated the library in question to <PackageReference Include="MySqlConnector" Version="1.1.0" />. Here $(MySqlDataVersion) is defined as <MySqlDataVersion>8.0.18</MySqlDataVersion>. I tested with both old and new version of MySqlConnector to see that this refactoring does not break deployments for users who update Orleans libraries but not their connector libraries.

I also run SQL Server version tests and they pass, so the refactoring functions regarding that also. Looking at how the refactoring is done, it would stand to reason also PostgreSQL and Oracle works even if not tested explicitly.

So the tests pass.


Side notes:

  • The tests need to be refactored manually in multiple places so that AdoNetInvariants.InvariantNameMySqlConnector is used instead of the default AdoNetInvariants.InvariantNameMySql.
  • The MySql.Data version should be updated.

Also

  • SQL Server connection string fails on asynchronous connection string (I thought this was fixed, something fishy).
  • There is a SQL Server database (.mdf) file, probably there isn't a real reason there ought to be.

These side notes warrant considering refactoring on how the constants are used in the tests and how libraries are loaded. As for tests specifically, the chief cause here is the inheritance structure and historical lack of DI arrangements (also the #ifdef nature of stitching ADO.NET code may make this a bit more difficult, could be avoided by having a common core as a .dll/library used by membership, reminders, persistence etc.).

The broader aspect of why this refactoring was needed dates back to when Orleans was fixed to SQL Server only and how other versions of databases were refactored into the system and configuration "sneaked" in. Now that we have a DI system, we should allow these fixed libraries we predefined in a more flexible manner (e.g. pre-defined options used by lambda?). One thing to keep in mind is that Orleans is being used with databases that are not officially supported by Microsoft related libraries (e.g. those by Entity Framework) and so may use specific versions of connectors that implement some databas specific protocol (MySQL and PostgreSQL subsets specifically).

Also: DbProvideFactory has made a comeback, which could be considered also with Orleans configuration surface.

Linking #6752 as this basically hardcodes options that could/should be available via a configuration surface.

@benjaminpetit
Copy link
Member

I am ok to merge as is so we can include it in 3.4.0, but @veikkoeeva or @buzzers , could you update the tests?

I agree this library needs refactoring, DI should makes thing easier

@veikkoeeva
Copy link
Contributor

veikkoeeva commented Dec 14, 2020

@benjaminpetit How would you would like to have the tests updated?

Edit: If the idea is to test loading assemblies and some of their classes dynamically when their namespace changes between versions, I think it would be more robust and purposeful to first refactor like this:

  1. Have default loaders for connector, they can do what is being done now (but it would be simpler).
  2. Then if 1. exists, the user can simply use own lambda (f.ex.) that has custom dll and namespace and that is used to load. Here introduce tests that take this old library and new (I'm not sure it makes sense, though).
  3. Add optionally DbProviderFactory (Orleans should use this internally with the provided information).
  4. DbProviderFactory could be used otherwise too.
  5. Optionally provide extension methods (or something alike) that are like .LoadSqlServer, LoadMysql etc. if it makes easier (no separate dll needed for that).

The tests in ADO.NET should be refactored and they're more complicated than others since they tests multiple databases, any database can be tested if it can be connected to. I would like to introduce more DI there.

I would like to refactor ADO.NET library to removed the if-defs, but it would introduce a new core library that the separate libraries of persistence, reminders etc. would use. I think at some point the idea was not to have that but always "one library" for the parts.

@benjaminpetit
Copy link
Member

I was thing about that for the tests, but I maybe did not understand:

Side notes:

The tests need to be refactored manually in multiple places so that AdoNetInvariants.InvariantNameMySqlConnector is used instead of the default AdoNetInvariants.InvariantNameMySql.
The MySql.Data version should be updated.

@veikkoeeva
Copy link
Contributor

veikkoeeva commented Dec 15, 2020

@benjaminpetit

I was thing about that for the tests, but I maybe did not understand:

Ah, that! :) I wrote that in case someone searches help for a related case. I think the tests failing and the error are fairly easy to understand if one has seen connectionstring failure, but maybe useful to have here. Or maybe it's confusing.

The ADO.NET tests would need refactoring, I think along with the rest of the test suite. :)

But excluding that notion, would this be good to be merged? The failure mode that happens is that one cannot load a connector and an exception is thrown when trying to do database operations. So it would not be something subtle and the tests pass.

@benjaminpetit
Copy link
Member

Nothing blocking, let's merge it now if it can unlock people

@benjaminpetit benjaminpetit merged commit 8462235 into dotnet:master Dec 16, 2020
benjaminpetit pushed a commit to benjaminpetit/orleans that referenced this pull request Dec 16, 2020
ReubenBond pushed a commit that referenced this pull request Jan 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants