Merge pull request #8 from r7kamura/add-index-concurrently-twice

Remove duplicated `disable_ddl_transaction!` on `Migration/AddIndexConcurrently` cop

lib/rubocop/cop/migration/add_index_concurrently.rb

@@ -30,9 +30,12 @@ class AddIndexConcurrently < RuboCop::Cop::Base
         extend AutoCorrector
 
         include ::RuboCop::Migration::CopConcerns::DisableDdlTransaction
+        include RangeHelp
 
         MSG = 'Use `algorithm: :concurrently` on adding indexes to existing tables in PostgreSQL.'
 
+        MESSAGE_FOR_DUPLICATED_DISABLE_DDL_TRANSACTION = 'Remove duplicated `disable_ddl_transaction!`.'
+
         RESTRICT_ON_SEND = %i[
           add_index
           index
@@ -41,10 +44,21 @@ class AddIndexConcurrently < RuboCop::Cop::Base
         # @param node [RuboCop::AST::SendNode]
         # @return [void]
         def on_send(node)
-          return unless bad?(node)
+          if add_index_without_concurrency?(node)
+            add_offense(node) do |corrector|
+              autocorrect(corrector, node)
+            end
+          end
 
-          add_offense(node) do |corrector|
-            autocorrect(corrector, node)
+          duplicated_disable_ddl_transactions_from(node).each do |disable_ddl_transactions_node|
+            add_offense(node, message: MESSAGE_FOR_DUPLICATED_DISABLE_DDL_TRANSACTION) do |corrector|
+              corrector.remove(
+                range_with_surrounding_space(
+                  disable_ddl_transactions_node.source_range,
+                  side: :left
+                )
+              )
+            end
           end
         end
 
@@ -116,6 +130,17 @@ def on_send(node)
           )
         PATTERN
 
+        # @param node [RuboCop::AST::SendNode]
+        # @return [Boolean]
+        def add_index_without_concurrency?(node)
+          case node.method_name
+          when :add_index
+            add_index?(node) && !add_index_concurrently?(node)
+          when :index
+            index?(node) && in_change_table?(node) && !index_concurrently?(node)
+          end
+        end
+
         # @param corrector [RuboCop::Cop::Corrector]
         # @param node [RuboCop::AST::SendNode]
         # @return [void]
@@ -128,14 +153,9 @@ def autocorrect(
         end
 
         # @param node [RuboCop::AST::SendNode]
-        # @return [Boolean]
-        def bad?(node)
-          case node.method_name
-          when :add_index
-            add_index?(node) && !add_index_concurrently?(node)
-          when :index
-            index?(node) && in_change_table?(node) && !index_concurrently?(node)
-          end
+        # @return [Array<RuboCop::AST::SendNode>]
+        def duplicated_disable_ddl_transactions_from(node)
+          disable_ddl_transactions_from(node)[1..] || []
         end
 
         # @param node [RuboCop::AST::SendNode]

lib/rubocop/migration/cop_concerns/disable_ddl_transaction.rb

@@ -24,6 +24,15 @@ def included(klass)
 
         private
 
+        # @param node [RuboCop::AST::SendNode]
+        # @return [Array<RuboCop::AST::SendNode>]
+        def disable_ddl_transactions_from(node)
+          node.each_ancestor(:def).first&.left_siblings&.select do |sibling|
+            sibling.is_a?(::RuboCop::AST::SendNode) &&
+              disable_ddl_transaction?(sibling)
+          end || []
+        end
+
         # @param corrector [RuboCop::Cop::Corrector]
         # @param node [RuboCop::AST::SendNode]
         # @return [void]
@@ -40,10 +49,7 @@ def insert_disable_ddl_transaction(
         # @param node [RuboCop::AST::SendNode]
         # @return [Boolean]
         def within_disable_ddl_transaction?(node)
-          node.each_ancestor(:def).first&.left_siblings&.any? do |sibling|
-            sibling.is_a?(::RuboCop::AST::SendNode) &&
-              disable_ddl_transaction?(sibling)
-          end
+          !disable_ddl_transactions_from(node).empty?
         end
       end
     end

spec/rubocop/cop/migration/add_index_concurrently_spec.rb

@@ -116,4 +116,30 @@ def change
       RUBY
     end
   end
+
+  context 'when `add_index` is used without `:algorithm` option twice' do
+    it 'registers an offense' do
+      expect_offense(<<~TEXT)
+        class AddIndexToUsersName < ActiveRecord::Migration[7.0]
+          def change
+            add_index :users, :name1
+            ^^^^^^^^^^^^^^^^^^^^^^^^ Use `algorithm: :concurrently` on adding indexes to existing tables in PostgreSQL.
+            add_index :users, :name2
+            ^^^^^^^^^^^^^^^^^^^^^^^^ Use `algorithm: :concurrently` on adding indexes to existing tables in PostgreSQL.
+          end
+        end
+      TEXT
+
+      expect_correction(<<~RUBY)
+        class AddIndexToUsersName < ActiveRecord::Migration[7.0]
+          disable_ddl_transaction!
+
+          def change
+            add_index :users, :name1, algorithm: :concurrently
+            add_index :users, :name2, algorithm: :concurrently
+          end
+        end
+      RUBY
+    end
+  end
 end