Add a cop for `FinderMethods`
This notifies developers when calling `find(_by!)` chained on `execute`. And suggests using the methods from `FinderMethods`. These will perform the correct authorization checks on the resource when it is found.
This commit is contained in:
parent
03b8893775
commit
f3f1df1476
|
@ -14,7 +14,6 @@ module Issues
|
|||
def merge_request_to_resolve_discussions_of
|
||||
strong_memoize(:merge_request_to_resolve_discussions_of) do
|
||||
MergeRequestsFinder.new(current_user, project_id: project.id)
|
||||
.execute
|
||||
.find_by(iid: merge_request_to_resolve_discussions_of_iid)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,52 @@
|
|||
module RuboCop
|
||||
module Cop
|
||||
module Gitlab
|
||||
class FinderWithFindBy < RuboCop::Cop::Cop
|
||||
FIND_PATTERN = /\Afind(_by\!?)?\z/
|
||||
ALLOWED_MODULES = ['FinderMethods'].freeze
|
||||
|
||||
def message(used_method)
|
||||
<<~MSG
|
||||
Don't chain finders `#execute` method with `##{used_method}`.
|
||||
Instead include `FinderMethods` in the Finder and call `##{used_method}`
|
||||
directly on the finder instance.
|
||||
|
||||
This will make sure all authorization checks are performed on the resource.
|
||||
MSG
|
||||
end
|
||||
|
||||
def on_send(node)
|
||||
if find_on_execute?(node) && !allowed_module?(node)
|
||||
add_offense(node, location: :selector, message: message(node.method_name))
|
||||
end
|
||||
end
|
||||
|
||||
def autocorrect(node)
|
||||
lambda do |corrector|
|
||||
upto_including_execute = node.descendants.first.source_range
|
||||
before_execute = node.descendants[1].source_range
|
||||
range_to_remove = node.source_range
|
||||
.with(begin_pos: before_execute.end_pos,
|
||||
end_pos: upto_including_execute.end_pos)
|
||||
|
||||
corrector.remove(range_to_remove)
|
||||
end
|
||||
end
|
||||
|
||||
def find_on_execute?(node)
|
||||
chained_on_node = node.descendants.first
|
||||
node.method_name.to_s =~ FIND_PATTERN &&
|
||||
chained_on_node&.method_name == :execute
|
||||
end
|
||||
|
||||
def allowed_module?(node)
|
||||
ALLOWED_MODULES.include?(node.parent_module_name)
|
||||
end
|
||||
|
||||
def method_name_for_node(node)
|
||||
children[1].to_s
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -2,6 +2,7 @@
|
|||
require_relative 'cop/gitlab/module_with_instance_variables'
|
||||
require_relative 'cop/gitlab/predicate_memoization'
|
||||
require_relative 'cop/gitlab/httparty'
|
||||
require_relative 'cop/gitlab/finder_with_find_by'
|
||||
require_relative 'cop/include_sidekiq_worker'
|
||||
require_relative 'cop/avoid_return_from_blocks'
|
||||
require_relative 'cop/avoid_break_from_strong_memoize'
|
||||
|
|
|
@ -0,0 +1,56 @@
|
|||
require 'spec_helper'
|
||||
|
||||
require 'rubocop'
|
||||
require 'rubocop/rspec/support'
|
||||
|
||||
require_relative '../../../../rubocop/cop/gitlab/finder_with_find_by'
|
||||
|
||||
describe RuboCop::Cop::Gitlab::FinderWithFindBy do
|
||||
include CopHelper
|
||||
|
||||
subject(:cop) { described_class.new }
|
||||
|
||||
context 'when calling execute.find' do
|
||||
let(:source) do
|
||||
<<~SRC
|
||||
DummyFinder.new(some_args)
|
||||
.execute
|
||||
.find_by!(1)
|
||||
SRC
|
||||
end
|
||||
let(:corrected_source) do
|
||||
<<~SRC
|
||||
DummyFinder.new(some_args)
|
||||
.find_by!(1)
|
||||
SRC
|
||||
end
|
||||
|
||||
it 'registers an offence' do
|
||||
inspect_source(source)
|
||||
|
||||
expect(cop.offenses.size).to eq(1)
|
||||
end
|
||||
|
||||
it 'can autocorrect the source' do
|
||||
expect(autocorrect_source(source)).to eq(corrected_source)
|
||||
end
|
||||
|
||||
context 'when called within the `FinderMethods` module' do
|
||||
let(:source) do
|
||||
<<~SRC
|
||||
module FinderMethods
|
||||
def find_by!(*args)
|
||||
execute.find_by!(args)
|
||||
end
|
||||
end
|
||||
SRC
|
||||
end
|
||||
|
||||
it 'does not register an offence' do
|
||||
inspect_source(source)
|
||||
|
||||
expect(cop.offenses).to be_empty
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -31,10 +31,8 @@ describe Issues::ResolveDiscussions do
|
|||
|
||||
it "only queries for the merge request once" do
|
||||
fake_finder = double
|
||||
fake_results = double
|
||||
|
||||
expect(fake_finder).to receive(:execute).and_return(fake_results).exactly(1)
|
||||
expect(fake_results).to receive(:find_by).exactly(1)
|
||||
expect(fake_finder).to receive(:find_by).exactly(1)
|
||||
expect(MergeRequestsFinder).to receive(:new).and_return(fake_finder).exactly(1)
|
||||
|
||||
2.times { service.merge_request_to_resolve_discussions_of }
|
||||
|
|
Loading…
Reference in New Issue