From 0653e08efd039a5905f3fa4f6e9cef9f5d2f799c Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 20 Sep 2021 13:18:24 +0000 Subject: Add latest changes from gitlab-org/gitlab@14-3-stable-ee --- rubocop/cop/gitlab/mark_used_feature_flags.rb | 29 ++-- rubocop/cop/migration/add_limit_to_text_columns.rb | 27 +++- rubocop/cop/migration/prevent_index_creation.rb | 33 ++++- rubocop/cop/migration/versioned_migration_class.rb | 56 ++++++++ .../active_record_subtransaction_methods.rb | 29 ++++ .../performance/active_record_subtransactions.rb | 30 ++++ .../worker_data_consistency.rb | 65 +++++++++ .../worker_data_consistency_with_deduplication.rb | 154 +++++++++++++++++++++ rubocop/cop/worker_data_consistency.rb | 63 --------- rubocop/rubocop-migrations.yml | 1 + 10 files changed, 405 insertions(+), 82 deletions(-) create mode 100644 rubocop/cop/migration/versioned_migration_class.rb create mode 100644 rubocop/cop/performance/active_record_subtransaction_methods.rb create mode 100644 rubocop/cop/performance/active_record_subtransactions.rb create mode 100644 rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb create mode 100644 rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication.rb delete mode 100644 rubocop/cop/worker_data_consistency.rb (limited to 'rubocop') diff --git a/rubocop/cop/gitlab/mark_used_feature_flags.rb b/rubocop/cop/gitlab/mark_used_feature_flags.rb index a0de43abe85..03ee4805f4e 100644 --- a/rubocop/cop/gitlab/mark_used_feature_flags.rb +++ b/rubocop/cop/gitlab/mark_used_feature_flags.rb @@ -47,10 +47,21 @@ module RuboCop :usage_data_static_site_editor_merge_requests # https://gitlab.com/gitlab-org/gitlab/-/issues/284083 ].freeze + class << self + # We track feature flags in `on_new_investigation` only once per + # rubocop whole run instead once per file. + attr_accessor :feature_flags_already_tracked + end + # Called before all on_... have been called # When refining this method, always call `super` def on_new_investigation super + + return if self.class.feature_flags_already_tracked + + self.class.feature_flags_already_tracked = true + track_dynamic_feature_flags! track_usage_data_counters_known_events! end @@ -69,7 +80,7 @@ module RuboCop flag_value = flag_value(node) return unless flag_value - if flag_arg_is_str_or_sym?(node) + if flag_arg_is_str_or_sym?(flag_arg) if caller_is_feature_gitaly?(node) save_used_feature_flag("gitaly_#{flag_value}") else @@ -84,9 +95,9 @@ module RuboCop save_used_feature_flag(matching_feature_flag) end end - elsif flag_arg_is_send_type?(node) + elsif flag_arg_is_send_type?(flag_arg) puts_if_ci(node, "Feature flag is dynamic: '#{flag_value}.") - elsif flag_arg_is_dstr_or_dsym?(node) + elsif flag_arg_is_dstr_or_dsym?(flag_arg) str_prefix = flag_arg.children[0] rest_children = flag_arg.children[1..] @@ -159,18 +170,16 @@ module RuboCop end.to_s.tr("\n/", ' _') end - def flag_arg_is_str_or_sym?(node) - flag_arg = flag_arg(node) + def flag_arg_is_str_or_sym?(flag_arg) flag_arg.str_type? || flag_arg.sym_type? end - def flag_arg_is_send_type?(node) - flag_arg(node).send_type? + def flag_arg_is_send_type?(flag_arg) + flag_arg.send_type? end - def flag_arg_is_dstr_or_dsym?(node) - flag = flag_arg(node) - (flag.dstr_type? || flag.dsym_type?) && flag.children[0].str_type? + def flag_arg_is_dstr_or_dsym?(flag_arg) + (flag_arg.dstr_type? || flag_arg.dsym_type?) && flag_arg.children[0].str_type? end def caller_is_feature?(node) diff --git a/rubocop/cop/migration/add_limit_to_text_columns.rb b/rubocop/cop/migration/add_limit_to_text_columns.rb index f45551e60a4..b5780e87c19 100644 --- a/rubocop/cop/migration/add_limit_to_text_columns.rb +++ b/rubocop/cop/migration/add_limit_to_text_columns.rb @@ -13,8 +13,13 @@ module RuboCop class AddLimitToTextColumns < RuboCop::Cop::Cop include MigrationHelpers + TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE = 2021_09_10_00_00_00 + MSG = 'Text columns should always have a limit set (255 is suggested). ' \ - 'You can add a limit to a `text` column by using `add_text_limit`' + 'You can add a limit to a `text` column by using `add_text_limit` or by using `.text... limit: 255` inside `create_table`' + + TEXT_LIMIT_ATTRIBUTE_NOT_ALLOWED = 'Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. ' \ + 'You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table`' def_node_matcher :reverting?, <<~PATTERN (def :down ...) @@ -37,15 +42,29 @@ module RuboCop node.each_descendant(:send) do |send_node| next unless text_operation?(send_node) - # We require a limit for the same table and attribute name - if text_limit_missing?(node, *table_and_attribute_name(send_node)) - add_offense(send_node, location: :selector) + if text_operation_with_limit?(send_node) + add_offense(send_node, location: :selector, message: TEXT_LIMIT_ATTRIBUTE_NOT_ALLOWED) if version(node) < TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE + else + # We require a limit for the same table and attribute name + if text_limit_missing?(node, *table_and_attribute_name(send_node)) + add_offense(send_node, location: :selector) + end end end end private + def text_operation_with_limit?(node) + migration_method = node.children[1] + + return unless migration_method == :text + + if attributes = node.children[3] + attributes.pairs.find { |pair| pair.key.value == :limit }.present? + end + end + def text_operation?(node) # Don't complain about text arrays return false if array_column?(node) diff --git a/rubocop/cop/migration/prevent_index_creation.rb b/rubocop/cop/migration/prevent_index_creation.rb index c90f911d24e..c383466f73b 100644 --- a/rubocop/cop/migration/prevent_index_creation.rb +++ b/rubocop/cop/migration/prevent_index_creation.rb @@ -12,16 +12,29 @@ module RuboCop MSG = "Adding new index to #{FORBIDDEN_TABLES.join(", ")} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886" + def on_new_investigation + super + @forbidden_tables_used = false + end + def_node_matcher :add_index?, <<~PATTERN - (send nil? :add_index (sym #forbidden_tables?) ...) + (send nil? :add_index ({sym|str} #forbidden_tables?) ...) PATTERN def_node_matcher :add_concurrent_index?, <<~PATTERN - (send nil? :add_concurrent_index (sym #forbidden_tables?) ...) + (send nil? :add_concurrent_index ({sym|str} #forbidden_tables?) ...) PATTERN - def forbidden_tables?(node) - FORBIDDEN_TABLES.include?(node) + def_node_matcher :forbidden_constant_defined?, <<~PATTERN + (casgn nil? _ ({sym|str} #forbidden_tables?)) + PATTERN + + def_node_matcher :add_concurrent_index_with_constant?, <<~PATTERN + (send nil? :add_concurrent_index (const nil? _) ...) + PATTERN + + def on_casgn(node) + @forbidden_tables_used = !!forbidden_constant_defined?(node) end def on_def(node) @@ -32,8 +45,18 @@ module RuboCop end end + private + + def forbidden_tables?(node) + FORBIDDEN_TABLES.include?(node.to_sym) + end + def offense?(node) - add_index?(node) || add_concurrent_index?(node) + add_index?(node) || add_concurrent_index?(node) || any_constant_used_with_forbidden_tables?(node) + end + + def any_constant_used_with_forbidden_tables?(node) + add_concurrent_index_with_constant?(node) && @forbidden_tables_used end end end diff --git a/rubocop/cop/migration/versioned_migration_class.rb b/rubocop/cop/migration/versioned_migration_class.rb new file mode 100644 index 00000000000..f2e4550c691 --- /dev/null +++ b/rubocop/cop/migration/versioned_migration_class.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + class VersionedMigrationClass < RuboCop::Cop::Cop + include MigrationHelpers + + ENFORCED_SINCE = 2021_09_02_00_00_00 + + MSG_INHERIT = 'Don\'t inherit from ActiveRecord::Migration but use Gitlab::Database::Migration[1.0] 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[1.0] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning.' + + ACTIVERECORD_MIGRATION_CLASS = 'ActiveRecord::Migration' + + def_node_search :includes_helpers?, <<~PATTERN + (send nil? :include + (const + (const + (const nil? :Gitlab) :Database) :MigrationHelpers)) + PATTERN + + def on_class(node) + return unless relevant_migration?(node) + return unless activerecord_migration_class?(node) + + add_offense(node, location: :expression, message: MSG_INHERIT) + end + + def on_send(node) + return unless relevant_migration?(node) + + add_offense(node, location: :expression, message: MSG_INCLUDE) if includes_helpers?(node) + end + + private + + def relevant_migration?(node) + in_migration?(node) && version(node) >= ENFORCED_SINCE + end + + def activerecord_migration_class?(node) + superclass(node) == ACTIVERECORD_MIGRATION_CLASS + end + + def superclass(class_node) + _, *others = class_node.descendants + + others.find { |node| node.const_type? && node&.const_name != 'Types' }&.const_name + end + end + end + end +end diff --git a/rubocop/cop/performance/active_record_subtransaction_methods.rb b/rubocop/cop/performance/active_record_subtransaction_methods.rb new file mode 100644 index 00000000000..3b89d3ab858 --- /dev/null +++ b/rubocop/cop/performance/active_record_subtransaction_methods.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # Cop that disallows certain methods that rely on subtransactions in their implementation. + # Companion to Performance/ActiveRecordSubtransactions, which bans direct usage of subtransactions. + class ActiveRecordSubtransactionMethods < RuboCop::Cop::Cop + MSG = 'Methods that rely on subtransactions should not be used. ' \ + 'For more information see: https://gitlab.com/gitlab-org/gitlab/-/issues/338346' + + DISALLOWED_METHODS = %i[ + safe_ensure_unique + safe_find_or_create_by + safe_find_or_create_by! + with_fast_read_statement_timeout + create_or_find_by + create_or_find_by! + ].to_set.freeze + + def on_send(node) + return unless DISALLOWED_METHODS.include?(node.method_name) + + add_offense(node, location: :selector) + end + end + end + end +end diff --git a/rubocop/cop/performance/active_record_subtransactions.rb b/rubocop/cop/performance/active_record_subtransactions.rb new file mode 100644 index 00000000000..a550b558e52 --- /dev/null +++ b/rubocop/cop/performance/active_record_subtransactions.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + class ActiveRecordSubtransactions < RuboCop::Cop::Cop + MSG = 'Subtransactions should not be used. ' \ + 'For more information see: https://gitlab.com/gitlab-org/gitlab/-/issues/338346' + + def_node_matcher :match_transaction_with_options, <<~PATTERN + (send _ :transaction (hash $...)) + PATTERN + + def_node_matcher :subtransaction_option?, <<~PATTERN + (pair (:sym :requires_new) (true)) + PATTERN + + def on_send(node) + match_transaction_with_options(node) do |option_nodes| + option_nodes.each do |option_node| + next unless subtransaction_option?(option_node) + + add_offense(option_node) + end + end + end + 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 new file mode 100644 index 00000000000..7a2e7ee00b4 --- /dev/null +++ b/rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module SidekiqLoadBalancing + # This cop checks for a call to `data_consistency` to exist in Sidekiq workers. + # + # @example + # + # # bad + # class BadWorker + # def perform + # end + # end + # + # # good + # class GoodWorker + # data_consistency :delayed + # + # def perform + # end + # end + # + class WorkerDataConsistency < RuboCop::Cop::Cop + include CodeReuseHelpers + + HELP_LINK = 'https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#job-data-consistency-strategies' + + MSG = <<~MSG + Should define data_consistency expectation. + + It is encouraged for workers to use database replicas as much as possible by declaring + data_consistency to use the :delayed or :sticky modes. Mode :always will result in the + worker always hitting the primary database node for both reads and writes, which limits + scalability. + + Some guidelines: + + 1. If your worker mostly writes or reads its own writes, use mode :always. TRY TO AVOID THIS. + 2. If your worker performs mostly reads and can tolerate small delays, use mode :delayed. + 3. If your worker performs mostly reads but cannot tolerate any delays, use mode :sticky. + + See #{HELP_LINK} for a more detailed explanation of these settings. + MSG + + def_node_search :application_worker?, <<~PATTERN + `(send nil? :include (const nil? :ApplicationWorker)) + PATTERN + + def_node_search :data_consistency_defined?, <<~PATTERN + `(send nil? :data_consistency ...) + PATTERN + + def on_class(node) + return unless in_worker?(node) && application_worker?(node) + return if data_consistency_defined?(node) + + add_offense(node, location: :expression) + end + end + end + end +end diff --git a/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication.rb b/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication.rb new file mode 100644 index 00000000000..e8b4b513a23 --- /dev/null +++ b/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module SidekiqLoadBalancing + # This cop checks for including_scheduled: true option in idempotent Sidekiq workers that utilize load balancing capabilities. + # + # @example + # + # # bad + # class BadWorker + # include ApplicationWorker + # + # data_consistency :delayed + # idempotent! + # + # def perform + # end + # end + # + # # bad + # class BadWorker + # include ApplicationWorker + # + # data_consistency :delayed + # + # deduplicate :until_executing + # idempotent! + # + # def perform + # end + # end + # + # # good + # class GoodWorker + # include ApplicationWorker + # + # data_consistency :delayed + # + # deduplicate :until_executing, including_scheduled: true + # idempotent! + # + # def perform + # end + # end + # + class WorkerDataConsistencyWithDeduplication < RuboCop::Cop::Base + include CodeReuseHelpers + extend AutoCorrector + + HELP_LINK = 'https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#scheduling-jobs-in-the-future' + REPLACEMENT = ', including_scheduled: true' + DEFAULT_STRATEGY = ':until_executing' + + MSG = <<~MSG + Workers that declare either `:sticky` or `:delayed` data consistency become eligible for database load-balancing. + In both cases, jobs are enqueued with a short delay. + + If you do want to deduplicate jobs that utilize load-balancing, you need to specify including_scheduled: true + argument when defining deduplication strategy. + + See #{HELP_LINK} for a more detailed explanation of these settings. + MSG + + def_node_search :application_worker?, <<~PATTERN + `(send nil? :include (const nil? :ApplicationWorker)) + PATTERN + + def_node_search :idempotent_worker?, <<~PATTERN + `(send nil? :idempotent!) + PATTERN + + def_node_search :data_consistency_defined?, <<~PATTERN + `(send nil? :data_consistency (sym {:sticky :delayed })) + PATTERN + + def_node_matcher :including_scheduled?, <<~PATTERN + `(hash <(pair (sym :including_scheduled) (%1)) ...>) + PATTERN + + def_node_matcher :deduplicate_strategy?, <<~PATTERN + `(send nil? :deduplicate (sym $_) $(...)?) + PATTERN + + def on_class(node) + return unless in_worker?(node) + return unless application_worker?(node) + return unless idempotent_worker?(node) + return unless data_consistency_defined?(node) + + @strategy, options = deduplicate_strategy?(node) + including_scheduled = false + if options + @deduplicate_options = options[0] + including_scheduled = including_scheduled?(@deduplicate_options, :true) # rubocop:disable Lint/BooleanSymbol + end + + @offense = !(including_scheduled || @strategy == :none) + end + + def on_send(node) + return unless offense + + if node.children[1] == :deduplicate + add_offense(node.loc.expression) do |corrector| + autocorrect_deduplicate_strategy(node, corrector) + end + elsif node.children[1] == :idempotent! && !strategy + add_offense(node.loc.expression) do |corrector| + autocorrect_missing_deduplicate_strategy(node, corrector) + end + end + end + + private + + attr_reader :offense, :deduplicate_options, :strategy + + def autocorrect_deduplicate_with_options(corrector) + if including_scheduled?(deduplicate_options, :false) # rubocop:disable Lint/BooleanSymbol + replacement = deduplicate_options.source.sub("including_scheduled: false", "including_scheduled: true") + corrector.replace(deduplicate_options.loc.expression, replacement) + else + corrector.insert_after(deduplicate_options.loc.expression, REPLACEMENT) + end + end + + def autocorrect_deduplicate_without_options(node, corrector) + corrector.insert_after(node.loc.expression, REPLACEMENT) + end + + def autocorrect_missing_deduplicate_strategy(node, corrector) + indent_found = node.source_range.source_line =~ /^( +)/ + # Get indentation size + whitespaces = Regexp.last_match(1).size if indent_found + replacement = "deduplicate #{DEFAULT_STRATEGY}#{REPLACEMENT}\n" + # Add indentation in the end since we are inserting a whole line before idempotent! + replacement += ' ' * whitespaces.to_i + corrector.insert_before(node.source_range, replacement) + end + + def autocorrect_deduplicate_strategy(node, corrector) + if deduplicate_options + autocorrect_deduplicate_with_options(corrector) + else + autocorrect_deduplicate_without_options(node, corrector) + end + end + end + end + end +end diff --git a/rubocop/cop/worker_data_consistency.rb b/rubocop/cop/worker_data_consistency.rb deleted file mode 100644 index e9047750f3e..00000000000 --- a/rubocop/cop/worker_data_consistency.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -require_relative '../code_reuse_helpers' - -module RuboCop - module Cop - # This cop checks for a call to `data_consistency` to exist in Sidekiq workers. - # - # @example - # - # # bad - # class BadWorker - # def perform - # end - # end - # - # # good - # class GoodWorker - # data_consistency :delayed - # - # def perform - # end - # end - # - class WorkerDataConsistency < RuboCop::Cop::Cop - include CodeReuseHelpers - - HELP_LINK = 'https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#job-data-consistency-strategies' - - MSG = <<~MSG - Should define data_consistency expectation. - - It is encouraged for workers to use database replicas as much as possible by declaring - data_consistency to use the :delayed or :sticky modes. Mode :always will result in the - worker always hitting the primary database node for both reads and writes, which limits - scalability. - - Some guidelines: - - 1. If your worker mostly writes or reads its own writes, use mode :always. TRY TO AVOID THIS. - 2. If your worker performs mostly reads and can tolerate small delays, use mode :delayed. - 3. If your worker performs mostly reads but cannot tolerate any delays, use mode :sticky. - - See #{HELP_LINK} for a more detailed explanation of these settings. - MSG - - def_node_search :application_worker?, <<~PATTERN - `(send nil? :include (const nil? :ApplicationWorker)) - PATTERN - - def_node_search :data_consistency_defined?, <<~PATTERN - `(send nil? :data_consistency ...) - PATTERN - - def on_class(node) - return unless in_worker?(node) && application_worker?(node) - return if data_consistency_defined?(node) - - add_offense(node, location: :expression) - end - end - end -end diff --git a/rubocop/rubocop-migrations.yml b/rubocop/rubocop-migrations.yml index 979b0fa2936..a98a059df1d 100644 --- a/rubocop/rubocop-migrations.yml +++ b/rubocop/rubocop-migrations.yml @@ -14,6 +14,7 @@ Migration/UpdateLargeTable: - :events - :gitlab_subscriptions - :issues + - :members - :merge_request_diff_commits - :merge_request_diff_files - :merge_request_diffs -- cgit v1.2.3