Merge pull request #8613 from senny/8264_character_limit_for_indices

deal with long index names and internal sqlite3 operations

6 files changed, 69 insertions, 14 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 6098c2c425..f45b9219c7 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,13 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Prevent the creation of indices with too long names, which cause
+    internal operations to fail (sqlite3 adapter only). The method
+    `allowed_index_name_length` defines the length limit enforced by
+    rails. It's value defaults to `index_name_length` but can vary per adapter.
+    Fix #8264.
+
+    *Yves Senn*
+
 *   Fixing issue #776.
 
     Memory bloat in transactions is handled by having the transaction hold only
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_limits.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_limits.rb
index 30ccb8f0a4..2859fb31e8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_limits.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_limits.rb
@@ -17,6 +17,15 @@ module ActiveRecord
         64
       end
 
+      # Returns the maximum allowed length for an index name. This
+      # limit is enforced by rails and Is less than or equal to
+      # <tt>index_name_length</tt>. The gap between
+      # <tt>index_name_length</tt> is to allow internal rails
+      # opreations to use prefixes in temporary opreations.
+      def allowed_index_name_length
+        index_name_length
+      end
+
       # Returns the maximum length of an index name.
       def index_name_length
         64
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 cdc8433185..2c0a18fd01 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -647,10 +647,11 @@ module ActiveRecord
           index_name   = index_name(table_name, column: column_names)
 
           if Hash === options # legacy support, since this param was a string
-            options.assert_valid_keys(:unique, :order, :name, :where, :length)
+            options.assert_valid_keys(:unique, :order, :name, :where, :length, :internal)
 
             index_type = options[:unique] ? "UNIQUE" : ""
             index_name = options[:name].to_s if options.key?(:name)
+            max_index_length = options.fetch(:internal, false) ? index_name_length : allowed_index_name_length
 
             if supports_partial_index?
               index_options = options[:where] ? " WHERE #{options[:where]}" : ""
@@ -665,10 +666,11 @@ module ActiveRecord
             end
 
             index_type = options
+            max_index_length = allowed_index_name_length
           end
 
-          if index_name.length > index_name_length
-            raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' is too long; the limit is #{index_name_length} characters"
+          if index_name.length > max_index_length
+            raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' is too long; the limit is #{max_index_length} characters"
           end
           if index_name_exists?(table_name, index_name, false)
             raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' already exists"
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index 91444950be..105ba69028 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -187,6 +187,13 @@ module ActiveRecord
         true
       end
 
+      # Returns 62. SQLite supports index names up to 64
+      # characters. The rest is used by rails internally to perform
+      # temporary rename operations
+      def allowed_index_name_length
+        index_name_length - 2
+      end
+
       def native_database_types #:nodoc:
         {
           :primary_key => default_primary_key_type,
@@ -502,7 +509,7 @@ module ActiveRecord
         end
 
         def alter_table(table_name, options = {}) #:nodoc:
-          altered_table_name = "altered_#{table_name}"
+          altered_table_name = "a#{table_name}"
           caller = lambda {|definition| yield definition if block_given?}
 
           transaction do
@@ -546,10 +553,10 @@ module ActiveRecord
         def copy_table_indexes(from, to, rename = {}) #:nodoc:
           indexes(from).each do |index|
             name = index.name
-            if to == "altered_#{from}"
-              name = "temp_#{name}"
-            elsif from == "altered_#{to}"
-              name = name[5..-1]
+            if to == "a#{from}"
+              name = "t#{name}"
+            elsif from == "a#{to}"
+              name = name[1..-1]
             end
 
             to_column_names = columns(to).map { |c| c.name }
@@ -559,7 +566,7 @@ module ActiveRecord
 
             unless columns.empty?
               # index name can't be the same
-              opts = { name: name.gsub(/(^|_)(#{from})_/, "\\1#{to}_") }
+              opts = { name: name.gsub(/(^|_)(#{from})_/, "\\1#{to}_"), internal: true }
               opts[:unique] = true if index.unique
               add_index(to, columns, opts)
             end
diff --git a/activerecord/test/cases/migration/index_test.rb b/activerecord/test/cases/migration/index_test.rb
index a41f2c10f0..5df9a5342d 100644
--- a/activerecord/test/cases/migration/index_test.rb
+++ b/activerecord/test/cases/migration/index_test.rb
@@ -55,19 +55,31 @@ module ActiveRecord
         assert_raise(ArgumentError) { connection.remove_index(table_name, "no_such_index") }
       end
 
-      def test_add_index_name_length_limit
-        good_index_name = 'x' * connection.index_name_length
+      def test_add_index_works_with_long_index_names
+        connection.add_index(table_name, "foo", name: good_index_name)
+
+        assert connection.index_name_exists?(table_name, good_index_name, false)
+        connection.remove_index(table_name, name: good_index_name)
+      end
+
+      def test_add_index_does_not_accept_too_long_index_names
         too_long_index_name = good_index_name + 'x'
 
-        assert_raises(ArgumentError) {
-          connection.add_index(table_name, "foo", :name => too_long_index_name)
+        e = assert_raises(ArgumentError) {
+          connection.add_index(table_name, "foo", name: too_long_index_name)
         }
+        assert_match /too long; the limit is #{connection.allowed_index_name_length} characters/, e.message
 
         assert_not connection.index_name_exists?(table_name, too_long_index_name, false)
         connection.add_index(table_name, "foo", :name => good_index_name)
+      end
+
+      def test_internal_index_with_name_matching_database_limit
+        good_index_name = 'x' * connection.index_name_length
+        connection.add_index(table_name, "foo", name: good_index_name, internal: true)
 
         assert connection.index_name_exists?(table_name, good_index_name, false)
-        connection.remove_index(table_name, :name => good_index_name)
+        connection.remove_index(table_name, name: good_index_name)
       end
 
       def test_index_symbol_names
@@ -196,6 +208,12 @@ module ActiveRecord
         connection.remove_index("testings", "last_name")
         assert !connection.index_exists?("testings", "last_name")
       end
+
+      private
+        def good_index_name
+          'x' * connection.allowed_index_name_length
+        end
+
     end
   end
 end
diff --git a/activerecord/test/cases/migration/rename_column_test.rb b/activerecord/test/cases/migration/rename_column_test.rb
index 8f6918d06a..b116753d04 100644
--- a/activerecord/test/cases/migration/rename_column_test.rb
+++ b/activerecord/test/cases/migration/rename_column_test.rb
@@ -197,6 +197,17 @@ module ActiveRecord
         assert_equal ['test_models_categories_idx'], connection.indexes('test_models').map(&:name)
       end
 
+      def test_change_column_with_long_index_name
+        table_name_prefix = 'test_models_'
+        long_index_name = table_name_prefix + ('x' * (connection.allowed_index_name_length - table_name_prefix.length))
+        add_column "test_models", "category", :string
+        add_index :test_models, :category, name: long_index_name
+
+        change_column "test_models", "category", :string, null: false, default: 'article'
+
+        assert_equal [long_index_name], connection.indexes('test_models').map(&:name)
+      end
+
       def test_change_column_default
         add_column "test_models", "first_name", :string
         connection.change_column_default "test_models", "first_name", "Tester"