Merge branch '1819-override-ce' into 'master'

CE: Override module to specify that we're overriding See merge request gitlab-org/gitlab-ce!16131

Merge branch '1819-override-ce' into 'master'
CE: Override module to specify that we're overriding See merge request gitlab-org/gitlab-ce!16131
9e70ff34 · Rémy Coutable · c75a8af1 · fd54d7a2 · 9e70ff34 · 9e70ff34
--- a/app/models/concerns/sha_attribute.rb
+++ b/app/models/concerns/sha_attribute.rb
@@ -3,6 +3,7 @@ module ShaAttribute

  module ClassMethods
    def sha_attribute(name)
+      return if ENV['STATIC_VERIFICATION']
      return unless table_exists?

      column = columns.find { |c| c.name == name.to_s }

--- a/doc/development/ee_features.md
+++ b/doc/development/ee_features.md
@@ -87,9 +87,9 @@ still having access the class's implementation with `super`.

 There are a few gotchas with it:

- you should always add a `raise NotImplementedError unless defined?(super)`
-  guard clause in the "overrider" method to ensure that if the method gets
-  renamed in CE, the EE override won't be silently forgotten.
+- you should always [`extend ::Gitlab::Utils::Override`] and use `override` to
+  guard the "overrider" method to ensure that if the method gets renamed in
+  CE, the EE override won't be silently forgotten.
 - when the "overrider" would add a line in the middle of the CE
  implementation, you should refactor the CE method and split it in
  smaller methods. Or create a "hook" method that is empty in CE,
@@ -134,6 +134,9 @@ There are a few gotchas with it:
  guards:
  ``` ruby
    module EE::Base
+      extend ::Gitlab::Utils::Override
+
+      override :do_something
      def do_something
        # Follow the above pattern to call super and extend it
      end
@@ -174,10 +177,11 @@ implementation:

 ```ruby
 module EE
-  class ApplicationController
-    def after_sign_out_path_for(resource)
-      raise NotImplementedError unless defined?(super)
+  module ApplicationController
+    extend ::Gitlab::Utils::Override

+    override :after_sign_out_path_for
+    def after_sign_out_path_for(resource)
      if Gitlab::Geo.secondary?
        Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state)
      else
@@ -188,6 +192,8 @@ module EE
 end
 ```

+[`extend ::Gitlab::Utils::Override`]: utilities.md#override
+
 #### Use self-descriptive wrapper methods

 When it's not possible/logical to modify the implementation of a
@@ -208,8 +214,8 @@ end
 In EE, the implementation `ee/app/models/ee/users.rb` would be:

 ```ruby
+override :full_private_access?
 def full_private_access?
-  raise NotImplementedError unless defined?(super)
  super || auditor?
 end
 ```

--- a/doc/development/utilities.md
+++ b/doc/development/utilities.md
@@ -45,6 +45,51 @@ We developed a number of utilities to ease development.
    [:hello, "world", :this, :crushes, "an entire", "hash"]
    ```

+## [`Override`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/utils/override.rb)
+
+* This utility could help us check if a particular method would override
+  another method or not. It has the same idea of Java's `@Override` annotation
+  or Scala's `override` keyword. However we only do this check when
+  `ENV['STATIC_VERIFICATION']` is set to avoid production runtime overhead.
+  This is useful to check:
+
+    * If we have typos in overriding methods.
+    * If we renamed the overridden methods, making original overriding methods
+      overrides nothing.
+
+    Here's a simple example:
+
+    ``` ruby
+    class Base
+      def execute
+      end
+    end
+
+    class Derived < Base
+      extend ::Gitlab::Utils::Override
+
+      override :execute # Override check happens here
+      def execute
+      end
+    end
+    ```
+
+    This also works on modules:
+
+    ``` ruby
+    module Extension
+      extend ::Gitlab::Utils::Override
+
+      override :execute # Modules do not check this immediately
+      def execute
+      end
+    end
+
+    class Derived < Base
+      prepend Extension # Override check happens here, not in the module
+    end
+    ```
+
 ## [`StrongMemoize`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/utils/strong_memoize.rb)

 * Memoize the value even if it is `nil` or `false`.

--- a/lib/gitlab/utils/override.rb
+++ b/lib/gitlab/utils/override.rb
+module Gitlab
+  module Utils
+    module Override
+      class Extension
+        def self.verify_class!(klass, method_name)
+          instance_method_defined?(klass, method_name) ||
+            raise(
+              NotImplementedError.new(
+                "#{klass}\##{method_name} doesn't exist!"))
+        end
+
+        def self.instance_method_defined?(klass, name, include_super: true)
+          klass.instance_methods(include_super).include?(name) ||
+            klass.private_instance_methods(include_super).include?(name)
+        end
+
+        attr_reader :subject
+
+        def initialize(subject)
+          @subject = subject
+        end
+
+        def add_method_name(method_name)
+          method_names << method_name
+        end
+
+        def add_class(klass)
+          classes << klass
+        end
+
+        def verify!
+          classes.each do |klass|
+            index = klass.ancestors.index(subject)
+            parents = klass.ancestors.drop(index + 1)
+
+            method_names.each do |method_name|
+              parents.any? do |parent|
+                self.class.instance_method_defined?(
+                  parent, method_name, include_super: false)
+              end ||
+                raise(
+                  NotImplementedError.new(
+                    "#{klass}\##{method_name} doesn't exist!"))
+            end
+          end
+        end
+
+        private
+
+        def method_names
+          @method_names ||= []
+        end
+
+        def classes
+          @classes ||= []
+        end
+      end
+
+      # Instead of writing patterns like this:
+      #
+      #     def f
+      #       raise NotImplementedError unless defined?(super)
+      #
+      #       true
+      #     end
+      #
+      # We could write it like:
+      #
+      #     extend ::Gitlab::Utils::Override
+      #
+      #     override :f
+      #     def f
+      #       true
+      #     end
+      #
+      # This would make sure we're overriding something. See:
+      # https://gitlab.com/gitlab-org/gitlab-ee/issues/1819
+      def override(method_name)
+        return unless ENV['STATIC_VERIFICATION']
+
+        if is_a?(Class)
+          Extension.verify_class!(self, method_name)
+        else # We delay the check for modules
+          Override.extensions[self] ||= Extension.new(self)
+          Override.extensions[self].add_method_name(method_name)
+        end
+      end
+
+      def included(base = nil)
+        return super if base.nil? # Rails concern, ignoring it
+
+        super
+
+        if base.is_a?(Class) # We could check for Class in `override`
+          # This could be `nil` if `override` was never called
+          Override.extensions[self]&.add_class(base)
+        end
+      end
+
+      alias_method :prepended, :included
+
+      def self.extensions
+        @extensions ||= {}
+      end
+
+      def self.verify!
+        extensions.values.each(&:verify!)
+      end
+    end
+  end
+end
--- a/lib/tasks/dev.rake
+++ b/lib/tasks/dev.rake
@@ -7,4 +7,9 @@ namespace :dev do
    Rake::Task["gitlab:setup"].invoke
    Rake::Task["gitlab:shell:setup"].invoke
  end
+
+  desc "GitLab | Eager load application"
+  task load: :environment do
+    Rails.application.eager_load!
+  end
 end
--- a/lib/tasks/lint.rake
+++ b/lib/tasks/lint.rake
 unless Rails.env.production?
  namespace :lint do
+    task :static_verification_env do
+      ENV['STATIC_VERIFICATION'] = 'true'
+    end
+
+    desc "GitLab | lint | Static verification"
+    task static_verification: %w[
+      lint:static_verification_env
+      dev:load
+    ] do
+      Gitlab::Utils::Override.verify!
+    end
+
    desc "GitLab | lint | Lint JavaScript files using ESLint"
    task :javascript do
      Rake::Task['eslint'].invoke

--- a/scripts/static-analysis
+++ b/scripts/static-analysis
@@ -10,9 +10,10 @@ tasks = [
  %w[bundle exec license_finder],
  %w[yarn run eslint],
  %w[bundle exec rubocop --parallel],
-  %w[scripts/lint-conflicts.sh],
  %w[bundle exec rake gettext:lint],
-  %w[scripts/lint-changelog-yaml]
+  %w[bundle exec rake lint:static_verification],
+  %w[scripts/lint-changelog-yaml],
+  %w[scripts/lint-conflicts.sh]
 ]

 failed_tasks = tasks.reduce({}) do |failures, task|

--- a/spec/lib/gitlab/utils/override_spec.rb
+++ b/spec/lib/gitlab/utils/override_spec.rb
+require 'spec_helper'
+
+describe Gitlab::Utils::Override do
+  let(:base) { Struct.new(:good) }
+
+  let(:derived) { Class.new(base).tap { |m| m.extend described_class } }
+  let(:extension) { Module.new.tap { |m| m.extend described_class } }
+
+  let(:prepending_class) { base.tap { |m| m.prepend extension } }
+  let(:including_class) { base.tap { |m| m.include extension } }
+
+  let(:klass) { subject }
+
+  def good(mod)
+    mod.module_eval do
+      override :good
+      def good
+        super.succ
+      end
+    end
+
+    mod
+  end
+
+  def bad(mod)
+    mod.module_eval do
+      override :bad
+      def bad
+        true
+      end
+    end
+
+    mod
+  end
+
+  shared_examples 'checking as intended' do
+    it 'checks ok for overriding method' do
+      good(subject)
+      result = klass.new(0).good
+
+      expect(result).to eq(1)
+      described_class.verify!
+    end
+
+    it 'raises NotImplementedError when it is not overriding anything' do
+      expect do
+        bad(subject)
+        klass.new(0).bad
+        described_class.verify!
+      end.to raise_error(NotImplementedError)
+    end
+  end
+
+  shared_examples 'nothing happened' do
+    it 'does not complain when it is overriding something' do
+      good(subject)
+      result = klass.new(0).good
+
+      expect(result).to eq(1)
+      described_class.verify!
+    end
+
+    it 'does not complain when it is not overriding anything' do
+      bad(subject)
+      result = klass.new(0).bad
+
+      expect(result).to eq(true)
+      described_class.verify!
+    end
+  end
+
+  before do
+    # Make sure we're not touching the internal cache
+    allow(described_class).to receive(:extensions).and_return({})
+  end
+
+  describe '#override' do
+    context 'when STATIC_VERIFICATION is set' do
+      before do
+        stub_env('STATIC_VERIFICATION', 'true')
+      end
+
+      context 'when subject is a class' do
+        subject { derived }
+
+        it_behaves_like 'checking as intended'
+      end
+
+      context 'when subject is a module, and class is prepending it' do
+        subject { extension }
+        let(:klass) { prepending_class }
+
+        it_behaves_like 'checking as intended'
+      end
+
+      context 'when subject is a module, and class is including it' do
+        subject { extension }
+        let(:klass) { including_class }
+
+        it 'raises NotImplementedError because it is not overriding it' do
+          expect do
+            good(subject)
+            klass.new(0).good
+            described_class.verify!
+          end.to raise_error(NotImplementedError)
+        end
+
+        it 'raises NotImplementedError when it is not overriding anything' do
+          expect do
+            bad(subject)
+            klass.new(0).bad
+            described_class.verify!
+          end.to raise_error(NotImplementedError)
+        end
+      end
+    end
+  end
+
+  context 'when STATIC_VERIFICATION is not set' do
+    before do
+      stub_env('STATIC_VERIFICATION', nil)
+    end
+
+    context 'when subject is a class' do
+      subject { derived }
+
+      it_behaves_like 'nothing happened'
+    end
+
+    context 'when subject is a module, and class is prepending it' do
+      subject { extension }
+      let(:klass) { prepending_class }
+
+      it_behaves_like 'nothing happened'
+    end
+
+    context 'when subject is a module, and class is including it' do
+      subject { extension }
+      let(:klass) { including_class }
+
+      it 'does not complain when it is overriding something' do
+        good(subject)
+        result = klass.new(0).good
+
+        expect(result).to eq(0)
+        described_class.verify!
+      end
+
+      it 'does not complain when it is not overriding anything' do
+        bad(subject)
+        result = klass.new(0).bad
+
+        expect(result).to eq(true)
+        described_class.verify!
+      end
+    end
+  end
+end