Permalink
Browse files

make add_index and remove_index more resilient; new rename_index meth…

…od; track database limits

[#3452 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information...
etiennebarrie authored and jeremy committed May 16, 2010
1 parent a5696e3 commit 99bcce7ec1e7f59db3e6f1e1d3cd02e15eb59602
@@ -0,0 +1,57 @@
+module ActiveRecord
+ module ConnectionAdapters # :nodoc:
+ module DatabaseLimits
+
+ # the maximum length of a table alias
+ def table_alias_length
+ 255
+ end
+
+ # the maximum length of a column name
+ def column_name_length
+ 64
+ end
+
+ # the maximum length of a table name
+ def table_name_length
+ 64
+ end
+
+ # the maximum length of an index name
+ def index_name_length
+ 64
+ end
+
+ # the maximum number of columns per table
+ def columns_per_table
+ 1024
+ end
+
+ # the maximum number of indexes per table
+ def indexes_per_table
+ 16
+ end
+
+ # the maximum number of columns in a multicolumn index
+ def columns_per_multicolumn_index
+ 16
+ end
+
+ # the maximum number of elements in an IN (x,y,z) clause
+ def in_clause_length
+ 65535
+ end
+
+ # the maximum length of a SQL query
+ def sql_query_length
+ 1048575
+ end
+
+ # maximum number of joins in a single query
+ def joins_per_query
+ 256
+ end
+
+ end
+ end
+end
@@ -8,11 +8,6 @@ def native_database_types
{}
end
- # This is the maximum length a table alias can be
- def table_alias_length
- 255
- end
-
# Truncates a table alias according to the limits of the current adapter.
def table_alias_for(table_name)
table_name[0..table_alias_length-1].gsub(/\./, '_')
@@ -283,6 +278,14 @@ def add_index(table_name, column_name, options = {})
index_type = options
end
+ if index_name.length > index_name_length
+ @logger.warn("Index name '#{index_name}' on table '#{table_name}' is too long; the limit is #{index_name_length} characters. Skipping.")
+ return
+ end
+ if index_exists?(table_name, index_name, false)
+ @logger.warn("Index name '#{index_name}' on table '#{table_name}' already exists. Skipping.")
+ return
+ end
quoted_column_names = quoted_columns_for_index(column_names, options).join(", ")
execute "CREATE #{index_type} INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} (#{quoted_column_names})"
@@ -299,7 +302,28 @@ def add_index(table_name, column_name, options = {})
# Remove the index named by_branch_party in the accounts table.
# remove_index :accounts, :name => :by_branch_party
def remove_index(table_name, options = {})
- execute "DROP INDEX #{quote_column_name(index_name(table_name, options))} ON #{quote_table_name(table_name)}"
+ index_name = index_name(table_name, options)
+ unless index_exists?(table_name, index_name, true)
+ @logger.warn("Index name '#{index_name}' on table '#{table_name}' does not exist. Skipping.")
+ return
+ end
+ remove_index!(table_name, index_name)
+ end
+
+ def remove_index!(table_name, index_name) #:nodoc:
+ execute "DROP INDEX #{quote_column_name(index_name)} ON #{table_name}"
+ end
+
+ # Rename an index.
+ #
+ # Rename the index_people_on_last_name index to index_users_on_last_name
+ # rename_index :people, 'index_people_on_last_name', 'index_users_on_last_name'
+ def rename_index(table_name, old_name, new_name)
+ # this is a naive implementation; some DBs may support this more efficiently (Postgres, for instance)
+ old_index_def = indexes(table_name).detect { |i| i.name == old_name }
+ return unless old_index_def
+ remove_index(table_name, :name => old_name)
+ add_index(table_name, old_index_def.columns, :name => new_name, :unique => old_index_def.unique)
end
def index_name(table_name, options) #:nodoc:
@@ -316,6 +340,15 @@ def index_name(table_name, options) #:nodoc:
end
end
+ # Verify the existence of an index.
+ #
+ # The default argument is returned if the underlying implementation does not define the indexes method,
+ # as there's no way to determine the correct answer in that case.
+ def index_exists?(table_name, index_name, default)
+ return default unless respond_to?(:indexes)
+ indexes(table_name).detect { |i| i.name == index_name }
+ end
+
# Returns a string of <tt>CREATE TABLE</tt> SQL statement(s) for recreating the
# entire structure of the database.
def structure_dump
@@ -11,6 +11,7 @@
require 'active_record/connection_adapters/abstract/connection_pool'
require 'active_record/connection_adapters/abstract/connection_specification'
require 'active_record/connection_adapters/abstract/query_cache'
+require 'active_record/connection_adapters/abstract/database_limits'
module ActiveRecord
module ConnectionAdapters # :nodoc:
@@ -29,6 +30,7 @@ module ConnectionAdapters # :nodoc:
# notably, the instance methods provided by SchemaStatement are very useful.
class AbstractAdapter
include Quoting, DatabaseStatements, SchemaStatements
+ include DatabaseLimits
include QueryCache
include ActiveSupport::Callbacks
define_callbacks :checkout, :checkin
@@ -859,9 +859,12 @@ def rename_column(table_name, column_name, new_column_name)
execute "ALTER TABLE #{quote_table_name(table_name)} RENAME COLUMN #{quote_column_name(column_name)} TO #{quote_column_name(new_column_name)}"
end
- # Drops an index from a table.
- def remove_index(table_name, options = {})
- execute "DROP INDEX #{quote_table_name(index_name(table_name, options))}"
+ def remove_index!(table_name, index_name) #:nodoc:
+ execute "DROP INDEX #{quote_table_name(index_name)}"
+ end
+
+ def index_name_length
+ 63
end
# Maps logical Rails types to PostgreSQL-specific data types.
@@ -244,8 +244,8 @@ def primary_key(table_name) #:nodoc:
column ? column['name'] : nil
end
- def remove_index(table_name, options={}) #:nodoc:
- execute "DROP INDEX #{quote_column_name(index_name(table_name, options))}"
+ def remove_index!(table_name, index_name) #:nodoc:
+ execute "DROP INDEX #{quote_column_name(index_name)}"
end
def rename_table(name, new_name)
@@ -16,6 +16,10 @@ def teardown
end
def test_add_index
+ # add_index calls index_exists? which can't work since execute is stubbed
+ ActiveRecord::ConnectionAdapters::MysqlAdapter.send(:define_method, :index_exists?) do |*|
+ false
+ end
expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`)"
assert_equal expected, add_index(:people, :last_name, :length => nil)
@@ -30,6 +34,7 @@ def test_add_index
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`(10))"
assert_equal expected, add_index(:people, [:last_name, :first_name], :length => {:last_name => 15, :first_name => 10})
+ ActiveRecord::ConnectionAdapters::MysqlAdapter.send(:remove_method, :index_exists?)
end
def test_drop_table
@@ -119,6 +119,40 @@ def test_add_index
end
end
+ def test_add_index_length_limit
+ good_index_name = 'x' * Person.connection.index_name_length
+ too_long_index_name = good_index_name + 'x'
+ assert_nothing_raised { Person.connection.add_index("people", "first_name", :name => too_long_index_name) }
+ assert !Person.connection.index_exists?("people", too_long_index_name, false)
+ assert_nothing_raised { Person.connection.add_index("people", "first_name", :name => good_index_name) }
+ assert Person.connection.index_exists?("people", good_index_name, false)
+ end
+
+ def test_remove_nonexistent_index
+ # we do this by name, so OpenBase is a wash as noted above
+ unless current_adapter?(:OpenBaseAdapter)
+ assert_nothing_raised { Person.connection.remove_index("people", "no_such_index") }
+ end
+ end
+
+ def test_rename_index
+ unless current_adapter?(:OpenBaseAdapter)
+ # keep the names short to make Oracle and similar behave
+ Person.connection.add_index('people', [:first_name], :name => 'old_idx')
+ assert_nothing_raised { Person.connection.rename_index('people', 'old_idx', 'new_idx') }
+ # if the adapter doesn't support the indexes call, pick defaults that let the test pass
+ assert !Person.connection.index_exists?('people', 'old_idx', false)
+ assert Person.connection.index_exists?('people', 'new_idx', true)
+ end
+ end
+
+ def test_double_add_index
+ unless current_adapter?(:OpenBaseAdapter)
+ Person.connection.add_index('people', [:first_name], :name => 'some_idx')
+ assert_nothing_raised { Person.connection.add_index('people', [:first_name], :name => 'some_idx') }
+ end
+ end
+
def testing_table_with_only_foo_attribute
Person.connection.create_table :testings, :id => false do |t|
t.column :foo, :string
@@ -137,11 +137,11 @@ def test_dump_indexes_for_schema_two
def test_with_uppercase_index_name
ActiveRecord::Base.connection.execute "CREATE INDEX \"things_Index\" ON #{SCHEMA_NAME}.things (name)"
- assert_nothing_raised { ActiveRecord::Base.connection.remove_index :things, :name => "#{SCHEMA_NAME}.things_Index"}
+ assert_nothing_raised { ActiveRecord::Base.connection.remove_index! "things", "#{SCHEMA_NAME}.things_Index"}
ActiveRecord::Base.connection.execute "CREATE INDEX \"things_Index\" ON #{SCHEMA_NAME}.things (name)"
ActiveRecord::Base.connection.schema_search_path = SCHEMA_NAME
- assert_nothing_raised { ActiveRecord::Base.connection.remove_index :things, :name => "things_Index"}
+ assert_nothing_raised { ActiveRecord::Base.connection.remove_index! "things", "things_Index"}
ActiveRecord::Base.connection.schema_search_path = "public"
end

2 comments on commit 99bcce7

@nirvdrum

This comment has been minimized.

Show comment Hide comment
@nirvdrum

nirvdrum Sep 9, 2010

Should we really be swallowing issues with the index name being too long? This seems like unexpected behavior and an exception should just be raised. Granted a warning is logged, but the console output looks like everything worked okay. I'm unaware of anyone that checks the detailed log when things look okay.

Should we really be swallowing issues with the index name being too long? This seems like unexpected behavior and an exception should just be raised. Granted a warning is logged, but the console output looks like everything worked okay. I'm unaware of anyone that checks the detailed log when things look okay.

Please sign in to comment.