Make cop Migration/AddConcurrentForeignKey aware of `with_lock_retries`
The following discussion from !43634 (merged) should be addressed:
-
@splattael started a discussion: @ahegyi Although we explicitly allow
add_foreign_key
to be withinwith_lock_retries
it's is still flagged as an offense due toMigration/AddConcurrentForeignKey
.Should we make it aware of
with_lock_retries
then?🤔 diff --git a/rubocop/cop/migration/add_concurrent_foreign_key.rb b/rubocop/cop/migration/add_concurrent_foreign_key.rb index 236de6224a4..70e9ae07a61 100644 --- a/rubocop/cop/migration/add_concurrent_foreign_key.rb +++ b/rubocop/cop/migration/add_concurrent_foreign_key.rb @@ -14,14 +14,20 @@ module RuboCop (false) PATTERN + def_node_matcher :with_lock_retries?, <<~PATTERN + (:send nil? :with_lock_retries) + PATTERN + def on_send(node) return unless in_migration?(node) name = node.children[1] - if name == :add_foreign_key && !not_valid_fk?(node) - add_offense(node, location: :selector) - end + return unless name == :add_foreign_key + return if not_valid_fk?(node) + return if in_with_lock_retries?(node) + + add_offense(node, location: :selector) end def method_name(node) @@ -33,6 +39,12 @@ module RuboCop pair.children[0].children[0] == :validate && false_node?(pair.children[1]) end end + + def in_with_lock_retries?(node) + node.each_ancestor(:block).any? do |parent| + with_lock_retries?(parent.to_a.first) + end + end end end end diff --git a/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb b/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb index b43d44dba65..86fe5abd15a 100644 --- a/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb +++ b/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb @@ -36,5 +36,15 @@ RSpec.describe RuboCop::Cop::Migration::AddConcurrentForeignKey, type: :rubocop expect(cop.offenses).to be_empty end + + it 'does not complain within `with_lock_retries`' do + inspect_source <<~RUBY + with_lock_retries do + add_foreign_key :key, :projects, column: :project_id, on_delete: :cascade + end + RUBY + + expect(cop.offenses).to be_empty + end end end
I'll open an follow-up
💪
Edited by Peter Leitzen