Permalink
Browse files

Merge pull request #8613 from senny/8264_character_limit_for_indices

deal with long index names and internal sqlite3 operations
  • Loading branch information...
2 parents 7bae72c + 72ca2d7 commit 92949d2a44f2c83a1537e0e4f29864ffc506cb1b @rafaelfranca rafaelfranca committed Feb 20, 2013
@@ -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
@@ -17,6 +17,15 @@ def table_name_length
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
@@ -647,10 +647,11 @@ def add_index_options(table_name, column_name, options = {})
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 @@ def add_index_options(table_name, column_name, options = {})
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"
@@ -187,6 +187,13 @@ def supports_index_sort_order?
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 @@ def table_structure(table_name)
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 @@ def copy_table(from, to, options = {}) #:nodoc:
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 @@ def copy_table_indexes(from, to, rename = {}) #:nodoc:
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
@@ -55,19 +55,31 @@ def test_remove_nonexistent_index
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 @@ def test_add_partial_index
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
@@ -197,6 +197,17 @@ def test_change_column_with_custom_index_name
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"

0 comments on commit 92949d2

Please sign in to comment.