ALTER TABLE ... RENAME COLUMN syntax incorrect for dm-migrations about dm-migrations HOT 17 OPEN

Paul, would you mind looking into this? If it is an issue, please mark it as confirmed. Also, if you do decide to take it on, please mark it as accepted so we know you’re working on it.

by Dan Kubb (dkubb)

from dm-migrations.

Yup, confirmed.

To fix this, all that needs done is adding a TableModifier class to http://github.com/sam/dm-more/tree/master/dm-migrations/lib/sql/mysql.rb and overriding the ’rename_column’ method, as found here: http://github.com/sam/dm-more/tree/master/dm-migrations/lib/sql/table_modifier.rb#L33

by Paul Sadauskas (Rando)

from dm-migrations.

I have written what I think is a patch for this problem.

There were no specs for the MySQL adapter in dm-migrations so I created the spec file. I also mimicked those from sqlite3 for the core functionality.

I then added the class TableModifier with the method rename_column. The method extracts the existing column from the table columns, updates it’s name, and then generates the correct ALTER TABLE statement.

I hope it is ok that both of these are in one patch, but the later relies on the former!

This is my first patch, so sorry if it’s all wrong, but let me know what any problems are and I’ll try to fix them.

Emma

by Emma Persky

from dm-migrations.

yikes, i spoke to soon. ignore that patch...

by Emma Persky

from dm-migrations.

When I attempted to fix this bug I found numerous problems with dm-migrations. I think dm-migrations should be queued up for a rewrite before DM 1.0 is released, since it’s one of the weakest official DM libs.

Going to put this on hold while we plan out how to approach this.

by Dan Kubb (dkubb)

from dm-migrations.

I should add that we ran out of time to rewrite dm-migrations before the 1.0 launch. I am going to see what I can do about getting this in before the 1.1 release.

by Dan Kubb (dkubb)

from dm-migrations.

Kick! Any news on this?

from dm-migrations.

@stephankaag I added it to 1.3.0 milestone

from dm-migrations.

I might attempt to shoehorn in this fix in the mean time. We shouldn't leave this broken while waiting for DM 2.0.

from dm-migrations.

There's pull request from me about this issue, but hasn't been migrated yet

from dm-migrations.

@rindek are there any tests covering this new behaviour? If so, could you add them? I'll merge in this change if it has tests.

A rewrite is going to happen, but later, and if people are willing to fix things w/tests then I think we should accept them.

from dm-migrations.

I was going to work on writing tests for @rindek's patches, but it appear that master cannot even pass mysql auto-migration specs.

DataObjects::SQLError in 'DataMapper::Migrations with default adapter#auto_migrate NumericString property before(:all)'
You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'PRIMARY KEY, `number` VARCHAR(50) DEFAULT '0', PRIMARY KEY(`id`)) ENGINE = MyISA' at line 1
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/adapters/dm-do-adapter.rb:100:in `execute_non_query'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/adapters/dm-do-adapter.rb:100:in `block (2 levels) in create_model_storage'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/adapters/dm-do-adapter.rb:98:in `each'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/adapters/dm-do-adapter.rb:98:in `block in create_model_storage'
/home/hal/.rvm/gems/ruby-1.9.3-p0/bundler/gems/dm-do-adapter-d295d77f3e4b/lib/dm-do-adapter/adapter.rb:276:in `with_connection'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/adapters/dm-do-adapter.rb:93:in `create_model_storage'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/auto_migration.rb:81:in `create_model_storage'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/auto_migration.rb:177:in `auto_migrate_up!'
/vault/1/code/forks/dm/dm-migrations/lib/dm-migrations/auto_migration.rb:132:in `auto_migrate!'
spec/integration/auto_migration_spec.rb:300:in `block (6 levels) in <top (required)>'
spec/integration/auto_migration_spec.rb:8:in `capture_log'
spec/integration/auto_migration_spec.rb:300:in `block (5 levels) in <top (required)>'

747 examples, 158 failures, 4 pending

I created the datamapper user and datamapper_default_tests / datamapper_alternate_tests databases.

from dm-migrations.

I ended up improving on @rindek's rename_column_type_statement for mysql (wasn't including DEFAULT or NOT NULL modifiers) and even added a basic spec for it:

https://github.com/postmodern/dm-migrations/commits/mysql_rename_column

Can we finally merge this?

from dm-migrations.

@postmodern are you saying that the dm-migrations master's specs don't pass? They appear to be passing in CI: http://ci.datamapper.org/job/dm-migrations/

from dm-migrations.

@dkubb maybe I'm forgetting something, or more likely it's an environment specific issue. Half of the auto-migration specs consistently fail on my Fedora 16 workstation running mysql 5.5.19. However, I was able to manually run the migration specs for this particular bug.

from dm-migrations.

@postmodern thanks for this. I was going to write those tests but I totally forgot about it :(

from dm-migrations.

@rindek dm-migrations is kind of a wasteland, none of the existing specs caught this obvious syntax error. I also noticed that the MySQL specific change_column method wasn't speced. The sooner we can fix this, the better. :)

from dm-migrations.