diff --git a/lib/schema_plus.rb b/lib/schema_plus.rb index a2c7d69..79bf079 100644 --- a/lib/schema_plus.rb +++ b/lib/schema_plus.rb @@ -13,6 +13,7 @@ require 'schema_plus/active_record/connection_adapters/column' require 'schema_plus/active_record/connection_adapters/foreign_key_definition' require 'schema_plus/active_record/connection_adapters/index_definition' +require 'schema_plus/active_record/migration/command_recorder' require 'schema_plus/railtie' if defined?(Rails::Railtie) module SchemaPlus @@ -124,6 +125,7 @@ def self.insert_connection_adapters #:nodoc: ::ActiveRecord::ConnectionAdapters::IndexDefinition.send(:include, SchemaPlus::ActiveRecord::ConnectionAdapters::IndexDefinition) ::ActiveRecord::ConnectionAdapters::SchemaStatements.send(:include, SchemaPlus::ActiveRecord::ConnectionAdapters::SchemaStatements) ::ActiveRecord::ConnectionAdapters::TableDefinition.send(:include, SchemaPlus::ActiveRecord::ConnectionAdapters::TableDefinition) + ::ActiveRecord::Migration::CommandRecorder.send(:include, SchemaPlus::ActiveRecord::Migration::CommandRecorder) if "#{::ActiveRecord::VERSION::MAJOR}.#{::ActiveRecord::VERSION::MINOR}".to_r >= "4.1".to_r ::ActiveRecord::ConnectionAdapters::AbstractAdapter::SchemaCreation.send(:include, SchemaPlus::ActiveRecord::ConnectionAdapters::AbstractAdapter::AddColumnOptions) diff --git a/lib/schema_plus/active_record/column_options_handler.rb b/lib/schema_plus/active_record/column_options_handler.rb index ae06022..d9b7c96 100644 --- a/lib/schema_plus/active_record/column_options_handler.rb +++ b/lib/schema_plus/active_record/column_options_handler.rb @@ -73,7 +73,7 @@ def get_fk_args(table_name, column_name, column_options = {}, config = {}) #:nod def remove_foreign_key_if_exists(table_name, column_name) #:nodoc: foreign_keys = ActiveRecord::Base.connection.foreign_keys(table_name.to_s) rescue [] # no fks if table_name doesn't exist fk = foreign_keys.detect { |fk| fk.table_name == table_name.to_s && fk.column_names == Array(column_name).collect(&:to_s) } - remove_foreign_key(table_name, fk.name) if fk + remove_foreign_key(table_name, fk.column_names, fk.references_table_name, fk.references_column_names) if fk end @@ -86,7 +86,7 @@ def column_index(table_name, column_name, options) #:nodoc: def remove_auto_index_if_exists(table_name, column_name) name = auto_index_name(table_name, column_name) - remove_index(table_name, :name => name) if index_exists?(table_name, column_name, :name => name) + remove_index(table_name, :name => name, :column => column_name, :if_exists => true) end def auto_index_name(table_name, column_name) diff --git a/lib/schema_plus/active_record/connection_adapters/abstract_adapter.rb b/lib/schema_plus/active_record/connection_adapters/abstract_adapter.rb index b0a511e..8894073 100644 --- a/lib/schema_plus/active_record/connection_adapters/abstract_adapter.rb +++ b/lib/schema_plus/active_record/connection_adapters/abstract_adapter.rb @@ -13,6 +13,7 @@ module ConnectionAdapters module AbstractAdapter def self.included(base) #:nodoc: base.alias_method_chain :initialize, :schema_plus + base.alias_method_chain :remove_index, :schema_plus end def initialize_with_schema_plus(*args) #:nodoc: @@ -64,8 +65,19 @@ def drop_view(view_name) # it's created. If you're using Sqlite3, this method will raise an # error.) def add_foreign_key(table_name, column_names, references_table_name, references_column_names, options = {}) - foreign_key = ForeignKeyDefinition.new(options[:name] || ForeignKeyDefinition.default_name(table_name, column_names), table_name, column_names, ::ActiveRecord::Migrator.proper_table_name(references_table_name), references_column_names, options[:on_update], options[:on_delete], options[:deferrable]) - execute "ALTER TABLE #{quote_table_name(table_name)} ADD #{foreign_key.to_sql}" + foreign_key_sql = add_foreign_key_sql(table_name, column_names, references_table_name, references_column_names, options) + execute "ALTER TABLE #{quote_table_name(table_name)} #{foreign_key_sql}" + end + + # called directly by AT's bulk_change_table, for migration + # change_table :name, :bulk => true { ... } + def add_foreign_key_sql(table_name, column_names, references_table_name, references_column_names, options = {}) #:nodoc: + foreign_key = _build_foreign_key(table_name, column_names, references_table_name, references_column_names, options) + "ADD #{foreign_key.to_sql}" + end + + def _build_foreign_key(table_name, column_names, references_table_name, references_column_names, options = {}) #:nodoc: + ForeignKeyDefinition.new(options[:name] || ForeignKeyDefinition.default_name(table_name, column_names), table_name, column_names, ::ActiveRecord::Migrator.proper_table_name(references_table_name), references_column_names, options[:on_update], options[:on_delete], options[:deferrable]) end # Remove a foreign key constraint @@ -73,8 +85,28 @@ def add_foreign_key(table_name, column_names, references_table_name, references_ # (NOTE: Sqlite3 does not support altering a table to remove # foreign-key constraints. If you're using Sqlite3, this method will # raise an error.) - def remove_foreign_key(table_name, foreign_key_name) - execute "ALTER TABLE #{quote_table_name(table_name)} DROP CONSTRAINT #{foreign_key_name}" + def remove_foreign_key(table_name, *args) + case sql = remove_foreign_key_sql(table_name, *args) + when String then execute "ALTER TABLE #{quote_table_name(table_name)} #{sql}" + end + end + + def remove_foreign_key_sql(table_name, *args) + column_names, references_table_name, references_column_names, options = args + options ||= {} + foreign_key_name = case + when args.length == 1 then args.first + when options[:name] then options[:name] + else + test_fk = _build_foreign_key(table_name, column_names, references_table_name, references_column_names, options) + if foreign_keys(table_name).detect { |fk| fk == test_fk } + test_fk.name + else + raise "SchemaPlus: no foreign key constraint found on #{table_name.inspect} matching #{args.inspect}" unless options[:if_exists] + nil + end + end + foreign_key_name ? "DROP CONSTRAINT #{foreign_key_name}" : [] # hack -- return empty array rather than nil, so that result will disappear when caller flattens but doesn't compact end # Extends rails' drop_table to include these options: @@ -89,6 +121,14 @@ def drop_table(name, options = {}) execute sql end + # Extends rails' remove_index to include this options: + # :if_exists + def remove_index_with_schema_plus(table_name, options={}) + return if options.delete(:if_exists) and not index_name_exists?(table_name, options[:name] || index_name(table_name, options), false) + options.delete(:column) if options[:name] and ::ActiveRecord::VERSION::MAJOR < 4 + remove_index_without_schema_plus(table_name, options) + end + # called from individual adpaters, after renaming table from old # name to def rename_indexes_and_foreign_keys(oldname, newname) #:nodoc: diff --git a/lib/schema_plus/active_record/connection_adapters/foreign_key_definition.rb b/lib/schema_plus/active_record/connection_adapters/foreign_key_definition.rb index 7e6a5c6..ea1dba2 100644 --- a/lib/schema_plus/active_record/connection_adapters/foreign_key_definition.rb +++ b/lib/schema_plus/active_record/connection_adapters/foreign_key_definition.rb @@ -49,9 +49,9 @@ class ForeignKeyDefinition def initialize(name, table_name, column_names, references_table_name, references_column_names, on_update = nil, on_delete = nil, deferrable = nil) @name = name @table_name = unquote(table_name) - @column_names = unquote(column_names) + @column_names = unquote(Array.wrap(column_names)) @references_table_name = unquote(references_table_name) - @references_column_names = unquote(references_column_names) + @references_column_names = unquote(Array.wrap(references_column_names)) @on_update = on_update @on_delete = on_delete @deferrable = deferrable @@ -124,6 +124,13 @@ def self.fixup_schema_name(table_name) table_name.to_s.gsub(/[.]/, '_') end + def ==(other) # note equality test ignores :name and options + [:table_name, + :column_names, + :references_table_name, + :references_column_names + ].all? { |attr| self.send(attr) == other.send(attr) } + end end end end diff --git a/lib/schema_plus/active_record/connection_adapters/mysql_adapter.rb b/lib/schema_plus/active_record/connection_adapters/mysql_adapter.rb index ecdc880..52982a0 100644 --- a/lib/schema_plus/active_record/connection_adapters/mysql_adapter.rb +++ b/lib/schema_plus/active_record/connection_adapters/mysql_adapter.rb @@ -27,11 +27,15 @@ def tables_with_schema_plus(name=nil, *args) tables_without_schema_plus(name, *args) - views(name) end - def remove_column_with_schema_plus(table_name, column_name) + def remove_column_with_schema_plus(table_name, column_name, type=nil, options={}) foreign_keys(table_name).select { |foreign_key| foreign_key.column_names.include?(column_name.to_s) }.each do |foreign_key| remove_foreign_key(table_name, foreign_key.name) end - remove_column_without_schema_plus(table_name, column_name) + if ::ActiveRecord::VERSION::MAJOR.to_i >= 4 + remove_column_without_schema_plus(table_name, column_name, type, options) + else + remove_column_without_schema_plus(table_name, column_name) + end end def rename_table_with_schema_plus(oldname, newname) @@ -59,8 +63,22 @@ def drop_table(name, options={}) super end - def remove_foreign_key(table_name, foreign_key_name, options = {}) - execute "ALTER TABLE #{quote_table_name(table_name)} DROP FOREIGN KEY #{foreign_key_name}" + def remove_index_sql(table_name, options) + return [] if options.delete(:if_exists) and not index_exists?(table_name, options) + super + end + + def remove_foreign_key_sql(table_name, *args) + case ret = super + when String then ret.sub(/DROP CONSTRAINT/, 'DROP FOREIGN KEY') + else ret + end + end + + def remove_foreign_key(table_name, *args) + case sql = remove_foreign_key_sql(table_name, *args) + when String then execute "ALTER TABLE #{quote_table_name(table_name)} #{sql}" + end end def foreign_keys(table_name, name = nil) diff --git a/lib/schema_plus/active_record/connection_adapters/schema_statements.rb b/lib/schema_plus/active_record/connection_adapters/schema_statements.rb index 8e77a02..08cdbb4 100644 --- a/lib/schema_plus/active_record/connection_adapters/schema_statements.rb +++ b/lib/schema_plus/active_record/connection_adapters/schema_statements.rb @@ -76,6 +76,7 @@ def self.included(base) #:nodoc: # an error.) # def add_index_with_schema_plus(table, columns, options={}) + options.delete(:if_exists) add_index_without_schema_plus(table, columns, options) rescue => e SchemaStatements.add_index_exception_handler(self, table, columns, options, e) diff --git a/lib/schema_plus/active_record/connection_adapters/table_definition.rb b/lib/schema_plus/active_record/connection_adapters/table_definition.rb index 96b4f33..dd1c0e9 100644 --- a/lib/schema_plus/active_record/connection_adapters/table_definition.rb +++ b/lib/schema_plus/active_record/connection_adapters/table_definition.rb @@ -171,23 +171,14 @@ def add_foreign_key(_, *args) #:nodoc: def remove_foreign_key(_, *args) #:nodoc: end - # This is a deliberately empty stub. In fact, verifying that it won't - # ever be called. The reason for it is that ColumnOptionsHandler will - # remove a previous index when changing a column. But we don't do - # column changes within table definitions. - def remove_index(_, options) raise "InternalError: remove_index called in a table definition" ; end - - # Determines if an indexes is queued to be created. Called from - # ColumnOptionsHandler as part of checking whether to auto-create an index - def index_exists?(_, column_name, options={}) - if ::ActiveRecord::VERSION::MAJOR.to_i < 4 - @indexes.find{|index| index.table == self.name && index.columns == Array.wrap(column_name) && options.all?{|k, v| index.send(k) == v}} - else - @indexes.find {|index_column_name, index_options| Array.wrap(index_column_name) == Array.wrap(column_name) && options.all?{|k, v| index_options[k] == v} } - end + # This is a deliberately empty stub. The reason for it is that + # ColumnOptionsHandler will remove a previous index when changing a + # column. But we don't do column changes within table definitions. + # Presumably will be called with :if_exists true. If not, will raise + # an error. + def remove_index(_, options) + raise "InternalError: remove_index called in a table definition" unless options[:if_exists] end - - end end diff --git a/lib/schema_plus/active_record/migration/command_recorder.rb b/lib/schema_plus/active_record/migration/command_recorder.rb new file mode 100644 index 0000000..35f76a1 --- /dev/null +++ b/lib/schema_plus/active_record/migration/command_recorder.rb @@ -0,0 +1,88 @@ +module SchemaPlus + module ActiveRecord + module Migration + module CommandRecorder + include SchemaPlus::ActiveRecord::ColumnOptionsHandler + + attr_accessor :schema_plus_config #:nodoc: + + def self.included(base) #:nodoc: + base.class_eval do + alias_method_chain :add_column, :schema_plus + alias_method_chain :add_reference, :schema_plus unless ::ActiveRecord::VERSION::MAJOR.to_i < 4 + alias_method_chain :invert_add_index, :schema_plus + end + end + + def add_column_with_schema_plus(table_name, name, type, options = {}) #:nodoc: + add_column_without_schema_plus(table_name, name, type, options) + revertable_schema_plus_handle_column_options(table_name, name, options, :config => schema_plus_config) + self + end + + # seems like this is fixing a rails bug: + # change_table foo, :bulk => true { |t| t.references :bar } + # results in an 'unknown method :add_reference_sql' (with mysql2) + # + # should track it down separately and submit a patch/fix to rails + # + def add_reference_with_schema_plus(table_name, ref_name, options = {}) #:nodoc: + # which is the worse hack...? + if RUBY_VERSION >= "2.0.0" + # .. rebinding a method from a different module? (can't do this in ruby 1.9.3) + ::ActiveRecord::ConnectionAdapters::SchemaStatements.instance_method(:add_reference).bind(self).call(table_name, ref_name, options) + else + # .. or copying and pasting the code? + polymorphic = options.delete(:polymorphic) + index_options = options.delete(:index) + add_column(table_name, "#{ref_name}_id", :integer, options) + add_column(table_name, "#{ref_name}_type", :string, polymorphic.is_a?(Hash) ? polymorphic : options) if polymorphic + add_index(table_name, polymorphic ? %w[id type].map{ |t| "#{ref_name}_#{t}" } : "#{ref_name}_id", index_options.is_a?(Hash) ? index_options : nil) if index_options + end + + self + end + + if ::ActiveRecord::VERSION::MAJOR >= 4 + def revertable_schema_plus_handle_column_options(table_name, name, options, config) + length = commands.length + schema_plus_handle_column_options(table_name, name, options, config) + if reverting + rev = [] + while commands.length > length + cmd = commands.pop + rev.unshift cmd unless cmd[0].to_s =~ /^add_/ + end + commands.concat rev + end + end + else + alias :revertable_schema_plus_handle_column_options :schema_plus_handle_column_options + end + + def add_foreign_key(*args) + record(:add_foreign_key, args) + end + + def invert_add_foreign_key(args) + table_name, column_names, references_table_name, references_column_names, options = args + [:remove_foreign_key, [table_name, column_names, references_table_name, references_column_names, (options||{}).merge(if_exists: true)]] + end + + def invert_add_index_with_schema_plus(args) + table, columns, options = *args + [:remove_index, [table, (options||{}).merge(column: columns, if_exists: true)]] + end + + def remove_foreign_key(*args) + record(:remove_foreign_key, args) + end + + def invert_remove_foreign_key(args) + [:add_foreign_key, args] + end + + end + end + end +end diff --git a/spec/migration_spec.rb b/spec/migration_spec.rb index f6b14e3..c6ce8af 100644 --- a/spec/migration_spec.rb +++ b/spec/migration_spec.rb @@ -386,6 +386,66 @@ class Comment < ::ActiveRecord::Base ; end end + context "when table is changed" do + before(:each) do + @model = Post + end + [false, true].each do |bulk| + suffix = bulk ? ' with :bulk option' : "" + + it "should create an index if specified on column"+suffix do + change_table(@model, :bulk => bulk) do |t| + t.integer :state, :index => true + end + @model.should have_index.on(:state) + end + + unless SchemaPlusHelpers.sqlite3? + + it "should create a foreign key constraint"+suffix do + change_table(@model, :bulk => bulk) do |t| + t.integer :user_id + end + @model.should reference(:users, :id).on(:user_id) + end + + context "migrate down" do + it "should remove a foreign key constraint"+suffix do + Comment.reset_column_information + Comment.should reference(:users, :id).on(:user_id) + migration = Class.new ::ActiveRecord::Migration do + define_method(:change) { + change_table("comments", :bulk => bulk) do |t| + t.integer :user_id + end + } + end + ActiveRecord::Migration.suppress_messages do + migration.migrate(:down) + end + Comment.reset_column_information + Comment.should_not reference(:users, :id).on(:user_id) + end + end if ActiveRecord::VERSION::MAJOR >= 4 + + it "should create a foreign key constraint using :references"+suffix do + change_table(@model, :bulk => bulk) do |t| + t.references :user + end + @model.should reference(:users, :id).on(:user_id) + end + + it "should create a foreign key constraint using :belongs_to"+suffix do + change_table(@model, :bulk => bulk) do |t| + t.belongs_to :user + end + @model.should reference(:users, :id).on(:user_id) + end + end + end + end + + unless SchemaPlusHelpers.sqlite3? context "when column is added" do @@ -736,7 +796,12 @@ def recreate_table(model, opts={}, &block) model.reset_column_information end - + def change_table(model, opts={}, &block) + ActiveRecord::Migration.suppress_messages do + ActiveRecord::Migration.change_table model.table_name, opts, &block + end + model.reset_column_information + end end