"bitsweat" has kicked AR/SQLServer while it was down...
Hi All,
Is there a specific reason why all those scripts in activerecord/test/fixtures/db_definitions exist instead of one database independent migration file?
Imo the ability or inability to get the migration file correctly imported in the database of your choice is already a test.
Regards,
Lawrence
+1 for schema in Ruby format … I don’t think that tests use db features that AR::Migrations don’t support currently.
As you can use execute in migrations, there should be the possibility of including tests for database-specific implementation. I don't know if there are any such tests right now, but unless the current situation is actually broken, I don't see a reason why to fix it.
Michael Glaesemann grzm seespotcode net
The current situation is not broken, but difficult to maintain as schema changes require multiple files to be updated with different syntaxes. AR::Migrations were created to solve exactly this problem, and I think they’re powerful enough to set up the database for testing.
By the time migrations were added, there were a lot of tests using those old schema files. Since then, all future additions should have been made in the ruby schema files. David has asked several times for someone to clean that up and convert to ruby schema files, but it is a larger job than it may initially look. Does someone want to give it a shot?
All those sql files under db_definitions need to be moved to schema.rb, right?
Does it have to be in 1 patch? Maybe we can do this 1 db adapter at a time, and initially just add a condition to check for the adapter in schema.rb.
Is there a specific reason why all those scripts in activerecord/test/fixtures/db_definitions exist instead of one database independent migration file?
Largely because they were created before migrations and the schema dumper.
There are also some db definitions that can't be done in schema.rb.
Imo the ability or inability to get the migration file correctly imported in the database of your choice is already a test.
All new additions use schema.rb in the same dir.
jeremy
Other issues include:
-
Inconsistencies between different databases: for example topics.content is of type VARCHAR(255) in SQL Server but of type TEXT in MySQL.
-
If any .sql files should exist, then they should exist within the plugin adapter, not within rails core.
I’ll have a shot at transforming this into a migrations file. I’ll attempt this first for SQL Server only, this should not affect core. If that works we can see if/how this can used by all the other adapters.
There are also some db definitions that can’t be done in schema.rb.
Ok, I see those. We should be able to work around 'm… I’ll have a look.
Regards,
Lawrence
All those sql files under db_definitions need to be moved to schema.rb, right?
Does it have to be in 1 patch? Maybe we can do this 1 db adapter at a time, and initially just add a condition to check for the adapter in schema.rb.
I think that'd be fine. If it's a temporary condition put in place until the transition is done, just keep it commented so it's obvious what's going on. Smaller patches are easier to deal with.
I've encountered some problems with simply moving the sql files to schema.rb. The sql files that end with 2 need to connect to activerecord_unittest2. Moving it to schema.rb would result to having the courses table created in activerecord_unittest1.
Is it OK to create a schema2.rb to be used for the courses table? That is, in AAACreateTablesTest#test_drop_and_create_courses_table, we can read schema2.rb in the same way that schema.rb is read in AAACreateTablesTest#test_load_schema.
See patch
Cheers,
Lawrence
Thanks for the link. I kinda liked the approach of doing this 1 db adapter at a time since I don't want to send a patch that affects other databases that I can't test locally. Anyway, I'll submit a patch soon as I'm almost done moving db/definitions/mysql*.sql to schema*.rb. I'll keep a close watch on your ticket's progress. I think I'll just have 2 patches in my new ticket, 1 for mysql.sql and the other for mysql2.sql so that in case your patch is accepted, we can just ignore the mysql2 patch. What do you think?
Btw, is anyone also working on a patch for MySQL? I'm just checking coz I'll stop my work on this if anyone already is. I plan to do this for PostgeSQL after.
> since I don’t want to send a patch that affects other databases
Note that when one adds a test, this already affects all databases. I’m pretty sure tests and patches get into core without being tested on all or at least a significant number of databases. So this is really a general issue, not just for this little project.
The problem with doing this one adapter at a time is that if you created a schema.rb based on mysql.sql and someone else creates a schema.rb based on say oracle.sql or postgresql.sql, you’d end up with significant different schema.rb files. So whose schema.rb is going to end up in core?
I had also moved all mysql.sql statement into a schema.rb (except for tables subscribers, fk_test_has_pk and fk_test_has_fk, for which no migrations equivalent exists). Works fine for mysql, but then shows quite a few regressions on quite a few other databases. For example, posts.body is of type text in mysql, so you’d think t.text :body would be the answer, unfortunately the tests do a search via that field which is not an allowed operation on e.g. SQL Server (which is why in the sql stament SQL Server defines the field as varchar(4096) instead of text; same for Oracle, Sybase and I suspect Firebird and DB2 as well).
I guess the one db adapter at a time approach works, but imo we need to do this sequentially. So if you create a patch for mysql, then we all should first get behind that one and get it into core before we move on to the next database adapter.
Lawrence
> since I don't want to send a patch that affects other databases
Note that when one adds a test, this already affects all databases. I'm pretty sure tests and patches get into core without being tested on all or at least a significant number of databases. So this is really a general issue, not just for this little project.
I actually ended up changing AAACreateTablesTest and I realized what you mean (that I've just added a patch that affects other databases), so I guess that wasn't the right argument. For my patch though, it surely won't work for other databases without the condition that I added to check if the adapter is MySQL. I guess what I meant is that since I'm not very familiar with the other database adapters, I'm not capable of creating a patch for this particular issue that would work for all. So I was hoping we could do this with the help of other contributors familiar with the other databases.
Btw, instead of creating 2 patches as I've mentioned before, I didn't create the patch for removing mysql2.sql and mysql2.drop.sql. Instead, I just referenced your ticket so there's no wasted effort.
I guess the one db adapter at a time approach works, but imo we need to do this sequentially. So if you create a patch for mysql, then we all should first get behind that one and get it into core before we move on to the next database adapter.
It would be great if we could do this sequentially.
I’m testing your patch. Works fine for MySQL, of course 
Nice solution for table subscribers.
While currently schema.rb begins with a line to test that the adapter is MySQL, the end goal is to remove that condition. So all databases would use the exact same migrations code.
Take e.g. table :tasks. It defines the date fields as not null with a default. However, all other database sql files define the fields as nullable and no default. If we accept this patch, then for the next db adapter this would need to change. I.e.: it would affect all adapters that so far had switched to migrations.
Other things include:
-
Remove :options => ‘TYPE=InnoDB DEFAULT CHARSET=utf8’. It will then work for all databases, and still works for MySQL as well (if you have created your database using charset UTF8)
-
Explicitly define :scale => 0 for :numeric_data.world_population and my_house_population
-
We can, for now, change all columns of type :text to type :string. It should then work for other databases as well. However, it then does add one failing test for MySQL, but that is a test specific to MySQL only (i.e., modify the test to use a new table with a column of type :text).
-
The statement to create the foreign key should probably go into a seperate file for use by MySQL only. I.e., some statements will stay to be adapter specific.
Doing the above I got it reasonably well working for SQL Server as
well. 
Generally I would have prefered the sexy migrations syntax, but that’s
a different issue
And some more:
-
You forgot to include the patch for the courses table.
-
In the rakefile, target mysql:build_databases, remove the statements that would import the sql files.
Btw, I am giving a +1 to the patch in its current state, as it does work for MySQL, which was the goal for this patch. The above can be modified later.
Happy to have this train moving
Cheers,
Lawrence
Exactly the kind of feedback I was hoping for.
I'll apply most if not all of your comments. I'll have a closer look later after work, and expect an updated patch in maybe 10 hours.
Thanks a lot!
New patches have been added to the ticket. FYI
Hi All,
As a continuation to migrate more database adapters towards use of schema.rb instead of .sql files to create the test environment:
Need 2 more +s please. In case you wonder why the sqlite_adapter needs a change: if you keep it as it is today, then the .sql files create the topics table with column bonus_time of type “TIME”. However, if you create the topics table via a migrations file, using “t.time bonus_time”, then currently it will be created with type “DATETIME”. Running the tests sqlite3 will then show these two failures:
- Failure: test_attributes_on_dummy_time(BasicsTest) [test/base_test.rb:959]: <Sat Jan 01 05:42:00 +1100 2000> expected but was .
- Failure: test_utc_as_time_zone(BasicsTest) [test/base_test.rb:688]: <Sat Jan 01 05:42:00 UTC 2000> expected but was . This is because now with type “DATETIME”, in schema_definitions.rb method type_cast(value) the :datetime type will go through method string_to_time. Instead, with type “TIME” is would go through the method string_to_dummy_time which is what it should do. After this change I propose the Postgresql adapter also starts using the schema.rb file. Cheers, Lawrence