Skip to content

Commit 4f59092

Browse files
committed
[Fix #1083] Add new Rails/BeforeDestroy cop
1 parent ec8e18e commit 4f59092

File tree

5 files changed

+1330
-0
lines changed

5 files changed

+1330
-0
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#1083](https://github.com/rubocop/rubocop-rails/issues/1083): Add `Rails/BeforeDestroy` cop. ([@ecbrodie][], [@ydakuka][])

config/default.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,12 @@ Rails/AttributeDefaultBlockValue:
240240
Include:
241241
- 'app/models/**/*'
242242

243+
Rails/BeforeDestroy:
244+
Description: 'Ensure the correct usage of `prepend: true` in `before_destroy` callbacks.'
245+
Enabled: pending
246+
Reference: 'https://guides.rubyonrails.org/active_record_callbacks.html#destroying-an-object'
247+
VersionAdded: '<<next>>'
248+
243249
Rails/BelongsTo:
244250
Description: >-
245251
Use `optional: true` instead of `required: false` for
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# Ensures that `before_destroy` callbacks are executed
7+
# before `dependent: :destroy` associations by requiring
8+
# the `prepend: true` option.
9+
#
10+
# Without `prepend: true`, `before_destroy` callbacks may run
11+
# after associated records are already deleted, leading to
12+
# unintended behavior.
13+
#
14+
# @example
15+
# # bad
16+
# has_many :entities, dependent: :destroy
17+
# before_destroy { do_something }
18+
#
19+
# # good
20+
# has_many :entities, dependent: :destroy
21+
# before_destroy(prepend: true) { do_something }
22+
#
23+
# @example
24+
# # bad
25+
# belongs_to :entity, dependent: :destroy
26+
# before_destroy :some_method
27+
#
28+
# # good
29+
# belongs_to :entity, dependent: :destroy
30+
# before_destroy :some_method, prepend: true
31+
#
32+
# @example
33+
# # bad
34+
# has_one :entity, dependent: :destroy
35+
# before_destroy MyClass.new
36+
#
37+
# # good
38+
# has_one :entity, dependent: :destroy
39+
# before_destroy MyClass.new, prepend: true
40+
#
41+
# @example
42+
# # bad
43+
# has_many :entities, dependent: :destroy
44+
# before_destroy -> { do_something }
45+
#
46+
# # good
47+
# has_many :entities, dependent: :destroy
48+
# before_destroy -> { do_something }, prepend: true
49+
#
50+
class BeforeDestroy < Base
51+
extend AutoCorrector
52+
53+
MSG = '"before_destroy" callbacks must be declared before "dependent: :destroy" associations ' \
54+
'or use `prepend: true`.'
55+
RESTRICT_ON_SEND = %i[before_destroy].freeze
56+
57+
def_node_search :association_nodes, <<~PATTERN
58+
(send nil? {:belongs_to :has_one :has_many} _ (hash ...))
59+
PATTERN
60+
61+
def_node_matcher :hash_options, <<~PATTERN
62+
`(hash $...)
63+
PATTERN
64+
65+
def_node_matcher :dependent_destroy?, <<~PATTERN
66+
(pair (sym :dependent) (sym :destroy))
67+
PATTERN
68+
69+
def_node_matcher :prepend_true?, <<~PATTERN
70+
(pair (sym :prepend) true)
71+
PATTERN
72+
73+
def on_send(node)
74+
check_add_prepend_true(node)
75+
check_remove_prepend_true(node)
76+
end
77+
78+
private
79+
80+
def check_add_prepend_true(node)
81+
return if contains_prepend_true?(node)
82+
return unless before_association_with_dependent_destroy?(node)
83+
84+
add_offense(node) { |corrector| autocorrect_add_prepend(corrector, node) }
85+
end
86+
87+
def check_remove_prepend_true(node)
88+
return unless contains_prepend_true?(node)
89+
return if before_any_association?(node)
90+
91+
add_offense(node) { |corrector| autocorrect_remove_prepend(corrector, node) }
92+
end
93+
94+
def autocorrect_remove_prepend(corrector, node)
95+
prepend_pair = hash_options(node).find { |pair| prepend_true?(pair) }
96+
prepend_range = prepend_pair.source_range
97+
start_pos, end_pos = adjust_removal_range(prepend_range)
98+
99+
corrector.remove(prepend_range.with(begin_pos: start_pos, end_pos: end_pos))
100+
remove_block_delimiters(corrector, node)
101+
end
102+
103+
def autocorrect_add_prepend(corrector, node)
104+
hash_node = node.arguments.find(&:hash_type?)
105+
106+
if hash_node
107+
corrector.insert_before(hash_node.children.first, 'prepend: true, ')
108+
elsif node.arguments.empty?
109+
corrector.insert_after(node.loc.selector, '(prepend: true)')
110+
else
111+
corrector.insert_after(node.last_argument, ', prepend: true')
112+
end
113+
end
114+
115+
def adjust_removal_range(prepend_range)
116+
start_pos = prepend_range.begin_pos
117+
end_pos = prepend_range.end_pos
118+
119+
source = processed_source.buffer.source
120+
121+
prev_match = source[0...start_pos].match(/,\s*$/)
122+
next_match = source[end_pos..].match(/^\s*,?/)
123+
124+
if prev_match
125+
start_pos = prev_match.begin(0)
126+
elsif next_match
127+
end_pos += next_match.end(0)
128+
end
129+
130+
[start_pos, end_pos]
131+
end
132+
133+
def remove_block_delimiters(corrector, node)
134+
return unless node.block_literal?
135+
return unless node.loc.begin
136+
137+
corrector.remove(node.loc.begin)
138+
corrector.remove(node.loc.end)
139+
end
140+
141+
def before_association_with_dependent_destroy?(node)
142+
root_class_node = find_root_class(node)
143+
association_nodes(root_class_node).any? do |assoc|
144+
contains_dependent_destroy?(assoc) && assoc.first_line < node.first_line
145+
end
146+
end
147+
148+
def before_any_association?(node)
149+
root_class_node = find_root_class(node)
150+
association_nodes(root_class_node).any? do |assoc|
151+
assoc.first_line < node.first_line
152+
end
153+
end
154+
155+
def find_root_class(node)
156+
node.each_ancestor(:class, :module).first
157+
end
158+
159+
def contains_prepend_true?(node)
160+
hash_options(node)&.any? { |pair| prepend_true?(pair) }
161+
end
162+
163+
def contains_dependent_destroy?(node)
164+
hash_options(node).any? { |pair| dependent_destroy?(pair) }
165+
end
166+
end
167+
end
168+
end
169+
end

lib/rubocop/cop/rails_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
require_relative 'rails/arel_star'
3030
require_relative 'rails/assert_not'
3131
require_relative 'rails/attribute_default_block_value'
32+
require_relative 'rails/before_destroy'
3233
require_relative 'rails/belongs_to'
3334
require_relative 'rails/blank'
3435
require_relative 'rails/bulk_change_table'

0 commit comments

Comments
 (0)