Merge branch '207088-add-cop-for-with-lock-retries-and-docs' into 'master'

Prevent using `with_lock_retries` within `change` method

See merge request gitlab-org/gitlab!27064

doc/development/migration_style_guide.md

@@ -176,9 +176,15 @@ Removing a column:
 ```ruby
 include Gitlab::Database::MigrationHelpers
 
-def change
+def up
   with_lock_retries do
-    remove_column :users, :full_name, :string
+    remove_column :users, :full_name
+  end
+end
+
+def down
+  with_lock_retries do
+    add_column :users, :full_name, :string
   end
 end
 ```
@@ -188,11 +194,17 @@ Removing a foreign key:
 ```ruby
 include Gitlab::Database::MigrationHelpers
 
-def change
+def up
   with_lock_retries do
     remove_foreign_key :issues, :projects
   end
 end
+
+def down
+  with_lock_retries do
+    add_foreign_key :issues, :projects
+  end
+end
 ```
 
 Changing default value for a column:
@@ -200,11 +212,17 @@ Changing default value for a column:
 ```ruby
 include Gitlab::Database::MigrationHelpers
 
-def change
+def up
   with_lock_retries do
     change_column_default :merge_requests, :lock_version, from: nil, to: 0
   end
 end
+
+def down
+  with_lock_retries do
+    change_column_default :merge_requests, :lock_version, from: 0, to: nil
+  end
+end
 ```
 
 ### When to use the helper method
@@ -231,6 +249,8 @@ Example changes:
 
 **Note:** `with_lock_retries` method **cannot** be used with `disable_ddl_transaction!`.
 
+**Note:** `with_lock_retries` method **cannot** be used within the `change` method, you must manually define the `up` and `down` methods to make the migration reversible.
+
 ### How the helper method works
 
 1. Iterate 50 times.

rubocop/cop/migration/with_lock_retries_with_change.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that prevents usage of `with_lock_retries` within the `change` method.
+      class WithLockRetriesWithChange < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = '`with_lock_retries` cannot be used within `change` so you must manually define ' \
+          'the `up` and `down` methods in your migration class and use `with_lock_retries` in both methods'.freeze
+
+        def on_send(node)
+          return unless in_migration?(node)
+          return unless node.children[1] == :with_lock_retries
+
+          node.each_ancestor(:def) do |def_node|
+            add_offense(def_node, location: :name) if method_name(def_node) == :change
+          end
+        end
+
+        def method_name(node)
+          node.children.first
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/migration/with_lock_retries_with_change_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/with_lock_retries_with_change'
+
+describe RuboCop::Cop::Migration::WithLockRetriesWithChange do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when `with_lock_retries` is used inside a `change` method' do
+      inspect_source('def change; with_lock_retries {}; end')
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+
+    it 'registers no offense when `with_lock_retries` is used inside an `up` method' do
+      inspect_source('def up; with_lock_retries {}; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source('def change; with_lock_retries {}; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end