From 9e1c4edeff43923146af9c625aefc9b9d0e41268 Mon Sep 17 00:00:00 2001 From: diachini Date: Fri, 5 Apr 2019 07:45:36 -0400 Subject: [PATCH] [Fix #6888] Fix an error for `Rails/ActiveRecordOverride` Fixes #6888. This PR fixes the following error for `Rails/ActiveRecordOverride` when there is no `parent_class` to match against. ```ruby % cat app/models/user.rb class User def initialize; end def create; end end ``` ```console % rubocop app/models/user.rb --only Rails/ActiveRecordOverride -d For /Users/dannyiachini/github/jpw/rubocop_ex: configuration from /Users/dannyiachini/github/jpw/rubocop_ex/.rubocop.yml Default configuration from /Users/dannyiachini/.gem/ruby/2.5.3/gems/rubocop-0.67.2/config/default.yml Inspecting 1 file Scanning /Users/dannyiachini/github/jpw/rubocop_ex/app/models/user.rb An error occurred while Rails/ActiveRecordOverride cop was inspecting /Users/dannyiachini/github/jpw/rubocop_ex/app/models/user.rb:6:2. undefined method `const_name' for nil:NilClass /Users/dannyiachini/.gem/ruby/2.5.3/gems/rubocop-0.67.2/lib/rubocop/cop/rails/active_record_override.rb:42:in `on_def' (snip) 1 file inspected, no offenses detected 1 error occurred: An error occurred while Rails/ActiveRecordOverride cop was inspecting /Users/dannyiachini/github/jpw/rubocop_ex/app/models/user.rb:6:2. Errors are usually caused by RuboCop bugs. Please, report your problems to RuboCop's issue tracker. https://github.com/rubocop-hq/rubocop/issues Mention the following information in the issue report: 0.67.2 (using Parser 2.6.2.0, running on ruby 2.5.3 x86_64-darwin17) ``` --- CHANGELOG.md | 2 ++ .../cop/rails/active_record_override.rb | 28 +++++++++++++++---- .../cop/rails/active_record_override_spec.rb | 14 ++++++++++ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b715bde3f2..97d2b2abf9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bug fixes * [#6914](https://github.com/rubocop-hq/rubocop/issues/6914): [Fix #6914] Fix an error for `Rails/RedundantAllowNil` when with interpolations. ([@Blue-Pix][]) +* [#6888](https://github.com/rubocop-hq/rubocop/issues/6888): Fix an error for `Rails/ActiveRecordOverride ` when no `parent_class` present. ([@diachini][]) ### Changes @@ -3937,3 +3938,4 @@ [@XrXr]: https://github.com/XrXr [@thomthom]: https://github.com/thomthom [@Blue-Pix]: https://github.com/Blue-Pix +[@diachini]: https://github.com/diachini diff --git a/lib/rubocop/cop/rails/active_record_override.rb b/lib/rubocop/cop/rails/active_record_override.rb index 19c392b1b5e..24851178553 100644 --- a/lib/rubocop/cop/rails/active_record_override.rb +++ b/lib/rubocop/cop/rails/active_record_override.rb @@ -29,17 +29,15 @@ class ActiveRecordOverride < Cop 'Use %s callbacks instead of overriding the Active Record ' \ 'method `%s`.'.freeze BAD_METHODS = %i[create destroy save update].freeze + ACTIVE_RECORD_CLASSES = %w[ApplicationRecord ActiveModel::Base + ActiveRecord::Base].freeze def on_def(node) method_name = node.method_name return unless BAD_METHODS.include?(node.method_name) - parent_parts = node.parent.node_parts - parent_class = parent_parts.take_while do |part| - !part.nil? && part.const_type? - end.last - return unless %w[ApplicationRecord ActiveModel::Base] - .include?(parent_class.const_name) + parent_class_name = find_parent_class_name(node) + return unless active_model?(parent_class_name) return unless node.descendants.any?(&:zsuper_type?) @@ -48,6 +46,10 @@ def on_def(node) private + def active_model?(parent_class_name) + ACTIVE_RECORD_CLASSES.include?(parent_class_name) + end + def callback_names(method_name) names = %w[before_ around_ after_].map do |prefix| "`#{prefix}#{method_name}`" @@ -61,6 +63,20 @@ def callback_names(method_name) def message(method_name) format(MSG, prefer: callback_names(method_name), bad: method_name) end + + def find_parent_class_name(node) + return nil unless node + + if node.class_type? + parent_class_name = node.node_parts[1] + + return nil if parent_class_name.nil? + + return parent_class_name.source + end + + find_parent_class_name(node.parent) + end end end end diff --git a/spec/rubocop/cop/rails/active_record_override_spec.rb b/spec/rubocop/cop/rails/active_record_override_spec.rb index 97bc6b85362..8469b264691 100644 --- a/spec/rubocop/cop/rails/active_record_override_spec.rb +++ b/spec/rubocop/cop/rails/active_record_override_spec.rb @@ -41,6 +41,8 @@ def save it 'registers an offense when overriding update' do expect_offense(<<-RUBY.strip_indent) class X < ActiveModel::Base + module_function + def update ^^^^^^^^^^ Use `before_update`, `around_update`, or `after_update` callbacks instead of overriding the Active Record method `update`. super @@ -84,4 +86,16 @@ def save RUBY end end + + context 'when class has no parent specified' do + it 'registers no offense when overriding save' do + expect_no_offenses(<<-RUBY.strip_indent) + class X + def initialize; end + + def save; end + end + RUBY + end + end end