Skip to content

Loading…

Fix Migration#reversible by not using `transaction`. #8588

Merged
merged 1 commit into from

3 participants

@marcandre

This was causing trouble in mysql/mysql2 because transactions
are not supported when modifying schema. [#8267]

@carlosantoniodasilva There are other failures to fix, but my PB is running out of juice, so I'll handle those later today.

@marcandre marcandre Fixes for PR [#8267]
* Fix Migration#reversible by not using `transaction`.

* Adapt mysql adapter to updated api for remove_column

* Update test after aedcd68
a4932d6
@marcandre

Updated, should address all failures.

Sorry for these issues. I somehow didn't realize I was only testing using sqlite... Also for some reason I was sure that travis was running on pull requests too.

@steveklabnik
Ruby on Rails member

Travis doesn't run on PRs for rails, we're super intense on resources at the moment.

@marcandre marcandre referenced this pull request
Merged

Reversible commands #8267

@carlosantoniodasilva carlosantoniodasilva merged commit 7ab469c into rails:master
@carlosantoniodasilva
Ruby on Rails member

Thanks @marcandre. Lets get travis running this so :)

@carlosantoniodasilva
Ruby on Rails member

Great, mysql seems to be fine. Railties tests haven't run though, lets wait for some other commit to get them running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 23, 2012
  1. @marcandre

    Fixes for PR [#8267]

    marcandre committed
    * Fix Migration#reversible by not using `transaction`.
    
    * Adapt mysql adapter to updated api for remove_column
    
    * Update test after aedcd68
View
9 activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -669,10 +669,13 @@ def rename_column_sql(table_name, column_name, new_column_name)
rename_column_sql
end
- def remove_column_sql(table_name, *column_names)
- columns_for_remove(table_name, *column_names).map {|column_name| "DROP #{column_name}" }
+ def remove_column_sql(table_name, column_name, type = nil, options = {})
+ "DROP #{quote_column_name(column_name)}"
+ end
+
+ def remove_columns_sql(table_name, *column_names)
+ column_names.map {|column_name| remove_column_sql(table_name, column_name) }
end
- alias :remove_columns_sql :remove_column
def add_index_sql(table_name, column_name, options = {})
index_name, index_type, index_columns = add_index_options(table_name, column_name, options)
View
11 activerecord/lib/active_record/migration.rb
@@ -477,7 +477,7 @@ def down
# end
def reversible
helper = ReversibleBlockHelper.new(reverting?)
- transaction{ yield helper }
+ execute_block{ yield helper }
end
# Runs the given migration classes.
@@ -639,6 +639,15 @@ def next_migration_number(number)
"%.3d" % number
end
end
+
+ private
+ def execute_block
+ if connection.respond_to? :execute_block
+ super # use normal delegation to record the block
+ else
+ yield
+ end
+ end
end
# MigrationProxy is used to defer loading of the actual migration classes
View
3 activerecord/lib/active_record/migration/command_recorder.rb
@@ -73,7 +73,7 @@ def respond_to?(*args) # :nodoc:
[:create_table, :create_join_table, :rename_table, :add_column, :remove_column,
:rename_index, :rename_column, :add_index, :remove_index, :add_timestamps, :remove_timestamps,
:change_column_default, :add_reference, :remove_reference, :transaction,
- :drop_join_table, :drop_table,
+ :drop_join_table, :drop_table, :execute_block,
:change_column, :execute, :remove_columns, # irreversible methods need to be here too
].each do |method|
class_eval <<-EOV, __FILE__, __LINE__ + 1
@@ -94,6 +94,7 @@ def change_table(table_name, options = {})
module StraightReversions
private
{ transaction: :transaction,
+ execute_block: :execute_block,
create_table: :drop_table,
create_join_table: :drop_join_table,
add_column: :remove_column,
View
6 activerecord/test/cases/invertible_migration_test.rb
@@ -89,8 +89,10 @@ def change
end
def teardown
- if ActiveRecord::Base.connection.table_exists?("horses")
- ActiveRecord::Base.connection.drop_table("horses")
+ %w[horses new_horses].each do |table|
+ if ActiveRecord::Base.connection.table_exists?(table)
+ ActiveRecord::Base.connection.drop_table(table)
+ end
end
end
View
4 railties/test/application/rake/migrations_test.rb
@@ -50,9 +50,9 @@ class AMigration < ActiveRecord::Migration
assert_match(/AddEmailToUsers: migrated/, output)
output = `rake db:rollback STEP=2`
- assert_match(/drop_table\("users"\)/, output)
+ assert_match(/drop_table\(:users\)/, output)
assert_match(/CreateUsers: reverted/, output)
- assert_match(/remove_column\("users", :email\)/, output)
+ assert_match(/remove_column\(:users, :email, :string\)/, output)
assert_match(/AddEmailToUsers: reverted/, output)
end
end
Something went wrong with that request. Please try again.