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

Support for MySQL databases was added into RoundhousE #3

Merged
merged 7 commits into from
Jun 20, 2011

Conversation

diyan
Copy link

@diyan diyan commented May 10, 2011

Hi,

I've added initial support for MySQL databases into RoundhousE migrator. It is expected that everything should work in cases when connection string parameter was provided to rh.exe

Here is full list of my changes:

Commit 1.
Migrate solution to Visual Studio 2010 (Sorry, but I was unable to work without this change)

Commit 2.
Change roundhouse.console.Program class from internal to public.
Add initial implementation MySqlDatabase and mapping for Version, ScriptsRun and ScriptsRunError entities into roundhouse.databases.mysql project.
Add NHibernate, Fluent NHibernate, MySQL ADO.NET Data Provider references into roundhouse.databases.mysql project.
Add MySQLDatabaseTest and NUnit test (as temporary solution) into roundhouse.tests.integration project.
Add roundhouse.console and roundhouse.databases.mysql references into roundhouse.tests.integration project.
Add App.config with DbProviderFactory for MySql.Data.MySqlClient into roundhouse.tests.integration project.
Update Log4NetAppender, when failed to load xml configuration for log4net, consider that assemblies was not merged.

TODO: Initialize configuration/system.data/DbProviderFactories app.config section in code.
TODO Investigate how to get provider name using MySQL API instead of hardcoded string.

Commit 3.
Add MySqlAdoNetProviderResolver which registers DbProviderFactory for MySQL and redirects assembly resolution from external MySql.Data into ILMerged version.
MySqlDatabase implementation will never use general ADO.NET batch statement splitter. Instead of that it will use MySqlScript class which handles batch queries and also a MySQL-specific DELIMITER command.
Admin connection for MySQLDatabase connects to "information_schema" system database.
Implement MySqlDatabase.create_database_script method.
In mapping for ScriptsRun and ScriptsRunError entities longtext type was changed into medumtext type because of NHibernate's defect. http://216.121.112.228/browse/NH-2424
Remove App.Config with DbProviderFactory registration.

FIXME: Quick workaround for MySQL's defect with reserved words auto-quoting. This change potentially could introduce regressions for Access, MS SQL Server or Oracle. IMO better solution is to migrate into NH 3.1 which has bugfix for this problem. http://216.121.112.228/browse/NH-1906

TODO Do we need anything for MySQL? For ex MS SQL Server implemenation has schema creation script and Oracle creates db-sequences.
TODO MS SQL Server could support different database schema for migration tables but MySQL doesn't support this actually. For MySQL CREATE SCHEMA just alias to CREATE DATABASE.
TODO Investigate how pass CommandTimeout into commands which will be during MySqlScript execution.
TODO Do MySQL has recovery modes which could be initialized by running SQL statement?

It would be great if you have a time to look into this changes.
I'm opened to any suggestions if they could help in pushing this changes into main upstream project.

Alexey Diyan added 4 commits May 6, 2011 20:34
Add initial implementation MySqlDatabase and mapping for Version, ScriptsRun and ScriptsRunError entities into roundhouse.databases.mysql project.
Add NHibernate, Fluent NHibernate, MySQL ADO.NET Data Provider references into roundhouse.databases.mysql project.
Add MySQLDatabaseTest and NUnit test (as temporary solution) into roundhouse.tests.integration project.
Add roundhouse.console and roundhouse.databases.mysql references into roundhouse.tests.integration project.
Add App.config with DbProviderFactory for MySql.Data.MySqlClient into roundhouse.tests.integration project.
Update Log4NetAppender, when failed to load xml configuration for log4net, consider that assemblies was not merged.

TODO: Initialize configuration/system.data/DbProviderFactories app.config section in code.
TODO Investigate how to get provider name using MySQL API instead of hardcoded string.
… MySQL and redirects assembly resolution from external MySql.Data into ILMerged version.

MySqlDatabase implementation will never use general ADO.NET batch statement splitter. Instead of that it will use MySqlScript class which handles batch queries and also a MySQL-specific DELIMITER command.
Admin connection for MySQLDatabase connects to "information_schema" system database.
Implement MySqlDatabase.create_database_script method.
In mapping for ScriptsRun and ScriptsRunError entities longtext type was changed into medumtext type because of NHibernate's defect. http://216.121.112.228/browse/NH-2424
Remove App.Config with DbProviderFactory registration.

FIXME: Quick workaround for MySQL's defect with reserved words auto-quoting. This change potentially could introduce regressions for Access, MS SQL Server or Oracle. IMO better solution is to migrate into NH 3.1 which has bugfix for this problem. http://216.121.112.228/browse/NH-1906

TODO Do we need anything for MySQL? For ex MS SQL Server implemenation has schema creation script and Oracle creates db-sequences.
TODO MS SQL Server could support different database schema for migration tables but MySQL doesn't support this actually. For MySQL CREATE SCHEMA just alias to CREATE DATABASE.
TODO Investigate how pass CommandTimeout into commands which will be during MySqlScript execution.
TODO Do MySQL has recovery modes which could be initialized by running SQL statement?
… has_run_script_already methods must fetch hash for the latest script version instead of the first version.
@diyan
Copy link
Author

diyan commented May 19, 2011

There is one more commit with defect correction. This change fixes issue for all database providers, so I think it is important.

Correct defect. In DefaultDatabase class, get_current_script_hash and has_run_script_already methods must fetch hash for the latest script version instead of the first version.

@diyan diyan closed this May 19, 2011
@diyan diyan reopened this May 19, 2011
@ferventcoder
Copy link
Member

Dude...this is awesome. I am in the process of upgrading to the latest NHibernate. If you can keep up with me and keep this updated, I will definitely pull it in.

@diyan
Copy link
Author

diyan commented May 21, 2011

@ferventcoder

Glad that you are opened to accept this pull request.

During development of this feature I was a little bit warred if there will be a problems with pushing my changes into upstream project.

yeah - you sent the pull request to chucknorris? Did you include me? I never saw it come across my email... :(

Yes, I did. I sent pull request only to chucknorris account and did not included you. Sorry about that.

Do you planning update to the stable NHibernate 3.1 or you are thinking about 3.2?

In any cases I'm ready to merge any changes into my Roundhouse.MySQL fork, so it would be easier to you to pull all my changes.

Should I wait for changes from your ferventcoder/roundhouse fork or in chucknorris/roundhouse?

Could you let me know when it will be better for me to pull those changes into my fork?

JFYI, I'm going into vacation and probably will not respond until May 28. After that I will merge any of new changes and resend pull request to you.

Thank you for your work on this project (... and waiting to see the new strongly-typed detached QueryOver instead of Criteria ;)

@ferventcoder
Copy link
Member

When you are ready, pull down the latest changes and integrate your changes into them. I have upgraded to NH 3.1

@ferventcoder
Copy link
Member

It's all updated in chucknorris. I usually use that as an integration point so that it is easier for collaborators.

@ferventcoder
Copy link
Member

Some more changes have been made since I last mentioned this was ready (including a new release!), but this is stabilizing.

@ferventcoder
Copy link
Member

Thank you for including tests. Taking a look at it though, you migrated over to NUnit, which I have plans to do at some point, but not yet.

Also, references are in lib/references (unless they are nuget packages, then they are in the packages folder). You may noticed I already have a folder for MySQL there.

@ferventcoder
Copy link
Member

Also some of the naming here is inconsistent.
I am not a fan of the R# comments on the top. I will end up removing them.

The naming convention is like this:

  • everything is lower case, including namespaces
  • classes, interfaces, and other types are the only thing that are PascalCased
  • methods read like sentences and use _ (underscores separating the words)
  • there is no _ starting class level variables (unless of course it supports FNH)
  • variables and parameters are lowercase and separated by _.
  • Interfaces do not have the sacred cow "I" in them.

Andy put together a ReSharper file that I am going to add that enforces all of this so you won't have to have those R# ignore comments on the top of files.

@ferventcoder
Copy link
Member

Regarding MySQL specifically:

  • the initialize_connections is to resolve the database name and instance from the connection string and vice versa.
    This should be valid:
    rh /d bob /dt MySQL
    I should be able to say I want a database only and it should be able to figure out that I want trusted connection to a local mysql instance to a database named bob. I know the initialize_connections code looks pretty bad, but it's a localized pretty bad. It's definitely something I want to come back to and find a better way (perhaps pipes and filters?), but at the moment it works.

I haven't used MySQL in such a long time, but if I remember right, there was a separation of instance and database. The create/restore should be to a database, not an instance. There should be a command to destroy a database in an instance.

@diyan
Copy link
Author

diyan commented Jun 2, 2011

Thank you for including tests. Taking a look at it though, you migrated over to NUnit, which I have plans to do at some point, but not yet.

To be hones, I've added only single integration test which were used more like entry point for debugging.

NUnit was chosen because I tried to configure MbUnit TestRunner for ReSharper but with no luck. I did not spent a lot of time for this and just ended up with single NUnit test class. This test class is not fit at all into MbUnit/BDD test approach, so it could be easily ignored.

Also, I didn't work with BDD approach at all. I would be glad to cover MySQL-related code with tests but I don't know how to write correct BDD-tests.

As for naming convention. I tried to follow existed naming convention but I think there could be mistakes because I usually write code by following Microsoft Coding Guidelines and write unit tests with naming convention which similar to BDD.

ReSharper naming rules for this particular naming convention will definitely help for coding.

the initialize_connections is to resolve the database name and instance from the connection string and vice versa.

Yes, I know that. I will try to add this functionality after merge.

I haven't used MySQL in such a long time, but if I remember right, there was a separation of instance and database. The create/restore should be to a database, not an instance. There should be a command to destroy a database in an instance.

Hmm. I know CREATE DATABASE and CREATE SCHEMA statements but they are sysonyms in MySQL.
http://stackoverflow.com/questions/1219711/mysql-create-schema-and-create-database-is-there-any-difference

I had take a look at your latest changes and will try to merge them next week.

Thank you for your feedback.

Alexey Diyan added 2 commits June 2, 2011 13:04
Conflicts:
	product/roundhouse.databases.access/roundhouse.databases.access.csproj
	product/roundhouse.databases.mysql/roundhouse.databases.mysql.csproj
	product/roundhouse.databases.oracle/roundhouse.databases.oracle.csproj
	product/roundhouse.databases.sqlserver/roundhouse.databases.sqlserver.csproj
	product/roundhouse.databases.sqlserver2000/roundhouse.databases.sqlserver2000.csproj
	product/roundhouse.tasks/roundhouse.tasks.csproj
	product/roundhouse.tests.integration/roundhouse.tests.integration.csproj
	product/roundhouse.tests/roundhouse.tests.csproj
	product/roundhouse/databases/DefaultDatabase.cs
	product/roundhouse/roundhouse.csproj
	roundhouse.sln
	sample/BuildDatabase/BuildDatabase.csproj
	sample/SampleProject/SampleProject.csproj
…rk because of some changes in UppercuT script.
@diyan
Copy link
Author

diyan commented Jun 2, 2011

Just merged all changes into my private fork and checked that MySQL migration works.

Unfortunately there are an issue with ILMerge task, I will investigate how fix it.

@ferventcoder
Copy link
Member

Working on pulling this in. Looks like you exposed a password. Might want to go change it now in your system.

@ferventcoder
Copy link
Member

I upgraded to NHibernate 3.1 prior to your last set of changes, did you not get the chance to fix the reserved keyword issue?

@ferventcoder ferventcoder merged commit 71f4ac0 into chucknorris:master Jun 20, 2011
@ferventcoder
Copy link
Member

You may have noticed that this is in. It's not quite complete, it still needs the idea of specifying just database and it uses conventions to finish out the rest. I should be able to rh /d somedatabase /dt mysql and it does it all.

@diyan
Copy link
Author

diyan commented Jun 21, 2011

This is really good news. Thanks!

Don't worry about exposed password. It was my stupid mistake and the most clever solution here will be just to update my password.

So my next changes will be:

  • Sync my fork with chucknorris/roundhouse
  • Update routine that initialize database connection as you described twice in your comments
  • Remove workaround for reserved keyword issue with MySQL dialect which were fixed in NH 3.1
  • Check functionality related to database destroying (will try to follow the same path as for MS SQL Server and Oracle)
  • Reference MySQL as NuGet package if possible
  • Debug on my environment
  • Open another pull request

@ferventcoder
Copy link
Member

database destroy is done.

Is there a mysql nuget package? That would be awesome.

@ferventcoder
Copy link
Member

What username/password would you suggest as the convention for mysql?

@diyan
Copy link
Author

diyan commented Jun 21, 2011

Default user for MySQL database is usually 'root' with no password.

http://dev.mysql.com/doc/refman/5.1/en/default-privileges.html

The initial root account passwords are empty, so anyone can connect to the MySQL server as root without a password and be granted all privileges.

We could use 'root' without password of username 'root' plus password 'root'. What do you think?

Did you have a look into ILMerge issue? Looks like UppercuT build script doesn't pass MySql.Data.dll into ILMerge.

@ferventcoder
Copy link
Member

I already fixed that as well.

@ferventcoder
Copy link
Member

The reason I am not a fan of root/"" is that the suggestion that was there was to secure that account. I wouldn't want a convention where RH is telling others not to secure the account down. I am thinking we could have it come back and ask the user/pass combo when running for mysql if it detects you don't have those filled out in the connection string.

@ferventcoder
Copy link
Member

The mysql nuget package I was talking about is really a chocolatey nuget package - http://nuget.org/List/Packages/mysql

I don't see the connector available here - http://nuget.org/List/Search?searchTerm=tag%3A%20MySQL

@ferventcoder ferventcoder modified the milestone: 0.8.5 Jun 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants