Merge branch...

Merge branch '39499-helper-overriden-method-doesn-t-seem-to-trigger-override-violation' into 'master'

Verify override for helpers and other prepended modules

Closes #39499

See merge request gitlab-org/gitlab!21746

app/helpers/todos_helper.rb

@@ -205,4 +205,4 @@ module TodosHelper
   end
 end
 
-TodosHelper.prepend_if_ee('EE::NotesHelper'); TodosHelper.prepend_if_ee('EE::TodosHelper') # rubocop: disable Style/Semicolon
+TodosHelper.prepend_if_ee('EE::TodosHelper')

config/initializers/zz_metrics.rb

@@ -8,6 +8,8 @@
 #
 # rubocop:disable Metrics/AbcSize
 def instrument_classes(instrumentation)
+  return if ENV['STATIC_VERIFICATION']
+
   instrumentation.instrument_instance_methods(Gitlab::Shell)
 
   instrumentation.instrument_methods(Gitlab::Git)

doc/development/utilities.md

@@ -53,7 +53,7 @@ Refer to <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/over
 
 - This utility can help you check if one method would override
   another or not. It is the same concept as Java's `@Override` annotation
-  or Scala's `override` keyword. However, you should only do this check when
+  or Scala's `override` keyword. However, we only run this check when
   `ENV['STATIC_VERIFICATION']` is set to avoid production runtime overhead.
   This is useful for checking:
 
@@ -94,6 +94,15 @@ Refer to <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/over
     end
     ```
 
+    Note that the check will only happen when either:
+
+    - The overriding method is defined in a class, or:
+    - The overriding method is defined in a module, and it's prepended to
+      a class or a module.
+
+    Because only a class or prepended module can actually override a method.
+    Including or extending a module into another cannot override anything.
+
 ## `StrongMemoize`
 
 Refer to <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/strong_memoize.rb>:

ee/app/helpers/ee/nav_helper.rb

@@ -4,8 +4,8 @@ module EE
   module NavHelper
     extend ::Gitlab::Utils::Override
 
-    override :show_separator?
-    def show_separator?
+    override :has_extra_nav_icons?
+    def has_extra_nav_icons?
       super || can?(current_user, :read_operations_dashboard)
     end
 

ee/lib/ee/gitlab/geo_git_access.rb

@@ -10,21 +10,6 @@ module EE
 
       GEO_SERVER_DOCS_URL = 'https://docs.gitlab.com/ee/administration/geo/replication/using_a_geo_server.html'.freeze
 
-      override :check_custom_action
-      def check_custom_action(cmd)
-        custom_action = custom_action_for(cmd)
-        return custom_action if custom_action
-
-        super
-      end
-
-      override :check_for_console_messages
-      def check_for_console_messages(cmd)
-        super.push(
-          *current_replication_lag_message
-        )
-      end
-
       protected
 
       def project_or_wiki

ee/lib/ee/gitlab/git_access.rb

@@ -34,6 +34,21 @@ module EE
 
       private
 
+      override :check_custom_action
+      def check_custom_action(cmd)
+        custom_action = custom_action_for(cmd)
+        return custom_action if custom_action
+
+        super
+      end
+
+      override :check_for_console_messages
+      def check_for_console_messages(cmd)
+        super.push(
+          *current_replication_lag_message
+        )
+      end
+
       override :check_download_access!
       def check_download_access!
         return if geo?

lib/gitlab/utils/override.rb

@@ -146,7 +146,8 @@ module Gitlab
       def prepended(base = nil)
         super
 
-        queue_verification(base) if base
+        # prepend can override methods, thus we need to verify it like classes
+        queue_verification(base, verify: true) if base
       end
 
       def extended(mod = nil)
@@ -155,11 +156,15 @@ module Gitlab
         queue_verification(mod.singleton_class) if mod
       end
 
-      def queue_verification(base)
+      def queue_verification(base, verify: false)
         return unless ENV['STATIC_VERIFICATION']
 
-        if base.is_a?(Class) # We could check for Class in `override`
-          # This could be `nil` if `override` was never called
+        # We could check for Class in `override`
+        # This could be `nil` if `override` was never called.
+        # We also force verification for prepend because it can also override
+        # a method like a class, but not the cases for include or extend.
+        # This includes Rails helpers but not limited to.
+        if base.is_a?(Class) || verify
           Override.extensions[self]&.add_class(base)
         end
       end