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.

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

WIP: Add cop for unscoped AR class methods #230

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# frozen_string_literal: true

require "rubocop"

module RuboCop
module Cop
module GitHub
# Public: A Rubocop to discourage unscoped calls to potentially dangerous ActiveRecord class methods.
#
# Examples:
#
# # bad
# class Widget < ActiveRecord::Base
# def self.do_class_business
# widget_count = ids.size
# end
# end
#
# # good
# class Widget < ActiveRecord::Base
# def self.do_class_business(user)
# user_widget_count = where(user: user).ids.size
# end
# end
class AvoidUnscopedActiveRecordClassMethodCalls < Base
MESSAGE_TEMPLATE = "Avoid using ActiveModel.%s without a scope."
DANGEROUS_METHODS = %i(ids).freeze

def_node_matcher :active_record?, <<~PATTERN
{
(const {nil? cbase} :ApplicationRecord)
(const (const {nil? cbase} :ActiveRecord) :Base)
}
PATTERN

def on_send(node)
return unless dangerous_method?(node)
return unless nil_receiver?(node)
return unless used_in_class_method?(node)
return unless class_is_active_record?(node)

add_offense(node, message: MESSAGE_TEMPLATE % node.method_name)
end

private

def dangerous_method?(node)
DANGEROUS_METHODS.include?(node.method_name)
end

def nil_receiver?(node)
node.receiver.nil?
end

def used_in_class_method?(node)
return false if node.nil?
return true if node.defs_type?
return false if node.def_type?
used_in_class_method?(node.parent)
end

def class_is_active_record?(node)
node.each_ancestor(:class).any? { |class_node| active_record?(class_node.parent_class) }
end
end
end
end
end
48 changes: 48 additions & 0 deletions test/test_avoid_unscoped_active_record_class_method_calls.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# frozen_string_literal: true

require_relative "cop_test"
require "minitest/autorun"
require "rubocop/cop/github/avoid_unscoped_active_record_class_method_calls"

class TestAvoidUnscopedActiveRecordClassMethodCalls < CopTest
def cop_class
RuboCop::Cop::GitHub::AvoidUnscopedActiveRecordClassMethodCalls
end

def test_offended_by_unscoped_call_from_active_record_class_method
offenses = investigate cop, <<-RUBY
class Widget < ApplicationRecord
def self.do_class_business
count = ids.size
end
end
RUBY

assert_equal 1, offenses.size
assert_equal "#{cop.name}: Avoid using ActiveModel.ids without a scope.", offenses.first.message
end

def test_unoffended_by_unscoped_call_from_non_active_record_class_method
offenses = investigate cop, <<-RUBY
class Widget < OtherClass
def self.do_class_business
count = ids.size
end
end
RUBY

assert_equal 0, offenses.size
end

def test_unoffended_by_unscoped_call_from_active_record_instance_method
offenses = investigate cop, <<-RUBY
class Widget < ApplicationRecord::Thing
def do_instance_business
count = ids.size
end
end
RUBY

assert_equal 0, offenses.size
end
end