diff options
Diffstat (limited to 'rubocop')
| -rw-r--r-- | rubocop/cop/gitlab/httparty.rb | 6 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/rails_logger.rb | 51 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/union.rb | 4 | ||||
| -rw-r--r-- | rubocop/cop/graphql/authorize_types.rb | 6 | ||||
| -rw-r--r-- | rubocop/cop/include_action_view_context.rb | 5 | ||||
| -rw-r--r-- | rubocop/cop/include_sidekiq_worker.rb | 5 | ||||
| -rw-r--r-- | rubocop/cop/inject_enterprise_edition_module.rb | 48 | ||||
| -rw-r--r-- | rubocop/cop/migration/add_limit_to_string_columns.rb | 59 | ||||
| -rw-r--r-- | rubocop/cop/qa/element_with_pattern.rb | 2 | ||||
| -rw-r--r-- | rubocop/cop/rspec/env_assignment.rb | 5 | ||||
| -rw-r--r-- | rubocop/cop/rspec/factories_in_migration_specs.rb | 5 | ||||
| -rw-r--r-- | rubocop/cop/rspec/top_level_describe_path.rb | 35 | ||||
| -rw-r--r-- | rubocop/cop/sidekiq_options_queue.rb | 5 | ||||
| -rw-r--r-- | rubocop/rubocop.rb | 3 | ||||
| -rw-r--r-- | rubocop/spec_helpers.rb | 30 |
15 files changed, 195 insertions, 74 deletions
diff --git a/rubocop/cop/gitlab/httparty.rb b/rubocop/cop/gitlab/httparty.rb index 215f18b6993..8acebff624d 100644 --- a/rubocop/cop/gitlab/httparty.rb +++ b/rubocop/cop/gitlab/httparty.rb @@ -1,11 +1,9 @@ -require_relative '../../spec_helpers' +# frozen_string_literal: true module RuboCop module Cop module Gitlab class HTTParty < RuboCop::Cop::Cop - include SpecHelpers - MSG_SEND = <<~EOL.freeze Avoid calling `HTTParty` directly. Instead, use the Gitlab::HTTP wrapper. To allow request to localhost or the private network set @@ -27,8 +25,6 @@ module RuboCop PATTERN def on_send(node) - return if in_spec?(node) - add_offense(node, location: :expression, message: MSG_SEND) if httparty_node?(node) add_offense(node, location: :expression, message: MSG_INCLUDE) if includes_httparty?(node) end diff --git a/rubocop/cop/gitlab/rails_logger.rb b/rubocop/cop/gitlab/rails_logger.rb new file mode 100644 index 00000000000..d1a06a9a100 --- /dev/null +++ b/rubocop/cop/gitlab/rails_logger.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module Gitlab + class RailsLogger < ::RuboCop::Cop::Cop + include CodeReuseHelpers + + # This cop checks for the Rails.logger in the codebase + # + # @example + # + # # bad + # Rails.logger.error("Project #{project.full_path} could not be saved") + # + # # good + # Gitlab::AppLogger.error("Project %{project_path} could not be saved" % { project_path: project.full_path }) + MSG = 'Use a structured JSON logger instead of `Rails.logger`. ' \ + 'https://docs.gitlab.com/ee/development/logging.html'.freeze + + def_node_matcher :rails_logger?, <<~PATTERN + (send (const nil? :Rails) :logger ... ) + PATTERN + + WHITELISTED_DIRECTORIES = %w[ + spec + ].freeze + + def on_send(node) + return if in_whitelisted_directory?(node) + return unless rails_logger?(node) + + add_offense(node, location: :expression) + end + + 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 + end + end + end +end diff --git a/rubocop/cop/gitlab/union.rb b/rubocop/cop/gitlab/union.rb index 09541d8af3b..c44c847657b 100644 --- a/rubocop/cop/gitlab/union.rb +++ b/rubocop/cop/gitlab/union.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true -require_relative '../../spec_helpers' module RuboCop module Cop @@ -7,8 +6,6 @@ module RuboCop # Cop that disallows the use of `Gitlab::SQL::Union`, in favour of using # the `FromUnion` module. class Union < RuboCop::Cop::Cop - include SpecHelpers - MSG = 'Use the `FromUnion` concern, instead of using `Gitlab::SQL::Union` directly' def_node_matcher :raw_union?, <<~PATTERN @@ -17,7 +14,6 @@ module RuboCop def on_send(node) return unless raw_union?(node) - return if in_spec?(node) add_offense(node, location: :expression) end diff --git a/rubocop/cop/graphql/authorize_types.rb b/rubocop/cop/graphql/authorize_types.rb index 93fe80c3edf..cd8bdbaee59 100644 --- a/rubocop/cop/graphql/authorize_types.rb +++ b/rubocop/cop/graphql/authorize_types.rb @@ -1,13 +1,9 @@ # frozen_string_literal: true -require_relative '../../spec_helpers' - module RuboCop module Cop module Graphql class AuthorizeTypes < RuboCop::Cop::Cop - include SpecHelpers - MSG = 'Add an `authorize :ability` call to the type: '\ 'https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#type-authorization' @@ -32,8 +28,6 @@ module RuboCop private def in_type?(node) - return if in_spec?(node) - path = node.location.expression.source_buffer.name path.include?(TYPES_DIR) diff --git a/rubocop/cop/include_action_view_context.rb b/rubocop/cop/include_action_view_context.rb index 14662a33e95..52c84a711c9 100644 --- a/rubocop/cop/include_action_view_context.rb +++ b/rubocop/cop/include_action_view_context.rb @@ -1,13 +1,9 @@ # frozen_string_literal: true -require_relative '../spec_helpers' - module RuboCop module Cop # Cop that makes sure workers include `::Gitlab::ActionViewOutput::Context`, not `ActionView::Context`. class IncludeActionViewContext < RuboCop::Cop::Cop - include SpecHelpers - MSG = 'Include `::Gitlab::ActionViewOutput::Context`, not `ActionView::Context`, for Rails 5.'.freeze def_node_matcher :includes_action_view_context?, <<~PATTERN @@ -15,7 +11,6 @@ module RuboCop PATTERN def on_send(node) - return if in_spec?(node) return unless includes_action_view_context?(node) add_offense(node.arguments.first, location: :expression) diff --git a/rubocop/cop/include_sidekiq_worker.rb b/rubocop/cop/include_sidekiq_worker.rb index 8da4a147219..e69bc018add 100644 --- a/rubocop/cop/include_sidekiq_worker.rb +++ b/rubocop/cop/include_sidekiq_worker.rb @@ -1,11 +1,9 @@ -require_relative '../spec_helpers' +# frozen_string_literal: true module RuboCop module Cop # Cop that makes sure workers include `ApplicationWorker`, not `Sidekiq::Worker`. class IncludeSidekiqWorker < RuboCop::Cop::Cop - include SpecHelpers - MSG = 'Include `ApplicationWorker`, not `Sidekiq::Worker`.'.freeze def_node_matcher :includes_sidekiq_worker?, <<~PATTERN @@ -13,7 +11,6 @@ module RuboCop PATTERN def on_send(node) - return if in_spec?(node) return unless includes_sidekiq_worker?(node) add_offense(node.arguments.first, location: :expression) diff --git a/rubocop/cop/inject_enterprise_edition_module.rb b/rubocop/cop/inject_enterprise_edition_module.rb index 1d37b1bd12d..e0e1b2d6c7d 100644 --- a/rubocop/cop/inject_enterprise_edition_module.rb +++ b/rubocop/cop/inject_enterprise_edition_module.rb @@ -6,23 +6,39 @@ module RuboCop # the last line of a file. Injecting a module in the middle of a file will # cause merge conflicts, while placing it on the last line will not. class InjectEnterpriseEditionModule < RuboCop::Cop::Cop - MSG = 'Injecting EE modules must be done on the last line of this file' \ - ', outside of any class or module definitions' + INVALID_LINE = 'Injecting EE modules must be done on the last line of this file' \ + ', outside of any class or module definitions' - METHODS = Set.new(%i[include extend prepend]).freeze + DISALLOWED_METHOD = + 'EE modules must be injected using `include_if_ee`, `extend_if_ee`, or `prepend_if_ee`' + + INVALID_ARGUMENT = 'EE modules to inject must be specified as a String' + + CHECK_LINE_METHODS = + Set.new(%i[include_if_ee extend_if_ee prepend_if_ee]).freeze + + DISALLOW_METHODS = Set.new(%i[include extend prepend]).freeze def ee_const?(node) line = node.location.expression.source_line # We use `match?` here instead of RuboCop's AST matching, as this makes # it far easier to handle nested constants such as `EE::Foo::Bar::Baz`. - line.match?(/(\s|\()(::)?EE::/) + line.match?(/(\s|\()('|")?(::)?EE::/) end def on_send(node) - return unless METHODS.include?(node.children[1]) - return unless ee_const?(node.children[2]) + return unless check_method?(node) + if DISALLOW_METHODS.include?(node.children[1]) + add_offense(node, message: DISALLOWED_METHOD) + else + verify_line_number(node) + verify_argument_type(node) + end + end + + def verify_line_number(node) line = node.location.line buffer = node.location.expression.source_buffer last_line = buffer.last_line @@ -32,7 +48,25 @@ module RuboCop # the expression is the last line _of code_. last_line -= 1 if buffer.source.end_with?("\n") - add_offense(node) if line < last_line + add_offense(node, message: INVALID_LINE) if line < last_line + end + + def verify_argument_type(node) + argument = node.children[2] + + return if argument.str_type? + + add_offense(argument, message: INVALID_ARGUMENT) + end + + def check_method?(node) + name = node.children[1] + + if CHECK_LINE_METHODS.include?(name) || DISALLOW_METHODS.include?(name) + ee_const?(node.children[2]) + else + false + end end # Automatically correcting these offenses is not always possible, as diff --git a/rubocop/cop/migration/add_limit_to_string_columns.rb b/rubocop/cop/migration/add_limit_to_string_columns.rb new file mode 100644 index 00000000000..30affcbb089 --- /dev/null +++ b/rubocop/cop/migration/add_limit_to_string_columns.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that enforces length constraints to string columns + class AddLimitToStringColumns < RuboCop::Cop::Cop + include MigrationHelpers + + ADD_COLUMNS_METHODS = %i(add_column add_column_with_default).freeze + + MSG = 'String columns should have a limit constraint. 255 is suggested'.freeze + + def on_def(node) + return unless in_migration?(node) + + node.each_descendant(:send) do |send_node| + next unless string_operation?(send_node) + + add_offense(send_node, location: :selector) unless limit_on_string_column?(send_node) + end + end + + private + + def string_operation?(node) + modifier = node.children[0] + migration_method = node.children[1] + + if migration_method == :string + modifier.type == :lvar + elsif ADD_COLUMNS_METHODS.include?(migration_method) + modifier.nil? && string_column?(node.children[4]) + end + end + + def string_column?(column_type) + column_type.type == :sym && column_type.value == :string + end + + def limit_on_string_column?(node) + migration_method = node.children[1] + + if migration_method == :string + limit_present?(node.children) + elsif ADD_COLUMNS_METHODS.include?(migration_method) + limit_present?(node) + end + end + + def limit_present?(statement) + !(statement.to_s =~ /:limit/).nil? + end + end + end + end +end diff --git a/rubocop/cop/qa/element_with_pattern.rb b/rubocop/cop/qa/element_with_pattern.rb index d14eeaaeaf3..d48f4725488 100644 --- a/rubocop/cop/qa/element_with_pattern.rb +++ b/rubocop/cop/qa/element_with_pattern.rb @@ -30,7 +30,7 @@ module RuboCop return if args.first.nil? args.first.each_node(:str) do |arg| - add_offense(arg, message: MESSAGE % "qa-#{element_name.value.to_s.tr('_', '-')}") + add_offense(arg, message: MESSAGE % "data-qa-selector=#{element_name.value}") end end diff --git a/rubocop/cop/rspec/env_assignment.rb b/rubocop/cop/rspec/env_assignment.rb index 8b61fa8e264..73e108c2232 100644 --- a/rubocop/cop/rspec/env_assignment.rb +++ b/rubocop/cop/rspec/env_assignment.rb @@ -1,4 +1,4 @@ -require_relative '../../spec_helpers' +# frozen_string_literal: true module RuboCop module Cop @@ -17,8 +17,6 @@ module RuboCop # stub_env('FOO', 'bar') # end class EnvAssignment < RuboCop::Cop::Cop - include SpecHelpers - MESSAGE = "Don't assign to ENV, use `stub_env` instead.".freeze def_node_search :env_assignment?, <<~PATTERN @@ -28,7 +26,6 @@ module RuboCop # Following is what node.children looks like on a match # [s(:const, nil, :ENV), :[]=, s(:str, "key"), s(:str, "value")] def on_send(node) - return unless in_spec?(node) return unless env_assignment?(node) add_offense(node, location: :expression, message: MESSAGE) diff --git a/rubocop/cop/rspec/factories_in_migration_specs.rb b/rubocop/cop/rspec/factories_in_migration_specs.rb index 0c5aa838a2c..65c7638a0f4 100644 --- a/rubocop/cop/rspec/factories_in_migration_specs.rb +++ b/rubocop/cop/rspec/factories_in_migration_specs.rb @@ -1,4 +1,4 @@ -require_relative '../../spec_helpers' +# frozen_string_literal: true module RuboCop module Cop @@ -14,8 +14,6 @@ module RuboCop # let(:users) { table(:users) } # let(:user) { users.create!(name: 'User 1', username: 'user1') } class FactoriesInMigrationSpecs < RuboCop::Cop::Cop - include SpecHelpers - MESSAGE = "Don't use FactoryBot.%s in migration specs, use `table` instead.".freeze FORBIDDEN_METHODS = %i[build build_list create create_list].freeze @@ -27,7 +25,6 @@ module RuboCop # - Without FactoryBot namespace: [nil, :build, s(:sym, :user)] # - With FactoryBot namespace: [s(:const, nil, :FactoryBot), :build, s(:sym, :user)] def on_send(node) - return unless in_migration_spec?(node) return unless forbidden_factory_usage?(node) method = node.children[1] diff --git a/rubocop/cop/rspec/top_level_describe_path.rb b/rubocop/cop/rspec/top_level_describe_path.rb new file mode 100644 index 00000000000..61796e23af0 --- /dev/null +++ b/rubocop/cop/rspec/top_level_describe_path.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'rubocop/rspec/top_level_describe' + +module RuboCop + module Cop + module RSpec + class TopLevelDescribePath < RuboCop::Cop::Cop + include RuboCop::RSpec::TopLevelDescribe + + MESSAGE = 'A file with a top-level `describe` must end in _spec.rb.' + SHARED_EXAMPLES = %i[shared_examples shared_examples_for].freeze + + def on_top_level_describe(node, args) + return if acceptable_file_path?(processed_source.buffer.name) + return if shared_example?(node) + + add_offense(node, message: MESSAGE) + end + + private + + def acceptable_file_path?(path) + File.fnmatch?('*_spec.rb', path) || File.fnmatch?('*/frontend/fixtures/*', path) + end + + def shared_example?(node) + node.ancestors.any? do |node| + node.respond_to?(:method_name) && SHARED_EXAMPLES.include?(node.method_name) + end + end + end + end + end +end diff --git a/rubocop/cop/sidekiq_options_queue.rb b/rubocop/cop/sidekiq_options_queue.rb index 253d2958866..499c712175e 100644 --- a/rubocop/cop/sidekiq_options_queue.rb +++ b/rubocop/cop/sidekiq_options_queue.rb @@ -1,11 +1,9 @@ -require_relative '../spec_helpers' +# frozen_string_literal: true module RuboCop module Cop # Cop that prevents manually setting a queue in Sidekiq workers. class SidekiqOptionsQueue < RuboCop::Cop::Cop - include SpecHelpers - MSG = 'Do not manually set a queue; `ApplicationWorker` sets one automatically.'.freeze def_node_matcher :sidekiq_options?, <<~PATTERN @@ -13,7 +11,6 @@ module RuboCop PATTERN def on_send(node) - return if in_spec?(node) return unless sidekiq_options?(node) node.arguments.first.each_node(:pair) do |pair| diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 27c63d92ae5..d1328c4eb38 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -3,6 +3,7 @@ require_relative 'cop/gitlab/predicate_memoization' require_relative 'cop/gitlab/httparty' require_relative 'cop/gitlab/finder_with_find_by' require_relative 'cop/gitlab/union' +require_relative 'cop/gitlab/rails_logger' require_relative 'cop/include_action_view_context' require_relative 'cop/include_sidekiq_worker' require_relative 'cop/safe_params' @@ -16,6 +17,7 @@ require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_index' +require_relative 'cop/migration/add_limit_to_string_columns' require_relative 'cop/migration/add_reference' require_relative 'cop/migration/add_timestamps' require_relative 'cop/migration/datetime' @@ -31,6 +33,7 @@ require_relative 'cop/migration/update_large_table' require_relative 'cop/project_path_helper' require_relative 'cop/rspec/env_assignment' require_relative 'cop/rspec/factories_in_migration_specs' +require_relative 'cop/rspec/top_level_describe_path' require_relative 'cop/qa/element_with_pattern' require_relative 'cop/sidekiq_options_queue' require_relative 'cop/destroy_all' diff --git a/rubocop/spec_helpers.rb b/rubocop/spec_helpers.rb deleted file mode 100644 index ecd77c4351d..00000000000 --- a/rubocop/spec_helpers.rb +++ /dev/null @@ -1,30 +0,0 @@ -module RuboCop - module SpecHelpers - SPEC_HELPERS = %w[fast_spec_helper.rb rails_helper.rb spec_helper.rb].freeze - MIGRATION_SPEC_DIRECTORIES = ['spec/migrations', 'spec/lib/gitlab/background_migration'].freeze - - # Returns true if the given node originated from the spec directory. - def in_spec?(node) - path = node.location.expression.source_buffer.name - pwd = RuboCop::PathUtil.pwd - - !SPEC_HELPERS.include?(File.basename(path)) && - path.start_with?(File.join(pwd, 'spec'), File.join(pwd, 'ee', 'spec')) - end - - def migration_directories - @migration_directories ||= MIGRATION_SPEC_DIRECTORIES.map do |dir| - pwd = RuboCop::PathUtil.pwd - [File.join(pwd, dir), File.join(pwd, 'ee', dir)] - end.flatten - end - - # Returns true if the given node originated from a migration spec. - def in_migration_spec?(node) - path = node.location.expression.source_buffer.name - - in_spec?(node) && - path.start_with?(*migration_directories) - end - end -end |