From f3f1df1476ba7fe223e5d8d6707a7675dc9fa597 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 18 Jun 2018 10:31:41 +0200 Subject: 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. --- .../concerns/issues/resolve_discussions.rb | 1 - rubocop/cop/gitlab/finder_with_find_by.rb | 52 ++++++++++++++++++++ rubocop/rubocop.rb | 1 + .../rubocop/cop/gitlab/finder_with_find_by_spec.rb | 56 ++++++++++++++++++++++ spec/services/issues/resolve_discussions_spec.rb | 4 +- 5 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 rubocop/cop/gitlab/finder_with_find_by.rb create mode 100644 spec/rubocop/cop/gitlab/finder_with_find_by_spec.rb diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb index 26eb274f4d5..455f761ca9b 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -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 diff --git a/rubocop/cop/gitlab/finder_with_find_by.rb b/rubocop/cop/gitlab/finder_with_find_by.rb new file mode 100644 index 00000000000..f45a37ddc06 --- /dev/null +++ b/rubocop/cop/gitlab/finder_with_find_by.rb @@ -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 diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index f05990232ab..aa7ae601f75 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -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' diff --git a/spec/rubocop/cop/gitlab/finder_with_find_by_spec.rb b/spec/rubocop/cop/gitlab/finder_with_find_by_spec.rb new file mode 100644 index 00000000000..7f689b196c5 --- /dev/null +++ b/spec/rubocop/cop/gitlab/finder_with_find_by_spec.rb @@ -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 diff --git a/spec/services/issues/resolve_discussions_spec.rb b/spec/services/issues/resolve_discussions_spec.rb index 13accc6ae1b..b6cfc09da65 100644 --- a/spec/services/issues/resolve_discussions_spec.rb +++ b/spec/services/issues/resolve_discussions_spec.rb @@ -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 } -- cgit v1.2.1