From 91c40973dc620430b173ea2df968587d2a708dff Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 27 Aug 2018 17:30:20 +0200 Subject: Added RuboCop cops to enforce code reuse rules These Cops enforces the code reuse rules as defined in merge request https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21254. --- rubocop/code_reuse_helpers.rb | 156 ++++++++++++++ rubocop/cop/code_reuse/active_record.rb | 170 +++++++++++++++ rubocop/cop/code_reuse/finder.rb | 39 ++++ rubocop/cop/code_reuse/presenter.rb | 41 ++++ rubocop/cop/code_reuse/serializer.rb | 41 ++++ rubocop/cop/code_reuse/service_class.rb | 39 ++++ rubocop/cop/code_reuse/worker.rb | 56 +++++ rubocop/rubocop.rb | 6 + spec/rubocop/code_reuse_helpers_spec.rb | 249 ++++++++++++++++++++++ spec/rubocop/cop/code_reuse/active_record_spec.rb | 138 ++++++++++++ spec/rubocop/cop/code_reuse/finder_spec.rb | 77 +++++++ spec/rubocop/cop/code_reuse/presenter_spec.rb | 117 ++++++++++ spec/rubocop/cop/code_reuse/serializer_spec.rb | 117 ++++++++++ spec/rubocop/cop/code_reuse/service_class_spec.rb | 89 ++++++++ spec/rubocop/cop/code_reuse/worker_spec.rb | 104 +++++++++ 15 files changed, 1439 insertions(+) create mode 100644 rubocop/code_reuse_helpers.rb create mode 100644 rubocop/cop/code_reuse/active_record.rb create mode 100644 rubocop/cop/code_reuse/finder.rb create mode 100644 rubocop/cop/code_reuse/presenter.rb create mode 100644 rubocop/cop/code_reuse/serializer.rb create mode 100644 rubocop/cop/code_reuse/service_class.rb create mode 100644 rubocop/cop/code_reuse/worker.rb create mode 100644 spec/rubocop/code_reuse_helpers_spec.rb create mode 100644 spec/rubocop/cop/code_reuse/active_record_spec.rb create mode 100644 spec/rubocop/cop/code_reuse/finder_spec.rb create mode 100644 spec/rubocop/cop/code_reuse/presenter_spec.rb create mode 100644 spec/rubocop/cop/code_reuse/serializer_spec.rb create mode 100644 spec/rubocop/cop/code_reuse/service_class_spec.rb create mode 100644 spec/rubocop/cop/code_reuse/worker_spec.rb diff --git a/rubocop/code_reuse_helpers.rb b/rubocop/code_reuse_helpers.rb new file mode 100644 index 00000000000..0929a55d901 --- /dev/null +++ b/rubocop/code_reuse_helpers.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +module RuboCop + module CodeReuseHelpers + # Returns true for a `(send const ...)` node. + def send_to_constant?(node) + node.type == :send && node.children&.first&.type == :const + end + + # Returns `true` if the name of the receiving constant ends with a given + # `String`. + def send_receiver_name_ends_with?(node, suffix) + return false unless send_to_constant?(node) + + receiver_name = name_of_receiver(node) + + receiver_name != suffix && + receiver_name.end_with?(suffix) + end + + # Returns the file path (as a `String`) for an AST node. + def file_path_for_node(node) + node.location.expression.source_buffer.name + end + + # Returns the name of a constant node. + # + # Given the AST node `(const nil :Foo)`, this method will return `:Foo`. + def name_of_constant(node) + node.children[1] + end + + # Returns true if the given node resides in app/finders or ee/app/finders. + def in_finder?(node) + in_directory?(node, 'finders') + end + + # Returns true if the given node resides in app/models or ee/app/models. + def in_model?(node) + in_directory?(node, 'models') + end + + # Returns true if the given node resides in app/services or ee/app/services. + def in_service_class?(node) + in_directory?(node, 'services') + end + + # Returns true if the given node resides in app/presenters or + # ee/app/presenters. + def in_presenter?(node) + in_directory?(node, 'presenters') + end + + # Returns true if the given node resides in app/serializers or + # ee/app/serializers. + def in_serializer?(node) + in_directory?(node, 'serializers') + end + + # Returns true if the given node resides in app/workers or ee/app/workers. + def in_worker?(node) + in_directory?(node, 'workers') + end + + # Returns true if the given node resides in app/controllers or + # ee/app/controllers. + def in_controller?(node) + in_directory?(node, 'controllers') + end + + # Returns true if the given node resides in lib/api or ee/lib/api. + def in_api?(node) + file_path_for_node(node).start_with?( + File.join(ce_lib_directory, 'api'), + File.join(ee_lib_directory, 'api') + ) + end + + # Returns `true` if the given AST node resides in the given directory, + # relative to app and/or ee/app. + def in_directory?(node, directory) + file_path_for_node(node).start_with?( + File.join(ce_app_directory, directory), + File.join(ee_app_directory, directory) + ) + end + + # Returns the receiver name of a send node. + # + # For the AST node `(send (const nil :Foo) ...)` this would return + # `'Foo'`. + def name_of_receiver(node) + name_of_constant(node.children.first).to_s + end + + # Yields every defined class method in the given AST node. + def each_class_method(node) + return to_enum(__method__, node) unless block_given? + + # class << self + # def foo + # end + # end + node.each_descendant(:sclass) do |sclass| + sclass.each_descendant(:def) do |def_node| + yield def_node + end + end + + # def self.foo + # end + node.each_descendant(:defs) do |defs_node| + yield defs_node + end + end + + # Yields every send node found in the given AST node. + def each_send_node(node, &block) + node.each_descendant(:send, &block) + end + + # Registers a RuboCop offense for a `(send)` node with a receiver that ends + # with a given suffix. + # + # node - The AST node to check. + # suffix - The suffix of the receiver name, such as "Finder". + # message - The message to use for the offense. + def disallow_send_to(node, suffix, message) + each_send_node(node) do |send_node| + next unless send_receiver_name_ends_with?(send_node, suffix) + + add_offense(send_node, location: :expression, message: message) + end + end + + def ce_app_directory + File.join(rails_root, 'app') + end + + def ee_app_directory + File.join(rails_root, 'ee', 'app') + end + + def ce_lib_directory + File.join(rails_root, 'lib') + end + + def ee_lib_directory + File.join(rails_root, 'ee', 'lib') + end + + def rails_root + File.expand_path('..', __dir__) + end + end +end diff --git a/rubocop/cop/code_reuse/active_record.rb b/rubocop/cop/code_reuse/active_record.rb new file mode 100644 index 00000000000..d25e8548fd0 --- /dev/null +++ b/rubocop/cop/code_reuse/active_record.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module CodeReuse + # Cop that blacklists the use of ActiveRecord methods outside of models. + class ActiveRecord < RuboCop::Cop::Cop + include CodeReuseHelpers + + MSG = 'This method can only be used inside an ActiveRecord model' + + # Various methods from ActiveRecord::Querying that are blacklisted. We + # exclude some generic ones such as `any?` and `first`, as these may + # lead to too many false positives, since `Array` also supports these + # methods. + # + # The keys of this Hash are the blacklisted method names. The values are + # booleans that indicate if the method should only be blacklisted if any + # arguments are provided. + NOT_ALLOWED = { + average: true, + calculate: true, + count_by_sql: true, + create_with: true, + distinct: false, + eager_load: true, + except: true, + exists?: true, + find_by: true, + find_by!: true, + find_by_sql: true, + find_each: true, + find_in_batches: true, + find_or_create_by: true, + find_or_create_by!: true, + find_or_initialize_by: true, + first!: false, + first_or_create: true, + first_or_create!: true, + first_or_initialize: true, + from: true, + group: true, + having: true, + ids: false, + includes: true, + joins: true, + limit: true, + lock: false, + many?: false, + none: false, + offset: true, + order: true, + pluck: true, + preload: true, + readonly: false, + references: true, + reorder: true, + rewhere: true, + sum: false, + take: false, + take!: false, + unscope: false, + where: false, + with: true + }.freeze + + # Directories that allow the use of the blacklisted methods. These + # directories are checked relative to both . and ee/ + WHITELISTED_DIRECTORIES = %w[ + app/models + config + danger + db + lib/backup + lib/banzai + lib/gitlab/background_migration + lib/gitlab/cycle_analytics + lib/gitlab/database + lib/gitlab/import_export + lib/gitlab/project_authorizations + lib/gitlab/sql + lib/system_check + lib/tasks + qa + rubocop + spec + ].freeze + + def on_send(node) + return if in_whitelisted_directory?(node) + + receiver = node.children[0] + send_name = node.children[1] + first_arg = node.children[2] + + if receiver && NOT_ALLOWED.key?(send_name) + # If the rule requires an argument to be given, but none are + # provided, we won't register an offense. This prevents us from + # adding offenses for `project.group`, while still covering + # `Project.group(:name)`. + return if NOT_ALLOWED[send_name] && !first_arg + + add_offense(node, location: :selector) + end + end + + # Returns true if the node resides in one of the whitelisted + # directories. + def in_whitelisted_directory?(node) + path = file_path_for_node(node) + + WHITELISTED_DIRECTORIES.any? do |directory| + path.start_with?( + File.join(rails_root, directory), + File.join(rails_root, 'ee', directory) + ) + end + end + + # We can not auto correct code like this, as it requires manual + # refactoring. Instead, we'll just whitelist the surrounding scope. + # + # Despite this method's presence, you should not use it. This method + # exists to make it possible to whitelist large chunks of offenses we + # can't fix in the short term. If you are writing new code, follow the + # code reuse guidelines, instead of whitelisting any new offenses. + def autocorrect(node) + scope = surrounding_scope_of(node) + indent = indentation_of(scope) + + lambda do |corrector| + # This prevents us from inserting the same enable/disable comment + # for a method or block that has multiple offenses. + next if whitelisted_scopes.include?(scope) + + corrector.insert_before( + scope.source_range, + "# rubocop: disable #{cop_name}\n#{indent}" + ) + + corrector.insert_after( + scope.source_range, + "\n#{indent}# rubocop: enable #{cop_name}" + ) + + whitelisted_scopes << scope + end + end + + def indentation_of(node) + ' ' * node.loc.expression.source_line[/\A */].length + end + + def surrounding_scope_of(node) + %i[def defs block begin].each do |type| + if (found = node.each_ancestor(type).first) + return found + end + end + end + + def whitelisted_scopes + @whitelisted_scopes ||= Set.new + end + end + end + end +end diff --git a/rubocop/cop/code_reuse/finder.rb b/rubocop/cop/code_reuse/finder.rb new file mode 100644 index 00000000000..1d70befe79b --- /dev/null +++ b/rubocop/cop/code_reuse/finder.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module CodeReuse + # Cop that enforces various code reuse rules for Finders. + class Finder < RuboCop::Cop::Cop + include CodeReuseHelpers + + IN_FINDER = 'Finders can not be used inside a Finder.' + + IN_MODEL_CLASS_METHOD = + 'Finders can not be used inside model class methods.' + + SUFFIX = 'Finder' + + def on_class(node) + if in_finder?(node) + check_finder(node) + elsif in_model?(node) + check_model_class_methods(node) + end + end + + def check_finder(node) + disallow_send_to(node, SUFFIX, IN_FINDER) + end + + def check_model_class_methods(node) + each_class_method(node) do |def_node| + disallow_send_to(def_node, SUFFIX, IN_MODEL_CLASS_METHOD) + end + end + end + end + end +end diff --git a/rubocop/cop/code_reuse/presenter.rb b/rubocop/cop/code_reuse/presenter.rb new file mode 100644 index 00000000000..5f8f2839ca6 --- /dev/null +++ b/rubocop/cop/code_reuse/presenter.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers.rb' + +module RuboCop + module Cop + module CodeReuse + # Cop that enforces various code reuse rules for Presenter classes. + class Presenter < RuboCop::Cop::Cop + include CodeReuseHelpers + + IN_SERVICE = 'Presenters can not be used in a Service class.' + IN_FINDER = 'Presenters can not be used in a Finder.' + IN_PRESENTER = 'Presenters can not be used in a Presenter.' + IN_SERIALIZER = 'Presenters can not be used in a Serializer.' + IN_MODEL = 'Presenters can not be used in a model.' + IN_WORKER = 'Presenters can not be used in a worker.' + SUFFIX = 'Presenter' + + def on_class(node) + message = + if in_service_class?(node) + IN_SERVICE + elsif in_finder?(node) + IN_FINDER + elsif in_presenter?(node) + IN_PRESENTER + elsif in_serializer?(node) + IN_SERIALIZER + elsif in_model?(node) + IN_MODEL + elsif in_worker?(node) + IN_WORKER + end + + disallow_send_to(node, SUFFIX, message) if message + end + end + end + end +end diff --git a/rubocop/cop/code_reuse/serializer.rb b/rubocop/cop/code_reuse/serializer.rb new file mode 100644 index 00000000000..2212c50514e --- /dev/null +++ b/rubocop/cop/code_reuse/serializer.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers.rb' + +module RuboCop + module Cop + module CodeReuse + # Cop that enforces various code reuse rules for Serializer classes. + class Serializer < RuboCop::Cop::Cop + include CodeReuseHelpers + + IN_SERVICE = 'Serializers can not be used in a Service class.' + IN_FINDER = 'Serializers can not be used in a Finder.' + IN_PRESENTER = 'Serializers can not be used in a Presenter.' + IN_SERIALIZER = 'Serializers can not be used in a Serializer.' + IN_MODEL = 'Serializers can not be used in a model.' + IN_WORKER = 'Serializers can not be used in a worker.' + SUFFIX = 'Serializer' + + def on_class(node) + message = + if in_service_class?(node) + IN_SERVICE + elsif in_finder?(node) + IN_FINDER + elsif in_presenter?(node) + IN_PRESENTER + elsif in_serializer?(node) + IN_SERIALIZER + elsif in_model?(node) + IN_MODEL + elsif in_worker?(node) + IN_WORKER + end + + disallow_send_to(node, SUFFIX, message) if message + end + end + end + end +end diff --git a/rubocop/cop/code_reuse/service_class.rb b/rubocop/cop/code_reuse/service_class.rb new file mode 100644 index 00000000000..768b43fb684 --- /dev/null +++ b/rubocop/cop/code_reuse/service_class.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers.rb' + +module RuboCop + module Cop + module CodeReuse + # Cop that enforces various code reuse rules for Service classes. + class ServiceClass < RuboCop::Cop::Cop + include CodeReuseHelpers + + IN_FINDER = 'Service classes can not be used in a Finder.' + IN_PRESENTER = 'Service classes can not be used in a Presenter.' + IN_SERIALIZER = 'Service classes can not be used in a Serializer.' + IN_MODEL = 'Service classes can not be used in a model.' + SUFFIX = 'Service' + + def on_class(node) + check_all_send_nodes(node) + end + + def check_all_send_nodes(node) + message = + if in_finder?(node) + IN_FINDER + elsif in_presenter?(node) + IN_PRESENTER + elsif in_serializer?(node) + IN_SERIALIZER + elsif in_model?(node) + IN_MODEL + end + + disallow_send_to(node, SUFFIX, message) if message + end + end + end + end +end diff --git a/rubocop/cop/code_reuse/worker.rb b/rubocop/cop/code_reuse/worker.rb new file mode 100644 index 00000000000..e38d2783d0f --- /dev/null +++ b/rubocop/cop/code_reuse/worker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers.rb' + +module RuboCop + module Cop + module CodeReuse + # Cop that enforces various code reuse rules for workers. + class Worker < RuboCop::Cop::Cop + include CodeReuseHelpers + + IN_CONTROLLER = 'Workers can not be used in a controller.' + IN_API = 'Workers can not be used in a Grape API.' + IN_FINDER = 'Workers can not be used in a Finder.' + IN_PRESENTER = 'Workers can not be used in a Presenter.' + IN_SERIALIZER = 'Workers can not be used in a Serializer.' + + IN_MODEL_CLASS_METHOD = + 'Workers can not be used in model class methods.' + + SUFFIX = 'Worker' + + def on_class(node) + if in_model?(node) + check_model_class_methods(node) + else + check_all_send_nodes(node) + end + end + + def check_all_send_nodes(node) + message = + if in_controller?(node) + IN_CONTROLLER + elsif in_api?(node) + IN_API + elsif in_finder?(node) + IN_FINDER + elsif in_presenter?(node) + IN_PRESENTER + elsif in_serializer?(node) + IN_SERIALIZER + end + + disallow_send_to(node, SUFFIX, message) if message + end + + def check_model_class_methods(node) + each_class_method(node) do |def_node| + disallow_send_to(def_node, SUFFIX, IN_MODEL_CLASS_METHOD) + end + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index d823fa4edb1..46bd7d3bec6 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -30,3 +30,9 @@ require_relative 'cop/rspec/factories_in_migration_specs' require_relative 'cop/sidekiq_options_queue' require_relative 'cop/destroy_all' require_relative 'cop/ruby_interpolation_in_translation' +require_relative 'code_reuse_helpers' +require_relative 'cop/code_reuse/finder' +require_relative 'cop/code_reuse/service_class' +require_relative 'cop/code_reuse/presenter' +require_relative 'cop/code_reuse/serializer' +require_relative 'cop/code_reuse/active_record' diff --git a/spec/rubocop/code_reuse_helpers_spec.rb b/spec/rubocop/code_reuse_helpers_spec.rb new file mode 100644 index 00000000000..2720141aad2 --- /dev/null +++ b/spec/rubocop/code_reuse_helpers_spec.rb @@ -0,0 +1,249 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'parser/current' +require_relative '../../rubocop/code_reuse_helpers' + +describe RuboCop::CodeReuseHelpers do + def parse_source(source, path = 'foo.rb') + buffer = Parser::Source::Buffer.new(path) + buffer.source = source + + builder = RuboCop::AST::Builder.new + parser = Parser::CurrentRuby.new(builder) + + parser.parse(buffer) + end + + let(:cop) do + Class.new do + include RuboCop::CodeReuseHelpers + end.new + end + + describe '#send_to_constant?' do + it 'returns true when sending to a constant' do + node = parse_source('Foo.bar') + + expect(cop.send_to_constant?(node)).to eq(true) + end + + it 'returns false when sending to something other than a constant' do + node = parse_source('10') + + expect(cop.send_to_constant?(node)).to eq(false) + end + end + + describe '#send_receiver_name_ends_with?' do + it 'returns true when the receiver ends with a suffix' do + node = parse_source('FooFinder.new') + + expect(cop.send_receiver_name_ends_with?(node, 'Finder')).to eq(true) + end + + it 'returns false when the receiver is the same as a suffix' do + node = parse_source('Finder.new') + + expect(cop.send_receiver_name_ends_with?(node, 'Finder')).to eq(false) + end + end + + describe '#file_path_for_node' do + it 'returns the file path of a node' do + node = parse_source('10') + path = cop.file_path_for_node(node) + + expect(path).to eq('foo.rb') + end + end + + describe '#name_of_constant' do + it 'returns the name of a constant' do + node = parse_source('Foo') + + expect(cop.name_of_constant(node)).to eq(:Foo) + end + end + + describe '#in_finder?' do + it 'returns true for a node in the finders directory' do + node = parse_source('10', Rails.root.join('app', 'finders', 'foo.rb')) + + expect(cop.in_finder?(node)).to eq(true) + end + + it 'returns false for a node outside the finders directory' do + node = parse_source('10', Rails.root.join('app', 'foo', 'foo.rb')) + + expect(cop.in_finder?(node)).to eq(false) + end + end + + describe '#in_model?' do + it 'returns true for a node in the models directory' do + node = parse_source('10', Rails.root.join('app', 'models', 'foo.rb')) + + expect(cop.in_model?(node)).to eq(true) + end + + it 'returns false for a node outside the models directory' do + node = parse_source('10', Rails.root.join('app', 'foo', 'foo.rb')) + + expect(cop.in_model?(node)).to eq(false) + end + end + + describe '#in_service_class?' do + it 'returns true for a node in the services directory' do + node = parse_source('10', Rails.root.join('app', 'services', 'foo.rb')) + + expect(cop.in_service_class?(node)).to eq(true) + end + + it 'returns false for a node outside the services directory' do + node = parse_source('10', Rails.root.join('app', 'foo', 'foo.rb')) + + expect(cop.in_service_class?(node)).to eq(false) + end + end + + describe '#in_presenter?' do + it 'returns true for a node in the presenters directory' do + node = parse_source('10', Rails.root.join('app', 'presenters', 'foo.rb')) + + expect(cop.in_presenter?(node)).to eq(true) + end + + it 'returns false for a node outside the presenters directory' do + node = parse_source('10', Rails.root.join('app', 'foo', 'foo.rb')) + + expect(cop.in_presenter?(node)).to eq(false) + end + end + + describe '#in_serializer?' do + it 'returns true for a node in the serializers directory' do + node = parse_source('10', Rails.root.join('app', 'serializers', 'foo.rb')) + + expect(cop.in_serializer?(node)).to eq(true) + end + + it 'returns false for a node outside the serializers directory' do + node = parse_source('10', Rails.root.join('app', 'foo', 'foo.rb')) + + expect(cop.in_serializer?(node)).to eq(false) + end + end + + describe '#in_worker?' do + it 'returns true for a node in the workers directory' do + node = parse_source('10', Rails.root.join('app', 'workers', 'foo.rb')) + + expect(cop.in_worker?(node)).to eq(true) + end + + it 'returns false for a node outside the workers directory' do + node = parse_source('10', Rails.root.join('app', 'foo', 'foo.rb')) + + expect(cop.in_worker?(node)).to eq(false) + end + end + + describe '#in_api?' do + it 'returns true for a node in the API directory' do + node = parse_source('10', Rails.root.join('lib', 'api', 'foo.rb')) + + expect(cop.in_api?(node)).to eq(true) + end + + it 'returns false for a node outside the API directory' do + node = parse_source('10', Rails.root.join('lib', 'foo', 'foo.rb')) + + expect(cop.in_api?(node)).to eq(false) + end + end + + describe '#in_directory?' do + it 'returns true for a directory in the CE app/ directory' do + node = parse_source('10', Rails.root.join('app', 'models', 'foo.rb')) + + expect(cop.in_directory?(node, 'models')).to eq(true) + end + + it 'returns true for a directory in the EE app/ directory' do + node = + parse_source('10', Rails.root.join('ee', 'app', 'models', 'foo.rb')) + + expect(cop.in_directory?(node, 'models')).to eq(true) + end + + it 'returns false for a directory in the lib/ directory' do + node = + parse_source('10', Rails.root.join('lib', 'models', 'foo.rb')) + + expect(cop.in_directory?(node, 'models')).to eq(false) + end + end + + describe '#name_of_receiver' do + it 'returns the name of a send receiver' do + node = parse_source('Foo.bar') + + expect(cop.name_of_receiver(node)).to eq('Foo') + end + end + + describe '#each_class_method' do + it 'yields every class method to the supplied block' do + node = parse_source(<<~RUBY) + class Foo + class << self + def first + end + end + + def self.second + end + end + RUBY + + nodes = cop.each_class_method(node).to_a + + expect(nodes.length).to eq(2) + + expect(nodes[0].children[0]).to eq(:first) + expect(nodes[1].children[1]).to eq(:second) + end + end + + describe '#each_send_node' do + it 'yields every send node to the supplied block' do + node = parse_source("foo\nbar") + nodes = cop.each_send_node(node).to_a + + expect(nodes.length).to eq(2) + expect(nodes[0].children[1]).to eq(:foo) + expect(nodes[1].children[1]).to eq(:bar) + end + end + + describe '#disallow_send_to' do + it 'disallows sending a message to a constant' do + def_node = parse_source(<<~RUBY) + def foo + FooFinder.new + end + RUBY + + send_node = def_node.each_child_node(:send).first + + expect(cop) + .to receive(:add_offense) + .with(send_node, location: :expression, message: 'oops') + + cop.disallow_send_to(def_node, 'Finder', 'oops') + end + end +end diff --git a/spec/rubocop/cop/code_reuse/active_record_spec.rb b/spec/rubocop/cop/code_reuse/active_record_spec.rb new file mode 100644 index 00000000000..a30fc52d26f --- /dev/null +++ b/spec/rubocop/cop/code_reuse/active_record_spec.rb @@ -0,0 +1,138 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/code_reuse/active_record' + +describe RuboCop::Cop::CodeReuse::ActiveRecord do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of "where" without any arguments' do + expect_offense(<<~SOURCE) + def foo + User.where + ^^^^^ This method can only be used inside an ActiveRecord model + end + SOURCE + end + + it 'flags the use of "where" with arguments' do + expect_offense(<<~SOURCE) + def foo + User.where(id: 10) + ^^^^^ This method can only be used inside an ActiveRecord model + end + SOURCE + end + + it 'does not flag the use of "group" without any arguments' do + expect_no_offenses(<<~SOURCE) + def foo + project.group + end + SOURCE + end + + it 'flags the use of "group" with arguments' do + expect_offense(<<~SOURCE) + def foo + project.group(:name) + ^^^^^ This method can only be used inside an ActiveRecord model + end + SOURCE + end + + it 'does not flag the use of ActiveRecord models in a model' do + path = Rails.root.join('app', 'models', 'foo.rb').to_s + + expect_no_offenses(<<~SOURCE, path) + def foo + project.group(:name) + end + SOURCE + end + + it 'does not flag the use of ActiveRecord models in a spec' do + path = Rails.root.join('spec', 'foo_spec.rb').to_s + + expect_no_offenses(<<~SOURCE, path) + def foo + project.group(:name) + end + SOURCE + end + + it 'does not flag the use of ActiveRecord models in a background migration' do + path = Rails + .root + .join('lib', 'gitlab', 'background_migration', 'foo.rb') + .to_s + + expect_no_offenses(<<~SOURCE, path) + def foo + project.group(:name) + end + SOURCE + end + + it 'does not flag the use of ActiveRecord models in lib/gitlab/database' do + path = Rails.root.join('lib', 'gitlab', 'database', 'foo.rb').to_s + + expect_no_offenses(<<~SOURCE, path) + def foo + project.group(:name) + end + SOURCE + end + + it 'autocorrects offenses in instance methods by whitelisting them' do + corrected = autocorrect_source(<<~SOURCE) + def foo + User.where + end + SOURCE + + expect(corrected).to eq(<<~SOURCE) + # rubocop: disable CodeReuse/ActiveRecord + def foo + User.where + end + # rubocop: enable CodeReuse/ActiveRecord + SOURCE + end + + it 'autocorrects offenses in class methods by whitelisting them' do + corrected = autocorrect_source(<<~SOURCE) + def self.foo + User.where + end + SOURCE + + expect(corrected).to eq(<<~SOURCE) + # rubocop: disable CodeReuse/ActiveRecord + def self.foo + User.where + end + # rubocop: enable CodeReuse/ActiveRecord + SOURCE + end + + it 'autocorrects offenses in blocks by whitelisting them' do + corrected = autocorrect_source(<<~SOURCE) + get '/' do + User.where + end + SOURCE + + expect(corrected).to eq(<<~SOURCE) + # rubocop: disable CodeReuse/ActiveRecord + get '/' do + User.where + end + # rubocop: enable CodeReuse/ActiveRecord + SOURCE + end +end diff --git a/spec/rubocop/cop/code_reuse/finder_spec.rb b/spec/rubocop/cop/code_reuse/finder_spec.rb new file mode 100644 index 00000000000..b04e053a4c3 --- /dev/null +++ b/spec/rubocop/cop/code_reuse/finder_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/code_reuse/finder' + +describe RuboCop::Cop::CodeReuse::Finder do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of a Finder inside another Finder' do + allow(cop) + .to receive(:in_finder?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooFinder + def execute + BarFinder.new.execute + ^^^^^^^^^^^^^ Finders can not be used inside a Finder. + end + end + SOURCE + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of a Finder inside a model class method' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User + class << self + def second_method + BarFinder.new + ^^^^^^^^^^^^^ Finders can not be used inside model class methods. + end + end + + def self.second_method + FooFinder.new + ^^^^^^^^^^^^^ Finders can not be used inside model class methods. + end + end + SOURCE + end + + it 'does not flag the use of a Finder in a non Finder file' do + expect_no_offenses(<<~SOURCE) + class FooFinder + def execute + BarFinder.new.execute + end + end + SOURCE + end + + it 'does not flag the use of a Finder in a regular class method' do + expect_no_offenses(<<~SOURCE) + class User + class << self + def second_method + BarFinder.new + end + end + + def self.second_method + FooFinder.new + end + end + SOURCE + end +end diff --git a/spec/rubocop/cop/code_reuse/presenter_spec.rb b/spec/rubocop/cop/code_reuse/presenter_spec.rb new file mode 100644 index 00000000000..4fe72619273 --- /dev/null +++ b/spec/rubocop/cop/code_reuse/presenter_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/code_reuse/presenter' + +describe RuboCop::Cop::CodeReuse::Presenter do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of a Presenter in a Service class' do + allow(cop) + .to receive(:in_service_class?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooService + def execute + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a Service class. + end + end + SOURCE + end + + it 'flags the use of a Presenter in a Finder' do + allow(cop) + .to receive(:in_finder?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooFinder + def execute + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a Finder. + end + end + SOURCE + end + + it 'flags the use of a Service class in a Presenter' do + allow(cop) + .to receive(:in_presenter?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooPresenter + def execute + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a Presenter. + end + end + SOURCE + end + + it 'flags the use of a Presenter in a Serializer' do + allow(cop) + .to receive(:in_serializer?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooSerializer + def execute + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a Serializer. + end + end + SOURCE + end + + it 'flags the use of a Presenter in a model instance method' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User < ActiveRecord::Base + def execute + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a model. + end + end + SOURCE + end + + it 'flags the use of a Presenter in a model class method' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User < ActiveRecord::Base + def self.execute + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a model. + end + end + SOURCE + end + + it 'flags the use of a Presenter in a worker' do + allow(cop) + .to receive(:in_worker?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooWorker + def perform + FooPresenter.new.execute + ^^^^^^^^^^^^^^^^ Presenters can not be used in a worker. + end + end + SOURCE + end +end diff --git a/spec/rubocop/cop/code_reuse/serializer_spec.rb b/spec/rubocop/cop/code_reuse/serializer_spec.rb new file mode 100644 index 00000000000..4530b15eed7 --- /dev/null +++ b/spec/rubocop/cop/code_reuse/serializer_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/code_reuse/serializer' + +describe RuboCop::Cop::CodeReuse::Serializer do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of a Serializer in a Service class' do + allow(cop) + .to receive(:in_service_class?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooService + def execute + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a Service class. + end + end + SOURCE + end + + it 'flags the use of a Serializer in a Finder' do + allow(cop) + .to receive(:in_finder?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooFinder + def execute + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a Finder. + end + end + SOURCE + end + + it 'flags the use of a Serializer in a Presenter' do + allow(cop) + .to receive(:in_presenter?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooPresenter + def execute + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a Presenter. + end + end + SOURCE + end + + it 'flags the use of a Serializer in a Serializer' do + allow(cop) + .to receive(:in_serializer?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooSerializer + def execute + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a Serializer. + end + end + SOURCE + end + + it 'flags the use of a Serializer in a model instance method' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User < ActiveRecord::Base + def execute + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a model. + end + end + SOURCE + end + + it 'flags the use of a Serializer in a model class method' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User < ActiveRecord::Base + def self.execute + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a model. + end + end + SOURCE + end + + it 'flags the use of a Serializer in a worker' do + allow(cop) + .to receive(:in_worker?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooWorker + def perform + FooSerializer.new.execute + ^^^^^^^^^^^^^^^^^ Serializers can not be used in a worker. + end + end + SOURCE + end +end diff --git a/spec/rubocop/cop/code_reuse/service_class_spec.rb b/spec/rubocop/cop/code_reuse/service_class_spec.rb new file mode 100644 index 00000000000..7b8d82f332e --- /dev/null +++ b/spec/rubocop/cop/code_reuse/service_class_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/code_reuse/service_class' + +describe RuboCop::Cop::CodeReuse::ServiceClass do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of a Service class in a Finder' do + allow(cop) + .to receive(:in_finder?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooFinder + def execute + FooService.new.execute + ^^^^^^^^^^^^^^ Service classes can not be used in a Finder. + end + end + SOURCE + end + + it 'flags the use of a Service class in a Presenter' do + allow(cop) + .to receive(:in_presenter?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooPresenter + def execute + FooService.new.execute + ^^^^^^^^^^^^^^ Service classes can not be used in a Presenter. + end + end + SOURCE + end + + it 'flags the use of a Service class in a Serializer' do + allow(cop) + .to receive(:in_serializer?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooSerializer + def execute + FooService.new.execute + ^^^^^^^^^^^^^^ Service classes can not be used in a Serializer. + end + end + SOURCE + end + + it 'flags the use of a Service class in a model' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User < ActiveRecord::Model + class << self + def first + FooService.new.execute + ^^^^^^^^^^^^^^ Service classes can not be used in a model. + end + end + + def second + FooService.new.execute + ^^^^^^^^^^^^^^ Service classes can not be used in a model. + end + end + SOURCE + end + + it 'does not flag the use of a Service class in a regular class' do + expect_no_offenses(<<~SOURCE) + class Foo + def execute + FooService.new.execute + end + end + SOURCE + end +end diff --git a/spec/rubocop/cop/code_reuse/worker_spec.rb b/spec/rubocop/cop/code_reuse/worker_spec.rb new file mode 100644 index 00000000000..97acaeb7643 --- /dev/null +++ b/spec/rubocop/cop/code_reuse/worker_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/code_reuse/worker' + +describe RuboCop::Cop::CodeReuse::Worker do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of a worker in a controller' do + allow(cop) + .to receive(:in_controller?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooController + def index + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a controller. + end + end + SOURCE + end + + it 'flags the use of a worker in an API' do + allow(cop) + .to receive(:in_api?) + .and_return(true) + + expect_offense(<<~SOURCE) + class Foo < Grape::API + resource :projects do + get '/' do + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a Grape API. + end + end + end + SOURCE + end + + it 'flags the use of a worker in a Finder' do + allow(cop) + .to receive(:in_finder?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooFinder + def execute + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a Finder. + end + end + SOURCE + end + + it 'flags the use of a worker in a Presenter' do + allow(cop) + .to receive(:in_presenter?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooPresenter + def execute + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a Presenter. + end + end + SOURCE + end + + it 'flags the use of a worker in a Serializer' do + allow(cop) + .to receive(:in_serializer?) + .and_return(true) + + expect_offense(<<~SOURCE) + class FooSerializer + def execute + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a Serializer. + end + end + SOURCE + end + + it 'flags the use of a worker in a model class method' do + allow(cop) + .to receive(:in_model?) + .and_return(true) + + expect_offense(<<~SOURCE) + class User < ActiveRecord::Base + def self.execute + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in model class methods. + end + end + SOURCE + end +end -- cgit v1.2.3