From bf5268f9bc8e4c8cbafa9ee12ab3af1f56646b9f Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 31 Mar 2020 12:31:18 -0400 Subject: [PATCH 1/4] Add `if_exists` option to `remove_index` This PR allows for passing `if_exists` options to the `remove_index` method so that we can ignore already removed indexes. This work follows column `if/if_not_exists` from #38352 and `:if_not_exists` on `add_index` from #38555. We've found this useful at GitHub, there are migrations where we don't want to raise if an index was already removed. This will allow us to remove a monkey patch on `remove_index`. I considered raising after the `index_name_for_remove` method is called but that method will raise if the index doesn't exist before we get to execute. I have a commit that refactors this but after much consideration this change is cleaner and more straightforward than other ways of implementing this. This change also adds a little extra validation to the `add_index` test. Fix `nodoc` on edited methods. --- activerecord/CHANGELOG.md | 6 ++++++ .../abstract/schema_statements.rb | 8 ++++++++ .../postgresql/schema_statements.rb | 6 +++++- .../connection_adapters/sqlite3_adapter.rb | 5 ++++- .../test/cases/migration/index_test.rb | 18 +++++++++++++++++- 5 files changed, 40 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 2b8bc81a201b5..da88a47ae54c7 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Add support for `if_exists` option for removing an index. + + The `remove_index` method can take an `if_exists` option. If this is set to true an error won't be raised if the index doesn't exist. + + *Eileen M. Uchitelle* + * Remove ibm_db, informix, mssql, oracle, and oracle12 Arel visitors which are not used in the code base. *Ryuta Kamizono* 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 cd7f7c5186e6c..44a3325286e8a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -845,6 +845,11 @@ def add_index(table_name, column_name, options = {}) # # remove_index :accounts, :branch_id, name: :by_branch_party # + # Checks if the index exists before trying to remove it. Will silently ignore indexes that + # don't exist. + # + # remove_index :accounts, if_exists: true + # # Removes the index named +by_branch_party+ in the +accounts+ table +concurrently+. # # remove_index :accounts, name: :by_branch_party, algorithm: :concurrently @@ -855,7 +860,10 @@ def add_index(table_name, column_name, options = {}) # # For more information see the {"Transactional Migrations" section}[rdoc-ref:Migration]. def remove_index(table_name, column_name = nil, options = {}) + return if options[:if_exists] && !index_exists?(table_name, column_name, options) + index_name = index_name_for_remove(table_name, column_name, options) + execute "DROP INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)}" end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index 56bb3c16ae8f1..5df90a228eb76 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -448,7 +448,7 @@ def add_index(table_name, column_name, options = {}) #:nodoc: end end - def remove_index(table_name, column_name = nil, options = {}) #:nodoc: + def remove_index(table_name, column_name = nil, options = {}) # :nodoc: table = Utils.extract_schema_qualified_name(table_name.to_s) if column_name.is_a?(Hash) @@ -467,13 +467,17 @@ def remove_index(table_name, column_name = nil, options = {}) #:nodoc: end end + return if options[:if_exists] && !index_exists?(table_name, column_name, options) + index_to_remove = PostgreSQL::Name.new(table.schema, index_name_for_remove(table.to_s, column_name, options)) + algorithm = if options.key?(:algorithm) index_algorithms.fetch(options[:algorithm]) do raise ArgumentError.new("Algorithm must be one of the following: #{index_algorithms.keys.map(&:inspect).join(', ')}") end end + execute "DROP INDEX #{algorithm} #{quote_table_name(index_to_remove)}" end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 548f8e4e17653..32b9abf21345b 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -226,8 +226,11 @@ def primary_keys(table_name) # :nodoc: pks.sort_by { |f| f["pk"] }.map { |f| f["name"] } end - def remove_index(table_name, column_name, options = {}) #:nodoc: + def remove_index(table_name, column_name, options = {}) # :nodoc: + return if options[:if_exists] && !index_exists?(table_name, column_name, options) + index_name = index_name_for_remove(table_name, column_name, options) + exec_query "DROP INDEX #{quote_column_name(index_name)}" end diff --git a/activerecord/test/cases/migration/index_test.rb b/activerecord/test/cases/migration/index_test.rb index a24951941a755..9576d77985291 100644 --- a/activerecord/test/cases/migration/index_test.rb +++ b/activerecord/test/cases/migration/index_test.rb @@ -72,12 +72,28 @@ def test_add_index_does_not_accept_too_long_index_names connection.add_index(table_name, "foo", name: good_index_name) end - def test_add_index_which_already_exists_does_not_raise_error + def test_add_index_which_already_exists_does_not_raise_error_with_option connection.add_index(table_name, "foo", if_not_exists: true) assert_nothing_raised do connection.add_index(table_name, "foo", if_not_exists: true) end + + assert connection.index_name_exists?(table_name, "index_testings_on_foo") + end + + def test_remove_index_which_does_not_exist_doesnt_raise_with_option + connection.add_index(table_name, "foo") + + connection.remove_index(table_name, "foo") + + assert_raises ArgumentError do + connection.remove_index(table_name, "foo") + end + + assert_nothing_raised do + connection.remove_index(table_name, "foo", if_exists: true) + end end def test_internal_index_with_name_matching_database_limit From 6a4650ba207c0184889f74065b06bdc05ef7b292 Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Thu, 16 Apr 2020 22:51:51 +0100 Subject: [PATCH 2/4] Don't gitignore tmp/pids/.keep Since 04cfbc807f0567af5a27e8ba45eab52f7b9e6618, a keepfile is generated in `tmp/pids/` to ensure that the directory always exists. However, the gitignore pattern for `/tmp/*` meant it wasn't tracked in git. To override that pattern we have to allow the `tmp/pids/` directory, ignore everything inside it, then finally allow `tmp/pids/.keep` again. --- .../lib/rails/generators/rails/app/templates/gitignore.tt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/railties/lib/rails/generators/rails/app/templates/gitignore.tt b/railties/lib/rails/generators/rails/app/templates/gitignore.tt index e4e3d26971af0..b50e0a5e720e9 100644 --- a/railties/lib/rails/generators/rails/app/templates/gitignore.tt +++ b/railties/lib/rails/generators/rails/app/templates/gitignore.tt @@ -19,6 +19,11 @@ <% if keeps? -%> !/log/.keep !/tmp/.keep + +# Ignore pidfiles, but keep the directory. +/tmp/pids/* +!/tmp/pids/ +!/tmp/pids/.keep <% end -%> <% unless skip_active_storage? -%> From f4d770e68e66d765bcc46afdc5e697c3c980f110 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 17 Apr 2020 18:55:58 +0900 Subject: [PATCH 3/4] touch_attributes_with_time takes keyword arguments Follow up to 404e1a0acdf369ce6eaa65d70c6977a56ed8a277. --- activerecord/lib/active_record/counter_cache.rb | 5 ++++- activerecord/lib/active_record/relation.rb | 2 +- activerecord/test/cases/counter_cache_test.rb | 2 +- activerecord/test/cases/relation/update_all_test.rb | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb index 27c1b7a311206..2d91bd3da5b9d 100644 --- a/activerecord/lib/active_record/counter_cache.rb +++ b/activerecord/lib/active_record/counter_cache.rb @@ -51,7 +51,10 @@ def reset_counters(id, *counters, touch: nil) if touch names = touch if touch != true - updates.merge!(touch_attributes_with_time(*names)) + names = Array.wrap(names) + options = names.extract_options! + touch_updates = touch_attributes_with_time(*names, **options) + updates.merge!(touch_updates) end unscoped.where(primary_key => object.id).update_all(updates) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 089aff54261fb..c7ab8a2db706b 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -500,7 +500,7 @@ def update_counters(counters) if touch names = touch if touch != true - names = Array(names) + names = Array.wrap(names) options = names.extract_options! touch_updates = klass.touch_attributes_with_time(*names, **options) updates.merge!(touch_updates) unless touch_updates.empty? diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb index 155c637aa31b3..c4925929869ac 100644 --- a/activerecord/test/cases/counter_cache_test.rb +++ b/activerecord/test/cases/counter_cache_test.rb @@ -264,7 +264,7 @@ class ::SpecialReply < ::Reply test "reset multiple counters with touch: true" do assert_touching @topic, :updated_at do Topic.update_counters(@topic.id, replies_count: 1, unique_replies_count: 1) - Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: true) + Topic.reset_counters(@topic.id, :replies, :unique_replies, touch: { time: Time.now.utc }) end end diff --git a/activerecord/test/cases/relation/update_all_test.rb b/activerecord/test/cases/relation/update_all_test.rb index c50f715013589..fc4566e53f69f 100644 --- a/activerecord/test/cases/relation/update_all_test.rb +++ b/activerecord/test/cases/relation/update_all_test.rb @@ -194,7 +194,7 @@ def test_update_counters_cares_about_optimistic_locking people = Person.where(id: people(:michael, :david, :susan)) expected = people.pluck(:lock_version) expected.map! { |version| version + 1 } - people.update_counters(touch: [time: now]) + people.update_counters(touch: { time: now }) assert_equal [now] * 3, people.pluck(:updated_at) assert_equal expected, people.pluck(:lock_version) From 3e9b3bf533f7a4f90da539ece83ee2cfec18c1c0 Mon Sep 17 00:00:00 2001 From: Liroy Leshed <37022194+liroyleshed@users.noreply.github.com> Date: Fri, 17 Apr 2020 13:42:28 +0300 Subject: [PATCH 4/4] Convert CoffeeScript to ES6 syntax Convert CoffeeScript to ES6 syntax --- actioncable/lib/action_cable/helpers/action_cable_helper.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/actioncable/lib/action_cable/helpers/action_cable_helper.rb b/actioncable/lib/action_cable/helpers/action_cable_helper.rb index df16c02e837e7..aa0c194e2844e 100644 --- a/actioncable/lib/action_cable/helpers/action_cable_helper.rb +++ b/actioncable/lib/action_cable/helpers/action_cable_helper.rb @@ -12,11 +12,11 @@ module ActionCableHelper # # # This is then used by Action Cable to determine the URL of your WebSocket server. - # Your CoffeeScript can then connect to the server without needing to specify the + # Your JavaScript can then connect to the server without needing to specify the # URL directly: # - # #= require cable - # @App = {} + # window.Cable = require("@rails/actioncable") + # window.App = {} # App.cable = Cable.createConsumer() # # Make sure to specify the correct server location in each of your environment