Added support for MySql within the SqlSagaRepository #37
Conversation
There was a problem hiding this comment.
My biggest problem is that you add MySql dependency to the core project. That's a big NO. I'm not using MySql and not planning to do so in a near future. I don't want my projects to have that package added. So if accessing MySql can't be done without adding packages, repository for MySql should be a separate project and a separate NuGet package and that can bring in whatever dependencies needed. And if that is done as a separate package, a lot of your extra code is not needed. Have a look on NSaga.AzureTables project - it has a repository and a client factory. And it'll be possible to remove the factory as well.
So you don't need any of that. All you need is to implement ISagaRepository that will work with MySql. And for that you don't even need to do a NuGet package, just write it in your own projects and register it as .UseRepository<NSagaMySqlRepositry>()
| -- Create the database schema if it doesn't exists | ||
| IF NOT EXISTS (SELECT [schema_id] FROM [sys].[schemas] WHERE [name] = 'NSaga') | ||
| BEGIN | ||
| EXEC (N'CREATE SCHEMA [NSaga]'); |
There was a problem hiding this comment.
But you said MySql does not have schemas?
There was a problem hiding this comment.
This looks like a copy of install script for SqlServer and pretty sure is not valid for MySql. Did you even run this on MySql?
src/NSaga/Repository/Install.sql
Outdated
| @@ -0,0 +1,74 @@ | |||
| -- This script is inspired by Hangfire install.sql: https://github.com/HangfireIO/Hangfire/blob/master/src/Hangfire.SqlServer/Install.sql | |||
| internal const string SqlHeadersTableName = "NSaga.Headers"; | ||
|
|
||
| internal const string MySqlSagaDataTableName = "Sagas"; | ||
| internal const string MySqlHeadersTableName = "Headers"; |
There was a problem hiding this comment.
What if somebody already have table Headers or Sagas in their database? It should be more unique, like _NSaga_Sagas so the chance of clash with any existing table is minimal
There was a problem hiding this comment.
True, it should be like you said
| @@ -44,12 +44,10 @@ | |||
| <Reference Include="System.Data" /> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
| <Compile Include="..\..\SolutionInfo.cs"> | |||
There was a problem hiding this comment.
It most probably got deleted when copying over the changes
src/NSaga/NSaga.csproj
Outdated
| @@ -49,9 +54,6 @@ | |||
| <Reference Include="System.Xml" /> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
| <Compile Include="..\..\SolutionInfo.cs"> | |||
There was a problem hiding this comment.
It most probably got deleted when copying over the changes
| @@ -0,0 +1,16 @@ | |||
| using PetaPoco; | |||
There was a problem hiding this comment.
What do these do? I specifically did not put any implementations of *Data classes into the project, as this is a job of the consuming code.
There was a problem hiding this comment.
They are a wrapper over the PetaPoco table objects to allow easy configuration of the table objects with different names because the table name is set onto the attributes of SagaData and SagaHeaders objects directly
src/NSaga/app.config
Outdated
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
Library is not a place for app.config.
There was a problem hiding this comment.
I don't know about this and why it got added.
src/NSaga/packages.config
Outdated
| @@ -1,4 +1,5 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <packages> | |||
| <package id="Newtonsoft.Json" version="10.0.3" targetFramework="net45" /> | |||
| <package id="MySql.Data" version="6.9.9" targetFramework="net45" /> | |||
There was a problem hiding this comment.
So you adding MySql dependency for everyone? even for people who don't use MySql?
There was a problem hiding this comment.
It can be removed, and use a custom IConnectionFactory implementation
src/NSaga/packages.config
Outdated
| @@ -1,4 +1,5 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <packages> | |||
| <package id="Newtonsoft.Json" version="10.0.3" targetFramework="net45" /> | |||
| <package id="MySql.Data" version="6.9.9" targetFramework="net45" /> | |||
| <package id="Newtonsoft.Json" version="11.0.2" targetFramework="net45" /> | |||
There was a problem hiding this comment.
And pushing everyone to update Newtonsoft.Json? I know a ton of projects that demand a hard-coded Json.Net version. Usually not the latest version as well. And one more dependency (NSaga is a dependency) that demands yet another version of Json.Net is not going to cut it.
There was a problem hiding this comment.
Didn't take this into consideration. Thanks for giving me this insight.
| @@ -9,14 +9,14 @@ internal static class DatabaseHelpers | |||
| { | |||
| public static SagaData GetSagaData(Database database, Guid correlationId) | |||
| { | |||
| var data = database.SingleOrDefault<SagaData>($"select * from {SqlSagaRepository.SagaDataTableName} where correlationId = @0", correlationId); | |||
| var data = database.SingleOrDefault<SagaData>($"select * from {Settings.SqlSagaDataTableName} where correlationId = @0", correlationId); | |||
There was a problem hiding this comment.
Those are not settings. Those are constant values. Please change it back.
There was a problem hiding this comment.
The location of the settings can be changed back to SqlSagaRepository.
|
The problem with the particular implementation of the SqlSagaRepository is the fact that one must use manual implementations(adding PetaPoco or another ORM, the table objects, etc.) instead of being able to take advantage of the implementations that are already there. I will cut the unnecessary changes and make another pull request with only the ones for SqlSagaRepository, because those are the ones who matter and are the only ones that are needed. It will contain only the changes to SqlSagaRepository and the wrappers for the table and query objects |
#30