Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a NoMethodError for errors.keys in a model #740

Merged
merged 1 commit into from
Jul 8, 2022
Merged

Fix a NoMethodError for errors.keys in a model #740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/fix_deprecated_errors_nil_dereference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#740](https://github.com/rubocop/rubocop-rails/pull/740): Fix a NoMethodError on nil for `errors.keys` in a model in Rails/DeprecatedActiveModelErrorsMethods. ([@BrianHawley][])
10 changes: 3 additions & 7 deletions lib/rubocop/cop/rails/deprecated_active_model_errors_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,23 +121,19 @@ def on_send(node)
def autocorrect(corrector, node)
receiver = node.receiver

if receiver.receiver.send_type? && receiver.receiver.method?(:messages)
corrector.remove(receiver.receiver.loc.dot)
corrector.remove(receiver.receiver.loc.selector)
end

range = offense_range(node, receiver)
replacement = replacement(node, receiver)

corrector.replace(range, replacement)
end

def offense_range(node, receiver)
range_between(receiver.receiver.source_range.end_pos, node.source_range.end_pos)
receiver = receiver.receiver while receiver.send_type? && !receiver.method?(:errors) && receiver.receiver
Copy link
Contributor Author

@BrianHawley BrianHawley Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, for the current patterns, !receiver.method?(:errors) would be enough, but it seemed like a good idea to do the other checks in case new patterns are added. We don't want an infinite loop or bad method calls.

range_between(receiver.source_range.end_pos, node.source_range.end_pos)
end

def replacement(node, receiver)
return '.errors.attribute_names' if node.method?(:keys)
return '.attribute_names' if node.method?(:keys)

key = receiver.first_argument.source

Expand Down
162 changes: 157 additions & 5 deletions spec/rubocop/cop/rails/deprecated_active_model_errors_methods_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

RSpec.describe RuboCop::Cop::Rails::DeprecatedActiveModelErrorsMethods, :config do
shared_examples 'errors call with explicit receiver' do
context 'when modifying errors' do
context 'when accessing errors' do
it 'registers and corrects an offense' do
expect_offense(<<~RUBY, file_path)
user.errors[:name] << 'msg'
Expand All @@ -20,6 +20,8 @@
user.errors[:name] = []
^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_no_corrections
end
end

Expand All @@ -36,8 +38,8 @@
end
end

context 'when using `keys` method' do
context 'Rails >= 6.1', :rails61 do
context 'Rails >= 6.1', :rails61 do
context 'when using `keys` method' do
it 'registers and corrects an offense when root receiver is a variable' do
expect_offense(<<~RUBY, file_path)
user = create_user
Expand All @@ -62,8 +64,10 @@
RUBY
end
end
end

context 'Rails <= 6.0', :rails60 do
context 'Rails <= 6.0', :rails60 do
context 'when using `keys` method' do
it 'does not register an offense when root receiver is a variable' do
expect_no_offenses(<<~RUBY, file_path)
user = create_user
Expand Down Expand Up @@ -98,6 +102,29 @@
user.errors.messages[:name] = []
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_no_corrections
end
end

context 'when using `clear` method' do
it 'registers and corrects an offense' do
expect_offense(<<~RUBY, file_path)
user.errors.messages[:name].clear
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction(<<~RUBY)
user.errors.delete(:name)
RUBY
end
end

context 'when calling non-manipulative methods' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY, file_path)
errors.messages[:name].present?
RUBY
end
end
end
Expand All @@ -116,6 +143,29 @@
user.errors.details[:name] = []
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_no_corrections
end
end

context 'when using `clear` method' do
it 'registers and corrects an offense' do
expect_offense(<<~RUBY, file_path)
user.errors.details[:name].clear
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction(<<~RUBY)
user.errors.delete(:name)
RUBY
end
end

context 'when calling non-manipulative methods' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY, file_path)
errors.details[:name].present?
RUBY
end
end
end
Expand All @@ -131,12 +181,24 @@ def expect_offense_if_model_file(code, file_path)
end
end

context 'when modifying errors' do
def expect_correction_if_model_file(code, file_path)
expect_correction(code) if file_path.include?('/models/')
end

def expect_no_corrections_if_model_file(file_path)
expect_no_corrections if file_path.include?('/models/')
end

context 'when accessing errors' do
it 'registers an offense for model file' do
expect_offense_if_model_file(<<~RUBY, file_path)
errors[:name] << 'msg'
^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction_if_model_file(<<~RUBY, file_path)
errors.add(:name, 'msg')
RUBY
end

context 'when assigning' do
Expand All @@ -145,6 +207,46 @@ def expect_offense_if_model_file(code, file_path)
errors[:name] = []
^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_no_corrections_if_model_file(file_path)
end
end

context 'when using `clear` method' do
it 'registers and corrects an offense' do
expect_offense_if_model_file(<<~RUBY, file_path)
errors[:name].clear
^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction_if_model_file(<<~RUBY, file_path)
errors.delete(:name)
RUBY
end
end

context 'Rails >= 6.1', :rails61 do
context 'when using `keys` method' do
it 'registers and corrects an offense when root receiver is a variable' do
expect_offense_if_model_file(<<~RUBY, file_path)
errors.keys
^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the specific case that caused the NoMethodError for me. I added the rest of the test changes to look for any other such errors.

RUBY

expect_correction_if_model_file(<<~RUBY, file_path)
errors.attribute_names
RUBY
end
end
end

context 'Rails <= 6.0', :rails60 do
context 'when using `keys` method' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY, file_path)
errors.keys
RUBY
end
end
end

Expand All @@ -163,6 +265,10 @@ def expect_offense_if_model_file(code, file_path)
errors.messages[:name] << 'msg'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction_if_model_file(<<~RUBY, file_path)
errors.add(:name, 'msg')
RUBY
end

context 'when assigning' do
Expand All @@ -171,6 +277,29 @@ def expect_offense_if_model_file(code, file_path)
errors.messages[:name] = []
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_no_corrections_if_model_file(file_path)
end
end

context 'when using `clear` method' do
it 'registers and corrects an offense' do
expect_offense_if_model_file(<<~RUBY, file_path)
errors.messages[:name].clear
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction_if_model_file(<<~RUBY, file_path)
errors.delete(:name)
RUBY
end
end

context 'when using `keys` method' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY, file_path)
errors.messages[:name].keys
RUBY
end
end

Expand All @@ -197,6 +326,29 @@ def expect_offense_if_model_file(code, file_path)
errors.details[:name] = []
^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_no_corrections_if_model_file(file_path)
end
end

context 'when using `clear` method' do
it 'registers and corrects an offense' do
expect_offense_if_model_file(<<~RUBY, file_path)
errors.details[:name].clear
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
RUBY

expect_correction_if_model_file(<<~RUBY, file_path)
errors.delete(:name)
RUBY
end
end

context 'when using `keys` method' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY, file_path)
errors.details[:name].keys
RUBY
end
end

Expand Down