Merge branch '50824-fix-prepend-concern' into 'master'

CE: Properly implement prepending for Concern

See merge request gitlab-org/gitlab-ce!21444

7 files changed, 318 insertions, 32 deletions

diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb
index 744f7f48dc8..58761fce952 100644
--- a/app/models/concerns/protected_branch_access.rb
+++ b/app/models/concerns/protected_branch_access.rb
@@ -2,18 +2,17 @@
 
 module ProtectedBranchAccess
   extend ActiveSupport::Concern
+  include ProtectedRefAccess
 
   included do
-    include ProtectedRefAccess
-
     belongs_to :protected_branch
 
     delegate :project, to: :protected_branch
+  end
 
-    def check_access(user)
-      return false if access_level == Gitlab::Access::NO_ACCESS
+  def check_access(user)
+    return false if access_level == Gitlab::Access::NO_ACCESS
 
-      super
-    end
+    super
   end
 end
diff --git a/app/models/concerns/protected_tag_access.rb b/app/models/concerns/protected_tag_access.rb
index 04bd54d6b1c..3f5696c0749 100644
--- a/app/models/concerns/protected_tag_access.rb
+++ b/app/models/concerns/protected_tag_access.rb
@@ -2,10 +2,9 @@
 
 module ProtectedTagAccess
   extend ActiveSupport::Concern
+  include ProtectedRefAccess
 
   included do
-    include ProtectedRefAccess
-
     belongs_to :protected_tag
 
     delegate :project, to: :protected_tag
diff --git a/config/initializers/0_as_concern.rb b/config/initializers/0_as_concern.rb
index 40232bd6252..ff132547225 100644
--- a/config/initializers/0_as_concern.rb
+++ b/config/initializers/0_as_concern.rb
@@ -1,25 +1,7 @@
-# This module is based on: https://gist.github.com/bcardarella/5735987
-
-module Prependable
-  def prepend_features(base)
-    if base.instance_variable_defined?(:@_dependencies)
-      base.instance_variable_get(:@_dependencies) << self
-      false
-    else
-      return false if base < self
-
-      super
-      base.singleton_class.send(:prepend, const_get('ClassMethods')) if const_defined?(:ClassMethods)
-      @_dependencies.each { |dep| base.send(:prepend, dep) } # rubocop:disable Gitlab/ModuleWithInstanceVariables
-      base.class_eval(&@_included_block) if instance_variable_defined?(:@_included_block) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-    end
-  end
-end
+# frozen_string_literal: true
 
 module ActiveSupport
   module Concern
-    prepend Prependable
-
-    alias_method :prepended, :included
+    prepend Gitlab::Patch::Prependable
   end
 end
diff --git a/lib/gitlab/patch/prependable.rb b/lib/gitlab/patch/prependable.rb
new file mode 100644
index 00000000000..a9f6cfb19cb
--- /dev/null
+++ b/lib/gitlab/patch/prependable.rb
@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+# We're patching `ActiveSupport::Concern` in
+# config/initializers/0_as_concern.rb
+#
+# We want to patch `ActiveSupport::Concern` for two reasons:
+# 1. Allow defining class methods via: `class_methods` method
+# 2. Allow `prepended do; end` work like `included do; end`
+# If we don't need anything above, we don't need this patch nor the concern!
+
+# rubocop:disable Gitlab/ModuleWithInstanceVariables
+module Gitlab
+  module Patch
+    module Prependable
+      class MultiplePrependedBlocks < StandardError
+        def initialize
+          super "Cannot define multiple 'prepended' blocks for a Concern"
+        end
+      end
+
+      def prepend_features(base)
+        return false if prepended?(base)
+
+        super
+
+        if const_defined?(:ClassMethods)
+          klass_methods = const_get(:ClassMethods)
+          base.singleton_class.prepend klass_methods
+          base.instance_variable_set(:@_prepended_class_methods, klass_methods)
+        end
+
+        if instance_variable_defined?(:@_prepended_block)
+          base.class_eval(&@_prepended_block)
+        end
+
+        true
+      end
+
+      def class_methods
+        super
+
+        if instance_variable_defined?(:@_prepended_class_methods)
+          const_get(:ClassMethods).prepend @_prepended_class_methods
+        end
+      end
+
+      def prepended(base = nil, &block)
+        if base.nil?
+          raise MultiplePrependedBlocks if
+            instance_variable_defined?(:@_prepended_block)
+
+          @_prepended_block = block
+        else
+          super
+        end
+      end
+
+      def prepended?(base)
+        index = base.ancestors.index(base)
+
+        base.ancestors[0...index].index(self)
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/utils/override.rb b/lib/gitlab/utils/override.rb
index 7b2a62fed48..d00921e6cdc 100644
--- a/lib/gitlab/utils/override.rb
+++ b/lib/gitlab/utils/override.rb
@@ -89,15 +89,19 @@ module Gitlab
       def included(base = nil)
         super
 
-        queue_verification(base)
+        queue_verification(base) if base
       end
 
-      alias_method :prepended, :included
+      def prepended(base = nil)
+        super
+
+        queue_verification(base) if base
+      end
 
-      def extended(mod)
+      def extended(mod = nil)
         super
 
-        queue_verification(mod.singleton_class)
+        queue_verification(mod.singleton_class) if mod
       end
 
       def queue_verification(base)
diff --git a/spec/lib/gitlab/patch/prependable_spec.rb b/spec/lib/gitlab/patch/prependable_spec.rb
new file mode 100644
index 00000000000..725d733d176
--- /dev/null
+++ b/spec/lib/gitlab/patch/prependable_spec.rb
@@ -0,0 +1,234 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+# Patching ActiveSupport::Concern
+require_relative '../../../../config/initializers/0_as_concern'
+
+describe Gitlab::Patch::Prependable do
+  before do
+    @prepended_modules = []
+  end
+
+  let(:ee) do
+    # So that block in Module.new could see them
+    prepended_modules = @prepended_modules
+
+    Module.new do
+      extend ActiveSupport::Concern
+
+      class_methods do
+        def class_name
+          super.tr('C', 'E')
+        end
+      end
+
+      this = self
+      prepended do
+        prepended_modules << [self, this]
+      end
+
+      def name
+        super.tr('c', 'e')
+      end
+    end
+  end
+
+  let(:ce) do
+    # So that block in Module.new could see them
+    prepended_modules = @prepended_modules
+    ee_ = ee
+
+    Module.new do
+      extend ActiveSupport::Concern
+      prepend ee_
+
+      class_methods do
+        def class_name
+          'CE'
+        end
+      end
+
+      this = self
+      prepended do
+        prepended_modules << [self, this]
+      end
+
+      def name
+        'ce'
+      end
+    end
+  end
+
+  describe 'a class including a concern prepending a concern' do
+    subject { Class.new.include(ce) }
+
+    it 'returns values from prepended module ee' do
+      expect(subject.new.name).to eq('ee')
+      expect(subject.class_name).to eq('EE')
+    end
+
+    it 'has the expected ancestors' do
+      expect(subject.ancestors.take(3)).to eq([subject, ee, ce])
+      expect(subject.singleton_class.ancestors.take(3))
+        .to eq([subject.singleton_class,
+                ee.const_get(:ClassMethods),
+                ce.const_get(:ClassMethods)])
+    end
+
+    it 'prepends only once even if called twice' do
+      2.times { ce.prepend(ee) }
+
+      subject
+
+      expect(@prepended_modules).to eq([[ce, ee]])
+    end
+
+    context 'overriding methods' do
+      before do
+        subject.module_eval do
+          def self.class_name
+            'Custom'
+          end
+
+          def name
+            'custom'
+          end
+        end
+      end
+
+      it 'returns values from the class' do
+        expect(subject.new.name).to eq('custom')
+        expect(subject.class_name).to eq('Custom')
+      end
+    end
+  end
+
+  describe 'a class prepending a concern prepending a concern' do
+    subject { Class.new.prepend(ce) }
+
+    it 'returns values from prepended module ee' do
+      expect(subject.new.name).to eq('ee')
+      expect(subject.class_name).to eq('EE')
+    end
+
+    it 'has the expected ancestors' do
+      expect(subject.ancestors.take(3)).to eq([ee, ce, subject])
+      expect(subject.singleton_class.ancestors.take(3))
+        .to eq([ee.const_get(:ClassMethods),
+                ce.const_get(:ClassMethods),
+                subject.singleton_class])
+    end
+
+    it 'prepends only once' do
+      subject.prepend(ce)
+
+      expect(@prepended_modules).to eq([[ce, ee], [subject, ce]])
+    end
+  end
+
+  describe 'a class prepending a concern' do
+    subject do
+      ee_ = ee
+
+      Class.new do
+        prepend ee_
+
+        def self.class_name
+          'CE'
+        end
+
+        def name
+          'ce'
+        end
+      end
+    end
+
+    it 'returns values from prepended module ee' do
+      expect(subject.new.name).to eq('ee')
+      expect(subject.class_name).to eq('EE')
+    end
+
+    it 'has the expected ancestors' do
+      expect(subject.ancestors.take(2)).to eq([ee, subject])
+      expect(subject.singleton_class.ancestors.take(2))
+        .to eq([ee.const_get(:ClassMethods),
+                subject.singleton_class])
+    end
+
+    it 'prepends only once' do
+      subject.prepend(ee)
+
+      expect(@prepended_modules).to eq([[subject, ee]])
+    end
+  end
+
+  describe 'simple case' do
+    subject do
+      foo_ = foo
+
+      Class.new do
+        prepend foo_
+
+        def value
+          10
+        end
+      end
+    end
+
+    let(:foo) do
+      Module.new do
+        extend ActiveSupport::Concern
+
+        prepended do
+          def self.class_value
+            20
+          end
+        end
+
+        def value
+          super * 10
+        end
+      end
+    end
+
+    context 'class methods' do
+      it "has a method" do
+        expect(subject).to respond_to(:class_value)
+      end
+
+      it 'can execute a method' do
+        expect(subject.class_value).to eq(20)
+      end
+    end
+
+    context 'instance methods' do
+      it "has a method" do
+        expect(subject.new).to respond_to(:value)
+      end
+
+      it 'chains a method execution' do
+        expect(subject.new.value).to eq(100)
+      end
+    end
+  end
+
+  context 'having two prepended blocks' do
+    subject do
+      Module.new do
+        extend ActiveSupport::Concern
+
+        prepended do
+        end
+
+        prepended do
+        end
+      end
+    end
+
+    it "raises an error" do
+      expect { subject }
+        .to raise_error(described_class::MultiplePrependedBlocks)
+    end
+  end
+end
diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb
index 8475f91799b..776119564ec 100644
--- a/spec/support/helpers/stub_configuration.rb
+++ b/spec/support/helpers/stub_configuration.rb
@@ -1,5 +1,8 @@
 require 'active_support/core_ext/hash/transform_values'
 require 'active_support/hash_with_indifferent_access'
+require 'active_support/dependencies'
+
+require_dependency 'gitlab'
 
 module StubConfiguration
   def stub_application_setting(messages)