ALTER TABLE ... RENAME COLUMN syntax incorrect for dm-migrations

[solnic]

The default syntax used to rename a column is not compatible with MySQL. From the docs it should instead be

ALTER TABLE tbl_name CHANGE [COLUMN] old_col_name new_col_name column_definition

---

Created by Phil - 2008-12-01 06:38:33 UTC

Original Lighthouse ticket: http://datamapper.lighthouseapp.com/projects/20609/tickets/680

[solnic]

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)

[solnic]

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)

[solnic]

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

[solnic]

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

by Emma Persky

[solnic]

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)

[solnic]

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)

[stephankaag]

Kick! Any news on this?

[solnic]

@stephankaag I added it to 1.3.0 milestone

[postmodern]

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

[rindek]

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

[dkubb]

@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.

[postmodern]

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.

[postmodern]

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?

[dkubb]

@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/

[postmodern]

@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.

[rindek]

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

[postmodern]

@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. :)