Merge branch...

Merge branch '325516-add-rubocop-rule-for-preventing-using-histogram-method-on-large-tables' into 'master'

Add RuboCop rule for preventing using histogram method on large tables

See merge request gitlab-org/gitlab!61836

rubocop/cop/usage_data/histogram_with_large_table.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module UsageData
+      # This cop checks that histogram method is not used in usage_data.rb files
+      # for models representing large tables, as defined by migration helpers.
+      #
+      # @example
+      #
+      # # bad
+      # histogram(Issue, buckets: 1..100)
+      # histogram(User.active, buckets: 1..100)
+      class HistogramWithLargeTable < RuboCop::Cop::Cop
+        MSG = 'Avoid histogram method on %{model_name}'
+
+        # Match one level const as Issue, Gitlab
+        def_node_matcher :one_level_node, <<~PATTERN
+          (send nil? :histogram
+            `(const {nil? cbase} $_)
+          ...)
+        PATTERN
+
+        # Match two level const as ::Clusters::Cluster, ::Ci::Pipeline
+        def_node_matcher :two_level_node, <<~PATTERN
+          (send nil? :histogram
+            `(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?
+
+          class_name = two_level_matches ? two_level_matches.join('::').to_sym : one_level_matches
+
+          if large_table?(class_name)
+            add_offense(node, location: :expression, message: format(MSG, model_name: class_name))
+          end
+        end
+
+        private
+
+        def large_table?(model)
+          high_traffic_models.include?(model.to_s)
+        end
+
+        def high_traffic_models
+          cop_config['HighTrafficModels'] || []
+        end
+      end
+    end
+  end
+end

rubocop/rubocop-usage-data.yml

@@ -39,6 +39,64 @@ UsageData/LargeTable:
     - :arel_table
     - :minimum
     - :maximum
+UsageData/HistogramWithLargeTable:
+  Enabled: true
+  Include:
+    - 'lib/gitlab/usage_data.rb'
+    - 'ee/lib/ee/gitlab/usage_data.rb'
+  HighTrafficModels: &high_traffic_models # models for all high traffic tables in Migration/UpdateLargeTable
+    - 'AuditEvent'
+    - 'Ci::BuildTraceSection'
+    - 'CommitStatus'
+    - 'Ci::Processable'
+    - 'Ci::Bridge'
+    - 'Ci::Build'
+    - 'GenericCommitStatus'
+    - 'Ci::BuildMetadata'
+    - 'Ci::JobArtifact'
+    - 'Ci::PipelineVariable'
+    - 'Ci::Pipeline'
+    - 'Ci::Stage'
+    - 'Deployment'
+    - 'Event'
+    - 'PushEvent'
+    - 'Issue'
+    - 'MergeRequestDiffCommit'
+    - 'MergeRequestDiffFile'
+    - 'MergeRequestDiff'
+    - 'MergeRequest::Metrics'
+    - 'MergeRequest'
+    - 'NamespaceSetting'
+    - 'Namespace'
+    - 'Group'
+    - 'NoteDiffFile'
+    - 'Note'
+    - 'DiffNote'
+    - 'DiscussionNote'
+    - 'SyntheticNote'
+    - 'LabelNote'
+    - 'MilestoneNote'
+    - 'StateNote'
+    - 'LegacyDiffNote'
+    - 'ProjectAuthorization'
+    - 'Project'
+    - 'ProjectCiCdSetting'
+    - 'ProjectSetting'
+    - 'ProjectFeature'
+    - 'ProtectedBranch'
+    - 'ExportedProtectedBranch'
+    - 'PushEventPayload'
+    - 'ResourceLabelEvent'
+    - 'Route'
+    - 'SentNotification'
+    - 'SystemNoteMetadata'
+    - 'ActsAsTaggableOn::Tagging'
+    - 'Todo'
+    - 'User'
+    - 'UserPreference'
+    - 'UserDetail'
+    - 'Vulnerabilities::Finding'
+    - 'WebHookLog'
 UsageData/DistinctCountByLargeForeignKey:
   Enabled: true
   Include:

spec/rubocop/cop/usage_data/histogram_with_large_table_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+require_relative '../../../../rubocop/cop/usage_data/histogram_with_large_table'
+
+RSpec.describe RuboCop::Cop::UsageData::HistogramWithLargeTable do
+  let(:high_traffic_models) { %w[Issue Ci::Build] }
+  let(:msg) { 'Avoid histogram method on' }
+
+  let(:config) do
+    RuboCop::Config.new('UsageData/HistogramWithLargeTable' => {
+                          'HighTrafficModels' => high_traffic_models
+                        })
+  end
+
+  subject(:cop) { described_class.new(config) }
+
+  context 'with large tables' do
+    context 'with one-level constants' do
+      context 'when calling histogram(Issue)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(Issue, :project_id, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Issue
+          CODE
+        end
+      end
+
+      context 'when calling histogram(::Issue)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(::Issue, :project_id, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Issue
+          CODE
+        end
+      end
+
+      context 'when calling histogram(Issue.closed)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(Issue.closed, :project_id, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Issue
+          CODE
+        end
+      end
+
+      context 'when calling histogram(::Issue.closed)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(::Issue.closed, :project_id, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Issue
+          CODE
+        end
+      end
+    end
+
+    context 'with two-level constants' do
+      context 'when calling histogram(::Ci::Build)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(::Ci::Build, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Ci::Build
+          CODE
+        end
+      end
+
+      context 'when calling histogram(::Ci::Build.active)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(::Ci::Build.active, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Ci::Build
+          CODE
+        end
+      end
+
+      context 'when calling histogram(Ci::Build)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(Ci::Build, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Ci::Build
+          CODE
+        end
+      end
+
+      context 'when calling histogram(Ci::Build.active)' do
+        it 'registers an offense' do
+          expect_offense(<<~CODE)
+            histogram(Ci::Build.active, buckets: 1..100)
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} Ci::Build
+          CODE
+        end
+      end
+    end
+  end
+
+  context 'with non related class' do
+    it 'does not register an offense' do
+      expect_no_offenses('histogram(MergeRequest, buckets: 1..100)')
+    end
+  end
+
+  context 'with non related method' do
+    it 'does not register an offense' do
+      expect_no_offenses('count(Issue, buckets: 1..100)')
+    end
+  end
+end