From c737e89b42d8498fd001541c0ee0c9028dd09157 Mon Sep 17 00:00:00 2001 From: Brian Hawley Date: Tue, 5 Jul 2022 10:21:33 -0700 Subject: [PATCH] Fixes for errors in Rails/DeprecatedActiveModelErrorsMethods - Fixed a NoMethodError on nil for `errors.keys` in a model. - Fixed a bad autocorrection of `errors.details[:name] << value`. There isn't really a replacement for this one. - The `values`, `to_h`, and `to_xml` methods are deprecated too. - Made the range calculation more exact so the replacement is easier. - Did some refactors prompted by rubocop complaints. - Fixed a misspelling of autocorrectable. - Added missing correction assertions to test cases. - Added missing test cases. --- ...cated_errors_bad_details_autocorrection.md | 1 + .../fix_deprecated_errors_missing_methods.md | 1 + .../fix_deprecated_errors_nil_dereference.md | 1 + .../deprecated_active_model_errors_methods.rb | 30 +- ...ecated_active_model_errors_methods_spec.rb | 282 +++++++++++++++++- 5 files changed, 296 insertions(+), 19 deletions(-) create mode 100644 changelog/fix_deprecated_errors_bad_details_autocorrection.md create mode 100644 changelog/fix_deprecated_errors_missing_methods.md create mode 100644 changelog/fix_deprecated_errors_nil_dereference.md diff --git a/changelog/fix_deprecated_errors_bad_details_autocorrection.md b/changelog/fix_deprecated_errors_bad_details_autocorrection.md new file mode 100644 index 0000000000..d1b6b8a169 --- /dev/null +++ b/changelog/fix_deprecated_errors_bad_details_autocorrection.md @@ -0,0 +1 @@ +* [#739](https://github.com/rubocop/rubocop-rails/pull/739): Fix a bad autocorrection for `errors.details[:name] << value` in Rails/DeprecatedActiveModelErrorsMethods. ([@BrianHawley][]) diff --git a/changelog/fix_deprecated_errors_missing_methods.md b/changelog/fix_deprecated_errors_missing_methods.md new file mode 100644 index 0000000000..e52c1fcb72 --- /dev/null +++ b/changelog/fix_deprecated_errors_missing_methods.md @@ -0,0 +1 @@ +* [#739](https://github.com/rubocop/rubocop-rails/pull/739): Rails/DeprecatedActiveModelErrorsMethods was missing the deprecated `values`, `to_h`, and `to_xml` methods. ([@BrianHawley][]) diff --git a/changelog/fix_deprecated_errors_nil_dereference.md b/changelog/fix_deprecated_errors_nil_dereference.md new file mode 100644 index 0000000000..57d1bfe284 --- /dev/null +++ b/changelog/fix_deprecated_errors_nil_dereference.md @@ -0,0 +1 @@ +* [#739](https://github.com/rubocop/rubocop-rails/pull/739): Fix a NoMethodError on nil for `errors.keys` in a model in Rails/DeprecatedActiveModelErrorsMethods. ([@BrianHawley][]) diff --git a/lib/rubocop/cop/rails/deprecated_active_model_errors_methods.rb b/lib/rubocop/cop/rails/deprecated_active_model_errors_methods.rb index e2c9d79e93..a20cb42ce7 100644 --- a/lib/rubocop/cop/rails/deprecated_active_model_errors_methods.rb +++ b/lib/rubocop/cop/rails/deprecated_active_model_errors_methods.rb @@ -37,7 +37,8 @@ class DeprecatedActiveModelErrorsMethods < Base extend AutoCorrector MSG = 'Avoid manipulating ActiveModel errors as hash directly.' - AUTOCORECTABLE_METHODS = %i[<< clear keys].freeze + AUTOCORRECTABLE_METHODS = %i[<< clear keys].freeze + INCOMPATIBLE_METHODS = %i[keys values to_h to_xml].freeze MANIPULATIVE_METHODS = Set[ *%i[ @@ -55,7 +56,7 @@ class DeprecatedActiveModelErrorsMethods < Base { #root_manipulation? #root_assignment? - #errors_keys? + #errors_deprecated? #messages_details_manipulation? #messages_details_assignment? } @@ -77,10 +78,10 @@ class DeprecatedActiveModelErrorsMethods < Base ...) PATTERN - def_node_matcher :errors_keys?, <<~PATTERN + def_node_matcher :errors_deprecated?, <<~PATTERN (send (send #receiver_matcher :errors) - :keys) + {:keys :values :to_h :to_xml}) PATTERN def_node_matcher :messages_details_manipulation?, <<~PATTERN @@ -106,10 +107,10 @@ class DeprecatedActiveModelErrorsMethods < Base def on_send(node) any_manipulation?(node) do - next if node.method?(:keys) && target_rails_version <= 6.0 + next if target_rails_version <= 6.0 && INCOMPATIBLE_METHODS.include?(node.method_name) add_offense(node) do |corrector| - next unless AUTOCORECTABLE_METHODS.include?(node.method_name) + next if skip_autocorrect?(node) autocorrect(corrector, node) end @@ -118,14 +119,16 @@ def on_send(node) private + def skip_autocorrect?(node) + receiver = node.receiver.receiver + !AUTOCORRECTABLE_METHODS.include?(node.method_name) || ( + receiver&.send_type? && receiver&.method?(:details) && node.method?(:<<) + ) + end + 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) @@ -133,11 +136,12 @@ def autocorrect(corrector, node) 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 + 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 diff --git a/spec/rubocop/cop/rails/deprecated_active_model_errors_methods_spec.rb b/spec/rubocop/cop/rails/deprecated_active_model_errors_methods_spec.rb index 41e1219594..8f7a66109f 100644 --- a/spec/rubocop/cop/rails/deprecated_active_model_errors_methods_spec.rb +++ b/spec/rubocop/cop/rails/deprecated_active_model_errors_methods_spec.rb @@ -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' @@ -20,6 +20,8 @@ user.errors[:name] = [] ^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. RUBY + + expect_no_corrections end end @@ -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 @@ -63,7 +65,42 @@ end end - context 'Rails <= 6.0', :rails60 do + context 'when using `values` method' do + it 'registers an offense' do + expect_offense(<<~RUBY, file_path) + user.errors.values + ^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. + RUBY + + expect_no_corrections + end + end + + context 'when using `to_h` method' do + it 'registers an offense' do + expect_offense(<<~RUBY, file_path) + user.errors.to_h + ^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. + RUBY + + expect_no_corrections + end + end + + context 'when using `to_xml` method' do + it 'registers an offense' do + expect_offense(<<~RUBY, file_path) + user.errors.to_xml + ^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. + RUBY + + expect_no_corrections + end + end + end + + 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 @@ -78,6 +115,30 @@ end end end + + context 'when using `values` method' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, file_path) + user.errors.values + RUBY + end + end + + context 'when using `to_h` method' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, file_path) + user.errors.to_h + RUBY + end + end + + context 'when using `to_xml` method' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, file_path) + user.errors.to_xml + RUBY + end + end end context 'when modifying errors.messages' do @@ -98,16 +159,41 @@ 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 context 'when modifying errors.details' do - it 'registers an offense' do + it 'registers and corrects an offense' do expect_offense(<<~RUBY, file_path) user.errors.details[:name] << {} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. RUBY + + expect_no_corrections end context 'when assigning' do @@ -116,6 +202,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 @@ -131,12 +240,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 @@ -145,6 +266,103 @@ 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. + RUBY + + expect_correction_if_model_file(<<~RUBY, file_path) + errors.attribute_names + RUBY + end + end + + context 'when using `values` method' do + it 'registers an offense' do + expect_offense_if_model_file(<<~RUBY, file_path) + errors.values + ^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. + RUBY + + expect_no_corrections_if_model_file(file_path) + end + end + + context 'when using `to_h` method' do + it 'registers an offense' do + expect_offense_if_model_file(<<~RUBY, file_path) + errors.to_h + ^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. + RUBY + + expect_no_corrections_if_model_file(file_path) + end + end + + context 'when using `to_xml` method' do + it 'registers an offense' do + expect_offense_if_model_file(<<~RUBY, file_path) + errors.to_xml + ^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly. + RUBY + + expect_no_corrections_if_model_file(file_path) + 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 + + context 'when using `values` method' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, file_path) + user.errors.values + RUBY + end + end + + context 'when using `to_h` method' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, file_path) + user.errors.to_h + RUBY + end + end + + context 'when using `to_xml` method' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, file_path) + user.errors.to_xml + RUBY + end end end @@ -163,6 +381,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 @@ -171,6 +393,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 @@ -189,6 +434,8 @@ 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 context 'when assigning' do @@ -197,6 +444,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