Make change_table work properly (with and without :bulk option). Addr…

…esses #127.

To do this, added some new features:
- remove_foreign_key takes table & column args (addresses #129), also :if_exists option
- remove_index takes :if_exists option

(still need to document these, and flesh out specs code coverage)

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)

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)

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,17 +65,48 @@ 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
         #
         # (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:

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

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)

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)

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

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

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