From 9771f5e178ebc1b8c5a61f813373ca3b7bd983c8 Mon Sep 17 00:00:00 2001 From: Grey Baker Date: Sun, 22 Mar 2015 11:14:51 +0000 Subject: Ignore index name in `index_exists?` when not passed a name to check for --- activerecord/CHANGELOG.md | 5 +++++ .../connection_adapters/abstract/schema_statements.rb | 3 +-- activerecord/lib/active_record/migration/compatibility.rb | 11 +++++++++++ activerecord/test/cases/migration/index_test.rb | 2 ++ 4 files changed, 19 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 87420a0746..781b786012 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Ignore index name in `index_exists?` and `remove_index` when not passed a + name to check for. + + *Grey Baker* + * Version the API presented to migration classes, so we can change parameter defaults without breaking existing migrations, or forcing them to be rewritten through a deprecation cycle. 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 b50d28862c..93f0d9d0e7 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -82,11 +82,10 @@ module ActiveRecord # def index_exists?(table_name, column_name, options = {}) column_names = Array(column_name).map(&:to_s) - index_name = options.key?(:name) ? options[:name].to_s : index_name(table_name, column: column_names) checks = [] - checks << lambda { |i| i.name == index_name } checks << lambda { |i| i.columns == column_names } checks << lambda { |i| i.unique } if options[:unique] + checks << lambda { |i| i.name == options[:name].to_s } if options[:name] indexes(table_name).any? { |i| checks.all? { |check| check[i] } } end diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index 4c8db8a2d5..bc5b97a711 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -28,6 +28,17 @@ module ActiveRecord options[:null] = true if options[:null].nil? super end + + def index_exists?(table_name, column_name, options = {}) + column_names = Array(column_name).map(&:to_s) + options[:name] = + if options.key?(:name).present? + options[:name].to_s + else + index_name(table_name, column: column_names) + end + super + end end class V4_2 < V5_0 diff --git a/activerecord/test/cases/migration/index_test.rb b/activerecord/test/cases/migration/index_test.rb index b23b9a679f..70169e501c 100644 --- a/activerecord/test/cases/migration/index_test.rb +++ b/activerecord/test/cases/migration/index_test.rb @@ -130,7 +130,9 @@ module ActiveRecord def test_named_index_exists connection.add_index :testings, :foo, :name => "custom_index_name" + assert connection.index_exists?(:testings, :foo) assert connection.index_exists?(:testings, :foo, :name => "custom_index_name") + assert !connection.index_exists?(:testings, :foo, :name => "other_index_name") end def test_add_index_attribute_length_limit -- cgit v1.2.3 From 8ceb883b0630e0d010fbef0c621cc9690b0bcad6 Mon Sep 17 00:00:00 2001 From: Grey Baker Date: Fri, 3 Apr 2015 09:55:53 +0100 Subject: Support removing custom-names indexes when only specifying column names --- .../abstract/schema_statements.rb | 40 ++++++++++++++------- .../lib/active_record/migration/compatibility.rb | 25 +++++++++++++ .../adapters/postgresql/active_schema_test.rb | 8 +++-- .../test/cases/adapters/postgresql/schema_test.rb | 2 -- .../test/cases/migration/compatibility_test.rb | 42 ++++++++++++++++++++++ activerecord/test/cases/migration/index_test.rb | 8 +++++ 6 files changed, 107 insertions(+), 18 deletions(-) create mode 100644 activerecord/test/cases/migration/compatibility_test.rb (limited to 'activerecord') 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 93f0d9d0e7..7bf548fcba 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -699,15 +699,15 @@ module ActiveRecord # Removes the given index from the table. # - # Removes the +index_accounts_on_column+ in the +accounts+ table. + # Removes the index on +branch_id+ in the +accounts+ table if exactly one such index exists. # # remove_index :accounts, :branch_id # - # Removes the index named +index_accounts_on_branch_id+ in the +accounts+ table. + # Removes the index on +branch_id+ in the +accounts+ table if exactly one such index exists. # # remove_index :accounts, column: :branch_id # - # Removes the index named +index_accounts_on_branch_id_and_party_id+ in the +accounts+ table. + # Removes the index on +branch_id+ and +party_id+ in the +accounts+ table if exactly one such index exists. # # remove_index :accounts, column: [:branch_id, :party_id] # @@ -1126,21 +1126,35 @@ module ActiveRecord end def index_name_for_remove(table_name, options = {}) - index_name = index_name(table_name, options) + # if the adapter doesn't support the indexes call the best we can do + # is return the default index name for the options provided + return index_name(table_name, options) unless respond_to?(:indexes) - unless index_name_exists?(table_name, index_name, true) - if options.is_a?(Hash) && options.has_key?(:name) - options_without_column = options.dup - options_without_column.delete :column - index_name_without_column = index_name(table_name, options_without_column) + checks = [] - return index_name_without_column if index_name_exists?(table_name, index_name_without_column, false) - end + if options.is_a?(Hash) + checks << lambda { |i| i.name == options[:name].to_s } if options.has_key?(:name) + column_names = Array(options[:column]).map(&:to_s) + else + column_names = Array(options).map(&:to_s) + end - raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' does not exist" + if column_names.any? + checks << lambda { |i| i.columns.join('_and_') == column_names.join('_and_') } end - index_name + raise ArgumentError "No name or columns specified" if checks.none? + + matching_indexes = indexes(table_name).select { |i| checks.all? { |check| check[i] } } + + if matching_indexes.count > 1 + raise ArgumentError, "Multiple indexes found on #{table_name} columns #{column_names}. " \ + "Specify an index name from #{matching_indexes.map(&:name).join(', ')}" + elsif matching_indexes.none? + raise ArgumentError, "No indexes found on #{table_name} with the options provided." + else + matching_indexes.first.name + end end def rename_table_indexes(table_name, new_name) diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index bc5b97a711..831bfa2df3 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -39,6 +39,31 @@ module ActiveRecord end super end + + def remove_index(table_name, options = {}) + index_name = index_name_for_remove(table_name, options) + execute "DROP INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)}" + end + + private + + def index_name_for_remove(table_name, options = {}) + index_name = index_name(table_name, options) + + unless index_name_exists?(table_name, index_name, true) + if options.is_a?(Hash) && options.has_key?(:name) + options_without_column = options.dup + options_without_column.delete :column + index_name_without_column = index_name(table_name, options_without_column) + + return index_name_without_column if index_name_exists?(table_name, index_name_without_column, false) + end + + raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' does not exist" + end + + index_name + end end class V4_2 < V5_0 diff --git a/activerecord/test/cases/adapters/postgresql/active_schema_test.rb b/activerecord/test/cases/adapters/postgresql/active_schema_test.rb index 24def31e36..ed44bf7362 100644 --- a/activerecord/test/cases/adapters/postgresql/active_schema_test.rb +++ b/activerecord/test/cases/adapters/postgresql/active_schema_test.rb @@ -54,8 +54,10 @@ class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase end def test_remove_index - # remove_index calls index_name_exists? which can't work since execute is stubbed - ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send(:define_method, :index_name_exists?) { |*| true } + # remove_index calls index_name_for_remove which can't work since execute is stubbed + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send(:define_method, :index_name_for_remove) do |*| + 'index_people_on_last_name' + end expected = %(DROP INDEX CONCURRENTLY "index_people_on_last_name") assert_equal expected, remove_index(:people, name: "index_people_on_last_name", algorithm: :concurrently) @@ -64,7 +66,7 @@ class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase add_index(:people, :last_name, algorithm: :copy) end - ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send :remove_method, :index_name_exists? + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send :remove_method, :index_name_for_remove end private diff --git a/activerecord/test/cases/adapters/postgresql/schema_test.rb b/activerecord/test/cases/adapters/postgresql/schema_test.rb index 7c9169f6e2..da30ad8a62 100644 --- a/activerecord/test/cases/adapters/postgresql/schema_test.rb +++ b/activerecord/test/cases/adapters/postgresql/schema_test.rb @@ -323,8 +323,6 @@ class SchemaTest < ActiveRecord::PostgreSQLTestCase def test_with_uppercase_index_name @connection.execute "CREATE INDEX \"things_Index\" ON #{SCHEMA_NAME}.things (name)" - assert_nothing_raised { @connection.remove_index "things", name: "#{SCHEMA_NAME}.things_Index"} - @connection.execute "CREATE INDEX \"things_Index\" ON #{SCHEMA_NAME}.things (name)" with_schema_search_path SCHEMA_NAME do assert_nothing_raised { @connection.remove_index "things", name: "things_Index"} diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb new file mode 100644 index 0000000000..267d2fcccc --- /dev/null +++ b/activerecord/test/cases/migration/compatibility_test.rb @@ -0,0 +1,42 @@ +require 'cases/helper' + +module ActiveRecord + class Migration + class CompatibilityTest < ActiveRecord::TestCase + attr_reader :connection + self.use_transactional_tests = false + + def setup + super + @connection = ActiveRecord::Base.connection + @verbose_was = ActiveRecord::Migration.verbose + ActiveRecord::Migration.verbose = false + + connection.create_table :testings do |t| + t.column :foo, :string, :limit => 100 + t.column :bar, :string, :limit => 100 + end + end + + teardown do + connection.drop_table :testings rescue nil + ActiveRecord::Migration.verbose = @verbose_was + end + + def test_migration_doesnt_remove_named_index + connection.add_index :testings, :foo, :name => "custom_index_name" + + migration = Class.new(ActiveRecord::Migration[4.2]) { + def version; 101 end + def migrate(x) + remove_index :testings, :foo + end + }.new + + assert connection.index_exists?(:testings, :foo, name: "custom_index_name") + assert_raise(StandardError) { ActiveRecord::Migrator.new(:up, [migration]).migrate } + assert connection.index_exists?(:testings, :foo, name: "custom_index_name") + end + end + end +end diff --git a/activerecord/test/cases/migration/index_test.rb b/activerecord/test/cases/migration/index_test.rb index 70169e501c..5b3d13d2f7 100644 --- a/activerecord/test/cases/migration/index_test.rb +++ b/activerecord/test/cases/migration/index_test.rb @@ -135,6 +135,14 @@ module ActiveRecord assert !connection.index_exists?(:testings, :foo, :name => "other_index_name") end + def test_remove_named_index + connection.add_index :testings, :foo, :name => "custom_index_name" + + assert connection.index_exists?(:testings, :foo) + connection.remove_index :testings, :foo + assert !connection.index_exists?(:testings, :foo) + end + def test_add_index_attribute_length_limit connection.add_index :testings, [:foo, :bar], :length => {:foo => 10, :bar => nil} -- cgit v1.2.3