diff options
Diffstat (limited to 'activerecord')
11 files changed, 591 insertions, 208 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 442b11dad9..272cef6fcf 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,22 @@ ## Rails 4.0.0 (unreleased) ## +* Improve ways to write `change` migrations, making the old `up` & `down` methods no longer necessary. + + * The methods `drop_table` and `remove_column` are now reversible, as long as the necessary information is given. + The method `remove_column` used to accept multiple column names; instead use `remove_columns` (which is not revertible). + The method `change_table` is also reversible, as long as its block doesn't call `remove`, `change` or `change_default` + + * New method `reversible` makes it possible to specify code to be run when migrating up or down. + See the [Guide on Migration](https://github.com/rails/rails/blob/master/guides/source/migrations.md#using-the-reversible-method) + + * New method `revert` will revert a whole migration or the given block. + If migrating down, the given migration / block is run normally. + See the [Guide on Migration](https://github.com/rails/rails/blob/master/guides/source/migrations.md#reverting-previous-migrations) + + Attempting to revert the methods `execute`, `remove_columns` and `change_column` will now raise an IrreversibleMigration instead of actually executing them without any output. + + *Marc-André Lafortune* + * Serialized attributes can be serialized in integer columns. Fix #8575. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 73012834c9..b1ec33d06c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -423,7 +423,7 @@ module ActiveRecord # t.remove(:qualification) # t.remove(:qualification, :experience) def remove(*column_names) - @base.remove_column(@table_name, *column_names) + @base.remove_columns(@table_name, *column_names) end # Removes the given index from the table. @@ -490,20 +490,8 @@ module ActiveRecord class_eval <<-EOV, __FILE__, __LINE__ + 1 def #{column_type}(*args) # def string(*args) options = args.extract_options! # options = args.extract_options! - column_names = args # column_names = args - type = :'#{column_type}' # type = :string - column_names.each do |name| # column_names.each do |name| - column = ColumnDefinition.new(@base, name.to_s, type) # column = ColumnDefinition.new(@base, name, type) - if options[:limit] # if options[:limit] - column.limit = options[:limit] # column.limit = options[:limit] - elsif native[type].is_a?(Hash) # elsif native[type].is_a?(Hash) - column.limit = native[type][:limit] # column.limit = native[type][:limit] - end # end - column.precision = options[:precision] # column.precision = options[:precision] - column.scale = options[:scale] # column.scale = options[:scale] - column.default = options[:default] # column.default = options[:default] - column.null = options[:null] # column.null = options[:null] - @base.add_column(@table_name, name, column.sql_type, options) # @base.add_column(@table_name, name, column.sql_type, options) + args.each do |name| # column_names.each do |name| + @base.add_column(@table_name, name, :#{column_type}, options) # @base.add_column(@table_name, name, :string, options) end # end end # end EOV diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index f1e42dfbbe..f2d6eb9d27 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -214,6 +214,17 @@ module ActiveRecord end end + # Drops the join table specified by the given arguments. + # See create_join_table for details. + # + # Although this command ignores the block if one is given, it can be helpful + # to provide one in a migration's +change+ method so it can be reverted. + # In that case, the block will be used by create_join_table. + def drop_join_table(table_1, table_2, options = {}) + join_table_name = find_join_table_name(table_1, table_2, options) + drop_table(join_table_name) + end + # A block for changing columns in +table+. # # # change_table() yields a Table instance @@ -294,6 +305,10 @@ module ActiveRecord end # Drops a table from the database. + # + # Although this command ignores +options+ and the block if one is given, it can be helpful + # to provide these in a migration's +change+ method so it can be reverted. + # In that case, +options+ and the block will be used by create_table. def drop_table(table_name, options = {}) execute "DROP TABLE #{quote_table_name(table_name)}" end @@ -306,14 +321,26 @@ module ActiveRecord execute(add_column_sql) end - # Removes the column(s) from the table definition. + # Removes the given columns from the table definition. # - # remove_column(:suppliers, :qualification) # remove_columns(:suppliers, :qualification, :experience) - def remove_column(table_name, *column_names) - columns_for_remove(table_name, *column_names).each {|column_name| execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{column_name}" } + def remove_columns(table_name, *column_names) + raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.empty? + column_names.each do |column_name| + remove_column(table_name, column_name) + end + end + + # Removes the column from the table definition. + # + # remove_column(:suppliers, :qualification) + # + # The +type+ and +options+ parameters will be ignored if present. It can be helpful + # to provide these in a migration's +change+ method so it can be reverted. + # In that case, +type+ and +options+ will be used by add_column. + def remove_column(table_name, column_name, type = nil, options = {}) + execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{quote_column_name{column_name}}" end - alias :remove_columns :remove_column # Changes the column's definition according to the new options. # See TableDefinition#column for details of the options you can use. @@ -662,7 +689,8 @@ module ActiveRecord end def columns_for_remove(table_name, *column_names) - raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.blank? + ActiveSupport::Deprecation.warn("columns_for_remove is deprecated and will be removed in the future") + raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.blank? column_names.map {|column_name| quote_column_name(column_name) } end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 8aa5707959..11e8197293 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -444,15 +444,11 @@ module ActiveRecord end end - def remove_column(table_name, *column_names) #:nodoc: - raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.empty? - column_names.each do |column_name| - alter_table(table_name) do |definition| - definition.columns.delete(definition[column_name]) - end + def remove_column(table_name, column_name, type = nil, options = {}) #:nodoc: + alter_table(table_name) do |definition| + definition.columns.delete(definition[column_name]) end end - alias :remove_columns :remove_column def change_column_default(table_name, column_name, default) #:nodoc: alter_table(table_name) do |definition| diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index ef2107ad24..806b367c6b 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -373,22 +373,129 @@ module ActiveRecord @name = name @version = version @connection = nil - @reverting = false end # instantiate the delegate object after initialize is defined self.verbose = true self.delegate = new - def revert - @reverting = true - yield - ensure - @reverting = false + # Reverses the migration commands for the given block and + # the given migrations. + # + # The following migration will remove the table 'horses' + # and create the table 'apples' on the way up, and the reverse + # on the way down. + # + # class FixTLMigration < ActiveRecord::Migration + # def change + # revert do + # create_table(:horses) do |t| + # t.text :content + # t.datetime :remind_at + # end + # end + # create_table(:apples) do |t| + # t.string :variety + # end + # end + # end + # + # Or equivalently, if +TenderloveMigration+ is defined as in the + # documentation for Migration: + # + # require_relative '2012121212_tenderlove_migration' + # + # class FixupTLMigration < ActiveRecord::Migration + # def change + # revert TenderloveMigration + # + # create_table(:apples) do |t| + # t.string :variety + # end + # end + # end + # + # This command can be nested. + def revert(*migration_classes) + run(*migration_classes.reverse, revert: true) unless migration_classes.empty? + if block_given? + if @connection.respond_to? :revert + @connection.revert { yield } + else + recorder = CommandRecorder.new(@connection) + @connection = recorder + suppress_messages do + @connection.revert { yield } + end + @connection = recorder.delegate + recorder.commands.each do |cmd, args, block| + send(cmd, *args, &block) + end + end + end end def reverting? - @reverting + @connection.respond_to?(:reverting) && @connection.reverting + end + + class ReversibleBlockHelper < Struct.new(:reverting) + def up + yield unless reverting + end + + def down + yield if reverting + end + end + + # Used to specify an operation that can be run in one direction or another. + # Call the methods +up+ and +down+ of the yielded object to run a block + # only in one given direction. + # The whole block will be called in the right order within the migration. + # + # In the following example, the looping on users will always be done + # when the three columns 'first_name', 'last_name' and 'full_name' exist, + # even when migrating down: + # + # class SplitNameMigration < ActiveRecord::Migration + # def change + # add_column :users, :first_name, :string + # add_column :users, :last_name, :string + # + # reversible do |dir| + # User.reset_column_information + # User.all.each do |u| + # dir.up { u.first_name, u.last_name = u.full_name.split(' ') } + # dir.down { u.full_name = "#{u.first_name} #{u.last_name}" } + # u.save + # end + # end + # + # revert { add_column :users, :full_name, :string } + # end + # end + def reversible + helper = ReversibleBlockHelper.new(reverting?) + transaction{ yield helper } + end + + # Runs the given migration classes. + # Last argument can specify options: + # - :direction (default is :up) + # - :revert (default is false) + def run(*migration_classes) + opts = migration_classes.extract_options! + dir = opts[:direction] || :up + dir = (dir == :down ? :up : :down) if opts[:revert] + if reverting? + # If in revert and going :up, say, we want to execute :down without reverting, so + revert { run(*migration_classes, direction: dir, revert: true) } + else + migration_classes.each do |migration_class| + migration_class.new.exec_migration(@connection, dir) + end + end end def up @@ -414,29 +521,9 @@ module ActiveRecord time = nil ActiveRecord::Base.connection_pool.with_connection do |conn| - @connection = conn - if respond_to?(:change) - if direction == :down - recorder = CommandRecorder.new(@connection) - suppress_messages do - @connection = recorder - change - end - @connection = conn - time = Benchmark.measure { - self.revert { - recorder.inverse.each do |cmd, args| - send(cmd, *args) - end - } - } - else - time = Benchmark.measure { change } - end - else - time = Benchmark.measure { send(direction) } + time = Benchmark.measure do + exec_migration(conn, direction) end - @connection = nil end case direction @@ -445,6 +532,21 @@ module ActiveRecord end end + def exec_migration(conn, direction) + @connection = conn + if respond_to?(:change) + if direction == :down + revert { change } + else + change + end + else + send(direction) + end + ensure + @connection = nil + end + def write(text="") puts(text) if verbose end @@ -483,7 +585,7 @@ module ActiveRecord arg_list = arguments.map{ |a| a.inspect } * ', ' say_with_time "#{method}(#{arg_list})" do - unless reverting? + unless @connection.respond_to? :revert unless arguments.empty? || method == :execute arguments[0] = Migrator.proper_table_name(arguments.first) arguments[1] = Migrator.proper_table_name(arguments.second) if method == :rename_table diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 95f4360578..13ce28330a 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -16,69 +16,116 @@ module ActiveRecord class CommandRecorder include JoinTable - attr_accessor :commands, :delegate + attr_accessor :commands, :delegate, :reverting def initialize(delegate = nil) @commands = [] @delegate = delegate + @reverting = false + end + + # While executing the given block, the recorded will be in reverting mode. + # All commands recorded will end up being recorded reverted + # and in reverse order. + # For example: + # + # recorder.revert{ recorder.record(:rename_table, [:old, :new]) } + # # same effect as recorder.record(:rename_table, [:new, :old]) + def revert + @reverting = !@reverting + previous = @commands + @commands = [] + yield + ensure + @commands = previous.concat(@commands.reverse) + @reverting = !@reverting end # record +command+. +command+ should be a method name and arguments. # For example: # # recorder.record(:method_name, [:arg1, :arg2]) - def record(*command) - @commands << command + def record(*command, &block) + if @reverting + @commands << inverse_of(*command, &block) + else + @commands << (command << block) + end end - # Returns a list that represents commands that are the inverse of the - # commands stored in +commands+. For example: + # Returns the inverse of the given command. For example: # - # recorder.record(:rename_table, [:old, :new]) - # recorder.inverse # => [:rename_table, [:new, :old]] + # recorder.inverse_of(:rename_table, [:old, :new]) + # # => [:rename_table, [:new, :old]] # # This method will raise an +IrreversibleMigration+ exception if it cannot - # invert the +commands+. - def inverse - @commands.reverse.map { |name, args| - method = :"invert_#{name}" - raise IrreversibleMigration unless respond_to?(method, true) - send(method, args) - } + # invert the +command+. + def inverse_of(command, args, &block) + method = :"invert_#{command}" + raise IrreversibleMigration unless respond_to?(method, true) + send(method, args, &block) end def respond_to?(*args) # :nodoc: super || delegate.respond_to?(*args) end - [:create_table, :create_join_table, :change_table, :rename_table, :add_column, :remove_column, :rename_index, :rename_column, :add_index, :remove_index, :add_timestamps, :remove_timestamps, :change_column, :change_column_default, :add_reference, :remove_reference].each do |method| + [: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, :change_column_default, :add_reference, :remove_reference, :transaction, + :drop_join_table, :drop_table, :remove_index, + :change_column, :execute, :remove_columns, # irreversible methods need to be here too + ].each do |method| class_eval <<-EOV, __FILE__, __LINE__ + 1 - def #{method}(*args) # def create_table(*args) - record(:"#{method}", args) # record(:create_table, args) - end # end + def #{method}(*args, &block) # def create_table(*args, &block) + record(:"#{method}", args, &block) # record(:create_table, args, &block) + end # end EOV end alias :add_belongs_to :add_reference alias :remove_belongs_to :remove_reference - private - - def invert_create_table(args) - [:drop_table, [args.first]] + def change_table(table_name, options = {}) + yield ConnectionAdapters::Table.new(table_name, self) end - def invert_create_join_table(args) - table_name = find_join_table_name(*args) + private - [:drop_table, [table_name]] + module StraightReversions + private + { transaction: :transaction, + create_table: :drop_table, + create_join_table: :drop_join_table, + add_column: :remove_column, + add_timestamps: :remove_timestamps, + add_reference: :remove_reference, + }.each do |cmd, inv| + [[inv, cmd], [cmd, inv]].each do |method, inverse| + class_eval <<-EOV, __FILE__, __LINE__ + 1 + def invert_#{method}(args, &block) # def invert_create_table(args, &block) + [:#{inverse}, args, block] # [:drop_table, args, block] + end # end + EOV + end + end + end + + include StraightReversions + + def invert_drop_table(args, &block) + if args.size == 1 && block == nil + raise ActiveRecord::IrreversibleMigration, "To avoid mistakes, drop_table is only reversible if given options or a block (can be empty)." + end + super end def invert_rename_table(args) [:rename_table, args.reverse] end - def invert_add_column(args) - [:remove_column, args.first(2)] + def invert_remove_column(args) + raise ActiveRecord::IrreversibleMigration, "remove_column is only reversible if given a type." if args.size <= 2 + super end def invert_rename_index(args) @@ -91,27 +138,18 @@ module ActiveRecord def invert_add_index(args) table, columns, options = *args - index_name = options.try(:[], :name) - options_hash = index_name ? {:name => index_name} : {:column => columns} - [:remove_index, [table, options_hash]] + [:remove_index, [table, (options || {}).merge(column: columns)]] end - def invert_remove_timestamps(args) - [:add_timestamps, args] - end + def invert_remove_index(args) + table, options = *args + raise ActiveRecord::IrreversibleMigration, "remove_index is only reversible if given a :column option." unless options && options[:column] - def invert_add_timestamps(args) - [:remove_timestamps, args] + options = options.dup + [:add_index, [table, options.delete(:column), options]] end - def invert_add_reference(args) - [:remove_reference, args] - end alias :invert_add_belongs_to :invert_add_reference - - def invert_remove_reference(args) - [:add_reference, args] - end alias :invert_remove_belongs_to :invert_remove_reference # Forwards any missing method call to the \target. diff --git a/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb b/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb index d5c07aecd3..ae9c74fd05 100644 --- a/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb +++ b/activerecord/lib/rails/generators/active_record/migration/templates/migration.rb @@ -21,28 +21,16 @@ class <%= migration_class_name %> < ActiveRecord::Migration end end <%- else -%> - def up + def change <% attributes.each do |attribute| -%> <%- if migration_action -%> <%- if attribute.reference? -%> - remove_reference :<%= table_name %>, :<%= attribute.name %><%= ', polymorphic: true' if attribute.polymorphic? %> + remove_reference :<%= table_name %>, :<%= attribute.name %><%= attribute.inject_options %> <%- else -%> - remove_column :<%= table_name %>, :<%= attribute.name %> - <%- end -%> -<%- end -%> -<%- end -%> - end - - def down -<% attributes.reverse.each do |attribute| -%> -<%- if migration_action -%> - <%- if attribute.reference? -%> - add_reference :<%= table_name %>, :<%= attribute.name %><%= attribute.inject_options %> - <%- else -%> - add_column :<%= table_name %>, :<%= attribute.name %>, :<%= attribute.type %><%= attribute.inject_options %> <%- if attribute.has_index? -%> - add_index :<%= table_name %>, :<%= attribute.index_name %><%= attribute.inject_index_options %> + remove_index :<%= table_name %>, :<%= attribute.index_name %><%= attribute.inject_index_options %> <%- end -%> + remove_column :<%= table_name %>, :<%= attribute.name %>, :<%= attribute.type %><%= attribute.inject_options %> <%- end -%> <%- end -%> <%- end -%> diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb index 8f1cdd47ea..cf4e39c4ef 100644 --- a/activerecord/test/cases/invertible_migration_test.rb +++ b/activerecord/test/cases/invertible_migration_test.rb @@ -17,6 +17,37 @@ module ActiveRecord end end + class InvertibleRevertMigration < SilentMigration + def change + revert do + create_table("horses") do |t| + t.column :content, :text + t.column :remind_at, :datetime + end + end + end + end + + class InvertibleByPartsMigration < SilentMigration + attr_writer :test + def change + create_table("new_horses") do |t| + t.column :breed, :string + end + reversible do |dir| + @test.yield :both + dir.up { @test.yield :up } + dir.down { @test.yield :down } + end + revert do + create_table("horses") do |t| + t.column :content, :text + t.column :remind_at, :datetime + end + end + end + end + class NonInvertibleMigration < SilentMigration def change create_table("horses") do |t| @@ -40,6 +71,23 @@ module ActiveRecord end end + class RevertWholeMigration < SilentMigration + def initialize(name = self.class.name, version = nil, migration) + @migration = migration + super(name, version) + end + + def change + revert @migration + end + end + + class NestedRevertWholeMigration < RevertWholeMigration + def change + revert { super } + end + end + def teardown if ActiveRecord::Base.connection.table_exists?("horses") ActiveRecord::Base.connection.drop_table("horses") @@ -67,6 +115,83 @@ module ActiveRecord assert !migration.connection.table_exists?("horses") end + def test_migrate_revert + migration = InvertibleMigration.new + revert = InvertibleRevertMigration.new + migration.migrate :up + revert.migrate :up + assert !migration.connection.table_exists?("horses") + revert.migrate :down + assert migration.connection.table_exists?("horses") + migration.migrate :down + assert !migration.connection.table_exists?("horses") + end + + def test_migrate_revert_by_part + InvertibleMigration.new.migrate :up + received = [] + migration = InvertibleByPartsMigration.new + migration.test = ->(dir){ + assert migration.connection.table_exists?("horses") + assert migration.connection.table_exists?("new_horses") + received << dir + } + migration.migrate :up + assert_equal [:both, :up], received + assert !migration.connection.table_exists?("horses") + assert migration.connection.table_exists?("new_horses") + migration.migrate :down + assert_equal [:both, :up, :both, :down], received + assert migration.connection.table_exists?("horses") + assert !migration.connection.table_exists?("new_horses") + end + + def test_migrate_revert_whole_migration + migration = InvertibleMigration.new + [LegacyMigration, InvertibleMigration].each do |klass| + revert = RevertWholeMigration.new(klass) + migration.migrate :up + revert.migrate :up + assert !migration.connection.table_exists?("horses") + revert.migrate :down + assert migration.connection.table_exists?("horses") + migration.migrate :down + assert !migration.connection.table_exists?("horses") + end + end + + def test_migrate_nested_revert_whole_migration + revert = NestedRevertWholeMigration.new(InvertibleRevertMigration) + revert.migrate :down + assert revert.connection.table_exists?("horses") + revert.migrate :up + assert !revert.connection.table_exists?("horses") + end + + def test_revert_order + block = Proc.new{|t| t.string :name } + recorder = ActiveRecord::Migration::CommandRecorder.new(ActiveRecord::Base.connection) + recorder.instance_eval do + create_table("apples", &block) + revert do + create_table("bananas", &block) + revert do + create_table("clementines") + create_table("dates") + end + create_table("elderberries") + end + revert do + create_table("figs") + create_table("grapes") + end + end + assert_equal [[:create_table, ["apples"], block], [:drop_table, ["elderberries"], nil], + [:create_table, ["clementines"], nil], [:create_table, ["dates"], nil], + [:drop_table, ["bananas"], block], [:drop_table, ["grapes"], nil], + [:drop_table, ["figs"], nil]], recorder.commands + end + def test_legacy_up LegacyMigration.migrate :up assert ActiveRecord::Base.connection.table_exists?("horses"), "horses should exist" diff --git a/activerecord/test/cases/migration/change_table_test.rb b/activerecord/test/cases/migration/change_table_test.rb index 8fb03cdee0..ac1ad176db 100644 --- a/activerecord/test/cases/migration/change_table_test.rb +++ b/activerecord/test/cases/migration/change_table_test.rb @@ -3,21 +3,8 @@ require "cases/migration/helper" module ActiveRecord class Migration class TableTest < ActiveRecord::TestCase - class MockConnection < MiniTest::Mock - def native_database_types - { - :string => 'varchar(255)', - :integer => 'integer', - } - end - - def type_to_sql(type, limit, precision, scale) - native_database_types[type] - end - end - def setup - @connection = MockConnection.new + @connection = MiniTest::Mock.new end def teardown @@ -98,26 +85,18 @@ module ActiveRecord end end - def string_column - @connection.native_database_types[:string] - end - - def integer_column - @connection.native_database_types[:integer] - end - def test_integer_creates_integer_column with_change_table do |t| - @connection.expect :add_column, nil, [:delete_me, :foo, integer_column, {}] - @connection.expect :add_column, nil, [:delete_me, :bar, integer_column, {}] + @connection.expect :add_column, nil, [:delete_me, :foo, :integer, {}] + @connection.expect :add_column, nil, [:delete_me, :bar, :integer, {}] t.integer :foo, :bar end end def test_string_creates_string_column with_change_table do |t| - @connection.expect :add_column, nil, [:delete_me, :foo, string_column, {}] - @connection.expect :add_column, nil, [:delete_me, :bar, string_column, {}] + @connection.expect :add_column, nil, [:delete_me, :foo, :string, {}] + @connection.expect :add_column, nil, [:delete_me, :bar, :string, {}] t.string :foo, :bar end end @@ -194,14 +173,14 @@ module ActiveRecord def test_remove_drops_single_column with_change_table do |t| - @connection.expect :remove_column, nil, [:delete_me, :bar] + @connection.expect :remove_columns, nil, [:delete_me, :bar] t.remove :bar end end def test_remove_drops_multiple_columns with_change_table do |t| - @connection.expect :remove_column, nil, [:delete_me, :bar, :baz] + @connection.expect :remove_columns, nil, [:delete_me, :bar, :baz] t.remove :bar, :baz end end diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb index f2213ee6aa..2cad8a6d96 100644 --- a/activerecord/test/cases/migration/command_recorder_test.rb +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -26,7 +26,7 @@ module ActiveRecord }.new) assert recorder.respond_to?(:create_table), 'respond_to? create_table' recorder.send(:create_table, :horses) - assert_equal [[:create_table, [:horses]]], recorder.commands + assert_equal [[:create_table, [:horses], nil]], recorder.commands end def test_unknown_commands_delegate @@ -34,10 +34,15 @@ module ActiveRecord assert_equal 'bar', recorder.foo end - def test_unknown_commands_raise_exception_if_they_cannot_delegate - @recorder.record :execute, ['some sql'] + def test_inverse_of_raise_exception_on_unknown_commands assert_raises(ActiveRecord::IrreversibleMigration) do - @recorder.inverse + @recorder.inverse_of :execute, ['some sql'] + end + end + + def test_irreversible_commands_raise_exception + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.revert{ @recorder.execute 'some sql' } end end @@ -46,121 +51,196 @@ module ActiveRecord assert_equal 1, @recorder.commands.length end - def test_inverse - @recorder.record :create_table, [:system_settings] - assert_equal 1, @recorder.inverse.length - - @recorder.record :rename_table, [:old, :new] - assert_equal 2, @recorder.inverse.length + def test_inverted_commands_are_reversed + @recorder.revert do + @recorder.record :create_table, [:hello] + @recorder.record :create_table, [:world] + end + tables = @recorder.commands.map{|_cmd, args, _block| args} + assert_equal [[:world], [:hello]], tables end - def test_inverted_commands_are_reveresed - @recorder.record :create_table, [:hello] - @recorder.record :create_table, [:world] - tables = @recorder.inverse.map(&:last) - assert_equal [[:world], [:hello]], tables + def test_revert_order + block = Proc.new{|t| t.string :name } + @recorder.instance_eval do + create_table("apples", &block) + revert do + create_table("bananas", &block) + revert do + create_table("clementines", &block) + create_table("dates") + end + create_table("elderberries") + end + revert do + create_table("figs", &block) + create_table("grapes") + end + end + assert_equal [[:create_table, ["apples"], block], [:drop_table, ["elderberries"], nil], + [:create_table, ["clementines"], block], [:create_table, ["dates"], nil], + [:drop_table, ["bananas"], block], [:drop_table, ["grapes"], nil], + [:drop_table, ["figs"], block]], @recorder.commands + end + + def test_invert_change_table + @recorder.revert do + @recorder.change_table :fruits do |t| + t.string :name + t.rename :kind, :cultivar + end + end + assert_equal [ + [:rename_column, [:fruits, :cultivar, :kind]], + [:remove_column, [:fruits, :name, :string, {}], nil], + ], @recorder.commands + + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.revert do + @recorder.change_table :fruits do |t| + t.remove :kind + end + end + end end def test_invert_create_table - @recorder.record :create_table, [:system_settings] - drop_table = @recorder.inverse.first - assert_equal [:drop_table, [:system_settings]], drop_table + @recorder.revert do + @recorder.record :create_table, [:system_settings] + end + drop_table = @recorder.commands.first + assert_equal [:drop_table, [:system_settings], nil], drop_table + end + + def test_invert_create_table_with_options_and_block + block = Proc.new{} + drop_table = @recorder.inverse_of :create_table, [:people_reminders, id: false], &block + assert_equal [:drop_table, [:people_reminders, id: false], block], drop_table + end + + def test_invert_drop_table + block = Proc.new{} + create_table = @recorder.inverse_of :drop_table, [:people_reminders, id: false], &block + assert_equal [:create_table, [:people_reminders, id: false], block], create_table end - def test_invert_create_table_with_options - @recorder.record :create_table, [:people_reminders, {:id => false}] - drop_table = @recorder.inverse.first - assert_equal [:drop_table, [:people_reminders]], drop_table + def test_invert_drop_table_without_a_block_nor_option + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.inverse_of :drop_table, [:people_reminders] + end end def test_invert_create_join_table - @recorder.record :create_join_table, [:musics, :artists] - drop_table = @recorder.inverse.first - assert_equal [:drop_table, [:artists_musics]], drop_table + drop_join_table = @recorder.inverse_of :create_join_table, [:musics, :artists] + assert_equal [:drop_join_table, [:musics, :artists], nil], drop_join_table end def test_invert_create_join_table_with_table_name - @recorder.record :create_join_table, [:musics, :artists, {:table_name => :catalog}] - drop_table = @recorder.inverse.first - assert_equal [:drop_table, [:catalog]], drop_table + drop_join_table = @recorder.inverse_of :create_join_table, [:musics, :artists, table_name: :catalog] + assert_equal [:drop_join_table, [:musics, :artists, table_name: :catalog], nil], drop_join_table + end + + def test_invert_drop_join_table + block = Proc.new{} + create_join_table = @recorder.inverse_of :drop_join_table, [:musics, :artists, table_name: :catalog], &block + assert_equal [:create_join_table, [:musics, :artists, table_name: :catalog], block], create_join_table end def test_invert_rename_table - @recorder.record :rename_table, [:old, :new] - rename = @recorder.inverse.first + rename = @recorder.inverse_of :rename_table, [:old, :new] assert_equal [:rename_table, [:new, :old]], rename end def test_invert_add_column - @recorder.record :add_column, [:table, :column, :type, {}] - remove = @recorder.inverse.first - assert_equal [:remove_column, [:table, :column]], remove + remove = @recorder.inverse_of :add_column, [:table, :column, :type, {}] + assert_equal [:remove_column, [:table, :column, :type, {}], nil], remove + end + + def test_invert_remove_column + add = @recorder.inverse_of :remove_column, [:table, :column, :type, {}] + assert_equal [:add_column, [:table, :column, :type, {}], nil], add + end + + def test_invert_remove_column_without_type + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.inverse_of :remove_column, [:table, :column] + end end def test_invert_rename_column - @recorder.record :rename_column, [:table, :old, :new] - rename = @recorder.inverse.first + rename = @recorder.inverse_of :rename_column, [:table, :old, :new] assert_equal [:rename_column, [:table, :new, :old]], rename end def test_invert_add_index - @recorder.record :add_index, [:table, [:one, :two], {:options => true}] - remove = @recorder.inverse.first - assert_equal [:remove_index, [:table, {:column => [:one, :two]}]], remove + remove = @recorder.inverse_of :add_index, [:table, [:one, :two], options: true] + assert_equal [:remove_index, [:table, {column: [:one, :two], options: true}]], remove end def test_invert_add_index_with_name - @recorder.record :add_index, [:table, [:one, :two], {:name => "new_index"}] - remove = @recorder.inverse.first - assert_equal [:remove_index, [:table, {:name => "new_index"}]], remove + remove = @recorder.inverse_of :add_index, [:table, [:one, :two], name: "new_index"] + assert_equal [:remove_index, [:table, {column: [:one, :two], name: "new_index"}]], remove end def test_invert_add_index_with_no_options - @recorder.record :add_index, [:table, [:one, :two]] - remove = @recorder.inverse.first - assert_equal [:remove_index, [:table, {:column => [:one, :two]}]], remove + remove = @recorder.inverse_of :add_index, [:table, [:one, :two]] + assert_equal [:remove_index, [:table, {column: [:one, :two]}]], remove + end + + def test_invert_remove_index + add = @recorder.inverse_of :remove_index, [:table, {column: [:one, :two], options: true}] + assert_equal [:add_index, [:table, [:one, :two], options: true]], add + end + + def test_invert_remove_index_with_name + add = @recorder.inverse_of :remove_index, [:table, {column: [:one, :two], name: "new_index"}] + assert_equal [:add_index, [:table, [:one, :two], name: "new_index"]], add + end + + def test_invert_remove_index_with_no_special_options + add = @recorder.inverse_of :remove_index, [:table, {column: [:one, :two]}] + assert_equal [:add_index, [:table, [:one, :two], {}]], add + end + + def test_invert_remove_index_with_no_column + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.inverse_of :remove_index, [:table, name: "new_index"] + end end def test_invert_rename_index - @recorder.record :rename_index, [:table, :old, :new] - rename = @recorder.inverse.first + rename = @recorder.inverse_of :rename_index, [:table, :old, :new] assert_equal [:rename_index, [:table, :new, :old]], rename end def test_invert_add_timestamps - @recorder.record :add_timestamps, [:table] - remove = @recorder.inverse.first - assert_equal [:remove_timestamps, [:table]], remove + remove = @recorder.inverse_of :add_timestamps, [:table] + assert_equal [:remove_timestamps, [:table], nil], remove end def test_invert_remove_timestamps - @recorder.record :remove_timestamps, [:table] - add = @recorder.inverse.first - assert_equal [:add_timestamps, [:table]], add + add = @recorder.inverse_of :remove_timestamps, [:table] + assert_equal [:add_timestamps, [:table], nil], add end def test_invert_add_reference - @recorder.record :add_reference, [:table, :taggable, { polymorphic: true }] - remove = @recorder.inverse.first - assert_equal [:remove_reference, [:table, :taggable, { polymorphic: true }]], remove + remove = @recorder.inverse_of :add_reference, [:table, :taggable, { polymorphic: true }] + assert_equal [:remove_reference, [:table, :taggable, { polymorphic: true }], nil], remove end def test_invert_add_belongs_to_alias - @recorder.record :add_belongs_to, [:table, :user] - remove = @recorder.inverse.first - assert_equal [:remove_reference, [:table, :user]], remove + remove = @recorder.inverse_of :add_belongs_to, [:table, :user] + assert_equal [:remove_reference, [:table, :user], nil], remove end def test_invert_remove_reference - @recorder.record :remove_reference, [:table, :taggable, { polymorphic: true }] - add = @recorder.inverse.first - assert_equal [:add_reference, [:table, :taggable, { polymorphic: true }]], add + add = @recorder.inverse_of :remove_reference, [:table, :taggable, { polymorphic: true }] + assert_equal [:add_reference, [:table, :taggable, { polymorphic: true }], nil], add end def test_invert_remove_belongs_to_alias - @recorder.record :remove_belongs_to, [:table, :user] - add = @recorder.inverse.first - assert_equal [:add_reference, [:table, :user]], add + add = @recorder.inverse_of :remove_belongs_to, [:table, :user] + assert_equal [:add_reference, [:table, :user], nil], add end end end diff --git a/activerecord/test/cases/migration/create_join_table_test.rb b/activerecord/test/cases/migration/create_join_table_test.rb index f262bbaad7..c099854ad8 100644 --- a/activerecord/test/cases/migration/create_join_table_test.rb +++ b/activerecord/test/cases/migration/create_join_table_test.rb @@ -78,6 +78,48 @@ module ActiveRecord assert_equal [%w(artist_id music_id)], connection.indexes(:artists_musics).map(&:columns) end + + def test_drop_join_table + connection.create_join_table :artists, :musics + connection.drop_join_table :artists, :musics + + assert !connection.tables.include?('artists_musics') + end + + def test_drop_join_table_with_strings + connection.create_join_table :artists, :musics + connection.drop_join_table 'artists', 'musics' + + assert !connection.tables.include?('artists_musics') + end + + def test_drop_join_table_with_the_proper_order + connection.create_join_table :videos, :musics + connection.drop_join_table :videos, :musics + + assert !connection.tables.include?('musics_videos') + end + + def test_drop_join_table_with_the_table_name + connection.create_join_table :artists, :musics, table_name: :catalog + connection.drop_join_table :artists, :musics, table_name: :catalog + + assert !connection.tables.include?('catalog') + end + + def test_drop_join_table_with_the_table_name_as_string + connection.create_join_table :artists, :musics, table_name: 'catalog' + connection.drop_join_table :artists, :musics, table_name: 'catalog' + + assert !connection.tables.include?('catalog') + end + + def test_create_join_table_with_column_options + connection.create_join_table :artists, :musics, column_options: {null: true} + connection.drop_join_table :artists, :musics, column_options: {null: true} + + assert !connection.tables.include?('artists_musics') + end end end end |