Skip to content

Commit c145336

Browse files
lulalalalulalala
committed
Add cop to check ActiveModel errors manipulation as hash directly.
These are deprecated in Rails 6.1 and will be removed in Rails 7. See rails/rails#32313 for details. The cop acts in two modes: For files under `/models` directory, any `errors` call, whether with receiver or not, will be checked. For general files, only `errors` calls with receivers will be checked. E.g. `errors[:bar] = []` is without receiver. It will record an offense if it is a model file. It will not record an offense if it is other general fie. This is to reduce false-positives, since other classes may also have a `errors` method.
1 parent c4d242d commit c145336

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)