diff options
Diffstat (limited to 'rubocop')
| -rw-r--r-- | rubocop/cop/avoid_becomes.rb | 30 | ||||
| -rw-r--r-- | rubocop/cop/graphql/json_type.rb | 40 | ||||
| -rw-r--r-- | rubocop/cop/migration/with_lock_retries_disallowed_method.rb | 1 | ||||
| -rw-r--r-- | rubocop/cop/put_group_routes_under_scope.rb | 32 | ||||
| -rw-r--r-- | rubocop/cop/put_project_routes_under_scope.rb | 32 | ||||
| -rw-r--r-- | rubocop/cop/usage_data/distinct_count_by_large_foreign_key.rb | 43 | ||||
| -rw-r--r-- | rubocop/cop/usage_data/large_table.rb | 87 | ||||
| -rw-r--r-- | rubocop/routes_under_scope.rb | 29 | ||||
| -rw-r--r-- | rubocop/rubocop-migrations.yml | 1 | ||||
| -rw-r--r-- | rubocop/rubocop-usage-data.yml | 46 |
10 files changed, 284 insertions, 57 deletions
diff --git a/rubocop/cop/avoid_becomes.rb b/rubocop/cop/avoid_becomes.rb new file mode 100644 index 00000000000..cfd6b3d2164 --- /dev/null +++ b/rubocop/cop/avoid_becomes.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Cop that blacklists the use of ".becomes(SomeConstant)". + # + # The use of becomes() will result in a new object being created, throwing + # away any eager loaded assocations. This in turn can cause N+1 query + # problems, even when a developer eager loaded all necessary associations. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/23182 for more information. + class AvoidBecomes < RuboCop::Cop::Cop + MSG = 'Avoid the use of becomes(SomeConstant), as this creates a ' \ + 'new object and throws away any eager loaded associations. ' \ + 'When creating URLs in views, just use the path helpers directly. ' \ + 'For example, instead of `link_to(..., [group.becomes(Namespace), ...])` ' \ + 'use `link_to(..., namespace_foo_path(group, ...))`. Most of the time there is no ' \ + 'need to pass in namespace to the path helpers after implementaton of ' \ + 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/12566' + + def_node_matcher :becomes?, <<~PATTERN + (send {send ivar lvar} :becomes ...) + PATTERN + + def on_send(node) + add_offense(node, location: :expression) if becomes?(node) + end + end + end +end diff --git a/rubocop/cop/graphql/json_type.rb b/rubocop/cop/graphql/json_type.rb new file mode 100644 index 00000000000..1e3e3d7a7ff --- /dev/null +++ b/rubocop/cop/graphql/json_type.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +# This cop checks for use of GraphQL::Types::JSON types in GraphQL fields +# and arguments. +# +# @example +# +# # bad +# class AwfulClass +# field :some_field, GraphQL::Types::JSON +# end +# +# # good +# class GreatClass +# field :some_field, GraphQL::STRING_TYPE +# end + +module RuboCop + module Cop + module Graphql + class JSONType < RuboCop::Cop::Cop + MSG = 'Avoid using GraphQL::Types::JSON. See: ' \ + 'https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#json'.freeze + + def_node_matcher :has_json_type?, <<~PATTERN + (send nil? {:field :argument} + (sym _) + (const + (const + (const nil? :GraphQL) :Types) :JSON) + (...)?) + PATTERN + + def on_send(node) + add_offense(node, location: :expression) if has_json_type?(node) + end + end + end + end +end diff --git a/rubocop/cop/migration/with_lock_retries_disallowed_method.rb b/rubocop/cop/migration/with_lock_retries_disallowed_method.rb index 9bf81e7db0c..a89c33c298b 100644 --- a/rubocop/cop/migration/with_lock_retries_disallowed_method.rb +++ b/rubocop/cop/migration/with_lock_retries_disallowed_method.rb @@ -18,6 +18,7 @@ module RuboCop remove_column execute change_column_default + change_column_null remove_foreign_key_if_exists remove_foreign_key_without_error table_exists? diff --git a/rubocop/cop/put_group_routes_under_scope.rb b/rubocop/cop/put_group_routes_under_scope.rb index bcdde01fdb5..9adec044da8 100644 --- a/rubocop/cop/put_group_routes_under_scope.rb +++ b/rubocop/cop/put_group_routes_under_scope.rb @@ -1,43 +1,19 @@ # frozen_string_literal: true +require_relative '../routes_under_scope' + module RuboCop module Cop # Checks for a group routes outside '/-/' scope. # For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572 class PutGroupRoutesUnderScope < RuboCop::Cop::Cop + include RoutesUnderScope + MSG = 'Put new group routes under /-/ scope' def_node_matcher :dash_scope?, <<~PATTERN (:send nil? :scope (hash <(pair (sym :path)(str "groups/*group_id/-")) ...>)) PATTERN - - def on_send(node) - return unless in_group_routes?(node) - return unless resource?(node) - return unless outside_scope?(node) - - add_offense(node) - end - - def outside_scope?(node) - node.each_ancestor(:block).none? do |parent| - dash_scope?(parent.to_a.first) - end - end - - def in_group_routes?(node) - path = node.location.expression.source_buffer.name - dirname = File.dirname(path) - filename = File.basename(path) - - dirname.end_with?('config/routes') && - filename.end_with?('group.rb') - end - - def resource?(node) - node.method_name == :resource || - node.method_name == :resources - end end end end diff --git a/rubocop/cop/put_project_routes_under_scope.rb b/rubocop/cop/put_project_routes_under_scope.rb index 02189f43ea0..cddb147324f 100644 --- a/rubocop/cop/put_project_routes_under_scope.rb +++ b/rubocop/cop/put_project_routes_under_scope.rb @@ -1,43 +1,19 @@ # frozen_string_literal: true +require_relative '../routes_under_scope' + module RuboCop module Cop # Checks for a project routes outside '/-/' scope. # For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572 class PutProjectRoutesUnderScope < RuboCop::Cop::Cop + include RoutesUnderScope + MSG = 'Put new project routes under /-/ scope' def_node_matcher :dash_scope?, <<~PATTERN (:send nil? :scope (:str "-")) PATTERN - - def on_send(node) - return unless in_project_routes?(node) - return unless resource?(node) - return unless outside_scope?(node) - - add_offense(node) - end - - def outside_scope?(node) - node.each_ancestor(:block).none? do |parent| - dash_scope?(parent.to_a.first) - end - end - - def in_project_routes?(node) - path = node.location.expression.source_buffer.name - dirname = File.dirname(path) - filename = File.basename(path) - - dirname.end_with?('config/routes') && - filename.end_with?('project.rb') - end - - def resource?(node) - node.method_name == :resource || - node.method_name == :resources - end end end end diff --git a/rubocop/cop/usage_data/distinct_count_by_large_foreign_key.rb b/rubocop/cop/usage_data/distinct_count_by_large_foreign_key.rb new file mode 100644 index 00000000000..c10ecbcb4ba --- /dev/null +++ b/rubocop/cop/usage_data/distinct_count_by_large_foreign_key.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module UsageData + # Allows counts only for selected tables' foreign keys for `distinct_count` method. + # + # Because distinct_counts over large tables' foreign keys will take a long time + # + # @example + # + # # bad because pipeline_id points to a large table + # distinct_count(Ci::Build, :commit_id) + # + class DistinctCountByLargeForeignKey < RuboCop::Cop::Cop + MSG = 'Avoid doing `%s` on foreign keys for large tables having above 100 million rows.'.freeze + + def_node_matcher :distinct_count?, <<-PATTERN + (send _ $:distinct_count $...) + PATTERN + + def on_send(node) + distinct_count?(node) do |method_name, method_arguments| + next unless method_arguments && method_arguments.length >= 2 + next if allowed_foreign_key?(method_arguments[1]) + + add_offense(node, location: :selector, message: format(MSG, method_name)) + end + end + + private + + def allowed_foreign_key?(key) + key.type == :sym && allowed_foreign_keys.include?(key.value) + end + + def allowed_foreign_keys + cop_config['AllowedForeignKeys'] || [] + end + end + end + end +end diff --git a/rubocop/cop/usage_data/large_table.rb b/rubocop/cop/usage_data/large_table.rb new file mode 100644 index 00000000000..d9d44f74d26 --- /dev/null +++ b/rubocop/cop/usage_data/large_table.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module UsageData + class LargeTable < RuboCop::Cop::Cop + # This cop checks that batch count and distinct_count are used in usage_data.rb files in metrics based on ActiveRecord models. + # + # @example + # + # # bad + # Issue.count + # List.assignee.count + # ::Ci::Pipeline.auto_devops_source.count + # ZoomMeeting.distinct.count(:issue_id) + # + # # Good + # count(Issue) + # count(List.assignee) + # count(::Ci::Pipeline.auto_devops_source) + # distinct_count(ZoomMeeting, :issue_id) + MSG = 'Use one of the %{count_methods} methods for counting on %{class_name}' + + # Match one level const as Issue, Gitlab + def_node_matcher :one_level_node, <<~PATTERN + (send + (const {nil? cbase} $...) + $...) + PATTERN + + # Match two level const as ::Clusters::Cluster, ::Ci::Pipeline + def_node_matcher :two_level_node, <<~PATTERN + (send + (const + (const {nil? cbase} $...) + $...) + $...) + PATTERN + + def on_send(node) + one_level_matches = one_level_node(node) + two_level_matches = two_level_node(node) + + return unless Array(one_level_matches).any? || Array(two_level_matches).any? + + if one_level_matches + class_name = one_level_matches[0].first + method_used = one_level_matches[1]&.first + else + class_name = "#{two_level_matches[0].first}::#{two_level_matches[1].first}".to_sym + method_used = two_level_matches[2]&.first + end + + return if non_related?(class_name) || allowed_methods.include?(method_used) + + counters_used = node.ancestors.any? { |ancestor| allowed_method?(ancestor) } + + unless counters_used + add_offense(node, location: :expression, message: format(MSG, count_methods: count_methods.join(', '), class_name: class_name)) + end + end + + private + + def count_methods + cop_config['CountMethods'] || [] + end + + def allowed_methods + cop_config['AllowedMethods'] || [] + end + + def non_related_classes + cop_config['NonRelatedClasses'] || [] + end + + def non_related?(class_name) + non_related_classes.include?(class_name) + end + + def allowed_method?(ancestor) + ancestor.send_type? && !ancestor.dot? && count_methods.include?(ancestor.method_name) + end + end + end + end +end diff --git a/rubocop/routes_under_scope.rb b/rubocop/routes_under_scope.rb new file mode 100644 index 00000000000..3f049bb09fa --- /dev/null +++ b/rubocop/routes_under_scope.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module RuboCop + # Common code used to implement cops checking routes outside of /-/ scope. + # + # Examples: + # * RuboCop::Cop::PutProjectRoutesUnderScope + # * RuboCop::Cop::PutGroupRoutesUnderScope + module RoutesUnderScope + ROUTE_METHODS = Set.new(%i[resource resources get post put patch delete]).freeze + + def on_send(node) + return unless route_method?(node) + return unless outside_scope?(node) + + add_offense(node) + end + + def outside_scope?(node) + node.each_ancestor(:block).none? do |parent| + dash_scope?(parent.to_a.first) + end + end + + def route_method?(node) + ROUTE_METHODS.include?(node.method_name) + end + end +end diff --git a/rubocop/rubocop-migrations.yml b/rubocop/rubocop-migrations.yml index 8699f7b9c68..f8820c0c6aa 100644 --- a/rubocop/rubocop-migrations.yml +++ b/rubocop/rubocop-migrations.yml @@ -28,7 +28,6 @@ Migration/UpdateLargeTable: - :resource_label_events - :routes - :sent_notifications - - :services - :system_note_metadata - :taggings - :todos diff --git a/rubocop/rubocop-usage-data.yml b/rubocop/rubocop-usage-data.yml new file mode 100644 index 00000000000..46e8a067431 --- /dev/null +++ b/rubocop/rubocop-usage-data.yml @@ -0,0 +1,46 @@ +UsageData/LargeTable: + Enabled: true + Include: + - 'lib/gitlab/usage_data.rb' + - 'ee/lib/ee/gitlab/usage_data.rb' + NonRelatedClasses: + - :Date + - :Feature + - :Gitlab + - :Gitlab::AppLogger + - :Gitlab::Auth + - :Gitlab::CurrentSettings + - :Gitlab::Database + - :Gitlab::ErrorTracking + - :Gitlab::Geo + - :Gitlab::Git + - :Gitlab::IncomingEmail + - :Gitlab::Metrics + - :Gitlab::Runtime + - :Gitaly::Server + - :Gitlab::UsageData + - :License + - :Rails + - :Time + - :SECURE_PRODUCT_TYPES + - :Settings + CountMethods: + - :count + - :distinct_count + AllowedMethods: + - :arel_table + - :minimum + - :maximum +UsageData/DistinctCountByLargeForeignKey: + Enabled: true + Include: + - 'lib/gitlab/usage_data.rb' + - 'ee/lib/ee/gitlab/usage_data.rb' + AllowedForeignKeys: + - :user_id + - :author_id + - :creator_id + - :owner_id + - :project_id + - :issue_id + - :merge_request_id |