summaryrefslogtreecommitdiff
path: root/rubocop
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2020-08-20 18:42:06 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2020-08-20 18:42:06 +0000
commit6e4e1050d9dba2b7b2523fdd1768823ab85feef4 (patch)
tree78be5963ec075d80116a932011d695dd33910b4e /rubocop
parent1ce776de4ae122aba3f349c02c17cebeaa8ecf07 (diff)
downloadgitlab-ce-6e4e1050d9dba2b7b2523fdd1768823ab85feef4.tar.gz
Add latest changes from gitlab-org/gitlab@13-3-stable-ee
Diffstat (limited to 'rubocop')
-rw-r--r--rubocop/cop/avoid_becomes.rb30
-rw-r--r--rubocop/cop/graphql/json_type.rb40
-rw-r--r--rubocop/cop/migration/with_lock_retries_disallowed_method.rb1
-rw-r--r--rubocop/cop/put_group_routes_under_scope.rb32
-rw-r--r--rubocop/cop/put_project_routes_under_scope.rb32
-rw-r--r--rubocop/cop/usage_data/distinct_count_by_large_foreign_key.rb43
-rw-r--r--rubocop/cop/usage_data/large_table.rb87
-rw-r--r--rubocop/routes_under_scope.rb29
-rw-r--r--rubocop/rubocop-migrations.yml1
-rw-r--r--rubocop/rubocop-usage-data.yml46
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