Skip to content

Commit 97faf22

Browse files
koickoic
authored
Merge pull request #491 from lulalala/model-errors-direct-manipulation
Add cop to check for ActiveModel errors hash direct manipulation
2 parents c4d242d + c145336 commit 97faf22

File tree

5 files changed

+278
-0
lines changed

5 files changed

+278
-0
lines changed

‎changelog/new_deprecated_active_model_errors_methods.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#491](https://github.com/rubocop/rubocop-rails/pull/491): New `Rails/DeprecatedActiveModelErrorsMethods` cop. ([@lulalala][])

‎config/default.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,12 @@ Rails/DelegateAllowBlank:
264264
Enabled: true
265265
VersionAdded: '0.44'
266266

267+
Rails/DeprecatedActiveModelErrorsMethods:
268+
Description: 'Avoid manipulating ActiveModel errors hash directly.'
269+
Enabled: pending
270+
Safe: false
271+
VersionAdded: '<<next>>'
272+
267273
Rails/DuplicateAssociation:
268274
Description: "Don't repeat associations in a model."
269275
Enabled: pending

‎lib/rubocop/cop/rails/deprecated_active_model_errors_methods.rb

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# This cop checks direct manipulation of ActiveModel#errors as hash.
7+
# These operations are deprecated in Rails 6.1 and will not work in Rails 7.
8+
#
9+
# @safety
10+
# This cop is unsafe because it can report `errors` manipulation on non-ActiveModel,
11+
# which is obviously valid.
12+
# The cop has no way of knowing whether a variable is an ActiveModel or not.
13+
#
14+
# @example
15+
# # bad
16+
# user.errors[:name] << 'msg'
17+
# user.errors.messages[:name] << 'msg'
18+
#
19+
# # good
20+
# user.errors.add(:name, 'msg')
21+
#
22+
# # bad
23+
# user.errors[:name].clear
24+
# user.errors.messages[:name].clear
25+
#
26+
# # good
27+
# user.errors.delete(:name)
28+
#
29+
class DeprecatedActiveModelErrorsMethods < Base
30+
MSG = 'Avoid manipulating ActiveModel errors as hash directly.'
31+
32+
MANIPULATIVE_METHODS = Set[
33+
*%i[
34+
<< append clear collect! compact! concat
35+
delete delete_at delete_if drop drop_while fill filter! keep_if
36+
flatten! insert map! pop prepend push reject! replace reverse!
37+
rotate! select! shift shuffle! slice! sort! sort_by! uniq! unshift
38+
]
39+
].freeze
40+
41+
def_node_matcher :receiver_matcher_outside_model, '{send ivar lvar}'
42+
def_node_matcher :receiver_matcher_inside_model, '{nil? send ivar lvar}'
43+
44+
def_node_matcher :any_manipulation?, <<~PATTERN
45+
{
46+
#root_manipulation?
47+
#root_assignment?
48+
#messages_details_manipulation?
49+
#messages_details_assignment?
50+
}
51+
PATTERN
52+
53+
def_node_matcher :root_manipulation?, <<~PATTERN
54+
(send
55+
(send
56+
(send #receiver_matcher :errors) :[] ...)
57+
MANIPULATIVE_METHODS
58+
...
59+
)
60+
PATTERN
61+
62+
def_node_matcher :root_assignment?, <<~PATTERN
63+
(send
64+
(send #receiver_matcher :errors)
65+
:[]=
66+
...)
67+
PATTERN
68+
69+
def_node_matcher :messages_details_manipulation?, <<~PATTERN
70+
(send
71+
(send
72+
(send
73+
(send #receiver_matcher :errors)
74+
{:messages :details})
75+
:[]
76+
...)
77+
MANIPULATIVE_METHODS
78+
...)
79+
PATTERN
80+
81+
def_node_matcher :messages_details_assignment?, <<~PATTERN
82+
(send
83+
(send
84+
(send #receiver_matcher :errors)
85+
{:messages :details})
86+
:[]=
87+
...)
88+
PATTERN
89+
90+
def on_send(node)
91+
any_manipulation?(node) do
92+
add_offense(node)
93+
end
94+
end
95+
96+
private
97+
98+
def receiver_matcher(node)
99+
model_file? ? receiver_matcher_inside_model(node) : receiver_matcher_outside_model(node)
100+
end
101+
102+
def model_file?
103+
processed_source.buffer.name.include?('/models/')
104+
end
105+
end
106+
end
107+
end
108+
end

‎lib/rubocop/cop/rails_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
require_relative 'rails/default_scope'
3434
require_relative 'rails/delegate'
3535
require_relative 'rails/delegate_allow_blank'
36+
require_relative 'rails/deprecated_active_model_errors_methods'
3637
require_relative 'rails/duplicate_association'
3738
require_relative 'rails/duplicate_scope'
3839
require_relative 'rails/duration_arithmetic'

‎spec/rubocop/cop/rails/deprecated_active_model_errors_methods_spec.rb

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Rails::DeprecatedActiveModelErrorsMethods, :config do
4+
shared_examples 'errors call with explicit receiver' do
5+
context 'when modifying errors' do
6+
it 'registers an offense' do
7+
expect_offense(<<~RUBY, file_path)
8+
user.errors[:name] << 'msg'
9+
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
10+
RUBY
11+
end
12+
13+
context 'when assigning' do
14+
it 'registers an offense' do
15+
expect_offense(<<~RUBY, file_path)
16+
user.errors[:name] = []
17+
^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
18+
RUBY
19+
end
20+
end
21+
end
22+
23+
context 'when modifying errors.messages' do
24+
it 'registers an offense' do
25+
expect_offense(<<~RUBY, file_path)
26+
user.errors.messages[:name] << 'msg'
27+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
28+
RUBY
29+
end
30+
31+
context 'when assigning' do
32+
it 'registers an offense' do
33+
expect_offense(<<~RUBY, file_path)
34+
user.errors.messages[:name] = []
35+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
36+
RUBY
37+
end
38+
end
39+
end
40+
41+
context 'when modifying errors.details' do
42+
it 'registers an offense' do
43+
expect_offense(<<~RUBY, file_path)
44+
user.errors.details[:name] << {}
45+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
46+
RUBY
47+
end
48+
49+
context 'when assigning' do
50+
it 'registers an offense' do
51+
expect_offense(<<~RUBY, file_path)
52+
user.errors.details[:name] = []
53+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
54+
RUBY
55+
end
56+
end
57+
end
58+
end
59+
60+
shared_examples 'errors call without explicit receiver' do
61+
def expect_offense_if_model_file(code, file_path)
62+
if file_path.include?('/models/')
63+
expect_offense(code, file_path)
64+
else
65+
code = code.gsub(/^\^+ .+$/, '')
66+
expect_no_offenses(code, file_path)
67+
end
68+
end
69+
70+
context 'when modifying errors' do
71+
it 'registers an offense for model file' do
72+
expect_offense_if_model_file(<<~RUBY, file_path)
73+
errors[:name] << 'msg'
74+
^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
75+
RUBY
76+
end
77+
78+
context 'when assigning' do
79+
it 'registers an offense' do
80+
expect_offense_if_model_file(<<~RUBY, file_path)
81+
errors[:name] = []
82+
^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
83+
RUBY
84+
end
85+
end
86+
87+
context 'when calling non-manipulative methods' do
88+
it 'does not register an offense' do
89+
expect_no_offenses(<<~RUBY, file_path)
90+
errors[:name].present?
91+
RUBY
92+
end
93+
end
94+
end
95+
96+
context 'when modifying errors.messages' do
97+
it 'registers an offense' do
98+
expect_offense_if_model_file(<<~RUBY, file_path)
99+
errors.messages[:name] << 'msg'
100+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
101+
RUBY
102+
end
103+
104+
context 'when assigning' do
105+
it 'registers an offense' do
106+
expect_offense_if_model_file(<<~RUBY, file_path)
107+
errors.messages[:name] = []
108+
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
109+
RUBY
110+
end
111+
end
112+
113+
context 'when calling non-manipulative methods' do
114+
it 'does not register an offense' do
115+
expect_no_offenses(<<~RUBY, file_path)
116+
errors.messages[:name].present?
117+
RUBY
118+
end
119+
end
120+
end
121+
122+
context 'when modifying errors.details' do
123+
it 'registers an offense' do
124+
expect_offense_if_model_file(<<~RUBY, file_path)
125+
errors.details[:name] << {}
126+
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
127+
RUBY
128+
end
129+
130+
context 'when assigning' do
131+
it 'registers an offense' do
132+
expect_offense_if_model_file(<<~RUBY, file_path)
133+
errors.details[:name] = []
134+
^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
135+
RUBY
136+
end
137+
end
138+
139+
context 'when calling non-manipulative methods' do
140+
it 'does not register an offense' do
141+
expect_no_offenses(<<~RUBY, file_path)
142+
errors.details[:name].present?
143+
RUBY
144+
end
145+
end
146+
end
147+
end
148+
149+
context 'when file is model file' do
150+
let(:file_path) { '/foo/app/models/bar.rb' }
151+
152+
it_behaves_like 'errors call with explicit receiver'
153+
it_behaves_like 'errors call without explicit receiver'
154+
end
155+
156+
context 'when file is generic' do
157+
let(:file_path) { '/foo/app/lib/bar.rb' }
158+
159+
it_behaves_like 'errors call with explicit receiver'
160+
it_behaves_like 'errors call without explicit receiver'
161+
end
162+
end

0 commit comments

Comments
 (0)