From 48aff82709769b098321c738f3444b9bdaa694c6 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 21 Oct 2020 07:08:36 +0000 Subject: Add latest changes from gitlab-org/gitlab@13-5-stable-ee --- rubocop/cop/api/base.rb | 53 ++++++++++ rubocop/cop/api/grape_api_instance.rb | 42 -------- rubocop/cop/code_reuse/active_record.rb | 35 ++++--- rubocop/cop/graphql/gid_expected_type.rb | 21 ++++ rubocop/cop/graphql/id_type.rb | 29 ++++++ .../cop/migration/add_concurrent_foreign_key.rb | 20 +++- rubocop/cop/migration/add_limit_to_text_columns.rb | 10 ++ .../migration/create_table_with_foreign_keys.rb | 2 +- .../with_lock_retries_disallowed_method.rb | 18 +++- rubocop/cop/rspec/expect_gitlab_tracking.rb | 66 +++++++++++++ .../cop/rspec/factory_bot/inline_association.rb | 109 +++++++++++++++++++++ rubocop/cop/rspec/timecop_travel.rb | 41 ++++++++ rubocop/migration_helpers.rb | 2 +- rubocop/rubocop-migrations.yml | 3 +- rubocop/rubocop-usage-data.yml | 2 + 15 files changed, 385 insertions(+), 68 deletions(-) create mode 100644 rubocop/cop/api/base.rb delete mode 100644 rubocop/cop/api/grape_api_instance.rb create mode 100644 rubocop/cop/graphql/gid_expected_type.rb create mode 100644 rubocop/cop/graphql/id_type.rb create mode 100644 rubocop/cop/rspec/expect_gitlab_tracking.rb create mode 100644 rubocop/cop/rspec/factory_bot/inline_association.rb create mode 100644 rubocop/cop/rspec/timecop_travel.rb (limited to 'rubocop') diff --git a/rubocop/cop/api/base.rb b/rubocop/cop/api/base.rb new file mode 100644 index 00000000000..85b19e9a833 --- /dev/null +++ b/rubocop/cop/api/base.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module API + class Base < RuboCop::Cop::Cop + # This cop checks that APIs subclass API::Base. + # + # @example + # + # # bad + # module API + # class Projects < Grape::API + # end + # end + # + # module API + # class Projects < Grape::API::Instance + # end + # end + # + # # good + # module API + # class Projects < ::API::Base + # end + # end + MSG = 'Inherit from ::API::Base instead of Grape::API::Instance or Grape::API. ' \ + 'For more details check https://gitlab.com/gitlab-org/gitlab/-/issues/215230.' + + def_node_matcher :grape_api, '(const (const {nil? (cbase)} :Grape) :API)' + def_node_matcher :grape_api_definition, <<~PATTERN + (class + (const _ _) + {#grape_api (const #grape_api :Instance)} + ... + ) + PATTERN + + def on_class(node) + grape_api_definition(node) do + add_offense(node.children[1]) + end + end + + def autocorrect(node) + lambda do |corrector| + corrector.replace(node, '::API::Base') + end + end + end + end + end +end diff --git a/rubocop/cop/api/grape_api_instance.rb b/rubocop/cop/api/grape_api_instance.rb deleted file mode 100644 index de11b9ef3f6..00000000000 --- a/rubocop/cop/api/grape_api_instance.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -module RuboCop - module Cop - module API - class GrapeAPIInstance < RuboCop::Cop::Cop - # This cop checks that APIs subclass Grape::API::Instance with Grape v1.3+. - # - # @example - # - # # bad - # module API - # class Projects < Grape::API - # end - # end - # - # # good - # module API - # class Projects < Grape::API::Instance - # end - # end - MSG = 'Inherit from Grape::API::Instance instead of Grape::API. ' \ - 'For more details check the https://gitlab.com/gitlab-org/gitlab/-/issues/215230.' - - def_node_matcher :grape_api_definition, <<~PATTERN - (class - (const _ _) - (const - (const nil? :Grape) :API) - ... - ) - PATTERN - - def on_class(node) - grape_api_definition(node) do - add_offense(node.children[1]) - end - end - end - end - end -end diff --git a/rubocop/cop/code_reuse/active_record.rb b/rubocop/cop/code_reuse/active_record.rb index 8c77c292315..f6b5d66647d 100644 --- a/rubocop/cop/code_reuse/active_record.rb +++ b/rubocop/cop/code_reuse/active_record.rb @@ -5,20 +5,20 @@ require_relative '../../code_reuse_helpers' module RuboCop module Cop module CodeReuse - # Cop that blacklists the use of ActiveRecord methods outside of models. + # Cop that denies 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: ' \ 'https://gitlab.com/gitlab-org/gitlab-foss/issues/49653' - # Various methods from ActiveRecord::Querying that are blacklisted. We + # Various methods from ActiveRecord::Querying that are denied. 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 + # The keys of this Hash are the denied method names. The values are + # booleans that indicate if the method should only be denied if any # arguments are provided. NOT_ALLOWED = { average: true, @@ -57,7 +57,6 @@ module RuboCop references: true, reorder: true, rewhere: true, - sum: false, take: false, take!: false, unscope: false, @@ -65,9 +64,9 @@ module RuboCop with: true }.freeze - # Directories that allow the use of the blacklisted methods. These + # Directories that allow the use of the denied methods. These # directories are checked relative to both . and ee/ - WHITELISTED_DIRECTORIES = %w[ + ALLOWED_DIRECTORIES = %w[ app/models config danger @@ -88,7 +87,7 @@ module RuboCop ].freeze def on_send(node) - return if in_whitelisted_directory?(node) + return if in_allowed_directory?(node) receiver = node.children[0] send_name = node.children[1] @@ -105,12 +104,12 @@ module RuboCop end end - # Returns true if the node resides in one of the whitelisted + # Returns true if the node resides in one of the allowed # directories. - def in_whitelisted_directory?(node) + def in_allowed_directory?(node) path = file_path_for_node(node) - WHITELISTED_DIRECTORIES.any? do |directory| + ALLOWED_DIRECTORIES.any? do |directory| path.start_with?( File.join(rails_root, directory), File.join(rails_root, 'ee', directory) @@ -119,12 +118,12 @@ module RuboCop end # We can not auto correct code like this, as it requires manual - # refactoring. Instead, we'll just whitelist the surrounding scope. + # refactoring. Instead, we'll just allow 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 + # exists to make it possible to allow 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. + # code reuse guidelines, instead of allowing any new offenses. def autocorrect(node) scope = surrounding_scope_of(node) indent = indentation_of(scope) @@ -132,7 +131,7 @@ module RuboCop 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) + next if allowed_scopes.include?(scope) corrector.insert_before( scope.source_range, @@ -144,7 +143,7 @@ module RuboCop "\n#{indent}# rubocop: enable #{cop_name}" ) - whitelisted_scopes << scope + allowed_scopes << scope end end @@ -160,8 +159,8 @@ module RuboCop end end - def whitelisted_scopes - @whitelisted_scopes ||= Set.new + def allowed_scopes + @allowed_scopes ||= Set.new end end end diff --git a/rubocop/cop/graphql/gid_expected_type.rb b/rubocop/cop/graphql/gid_expected_type.rb new file mode 100644 index 00000000000..354c5516752 --- /dev/null +++ b/rubocop/cop/graphql/gid_expected_type.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Graphql + class GIDExpectedType < RuboCop::Cop::Cop + MSG = 'Add an expected_type parameter to #object_from_id calls if possible.' + + def_node_search :id_from_object?, <<~PATTERN + (send ... :object_from_id (...)) + PATTERN + + def on_send(node) + return unless id_from_object?(node) + + add_offense(node) + end + end + end + end +end diff --git a/rubocop/cop/graphql/id_type.rb b/rubocop/cop/graphql/id_type.rb new file mode 100644 index 00000000000..96f90ac136a --- /dev/null +++ b/rubocop/cop/graphql/id_type.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Graphql + class IDType < RuboCop::Cop::Cop + MSG = 'Do not use GraphQL::ID_TYPE, use a specific GlobalIDType instead' + + WHITELISTED_ARGUMENTS = %i[iid full_path project_path group_path target_project_path].freeze + + def_node_search :graphql_id_type?, <<~PATTERN + (send nil? :argument (_ #does_not_match?) (const (const nil? :GraphQL) :ID_TYPE) ...) + PATTERN + + def on_send(node) + return unless graphql_id_type?(node) + + add_offense(node) + end + + private + + def does_not_match?(arg) + !WHITELISTED_ARGUMENTS.include?(arg) + end + end + end + end +end diff --git a/rubocop/cop/migration/add_concurrent_foreign_key.rb b/rubocop/cop/migration/add_concurrent_foreign_key.rb index 236de6224a4..31cf426b2d4 100644 --- a/rubocop/cop/migration/add_concurrent_foreign_key.rb +++ b/rubocop/cop/migration/add_concurrent_foreign_key.rb @@ -11,7 +11,11 @@ module RuboCop MSG = '`add_foreign_key` requires downtime, use `add_concurrent_foreign_key` instead'.freeze def_node_matcher :false_node?, <<~PATTERN - (false) + (false) + PATTERN + + def_node_matcher :with_lock_retries?, <<~PATTERN + (:send nil? :with_lock_retries) PATTERN def on_send(node) @@ -19,9 +23,11 @@ module RuboCop name = node.children[1] - if name == :add_foreign_key && !not_valid_fk?(node) - add_offense(node, location: :selector) - end + return unless name == :add_foreign_key + return if in_with_lock_retries?(node) + return if not_valid_fk?(node) + + add_offense(node, location: :selector) end def method_name(node) @@ -33,6 +39,12 @@ module RuboCop pair.children[0].children[0] == :validate && false_node?(pair.children[1]) end end + + def in_with_lock_retries?(node) + node.each_ancestor(:block).any? do |parent| + with_lock_retries?(parent.to_a.first) + end + end end end end diff --git a/rubocop/cop/migration/add_limit_to_text_columns.rb b/rubocop/cop/migration/add_limit_to_text_columns.rb index b578e73f19e..b2e37ad5137 100644 --- a/rubocop/cop/migration/add_limit_to_text_columns.rb +++ b/rubocop/cop/migration/add_limit_to_text_columns.rb @@ -6,6 +6,10 @@ module RuboCop module Cop module Migration # Cop that enforces always adding a limit on text columns + # + # Text columns starting with `encrypted_` are very likely used + # by `attr_encrypted` which controls the text length. Those columns + # should not add a text limit. class AddLimitToTextColumns < RuboCop::Cop::Cop include MigrationHelpers @@ -102,6 +106,8 @@ module RuboCop # Check if there is an `add_text_limit` call for the provided # table and attribute name def text_limit_missing?(node, table_name, attribute_name) + return false if encrypted_attribute_name?(attribute_name) + limit_found = false node.each_descendant(:send) do |send_node| @@ -118,6 +124,10 @@ module RuboCop !limit_found end + + def encrypted_attribute_name?(attribute_name) + attribute_name.to_s.start_with?('encrypted_') + end end end end diff --git a/rubocop/cop/migration/create_table_with_foreign_keys.rb b/rubocop/cop/migration/create_table_with_foreign_keys.rb index 01cab032049..382a2d6f65b 100644 --- a/rubocop/cop/migration/create_table_with_foreign_keys.rb +++ b/rubocop/cop/migration/create_table_with_foreign_keys.rb @@ -9,7 +9,7 @@ module RuboCop include MigrationHelpers MSG = 'Creating a table with more than one foreign key at once violates our migration style guide. ' \ - 'For more details check the https://docs.gitlab.com/ce/development/migration_style_guide.html#examples' + 'For more details check the https://docs.gitlab.com/ee/development/migration_style_guide.html#examples' def_node_matcher :create_table_with_block?, <<~PATTERN (block diff --git a/rubocop/cop/migration/with_lock_retries_disallowed_method.rb b/rubocop/cop/migration/with_lock_retries_disallowed_method.rb index a89c33c298b..aef19517a9d 100644 --- a/rubocop/cop/migration/with_lock_retries_disallowed_method.rb +++ b/rubocop/cop/migration/with_lock_retries_disallowed_method.rb @@ -29,9 +29,14 @@ module RuboCop ].sort.freeze MSG = "The method is not allowed to be called within the `with_lock_retries` block, the only allowed methods are: #{ALLOWED_MIGRATION_METHODS.join(', ')}" + MSG_ONLY_ONE_FK_ALLOWED = "Avoid adding more than one foreign key within the `with_lock_retries`. See https://docs.gitlab.com/ee/development/migration_style_guide.html#examples" def_node_matcher :send_node?, <<~PATTERN - send + send + PATTERN + + def_node_matcher :add_foreign_key?, <<~PATTERN + (send nil? :add_foreign_key ...) PATTERN def on_block(node) @@ -53,6 +58,17 @@ module RuboCop name = node.children[1] add_offense(node, location: :expression) unless ALLOWED_MIGRATION_METHODS.include?(name) + add_offense(node, location: :selector, message: MSG_ONLY_ONE_FK_ALLOWED) if multiple_fks?(node) + end + + def multiple_fks?(node) + return unless add_foreign_key?(node) + + count = node.parent.each_descendant(:send).count do |node| + add_foreign_key?(node) + end + + count > 1 end end end diff --git a/rubocop/cop/rspec/expect_gitlab_tracking.rb b/rubocop/cop/rspec/expect_gitlab_tracking.rb new file mode 100644 index 00000000000..ba658558705 --- /dev/null +++ b/rubocop/cop/rspec/expect_gitlab_tracking.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'rack/utils' + +module RuboCop + module Cop + module RSpec + # This cop checks for `expect(Gitlab::Tracking).to receive(:event)` usage in specs. + # See /spec/support/helpers/snowplow_helpers.rb for details on the replacement. + # + # @example + # + # # bad + # it 'expects a snowplow event' do + # expect(Gitlab::Tracking).to receive(:event).with("Category", "action", ...) + # end + # + # # good + # it 'expects a snowplow event', :snowplow do + # expect_snowplow_event(category: "Category", action: "action", ...) + # end + # + # # bad + # it 'does not expect a snowplow event' do + # expect(Gitlab::Tracking).not_to receive(:event) + # end + # + # # good + # it 'does not expect a snowplow event', :snowplow do + # expect_no_snowplow_event + # end + class ExpectGitlabTracking < RuboCop::Cop::Cop + MSG = 'Do not expect directly on `Gitlab::Tracking#event`, add the `snowplow` annotation and use ' \ + '`expect_snowplow_event` instead. ' \ + 'See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#test-snowplow-events' + + def_node_matcher :expect_gitlab_tracking?, <<~PATTERN + (send + (send nil? {:expect :allow} + (const (const nil? :Gitlab) :Tracking) + ) + ${:to :to_not :not_to} + { + ( + send nil? {:receive :have_received} (sym :event) ... + ) + + (send + (send nil? {:receive :have_received} (sym :event)) ... + ) + } + ... + ) + PATTERN + + RESTRICT_ON_SEND = [:expect, :allow].freeze + + def on_send(node) + return unless expect_gitlab_tracking?(node) + + add_offense(node) + end + end + end + end +end diff --git a/rubocop/cop/rspec/factory_bot/inline_association.rb b/rubocop/cop/rspec/factory_bot/inline_association.rb new file mode 100644 index 00000000000..1c2b8b55b46 --- /dev/null +++ b/rubocop/cop/rspec/factory_bot/inline_association.rb @@ -0,0 +1,109 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + module FactoryBot + # This cop encourages the use of inline associations in FactoryBot. + # The explicit use of `create` and `build` is discouraged. + # + # See https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#inline-definition + # + # @example + # + # Context: + # + # Factory.define do + # factory :project, class: 'Project' + # # EXAMPLE below + # end + # end + # + # # bad + # creator { create(:user) } + # creator { create(:user, :admin) } + # creator { build(:user) } + # creator { FactoryBot.build(:user) } + # creator { ::FactoryBot.build(:user) } + # add_attribute(:creator) { build(:user) } + # + # # good + # creator { association(:user) } + # creator { association(:user, :admin) } + # add_attribute(:creator) { association(:user) } + # + # # Accepted + # after(:build) do |instance| + # instance.creator = create(:user) + # end + # + # initialize_with do + # create(:project) + # end + # + # creator_id { create(:user).id } + # + class InlineAssociation < RuboCop::Cop::Cop + MSG = 'Prefer inline `association` over `%{type}`. ' \ + 'See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories' + + REPLACEMENT = 'association' + + def_node_matcher :create_or_build, <<~PATTERN + ( + send + ${ nil? (const { nil? (cbase) } :FactoryBot) } + ${ :create :build } + (sym _) + ... + ) + PATTERN + + def_node_matcher :association_definition, <<~PATTERN + (block + { + (send nil? $_) + (send nil? :add_attribute (sym $_)) + } + ... + ) + PATTERN + + def_node_matcher :chained_call?, <<~PATTERN + (send _ _) + PATTERN + + SKIP_NAMES = %i[initialize_with].to_set.freeze + + def on_send(node) + _receiver, type = create_or_build(node) + return unless type + return if chained_call?(node.parent) + return unless inside_assocation_definition?(node) + + add_offense(node, message: format(MSG, type: type)) + end + + def autocorrect(node) + lambda do |corrector| + receiver, type = create_or_build(node) + receiver = "#{receiver.source}." if receiver + expression = "#{receiver}#{type}" + replacement = node.source.sub(expression, REPLACEMENT) + corrector.replace(node.source_range, replacement) + end + end + + private + + def inside_assocation_definition?(node) + node.each_ancestor(:block).any? do |parent| + name = association_definition(parent) + name && !SKIP_NAMES.include?(name) + end + end + end + end + end + end +end diff --git a/rubocop/cop/rspec/timecop_travel.rb b/rubocop/cop/rspec/timecop_travel.rb new file mode 100644 index 00000000000..e5416953af7 --- /dev/null +++ b/rubocop/cop/rspec/timecop_travel.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # This cop checks for `Timecop.travel` usage in specs. + # + # @example + # + # # bad + # Timecop.travel(1.day.ago) { create(:issue) } + # + # # good + # travel_to(1.day.ago) { create(:issue) } + # + class TimecopTravel < RuboCop::Cop::Cop + include MatchRange + MESSAGE = 'Do not use `Timecop.travel`, use `travel_to` instead. ' \ + 'See https://gitlab.com/gitlab-org/gitlab/-/issues/214432 for more info.' + + def_node_matcher :timecop_travel?, <<~PATTERN + (send (const nil? :Timecop) :travel _) + PATTERN + + def on_send(node) + return unless timecop_travel?(node) + + add_offense(node, location: :expression, message: MESSAGE) + end + + def autocorrect(node) + -> (corrector) do + each_match_range(node.source_range, /^(Timecop\.travel)/) do |match_range| + corrector.replace(match_range, 'travel_to') + end + end + end + end + end + end +end diff --git a/rubocop/migration_helpers.rb b/rubocop/migration_helpers.rb index c26ba88f269..e9533fb65b2 100644 --- a/rubocop/migration_helpers.rb +++ b/rubocop/migration_helpers.rb @@ -21,7 +21,7 @@ module RuboCop TABLE_METHODS = %i(create_table create_table_if_not_exists change_table).freeze def high_traffic_tables - @high_traffic_tables ||= rubocop_migrations_config.dig('Migration/UpdateLargeTable', 'DeniedTables') + @high_traffic_tables ||= rubocop_migrations_config.dig('Migration/UpdateLargeTable', 'HighTrafficTables') end # Returns true if the given node originated from the db/migrate directory. diff --git a/rubocop/rubocop-migrations.yml b/rubocop/rubocop-migrations.yml index a919d570ccc..f8ff6e005f9 100644 --- a/rubocop/rubocop-migrations.yml +++ b/rubocop/rubocop-migrations.yml @@ -1,6 +1,7 @@ +# Make sure to update the docs if this file moves. Docs URL: https://docs.gitlab.com/ee/development/migration_style_guide.html#when-to-use-the-helper-method Migration/UpdateLargeTable: Enabled: true - DeniedTables: &denied_tables # size in GB (>= 10 GB on GitLab.com as of 02/2020) and/or number of records + HighTrafficTables: &high_traffic_tables # size in GB (>= 10 GB on GitLab.com as of 02/2020) and/or number of records - :audit_events - :ci_build_trace_sections - :ci_builds diff --git a/rubocop/rubocop-usage-data.yml b/rubocop/rubocop-usage-data.yml index 9f594bc5817..fdfe2a5e1aa 100644 --- a/rubocop/rubocop-usage-data.yml +++ b/rubocop/rubocop-usage-data.yml @@ -25,6 +25,8 @@ UsageData/LargeTable: - :Time - :SECURE_PRODUCT_TYPES - :Settings + - :CE_MEMOIZED_VALUES + - :EE_MEMOIZED_VALUES CountMethods: - :count - :distinct_count -- cgit v1.2.3