diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-02-20 16:49:51 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-02-20 16:49:51 +0300 |
commit | 71786ddc8e28fbd3cb3fcc4b3ff15e5962a1c82e (patch) | |
tree | 6a2d93ef3fb2d353bb7739e4b57e6541f51cdd71 /rubocop/cop | |
parent | a7253423e3403b8c08f8a161e5937e1488f5f407 (diff) |
Add latest changes from gitlab-org/gitlab@15-9-stable-eev15.9.0-rc42
Diffstat (limited to 'rubocop/cop')
-rw-r--r-- | rubocop/cop/avoid_return_from_blocks.rb | 2 | ||||
-rw-r--r-- | rubocop/cop/database/multiple_databases.rb | 1 | ||||
-rw-r--r-- | rubocop/cop/gitlab/doc_url.rb | 44 | ||||
-rw-r--r-- | rubocop/cop/gitlab/mark_used_feature_flags.rb | 2 | ||||
-rw-r--r-- | rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction.rb | 86 | ||||
-rw-r--r-- | rubocop/cop/migration/versioned_migration_class.rb | 24 | ||||
-rw-r--r-- | rubocop/cop/rspec/env_mocking.rb | 59 | ||||
-rw-r--r-- | rubocop/cop/rspec/invalid_feature_category.rb | 104 | ||||
-rw-r--r-- | rubocop/cop/rspec/missing_feature_category.rb | 44 | ||||
-rw-r--r-- | rubocop/cop/scalability/file_uploads.rb | 41 | ||||
-rw-r--r-- | rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb | 2 |
11 files changed, 376 insertions, 33 deletions
diff --git a/rubocop/cop/avoid_return_from_blocks.rb b/rubocop/cop/avoid_return_from_blocks.rb index 61edfd0a789..c6a7a87c548 100644 --- a/rubocop/cop/avoid_return_from_blocks.rb +++ b/rubocop/cop/avoid_return_from_blocks.rb @@ -32,7 +32,7 @@ module RuboCop return unless top_block?(node) block_body.each_node(:return) do |return_node| - next if parent_blocks(node, return_node).all?(&method(:whitelisted?)) + next if parent_blocks(node, return_node).all? { |block_node| whitelisted?(block_node) } add_offense(return_node) end diff --git a/rubocop/cop/database/multiple_databases.rb b/rubocop/cop/database/multiple_databases.rb index 26beecdeba7..eccb4a78219 100644 --- a/rubocop/cop/database/multiple_databases.rb +++ b/rubocop/cop/database/multiple_databases.rb @@ -20,6 +20,7 @@ module RuboCop ALLOWED_METHODS = %i[ no_touching configurations + connection_handler logger ].freeze diff --git a/rubocop/cop/gitlab/doc_url.rb b/rubocop/cop/gitlab/doc_url.rb new file mode 100644 index 00000000000..cbfbdf7eb57 --- /dev/null +++ b/rubocop/cop/gitlab/doc_url.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + # This cop encourages using helper to link to documentation + # in string literals. + # + # @example + # # bad + # 'See [the docs](https://docs.gitlab.com/ee/user/permissions#roles).' + # _('See [the docs](https://docs.gitlab.com/ee/user/permissions#roles).') + # + # # good + # docs_link = link_to _('the docs'), help_page_url('user/permissions', anchor: 'roles') + # "See #{docs_link}." + # _('See %{docs_link}.').html_safe % { docs_link: docs_link.html_safe } + class DocUrl < RuboCop::Cop::Base + include RangeHelp + + MSG = 'Use `#help_page_url` instead of directly including link. ' \ + 'See https://docs.gitlab.com/ee/development/documentation/#linking-to-help-in-ruby.' + + DOCS_URL_REGEXP = %r{https://docs.gitlab.com/ee/[\w#%./-]+}.freeze + + def on_str(node) + match = DOCS_URL_REGEXP.match(node.source) + return unless match + + add_offense(bad_range(node, match)) + end + + private + + def bad_range(node, match) + url_begin_pos, url_end_pos = match.offset(0) + begin_pos = node.loc.expression.begin_pos + url_begin_pos + + range_between(begin_pos, begin_pos + (url_end_pos - url_begin_pos)) + end + end + end + end +end diff --git a/rubocop/cop/gitlab/mark_used_feature_flags.rb b/rubocop/cop/gitlab/mark_used_feature_flags.rb index d1722a47c8a..ffd59c8fffc 100644 --- a/rubocop/cop/gitlab/mark_used_feature_flags.rb +++ b/rubocop/cop/gitlab/mark_used_feature_flags.rb @@ -187,7 +187,7 @@ module RuboCop # Marking all event's feature flags as used as Gitlab::UsageDataCounters::HLLRedisCounter.track_event{,context} # is mostly used with dynamic event name. def track_usage_data_counters_known_events! - usage_data_counters_known_event_feature_flags.each(&method(:save_used_feature_flag)) + usage_data_counters_known_event_feature_flags.each { |feature_flag_name| save_used_feature_flag(feature_flag_name) } end def usage_data_counters_known_event_feature_flags diff --git a/rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction.rb b/rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction.rb new file mode 100644 index 00000000000..547dfb60945 --- /dev/null +++ b/rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that prevents usage of `disable_ddl_transaction!` + # if the only statement being called in the migration is :validate_foreign_key. + # + # We do this because PostgreSQL will add an implicit transaction for single + # statements. So there's no reason to add the disable_ddl_transaction!. + # + # This cop was introduced to clarify the need for disable_ddl_transaction! + # and to avoid bike-shedding and review back-and-forth. + # + # @examples + # + # # bad + # class SomeMigration < Gitlab::Database::Migration[2.1] + # disable_ddl_transaction! + # def up + # validate_foreign_key :emails, :user_id + # end + # def down + # # no-op + # end + # end + # + # # good + # class SomeMigration < Gitlab::Database::Migration[2.1] + # def up + # validate_foreign_key :emails, :user_id + # end + # def down + # # no-op + # end + # end + class PreventSingleStatementWithDisableDdlTransaction < RuboCop::Cop::Base + include MigrationHelpers + + MSG = "PostgreSQL will add an implicit transaction for single statements. " \ + "So there's no reason to use `disable_ddl_transaction!`, if you're only " \ + "executing validate_foreign_key." + + def_node_matcher :disable_ddl_transaction?, <<~PATTERN + (send _ :disable_ddl_transaction! ...) + PATTERN + + def_node_matcher :validate_foreign_key?, <<~PATTERN + (send :validate_foreign_key ...) + PATTERN + + def on_begin(node) + return unless in_migration?(node) + + disable_ddl_transaction_node = nil + + # Only perform cop if disable_ddl_transaction! is present + node.each_descendant(:send) do |send_node| + disable_ddl_transaction_node = send_node if disable_ddl_transaction?(send_node) + end + + return unless disable_ddl_transaction_node + + # For each migration method, check if :validate_foreign_key is the only statement. + node.each_descendant(:def) do |def_node| + break unless [:up, :down, :change].include? def_node.children[0] + + statement_count = 0 + has_validate_foreign_key = false + + def_node.each_descendant(:send) do |send_node| + has_validate_foreign_key = true if send_node.children[1] == :validate_foreign_key + statement_count += 1 + end + + if disable_ddl_transaction_node && has_validate_foreign_key && statement_count == 1 + add_offense(disable_ddl_transaction_node, message: MSG) + end + end + end + end + end + end +end diff --git a/rubocop/cop/migration/versioned_migration_class.rb b/rubocop/cop/migration/versioned_migration_class.rb index 572ddcd1b12..27f1c0e16a1 100644 --- a/rubocop/cop/migration/versioned_migration_class.rb +++ b/rubocop/cop/migration/versioned_migration_class.rb @@ -8,13 +8,18 @@ module RuboCop class VersionedMigrationClass < RuboCop::Cop::Base include MigrationHelpers - ENFORCED_SINCE = 2021_09_02_00_00_00 - CURRENT_DATABASE_MIGRATION_CLASS = 'Gitlab::Database::Migration[2.1]' + ENFORCED_SINCE = 2023_01_12_00_00_00 + DOC_LINK = "https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning" - MSG_INHERIT = 'Don\'t inherit from ActiveRecord::Migration but use Gitlab::Database::Migration[2.1] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning.' - MSG_INCLUDE = 'Don\'t include migration helper modules directly. Inherit from Gitlab::Database::Migration[2.1] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning.' + MSG_INHERIT = "Don't inherit from ActiveRecord::Migration or old versions of Gitlab::Database::Migration. " \ + "Use Gitlab::Database::Migration[2.1] instead. See #{DOC_LINK}." + MSG_INCLUDE = "Don't include migration helper modules directly. " \ + "Inherit from Gitlab::Database::Migration[2.1] instead. See #{DOC_LINK}." + + GITLAB_MIGRATION_CLASS = 'Gitlab::Database::Migration' ACTIVERECORD_MIGRATION_CLASS = 'ActiveRecord::Migration' + CURRENT_MIGRATION_VERSION = 2.1 # Should be the same value as Gitlab::Database::Migration.current_version def_node_search :includes_helpers?, <<~PATTERN (send nil? :include @@ -25,7 +30,7 @@ module RuboCop def on_class(node) return unless relevant_migration?(node) - return unless activerecord_migration_class?(node) + return unless activerecord_migration_class?(node) || old_version_migration_class?(node) add_offense(node, message: MSG_INHERIT) end @@ -51,6 +56,15 @@ module RuboCop others.find { |node| node.const_type? && node&.const_name != 'Types' }&.const_name end + + # Returns true for any parent class of format Gitlab::Database::Migration[version] if version < current_version + def old_version_migration_class?(class_node) + parent_class_node = class_node.parent_class + return false unless parent_class_node.send_type? && parent_class_node.arguments.last.float_type? + return false unless parent_class_node.children[0].const_name == GITLAB_MIGRATION_CLASS + + parent_class_node.arguments[0].value < CURRENT_MIGRATION_VERSION + end end end end diff --git a/rubocop/cop/rspec/env_mocking.rb b/rubocop/cop/rspec/env_mocking.rb new file mode 100644 index 00000000000..96bd535df08 --- /dev/null +++ b/rubocop/cop/rspec/env_mocking.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'rubocop-rspec' + +module RuboCop + module Cop + module RSpec + # Check for ENV mocking in specs. + # See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#persistent-in-memory-application-state + # + # @example + # + # # bad + # allow(ENV).to receive(:[]).with('FOO').and_return('bar') + # allow(ENV).to receive(:fetch).with('FOO').and_return('bar') + # allow(ENV).to receive(:[]).with(key).and_return(value) + # allow(ENV).to receive(:[]).with(fetch_key(object)).and_return(fetch_value(object)) + # + # # good + # stub_env('FOO', 'bar') + # stub_env(key, value) + # stub_env(fetch_key(object), fetch_value(object)) + class EnvMocking < RuboCop::Cop::Base + extend RuboCop::Cop::AutoCorrector + + MESSAGE = "Don't mock the ENV, use `stub_env` instead." + + def_node_matcher :env_mocking?, <<~PATTERN + (send + (send nil? :allow + (const {nil? cbase} :ENV) + ) + :to + (send + (send + (send nil? :receive (sym {:[] :fetch})) + :with $_ + ) + :and_return $_ + ) + ... + ) + PATTERN + + def on_send(node) + env_mocking?(node) do |key, value| + add_offense(node, message: MESSAGE) do |corrector| + corrector.replace(node.loc.expression, stub_env(key.source, value.source)) + end + end + end + + def stub_env(key, value) + "stub_env(#{key}, #{value})" + end + end + end + end +end diff --git a/rubocop/cop/rspec/invalid_feature_category.rb b/rubocop/cop/rspec/invalid_feature_category.rb new file mode 100644 index 00000000000..9ef880d6aac --- /dev/null +++ b/rubocop/cop/rspec/invalid_feature_category.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'rubocop/cop/rspec/base' +require 'rubocop/cop/rspec/mixin/top_level_group' +require 'did_you_mean' + +module RuboCop + module Cop + module RSpec + # Ensures that feature categories in specs are valid. + # + # @example + # + # # bad + # RSpec.describe 'foo', feature_category: :invalid do + # end + # + # RSpec.describe 'foo', feature_category: :not_owned do + # end + # + # # good + # + # RSpec.describe 'foo', feature_category: :wiki do + # end + # + # RSpec.describe 'foo', feature_category: :tooling do + # end + # + class InvalidFeatureCategory < RuboCop::Cop::RSpec::Base + MSG = 'Please use a valid feature category. %{msg_suggestion}' \ + 'See https://docs.gitlab.com/ee/development/feature_categorization/#rspec-examples.' + + MSG_DID_YOU_MEAN = 'Did you mean `:%{suggestion}`? ' + + MSG_SYMBOL = 'Please use a symbol as value.' + + FEATURE_CATEGORIES_PATH = File.expand_path('../../../config/feature_categories.yml', __dir__).freeze + + # List of feature categories which are not defined in config/feature_categories.yml + CUSTOM_FEATURE_CATEGORIES = [ + # https://docs.gitlab.com/ee/development/feature_categorization/#tooling-feature-category + :tooling, + # https://docs.gitlab.com/ee/development/feature_categorization/#shared-feature-category + :shared + ].to_set.freeze + + # @!method feature_category?(node) + def_node_matcher :feature_category_value, <<~PATTERN + (block + (send #rspec? {#ExampleGroups.all #Examples.all} ... + (hash <(pair (sym :feature_category) $_) ...>) + ) + ... + ) + PATTERN + + def on_block(node) + value_node = feature_category_value(node) + return unless value_node + + unless value_node.sym_type? + add_offense(value_node, message: MSG_SYMBOL) + return + end + + return if valid_feature_category?(value_node) + + message = format(MSG, msg_suggestion: suggestion_message(value_node)) + add_offense(value_node, message: message) + end + + # Used by RuboCop to invalidate its cache if the contents of + # config/feature_categories.yml changes. + def external_dependency_checksum + @external_dependency_checksum ||= + Digest::SHA256.file(FEATURE_CATEGORIES_PATH).hexdigest + end + + private + + def suggestion_message(value_node) + spell = DidYouMean::SpellChecker.new(dictionary: self.class.feature_categories) + + suggestions = spell.correct(value_node.value) + return if suggestions.none? + + format(MSG_DID_YOU_MEAN, suggestion: suggestions.first) + end + + def valid_feature_category?(node) + self.class.feature_categories.include?(node.value) + end + + def self.feature_categories + @feature_categories ||= YAML + .load_file(FEATURE_CATEGORIES_PATH) + .map(&:to_sym) + .to_set + .union(CUSTOM_FEATURE_CATEGORIES) + end + end + end + end +end diff --git a/rubocop/cop/rspec/missing_feature_category.rb b/rubocop/cop/rspec/missing_feature_category.rb new file mode 100644 index 00000000000..0f517473982 --- /dev/null +++ b/rubocop/cop/rspec/missing_feature_category.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'rubocop/cop/rspec/base' +require 'rubocop/cop/rspec/mixin/top_level_group' + +module RuboCop + module Cop + module RSpec + # Ensures that top level example group contains a feature category. + # + # @example + # + # # bad + # RSpec.describe 'foo' do + # end + # + # # good + # RSpec.describe 'foo', feature_category: :wiki do + # end + class MissingFeatureCategory < RuboCop::Cop::RSpec::Base + include RuboCop::Cop::RSpec::TopLevelGroup + + MSG = 'Please add missing feature category. ' \ + 'See https://docs.gitlab.com/ee/development/feature_categorization/#rspec-examples.' + + # @!method feature_category?(node) + def_node_matcher :feature_category?, <<~PATTERN + (block + (send ... + (hash <(pair (sym :feature_category) _) ...>) + ) + ... + ) + PATTERN + + def on_top_level_example_group(node) + return if feature_category?(node) + + add_offense(node.children.first) + end + end + end + end +end diff --git a/rubocop/cop/scalability/file_uploads.rb b/rubocop/cop/scalability/file_uploads.rb index 3ccb9110e79..fc52444c551 100644 --- a/rubocop/cop/scalability/file_uploads.rb +++ b/rubocop/cop/scalability/file_uploads.rb @@ -26,34 +26,25 @@ module RuboCop # end # class FileUploads < RuboCop::Cop::Base - MSG = 'Do not upload files without workhorse acceleration. Please refer to https://docs.gitlab.com/ee/development/uploads.html' - - def_node_search :file_type_params?, <<~PATTERN - (send nil? {:requires :optional} (sym _) (hash <(pair (sym :type)(const nil? :File)) ...>)) - PATTERN - - def_node_search :file_types_params?, <<~PATTERN - (send nil? {:requires :optional} (sym _) (hash <(pair (sym :types)(array <(const nil? :File) ...>)) ...>)) + MSG = 'Do not upload files without workhorse acceleration. ' \ + 'Please refer to https://docs.gitlab.com/ee/development/uploads.html' + + def_node_matcher :file_in_type, <<~PATTERN + (send nil? {:requires :optional} + (sym _) + (hash + { + <(pair (sym :types) (array <$(const nil? :File) ...>)) ...> + <(pair (sym :type) $(const nil? :File)) ...> + } + ) + ) PATTERN - def be_file_param_usage?(node) - file_type_params?(node) || file_types_params?(node) - end - def on_send(node) - return unless be_file_param_usage?(node) - - add_offense(find_file_param(node)) - end - - private - - def find_file_param(node) - node.each_descendant.find { |children| file_node_pattern.match(children) } - end - - def file_node_pattern - @file_node_pattern ||= RuboCop::NodePattern.new("(const nil? :File)") + file_in_type(node) do |file_node| + add_offense(file_node) + end end end end diff --git a/rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb b/rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb index 34c3767ad11..7fe308d6b40 100644 --- a/rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb +++ b/rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb @@ -26,7 +26,7 @@ module RuboCop class WorkerDataConsistency < RuboCop::Cop::Base include CodeReuseHelpers - HELP_LINK = 'https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#job-data-consistency-strategies' + HELP_LINK = 'https://docs.gitlab.com/ee/development/sidekiq/worker_attributes.html#job-data-consistency-strategies' MSG = <<~MSG Should define data_consistency expectation. |