Merge branch '23932-hack-override-class-concern' into 'master'

Hack `Prependable` class methods for `override`

See merge request gitlab-org/gitlab!52419

doc/development/utilities.md

@@ -109,6 +109,48 @@ Refer to [`override.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gi
     Because only a class or prepended module can actually override a method.
     Including or extending a module into another cannot override anything.
 
+### Interactions with `ActiveSupport::Concern`, `prepend`, and `class_methods`
+
+When you use `ActiveSupport::Concern` that includes class methods, you do not
+get expected results because `ActiveSupport::Concern` doesn't work like a
+regular Ruby module.
+
+Since we already have `Prependable` as a patch for `ActiveSupport::Concern`
+to enable `prepend`, it has consequences with how it would interact with
+`override` and `class_methods`. We add a workaround directly into
+`Prependable` to resolve the problem, by `extend`ing `ClassMethods` into the
+defining module.
+
+This allows us to use `override` to verify `class_methods` used in the
+context mentioned above. This workaround only applies when we run the
+verification, not when running the application itself.
+
+Here are example code blocks that demonstrate the effect of this workaround:
+following codes:
+
+```ruby
+module Base
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def f
+    end
+  end
+end
+
+module Derived
+  include Base
+end
+
+# Without the workaround
+Base.f    # => NoMethodError
+Derived.f # => nil
+
+# With the workaround
+Base.f    # => nil
+Derived.f # => nil
+```
+
 ## `StrongMemoize`
 
 Refer to [`strong_memoize.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/strong_memoize.rb):

ee/app/models/concerns/ee/issue_available_features.rb

@@ -6,7 +6,9 @@ module EE
 
     class_methods do
       include ::Gitlab::Utils::StrongMemoize
+      extend ::Gitlab::Utils::Override
 
+      override :available_features_for_issue_types
       def available_features_for_issue_types
         strong_memoize(:available_features_for_issue_types) do
           super.merge(epics: %w(issue), sla: %w(incident))

lib/gitlab/patch/prependable.rb

@@ -39,9 +39,14 @@ module Gitlab
       def class_methods
         super
 
+        class_methods_module = const_get(:ClassMethods, false)
+
         if instance_variable_defined?(:@_prepended_class_methods)
-          const_get(:ClassMethods, false).prepend @_prepended_class_methods
+          class_methods_module.prepend @_prepended_class_methods
         end
+
+        # Hack to resolve https://gitlab.com/gitlab-org/gitlab/-/issues/23932
+        extend class_methods_module if ENV['STATIC_VERIFICATION']
       end
 
       def prepended(base = nil, &block)

lib/gitlab/utils/override.rb

@@ -153,7 +153,13 @@ module Gitlab
       def extended(mod = nil)
         super
 
-        queue_verification(mod.singleton_class) if mod
+        # Hack to resolve https://gitlab.com/gitlab-org/gitlab/-/issues/23932
+        is_not_concern_hack =
+          (mod.is_a?(Class) || !name&.end_with?('::ClassMethods'))
+
+        if mod && is_not_concern_hack
+          queue_verification(mod.singleton_class)
+        end
       end
 
       def queue_verification(base, verify: false)
@@ -174,7 +180,7 @@ module Gitlab
       end
 
       def self.verify!
-        extensions.values.each(&:verify!)
+        extensions.each_value(&:verify!)
       end
     end
   end

spec/lib/gitlab/patch/prependable_spec.rb

@@ -231,4 +231,22 @@ RSpec.describe Gitlab::Patch::Prependable do
         .to raise_error(described_class::MultiplePrependedBlocks)
     end
   end
+
+  describe 'the extra hack for override verification' do
+    context 'when ENV["STATIC_VERIFICATION"] is not defined' do
+      it 'does not extend ClassMethods onto the defining module' do
+        expect(ee).not_to respond_to(:class_name)
+      end
+    end
+
+    context 'when ENV["STATIC_VERIFICATION"] is defined' do
+      before do
+        stub_env('STATIC_VERIFICATION', 'true')
+      end
+
+      it 'does extend ClassMethods onto the defining module' do
+        expect(ee).to respond_to(:class_name)
+      end
+    end
+  end
 end

spec/lib/gitlab/utils/override_spec.rb

@@ -2,6 +2,9 @@
 
 require 'fast_spec_helper'
 
+# Patching ActiveSupport::Concern
+require_relative '../../../../config/initializers/0_as_concern'
+
 RSpec.describe Gitlab::Utils::Override do
   let(:base) do
     Struct.new(:good) do
@@ -164,6 +167,70 @@ RSpec.describe Gitlab::Utils::Override do
 
           it_behaves_like 'checking as intended, nothing was overridden'
         end
+
+        context 'when ActiveSupport::Concern and class_methods are used' do
+          # We need to give module names before using Override
+          let(:base) { stub_const('Base', Module.new) }
+          let(:extension) { stub_const('Extension', Module.new) }
+
+          def define_base(method_name:)
+            base.module_eval do
+              extend ActiveSupport::Concern
+
+              class_methods do
+                define_method(method_name) do
+                  :f
+                end
+              end
+            end
+          end
+
+          def define_extension(method_name:)
+            extension.module_eval do
+              extend ActiveSupport::Concern
+
+              class_methods do
+                extend Gitlab::Utils::Override
+
+                override method_name
+                define_method(method_name) do
+                  :g
+                end
+              end
+            end
+          end
+
+          context 'when it is defining a overriding method' do
+            before do
+              define_base(method_name: :f)
+              define_extension(method_name: :f)
+
+              base.prepend(extension)
+            end
+
+            it 'verifies' do
+              expect(base.f).to eq(:g)
+
+              described_class.verify!
+            end
+          end
+
+          context 'when it is not defining a overriding method' do
+            before do
+              define_base(method_name: :f)
+              define_extension(method_name: :g)
+
+              base.prepend(extension)
+            end
+
+            it 'raises NotImplementedError' do
+              expect(base.f).to eq(:f)
+
+              expect { described_class.verify! }
+                .to raise_error(NotImplementedError)
+            end
+          end
+        end
       end
 
       context 'when STATIC_VERIFICATION is not set' do