From 5c200f8448a3f8a3bcc09c42a6be4b4a698a589a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20G=C3=BCnther?= <mail@oliverguenther.de>
Date: Sat, 1 Jan 2022 21:25:28 +0100
Subject: [PATCH] Allow named expression indexes to be revertible.

Fixes #43331
---
 activerecord/CHANGELOG.md                     | 12 +++++
 .../abstract/schema_statements.rb             | 13 ++++--
 .../postgresql/invertible_migration_test.rb   | 46 +++++++++++++++++++
 3 files changed, 67 insertions(+), 4 deletions(-)
 create mode 100644 activerecord/test/cases/adapters/postgresql/invertible_migration_test.rb

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index a05d551051..942e2dcad0 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,15 @@
+*   Allow named expression indexes to be revertible.
+
+    Previously, the following code would raise an error in a reversible migration executed while rolling back, due to the index name not being used in the index removal.
+
+    ```ruby
+    add_index(:settings, "(data->'property')", using: :gin, name: :index_settings_data_property)
+    ```
+
+    Fixes #43331.
+
+    *Oliver Günther*
+
 *   Fix incorrect argument in PostgreSQL structure dump tasks.
 
     Updating the `--no-comment` argument added in Rails 7 to the correct `--no-comments` argument.
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 8d51e8d191..d393ce550e 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -1438,7 +1438,7 @@ module ActiveRecord
 
           checks = []
 
-          if !options.key?(:name) && column_name.is_a?(String) && /\W/.match?(column_name)
+          if !options.key?(:name) && expression_column_name?(column_name)
             options[:name] = index_name(table_name, column_name)
             column_names = []
           else
@@ -1447,7 +1447,7 @@ module ActiveRecord
 
           checks << lambda { |i| i.name == options[:name].to_s } if options.key?(:name)
 
-          if column_names.present?
+          if column_names.present? && !(options.key?(:name) && expression_column_name?(column_names))
             checks << lambda { |i| index_name(table_name, i.columns) == index_name(table_name, column_names) }
           end
 
@@ -1515,7 +1515,7 @@ module ActiveRecord
         end
 
         def index_column_names(column_names)
-          if column_names.is_a?(String) && /\W/.match?(column_names)
+          if expression_column_name?(column_names)
             column_names
           else
             Array(column_names)
@@ -1523,13 +1523,18 @@ module ActiveRecord
         end
 
         def index_name_options(column_names)
-          if column_names.is_a?(String) && /\W/.match?(column_names)
+          if expression_column_name?(column_names)
             column_names = column_names.scan(/\w+/).join("_")
           end
 
           { column: column_names }
         end
 
+        # Try to identify whether the given column name is an expression
+        def expression_column_name?(column_name)
+          column_name.is_a?(String) && /\W/.match?(column_name)
+        end
+
         def strip_table_name_prefix_and_suffix(table_name)
           prefix = Base.table_name_prefix
           suffix = Base.table_name_suffix
diff --git a/activerecord/test/cases/adapters/postgresql/invertible_migration_test.rb b/activerecord/test/cases/adapters/postgresql/invertible_migration_test.rb
new file mode 100644
index 0000000000..d41f18728b
--- /dev/null
+++ b/activerecord/test/cases/adapters/postgresql/invertible_migration_test.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+
+class PostgresqlInvertibleMigrationTest < ActiveRecord::PostgreSQLTestCase
+  class SilentMigration < ActiveRecord::Migration::Current
+    def write(*); end
+  end
+
+  class ExpressionIndexMigration < SilentMigration
+    def change
+      create_table("settings") do |t|
+        t.column :data, :jsonb
+      end
+
+      add_index :settings,
+                "(data->'foo')",
+                using: :gin,
+                name: "index_settings_data_foo"
+    end
+  end
+
+  self.use_transactional_tests = false
+
+  setup do
+    @connection = ActiveRecord::Base.connection
+  end
+
+  teardown do
+    @connection.drop_table "settings", if_exists: true
+  end
+
+  def test_migrate_revert_add_index_with_expression
+    ExpressionIndexMigration.new.migrate(:up)
+
+    assert @connection.table_exists?(:settings)
+    assert @connection.index_exists?(:settings, nil, name: "index_settings_data_foo"),
+           "index on index_settings_data_foo should exist"
+
+    ExpressionIndexMigration.new.migrate(:down)
+
+    assert_not @connection.table_exists?(:settings)
+    assert_not @connection.index_exists?(:settings, nil, name: "index_settings_data_foo"),
+               "index index_settings_data_foo should not exist"
+  end
+end