Merge pull request #5262 from joshuap/references_index

Automatically create indexes for references/belongs_to statements in migrations.

8 files changed, 159 insertions, 6 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 85cb3e0e20..44ff403582 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,27 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Added an :index option to automatically create indexes for references
+    and belongs_to statements in migrations.
+
+    The `references` and `belongs_to` methods now support an `index`
+    option that receives either a boolean value or an options hash
+    that is identical to options available to the add_index method:
+
+      create_table :messages do |t|
+        t.references :person, :index => true
+      end
+
+      Is the same as:
+
+      create_table :messages do |t|
+        t.references :person
+      end
+      add_index :messages, :person_id
+
+    Generators have also been updated to use the new syntax.
+
+    [Joshua Wood]
+
 *   Added bang methods for mutating `ActiveRecord::Relation` objects.
     For example, while `foo.where(:bar)` will return a new object
     leaving `foo` unchanged, `foo.where!(:bar)` will mutate the foo
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 7ee8f40631..3546873550 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -65,11 +65,12 @@ module ActiveRecord
     class TableDefinition
       # An array of ColumnDefinition objects, representing the column changes
       # that have been defined.
-      attr_accessor :columns
+      attr_accessor :columns, :indexes
 
       def initialize(base)
         @columns = []
         @columns_hash = {}
+        @indexes = {}
         @base = base
       end
 
@@ -212,19 +213,22 @@ module ActiveRecord
       #
       # TableDefinition#references will add an appropriately-named _id column, plus a corresponding _type
       # column if the <tt>:polymorphic</tt> option is supplied. If <tt>:polymorphic</tt> is a hash of
-      # options, these will be used when creating the <tt>_type</tt> column. So what can be written like this:
+      # options, these will be used when creating the <tt>_type</tt> column. The <tt>:index</tt> option
+      # will also create an index, similar to calling <tt>add_index</tt>. So what can be written like this:
       #
       #   create_table :taggings do |t|
       #     t.integer :tag_id, :tagger_id, :taggable_id
       #     t.string  :tagger_type
       #     t.string  :taggable_type, :default => 'Photo'
       #   end
+      #   add_index :taggings, :tag_id, :name => 'index_taggings_on_tag_id'
+      #   add_index :taggings, [:tagger_id, :tagger_type]
       #
       # Can also be written as follows using references:
       #
       #   create_table :taggings do |t|
-      #     t.references :tag
-      #     t.references :tagger, :polymorphic => true
+      #     t.references :tag, :index => { :name => 'index_taggings_on_tag_id' }
+      #     t.references :tagger, :polymorphic => true, :index => true
       #     t.references :taggable, :polymorphic => { :default => 'Photo' }
       #   end
       def column(name, type, options = {})
@@ -255,6 +259,14 @@ module ActiveRecord
           end                                                         # end
         EOV
       end
+      
+      # Adds index options to the indexes hash, keyed by column name
+      # This is primarily used to track indexes that need to be created after the table
+      # === Examples
+      #   index(:account_id, :name => 'index_projects_on_account_id')
+      def index(column_name, options = {})
+        indexes[column_name] = options
+      end
 
       # Appends <tt>:datetime</tt> columns <tt>:created_at</tt> and
       # <tt>:updated_at</tt> to the table.
@@ -267,9 +279,11 @@ module ActiveRecord
       def references(*args)
         options = args.extract_options!
         polymorphic = options.delete(:polymorphic)
+        index_options = options.delete(:index)
         args.each do |col|
           column("#{col}_id", :integer, options)
           column("#{col}_type", :string, polymorphic.is_a?(Hash) ? polymorphic : options) unless polymorphic.nil?
+          index(polymorphic ? %w(id type).map { |t| "#{col}_#{t}" } : "#{col}_id", index_options.is_a?(Hash) ? index_options : nil) if index_options
         end
       end
       alias :belongs_to :references
@@ -435,9 +449,11 @@ module ActiveRecord
       def references(*args)
         options = args.extract_options!
         polymorphic = options.delete(:polymorphic)
+        index_options = options.delete(:index)
         args.each do |col|
           @base.add_column(@table_name, "#{col}_id", :integer, options)
           @base.add_column(@table_name, "#{col}_type", :string, polymorphic.is_a?(Hash) ? polymorphic : options) unless polymorphic.nil?
+          @base.add_index(@table_name, polymorphic ? %w(id type).map { |t| "#{col}_#{t}" } : "#{col}_id", index_options.is_a?(Hash) ? index_options : nil) if index_options
         end
       end
       alias :belongs_to :references
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 0784b2d11a..10b3657ced 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -171,6 +171,7 @@ module ActiveRecord
         create_sql << td.to_sql
         create_sql << ") #{options[:options]}"
         execute create_sql
+        td.indexes.each_pair { |c,o| add_index table_name, c, o }
       end
 
       # Creates a new join table with the name created using the lexical order of the first two
diff --git a/activerecord/lib/rails/generators/active_record/model/model_generator.rb b/activerecord/lib/rails/generators/active_record/model/model_generator.rb
index f3bb70fb41..8e6ef20285 100644
--- a/activerecord/lib/rails/generators/active_record/model/model_generator.rb
+++ b/activerecord/lib/rails/generators/active_record/model/model_generator.rb
@@ -14,6 +14,7 @@ module ActiveRecord
 
       def create_migration_file
         return unless options[:migration] && options[:parent].nil?
+        attributes.each { |a| a.attr_options.delete(:index) if a.reference? && !a.has_index? } if options[:indexes] == false
         migration_template "migration.rb", "db/migrate/create_#{table_name}.rb"
       end
 
@@ -27,7 +28,7 @@ module ActiveRecord
       end
 
       def attributes_with_index
-        attributes.select { |a| a.has_index? || (a.reference? && options[:indexes]) }
+        attributes.select { |a| !a.reference? && a.has_index? }
       end
 
       def accessible_attributes
diff --git a/activerecord/test/cases/migration/references_index_test.rb b/activerecord/test/cases/migration/references_index_test.rb
new file mode 100644
index 0000000000..8ab1c59724
--- /dev/null
+++ b/activerecord/test/cases/migration/references_index_test.rb
@@ -0,0 +1,99 @@
+require 'cases/helper'
+
+module ActiveRecord
+  class Migration
+    class ReferencesIndexTest < ActiveRecord::TestCase
+      attr_reader :connection, :table_name
+
+      def setup
+        super
+        @connection = ActiveRecord::Base.connection
+        @table_name = :testings
+      end
+
+      def teardown
+        super
+        connection.drop_table :testings rescue nil
+      end
+
+      def test_creates_index
+        connection.create_table table_name do |t|
+          t.references :foo, :index => true
+        end
+
+        assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+      end
+
+      def test_does_not_create_index
+        connection.create_table table_name do |t|
+          t.references :foo
+        end
+
+        refute connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+      end
+
+      def test_does_not_create_index_explicit
+        connection.create_table table_name do |t|
+          t.references :foo, :index => false
+        end
+
+        refute connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+      end
+
+      def test_creates_index_with_options
+        connection.create_table table_name do |t|
+          t.references :foo, :index => {:name => :index_testings_on_yo_momma}
+          t.references :bar, :index => {:unique => true}
+        end
+
+        assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_yo_momma)
+        assert connection.index_exists?(table_name, :bar_id, :name => :index_testings_on_bar_id, :unique => true)
+      end
+
+      def test_creates_polymorphic_index
+        connection.create_table table_name do |t|
+          t.references :foo, :polymorphic => true, :index => true
+        end
+
+        assert connection.index_exists?(table_name, [:foo_id, :foo_type], :name => :index_testings_on_foo_id_and_foo_type)
+      end
+
+      def test_creates_index_for_existing_table
+        connection.create_table table_name
+        connection.change_table table_name do |t|
+          t.references :foo, :index => true
+        end
+
+        assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+      end
+
+      def test_does_not_create_index_for_existing_table
+        connection.create_table table_name
+        connection.change_table table_name do |t|
+          t.references :foo
+        end
+
+        refute connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+      end
+
+      def test_does_not_create_index_for_existing_table_explicit
+        connection.create_table table_name
+        connection.change_table table_name do |t|
+          t.references :foo, :index => false
+        end
+
+        refute connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+      end
+
+      def test_creates_polymorphic_index_for_existing_table
+        connection.create_table table_name
+        connection.change_table table_name do |t|
+          t.references :foo, :polymorphic => true, :index => true
+        end
+
+        assert connection.index_exists?(table_name, [:foo_id, :foo_type], :name => :index_testings_on_foo_id_and_foo_type)
+      end
+
+    end
+  end
+end
diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb
index e2936963d9..e14c2d072c 100644
--- a/activerecord/test/cases/migration_test.rb
+++ b/activerecord/test/cases/migration_test.rb
@@ -404,6 +404,7 @@ end
 class ChangeTableMigrationsTest < ActiveRecord::TestCase
   def setup
     @connection = Person.connection
+    @connection.stubs(:add_index)
     @connection.create_table :delete_me, :force => true do |t|
     end
   end
diff --git a/guides/source/migrations.textile b/guides/source/migrations.textile
index f663496854..aa75e9ab4a 100644
--- a/guides/source/migrations.textile
+++ b/guides/source/migrations.textile
@@ -475,7 +475,16 @@ end
 </ruby>
 
 will add an +attachment_id+ column and a string +attachment_type+ column with
-a default value of 'Photo'.
+a default value of 'Photo'. +references+ also allows you to define an
+index directly, instead of using +add_index+ after the +create_table+ call:
+
+<ruby>
+create_table :products do |t|
+  t.references :category, :index => true
+end
+</ruby>
+
+will create an index identical to calling `add_index :products, :category_id`.
 
 NOTE: The +references+ helper does not actually create foreign key constraints
 for you. You will need to use +execute+ or a plugin that adds "foreign key
diff --git a/railties/lib/rails/generators/generated_attribute.rb b/railties/lib/rails/generators/generated_attribute.rb
index 7dfc1aa599..7296068f04 100644
--- a/railties/lib/rails/generators/generated_attribute.rb
+++ b/railties/lib/rails/generators/generated_attribute.rb
@@ -21,6 +21,10 @@ module Rails
           has_index, type = type, nil if INDEX_OPTIONS.include?(type)
 
           type, attr_options = *parse_type_and_options(type)
+
+          references_index = (type.in?(%w(references belongs_to)) and UNIQ_INDEX_OPTIONS.include?(has_index) ? {:unique => true} : true)
+          attr_options.merge!({:index => references_index}) if references_index
+
           new(name, type, has_index, attr_options)
         end