Merge branch...

Merge branch '329103-add-rubocop-rule-to-make-sure-we-inherit-only-allowed-metric-classes' into 'master'

Add Rubocop rule to make sure we inherit only allowed metric classes

See merge request gitlab-org/gitlab!61729

rubocop/cop/usage_data/instrumentation_superclass.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module UsageData
+      # This cop checks that metric instrumentation classes subclass one of the allowed base classes.
+      #
+      # @example
+      #
+      #  # good
+      #  class CountIssues < DatabaseMetric
+      #    # ...
+      #  end
+      #
+      #  # bad
+      #  class CountIssues < BaseMetric
+      #    # ...
+      #  end
+      class InstrumentationSuperclass < RuboCop::Cop::Cop
+        MSG = "Instrumentation classes should subclass one of the following: %{allowed_classes}."
+
+        BASE_PATTERN = "(const nil? !#allowed_class?)"
+
+        def_node_matcher :class_definition, <<~PATTERN
+          (class (const _ !#allowed_class?) #{BASE_PATTERN} ...)
+        PATTERN
+
+        def_node_matcher :class_new_definition, <<~PATTERN
+          [!^(casgn {nil? cbase} #allowed_class? ...)
+           !^^(casgn {nil? cbase} #allowed_class? (block ...))
+           (send (const {nil? cbase} :Class) :new #{BASE_PATTERN})]
+        PATTERN
+
+        def on_class(node)
+          class_definition(node) do
+            register_offense(node.children[1])
+          end
+        end
+
+        def on_send(node)
+          class_new_definition(node) do
+            register_offense(node.children.last)
+          end
+        end
+
+        private
+
+        def allowed_class?(class_name)
+          allowed_classes.include?(class_name)
+        end
+
+        def allowed_classes
+          cop_config['AllowedClasses'] || []
+        end
+
+        def register_offense(offense_node)
+          message = format(MSG, allowed_classes: allowed_classes.join(', '))
+          add_offense(offense_node, message: message)
+        end
+      end
+    end
+  end
+end

rubocop/rubocop-usage-data.yml

@@ -57,3 +57,11 @@ UsageData/DistinctCountByLargeForeignKey:
     - 'user_id'
     - 'resource_owner_id'
     - 'requirement_id'
+UsageData/InstrumentationSuperclass:
+  Enabled: true
+  Include:
+    - 'lib/gitlab/usage/metrics/instrumentations/**/*.rb'
+  AllowedClasses:
+    - :DatabaseMetric
+    - :GenericMetric
+    - :RedisHLLMetric

spec/rubocop/cop/usage_data/instrumentation_superclass_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+require_relative '../../../../rubocop/cop/usage_data/instrumentation_superclass'
+
+RSpec.describe RuboCop::Cop::UsageData::InstrumentationSuperclass do
+  let(:allowed_classes) { %i[GenericMetric DatabaseMetric RedisHllMetric] }
+  let(:msg) { "Instrumentation classes should subclass one of the following: #{allowed_classes.join(', ')}." }
+
+  let(:config) do
+    RuboCop::Config.new('UsageData/InstrumentationSuperclass' => {
+                          'AllowedClasses' => allowed_classes
+                        })
+  end
+
+  subject(:cop) { described_class.new(config) }
+
+  context 'with class definition' do
+    context 'when inheriting from allowed superclass' do
+      it 'does not register an offense' do
+        expect_no_offenses('class NewMetric < GenericMetric; end')
+      end
+    end
+
+    context 'when inheriting from some other superclass' do
+      it 'registers an offense' do
+        expect_offense(<<~CODE)
+          class NewMetric < BaseMetric; end
+                            ^^^^^^^^^^ #{msg}
+        CODE
+      end
+    end
+
+    context 'when not inheriting' do
+      it 'does not register an offense' do
+        expect_no_offenses('class NewMetric; end')
+      end
+    end
+  end
+
+  context 'with dynamic class definition' do
+    context 'when inheriting from allowed superclass' do
+      it 'does not register an offense' do
+        expect_no_offenses('NewMetric = Class.new(GenericMetric)')
+      end
+    end
+
+    context 'when inheriting from some other superclass' do
+      it 'registers an offense' do
+        expect_offense(<<~CODE)
+          NewMetric = Class.new(BaseMetric)
+                                ^^^^^^^^^^ #{msg}
+        CODE
+      end
+    end
+
+    context 'when not inheriting' do
+      it 'does not register an offense' do
+        expect_no_offenses('NewMetric = Class.new')
+      end
+    end
+  end
+end