diff options
Diffstat (limited to 'rubocop')
| -rw-r--r-- | rubocop/cop/database/multiple_databases.rb | 3 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/avoid_feature_get.rb | 4 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/mark_used_feature_flags.rb | 14 | ||||
| -rw-r--r-- | rubocop/cop/gitlab/namespaced_class.rb | 59 | ||||
| -rw-r--r-- | rubocop/cop/migration/background_migration_base_class.rb | 33 | ||||
| -rw-r--r-- | rubocop/cop/migration/background_migration_record.rb | 41 | ||||
| -rw-r--r-- | rubocop/cop/migration/hash_index.rb | 53 | ||||
| -rw-r--r-- | rubocop/cop/migration/migration_record.rb | 37 | ||||
| -rw-r--r-- | rubocop/migration_helpers.rb | 11 |
9 files changed, 176 insertions, 79 deletions
diff --git a/rubocop/cop/database/multiple_databases.rb b/rubocop/cop/database/multiple_databases.rb index f20348d9d1f..1ac9bb4473b 100644 --- a/rubocop/cop/database/multiple_databases.rb +++ b/rubocop/cop/database/multiple_databases.rb @@ -19,10 +19,11 @@ module RuboCop ALLOWED_METHODS = %i[ no_touching + configurations ].freeze def_node_matcher :active_record_base_method_is_used?, <<~PATTERN - (send (const (const nil? :ActiveRecord) :Base) $_) + (send (const (const _ :ActiveRecord) :Base) $_) PATTERN def on_send(node) diff --git a/rubocop/cop/gitlab/avoid_feature_get.rb b/rubocop/cop/gitlab/avoid_feature_get.rb index e36e0b020c0..1bce89268d9 100644 --- a/rubocop/cop/gitlab/avoid_feature_get.rb +++ b/rubocop/cop/gitlab/avoid_feature_get.rb @@ -5,8 +5,8 @@ module RuboCop module Gitlab # Cop that blacklists the use of `Feature.get`. class AvoidFeatureGet < RuboCop::Cop::Cop - MSG = 'Use `Feature.enable/disable` methods instead of `Feature.get`. ' \ - 'See doc/development/testing_guide/best_practices.md#feature-flags-in-tests for more information.' + MSG = 'Use `stub_feature_flags` method instead of `Feature.get`. ' \ + 'See doc/development/feature_flags/index.md#feature-flags-in-tests for more information.' def_node_matcher :feature_get?, <<~PATTERN (send (const nil? :Feature) :get ...) diff --git a/rubocop/cop/gitlab/mark_used_feature_flags.rb b/rubocop/cop/gitlab/mark_used_feature_flags.rb index 290e62436e2..0bebd7901f3 100644 --- a/rubocop/cop/gitlab/mark_used_feature_flags.rb +++ b/rubocop/cop/gitlab/mark_used_feature_flags.rb @@ -43,13 +43,6 @@ module RuboCop File.expand_path("../../../lib/gitlab/usage_data_counters/known_events/*.yml", __dir__) ].freeze - DYNAMIC_FEATURE_FLAGS = [ - :usage_data_static_site_editor_commits, # https://gitlab.com/gitlab-org/gitlab/-/issues/284082 - :usage_data_static_site_editor_merge_requests, # https://gitlab.com/gitlab-org/gitlab/-/issues/284083 - :usage_data_users_clicking_license_testing_visiting_external_website, # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77866 - :usage_data_users_visiting_testing_license_compliance_full_report # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77866 - ].freeze - class << self # We track feature flags in `on_new_investigation` only once per # rubocop whole run instead once per file. @@ -65,7 +58,6 @@ module RuboCop self.class.feature_flags_already_tracked = true - track_dynamic_feature_flags! track_usage_data_counters_known_events! end @@ -231,12 +223,6 @@ 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_dynamic_feature_flags! - DYNAMIC_FEATURE_FLAGS.each(&method(:save_used_feature_flag)) - end - - # 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)) end diff --git a/rubocop/cop/gitlab/namespaced_class.rb b/rubocop/cop/gitlab/namespaced_class.rb index 1f1fd280922..914cc8720b9 100644 --- a/rubocop/cop/gitlab/namespaced_class.rb +++ b/rubocop/cop/gitlab/namespaced_class.rb @@ -5,33 +5,76 @@ module RuboCop module Gitlab # Cop that enforces use of namespaced classes in order to better identify # high level domains within the codebase. - + # # @example # # bad # class MyClass # end # + # module Gitlab + # class MyClass + # end + # end + # + # class Gitlab::MyClass + # end + # # # good # module MyDomain # class MyClass # end # end - + # + # module Gitlab + # module MyDomain + # class MyClass + # end + # end + # end + # + # class Gitlab::MyDomain::MyClass + # end class NamespacedClass < RuboCop::Cop::Cop MSG = 'Classes must be declared inside a module indicating a product domain namespace. For more info: https://gitlab.com/gitlab-org/gitlab/-/issues/212156' - def_node_matcher :compact_namespaced_class?, <<~PATTERN - (class (const (const ...) ...) ...) - PATTERN + # These namespaces are considered top-level semantically. + # Note: Nested namespace like Foo::Bar are also supported. + PSEUDO_TOPLEVEL = %w[Gitlab] + .map { _1.split('::') }.freeze def on_module(node) - @namespaced = true + add_potential_domain_namespace(node) end def on_class(node) - return if @namespaced + # Add potential namespaces from compact definitions like `class Foo::Bar`. + # Remove class name because it's not a domain namespace. + add_potential_domain_namespace(node) { _1.pop } + + add_offense(node) if domain_namespaces.none? + end + + private + + def domain_namespaces + @domain_namespaces ||= [] + end + + def add_potential_domain_namespace(node) + return if domain_namespaces.any? + + identifiers = identifiers_for(node) + yield(identifiers) if block_given? + + PSEUDO_TOPLEVEL.each do |namespaces| + identifiers.shift(namespaces.size) if namespaces == identifiers.first(namespaces.size) + end + + domain_namespaces.concat(identifiers) + end - add_offense(node) unless compact_namespaced_class?(node) + def identifiers_for(node) + node.identifier.source.sub(/^::/, '').split('::') end end end diff --git a/rubocop/cop/migration/background_migration_base_class.rb b/rubocop/cop/migration/background_migration_base_class.rb new file mode 100644 index 00000000000..50cbe3a69c3 --- /dev/null +++ b/rubocop/cop/migration/background_migration_base_class.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Migration + class BackgroundMigrationBaseClass < RuboCop::Cop::Cop + MSG = 'Batched background migration jobs should inherit from Gitlab::BackgroundMigration::BatchedMigrationJob' + + def_node_search :top_level_module?, <<~PATTERN + (module (const nil? :Gitlab) (module (const nil? :BackgroundMigration) ...)) + PATTERN + + def_node_matcher :matching_parent_namespace?, <<~PATTERN + {nil? (const (const {cbase nil?} :Gitlab) :BackgroundMigration)} + PATTERN + + def_node_search :inherits_batched_migration_job?, <<~PATTERN + (class _ (const #matching_parent_namespace? :BatchedMigrationJob) ...) + PATTERN + + def on_module(module_node) + return unless top_level_module?(module_node) + + top_level_class_node = module_node.each_descendant(:class).first + + return if top_level_class_node.nil? || inherits_batched_migration_job?(top_level_class_node) + + add_offense(top_level_class_node, location: :expression) + end + end + end + end +end diff --git a/rubocop/cop/migration/background_migration_record.rb b/rubocop/cop/migration/background_migration_record.rb new file mode 100644 index 00000000000..2ee6b9f7b42 --- /dev/null +++ b/rubocop/cop/migration/background_migration_record.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + class BackgroundMigrationRecord < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = <<~MSG + Don't use or inherit from ActiveRecord::Base. + Use ::ApplicationRecord or ::Ci::ApplicationRecord to ensure the correct database is used. + See https://docs.gitlab.com/ee/development/database/batched_background_migrations.html#accessing-data-for-multiple-databases. + MSG + + def_node_matcher :inherits_from_active_record_base?, <<~PATTERN + (class _ (const (const _ :ActiveRecord) :Base) _) + PATTERN + + def_node_search :class_new_active_record_base?, <<~PATTERN + (send (const _ :Class) :new (const (const _ :ActiveRecord) :Base) ...) + PATTERN + + def on_class(node) + return unless in_background_migration?(node) + return unless inherits_from_active_record_base?(node) + + add_offense(node, location: :expression) + end + + def on_send(node) + return unless in_background_migration?(node) + return unless class_new_active_record_base?(node) + + add_offense(node, location: :expression) + end + end + end + end +end diff --git a/rubocop/cop/migration/hash_index.rb b/rubocop/cop/migration/hash_index.rb deleted file mode 100644 index 8becef891af..00000000000 --- a/rubocop/cop/migration/hash_index.rb +++ /dev/null @@ -1,53 +0,0 @@ -# frozen_string_literal: true - -require 'set' -require_relative '../../migration_helpers' - -module RuboCop - module Cop - module Migration - # Cop that prevents the use of hash indexes in database migrations - class HashIndex < RuboCop::Cop::Cop - include MigrationHelpers - - MSG = 'hash indexes should be avoided at all costs since they are not ' \ - 'recorded in the PostgreSQL WAL, you should use a btree index instead' - - NAMES = Set.new([:add_index, :index, :add_concurrent_index]).freeze - - def on_send(node) - return unless in_migration?(node) - - name = node.children[1] - - return unless NAMES.include?(name) - - opts = node.children.last - - return unless opts && opts.type == :hash - - opts.each_node(:pair) do |pair| - next unless hash_key_type(pair) == :sym && - hash_key_name(pair) == :using - - if hash_key_value(pair).to_s == 'hash' - add_offense(pair, location: :expression) - end - end - end - - def hash_key_type(pair) - pair.children[0].type - end - - def hash_key_name(pair) - pair.children[0].children[0] - end - - def hash_key_value(pair) - pair.children[1].children[0] - end - end - end - end -end diff --git a/rubocop/cop/migration/migration_record.rb b/rubocop/cop/migration/migration_record.rb new file mode 100644 index 00000000000..bb81b3cbaf1 --- /dev/null +++ b/rubocop/cop/migration/migration_record.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + class MigrationRecord < RuboCop::Cop::Cop + include MigrationHelpers + + ENFORCED_SINCE = 2022_04_26_00_00_00 + + MSG = <<~MSG + Don't inherit from ActiveRecord::Base but use MigrationRecord instead. + See https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html#example-usage-of-activerecord-classes. + MSG + + def_node_search :inherits_from_active_record_base?, <<~PATTERN + (class _ (const (const _ :ActiveRecord) :Base) _) + PATTERN + + def on_class(node) + return unless relevant_migration?(node) + return unless inherits_from_active_record_base?(node) + + add_offense(node, location: :expression) + end + + private + + def relevant_migration?(node) + in_migration?(node) && version(node) >= ENFORCED_SINCE + end + end + end + end +end diff --git a/rubocop/migration_helpers.rb b/rubocop/migration_helpers.rb index 63b3766e126..f14f4d33709 100644 --- a/rubocop/migration_helpers.rb +++ b/rubocop/migration_helpers.rb @@ -32,6 +32,11 @@ module RuboCop in_deployment_migration?(node) || in_post_deployment_migration?(node) end + def in_background_migration?(node) + filepath(node).include?('/lib/gitlab/background_migration/') || + filepath(node).include?('/ee/lib/ee/gitlab/background_migration/') + end + def in_deployment_migration?(node) dirname(node).end_with?('db/migrate', 'db/geo/migrate') end @@ -56,8 +61,12 @@ module RuboCop private + def filepath(node) + node.location.expression.source_buffer.name + end + def dirname(node) - File.dirname(node.location.expression.source_buffer.name) + File.dirname(filepath(node)) end def rubocop_migrations_config |