Proposed refactor of MigrationServiceFactory

[Jabestrada]

Hi @rdagumampan,

This PR is not intended to be merged as-is (for starters, the unit tests won't build) but rather just a starting point for a discussion on a proposed refactor of the MigrationServiceFactory implementation.

As I was analyzing how to add support for SQLite, it occurred to me that SQLite would need a different IConfigurationDataService implementation since it's a file-based database rather than a server-based one. For instance, checking for database existence and creating the database are file-check and file-create operations respectively (instead of executing SQL queries).

Creating the SQLite-specific IConfigurationDataService is trivial but it's the MigrationServiceFactory implementation that's proving to be a challenge for the following reasons:

1. The concrete ConfigurationDataService implementation is created statically, which means one would need to add code conditions/switches on current platform to determine which specific type of IConfigurationDataService to create; this will make code harder to maintain over time.
2. Concrete IMigrationServiceFactory implementations are duplicated across projects where they are needed; this means any branching logic introduced by Consider the baseline implementation. #1 should also be duplicated across.

Here are the key points in this PR that aim to address the challenges above:

1. There is a single MigrationServiceFactory, and it's located in Core (which I think is where it belongs); hence, there's no need to duplicate code.
2. Adding a new platform doesn't involve any changes in the MigrationServiceFactory code; only new entries to the config file are needed (open/closed principle). Although in this PR, I hard-coded the dictionary of type definitions but they could easily be moved to config files since they are just simple strings; I pointed this out in the code comments as well. And one would notice that there's no longer a switch statement for the supported platforms.

Hope I explained things clearly; let me know what you think.

[rdagumampan]

Hi @Jabestrada , thanks again for continuous feedback and apologies for delayed response. I as much would like to make sure I spent decent time to craft my response.

In the beginning, I aimed for dynamic loading of platform assemblies thus the presence of --plugins-path parameter. This is just the right thing to do. I agree with you that we should have one factory class at the core and we can only achieve this if we can dynamically instantiate the target platform implementation. This has been very difficult for me to make this work for both CLI and Nuget package. .NET Core has very unique way of loading dependencies and thus MS introduced AssemblyLoadContext. Using AssemblyLoadContext seems to work in SqlServer but not for Npgsql and other dependencies.

I still have an opened case here https://stackoverflow.com/questions/59463164/failed-to-load-plugin-dependency-using-assemblyloadcontext-in-net-core-3-0 on this issue.

With respect to supporting SQL Lite, I discussed this with another contributor on different subject where what if we would like to support NoSqlDB. This is a valid point and I think one approach is to create another IDataService in Yuniql.Extensibility something like IFileBasedDataService or INoSqlDataService. Then we we create IFileBasedConfigurationDataService which consumes the implementation of IFilebasedDataService.

A more drastic approach is to put all the responsibility into the platform implementation. Such that MySqlDataService.IsDatabaseExists() return true/false, instead of GetSqlForIsDatabaseExists. This makes the platform implementation quite heavy. I don't see coming this way for SQL databases. For NoSql maybe this is the way to go.

[Jabestrada]

Cool. I hope you won't mind if I spend some time figuring out how to make AssemblyLoadContext work for this particular scenario instead of working straight on SQLite support. Perhaps I'll get lucky and stumble on to something.

[rdagumampan]

@Jabestrada oh that would be both fun and very challenging to do, so sure feel free to pick on it! Try to test dynamic loading of the plugins Yuniql.SqlServer and Yuniql.PostgreSql at least.

We have intial implementation here:

• https://github.com/rdagumampan/yuniql/blob/master/yuniql-core/PluginAssemblyLoadContext.cs

And this is largely inspired by these:

• https://github.com/dotnet/samples/tree/master/core/tutorials/Unloading

Nate seems to have solved this well here

• https://github.com/natemcmaster/DotNetCorePlugins

Though I think this issue has got higher priority and value over implementing dynamic plugins.
#131

Br, Rodel

[Jabestrada]

Thanks for the links. I'll just give it a few hours and see how far I can go.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

[Jabestrada]

Hi @rdagumampan ,

I've updated MigrationServiceFactory to use AssemblyLoadContext. High-level notes on the changes:

1. Removed all references of platform-specific projects in Yuniql.Core and YuniqlCLI (see Caveats for side-effects).
2. Modified AssemblyLoadContext a bit so that it doesn't throw an exception when the requested type is in the DefaultContext and not in the plugins path.
3. Limited testing on SQL Server (up to SqlConnection dependency only) and PostgreSQL (up to Npgsql.dll dependency only).
4. Commented out the previous implementations of MigrationServiceFactory just to make sure things aren't mixed up.

Some caveats/limitations:

1. After removing the Yuniql.SqlServer, Yuniql.PostgreSql and Yuniql.MySql projects from Core and CLI, I had to manually update the .deps.json file for yuniql.exe so that it knows how to resolve the direct dependencies of the plugins themselves (e.g., System.Data.SqlClient for Yuniql.SqlServer). I did this using these steps:

• Added dependency to SqlClient in CLI
• Built the CLI to generate the .deps.json file
• Kept a backup of .deps.json in a separate location
• Removed the SqlClient dependency, then rebuilt CLI
• Merged the contents of the backup .deps.json file to the latest .deps.json file of the CLI
• Copied Yuniql.SqlServer.dll and System.Data.SqlClient.dll to plugins path (same goes for Yuniql.PostgreSQL and Npgsql.dll)
• Tested the CLI using the built .exe file, and not from the IDE because that might revert the .deps.json file

I guess the update of the .deps.json file happens automatically when the platform-specific projects are installed to the CLI using Nuget but I haven't gotten that far.

2. For self-contained apps, I ran into a wall with SQL Server. While Yuniql.SqlServer.dll and System.Data.SqlClient were successfully resolved after I placed the DLLs in the plugins path, some of the inner dependencies of System.Data.SqlClient cannot be resolved (e.g., System.Data.SqlClient.resource). Again, I guess installing Yuniql.SqlServer via Nuget may resolve these issues but I haven't tried that out yet. I'm not really a big fan of deployment issues =)

Try cloning my branch and give it a try, and let me know what you think.

[rdagumampan]

Hi @Jabestrada , thanks again and I will look into it this week. I'm just bit occupied in preparing for release of v1.0 this week. I will clone and test this locally and give you feedback.

/ardi