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

Properly implement prepending for Concern

Closes gitlab-ce#50824

See merge request gitlab-org/gitlab-ee!7079

app/models/concerns/protected_branch_access.rb

@@ -2,19 +2,18 @@
 
 module ProtectedBranchAccess
   extend ActiveSupport::Concern
+  include ProtectedRefAccess
+  include EE::ProtectedRefAccess # Can't use prepend. It'll override wrongly
 
   included do
-    include ProtectedRefAccess
-    include EE::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

app/models/concerns/protected_tag_access.rb

@@ -2,11 +2,10 @@
 
 module ProtectedTagAccess
   extend ActiveSupport::Concern
+  include ProtectedRefAccess
+  include EE::ProtectedRefAccess # Can't use prepend. It'll override wrongly
 
   included do
-    include ProtectedRefAccess
-    include EE::ProtectedRefAccess
-
     belongs_to :protected_tag
 
     delegate :project, to: :protected_tag

app/models/merge_request.rb

@@ -24,7 +24,7 @@ class MergeRequest < ActiveRecord::Base
                 :ref_fetched,
                 :deleted_at
 
-  include ::EE::MergeRequest
+  prepend ::EE::MergeRequest
   include Elastic::MergeRequestsSearch
 
   belongs_to :target_project, class_name: "Project"

config/initializers/0_as_concern.rb

-# 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

ee/app/controllers/ee/groups/group_members_controller.rb

@@ -12,7 +12,7 @@ module EE
         end
       end
 
-      included do
+      prepended do
         before_action :authorize_update_group_member!, only: [:update, :override]
       end
 

ee/app/mailers/ee/notify.rb

@@ -3,14 +3,12 @@ module EE
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
 
-    included do
-      include ::Emails::AdminNotification
-      include ::Emails::CsvExport
-      include ::Emails::ServiceDesk
-      include ::Emails::Epics
+    include ::Emails::AdminNotification
+    include ::Emails::CsvExport
+    include ::Emails::ServiceDesk
+    include ::Emails::Epics
 
-      attr_reader :group
-    end
+    attr_reader :group
 
     private
 

ee/app/models/concerns/ee/protected_branch.rb

@@ -2,7 +2,7 @@ module EE
   module ProtectedBranch
     extend ActiveSupport::Concern
 
-    included do
+    prepended do
       protected_ref_access_levels :unprotect
     end
 

ee/app/models/ee/ci/build.rb

@@ -19,7 +19,7 @@ module EE
       CONTAINER_SCANNING_FILE = 'gl-container-scanning-report.json'.freeze
       DAST_FILE = 'gl-dast-report.json'.freeze
 
-      included do
+      prepended do
         after_save :stick_build_if_status_changed
       end
 

ee/app/models/ee/ci/pipeline.rb

@@ -8,7 +8,7 @@ module EE
         size_limit_exceeded: 21
       }.freeze
 
-      included do
+      prepended do
         has_one :chat_data, class_name: 'Ci::PipelineChatData'
 
         scope :with_security_reports, -> {

ee/app/models/ee/merge_request.rb

@@ -4,7 +4,7 @@ module EE
 
     include ::Approvable
 
-    included do
+    prepended do
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent

ee/app/models/ee/project_import_data.rb

@@ -17,7 +17,7 @@ module EE
 
     extend ActiveSupport::Concern
 
-    included do
+    prepended do
       validates :auth_method, inclusion: { in: %w[password ssh_public_key] }, allow_blank: true
 
       # We should generate a key even if there's no SSH URL present

ee/app/models/ee/repository.rb

@@ -9,7 +9,7 @@ module EE
 
     MIRROR_REMOTE = "upstream".freeze
 
-    included do
+    prepended do
       delegate :checksum, to: :raw_repository
     end
 

ee/app/models/ee/user.rb

@@ -11,7 +11,7 @@ module EE
 
     DEFAULT_ROADMAP_LAYOUT = 'months'.freeze
 
-    included do
+    prepended do
       EMAIL_OPT_IN_SOURCE_ID_GITLAB_COM = 1
 
       # We aren't using the `auditor?` method for the `if` condition here

ee/spec/lib/prependable_spec.rb

-require 'spec_helper'
-
-describe Prependable do
-  subject { FooObject }
-
-  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
-
-  module Foo
-    extend ActiveSupport::Concern
-
-    prepended do
-      def self.class_value
-        20
-      end
-    end
-
-    def value
-      super * 10
-    end
-  end
-
-  class FooObject
-    prepend Foo
-
-    def value
-      10
-    end
-  end
-end

lib/gitlab/patch/prependable.rb

+# 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

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)

spec/lib/gitlab/patch/prependable_spec.rb

+# 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

spec/support/helpers/stub_configuration.rb

 require 'active_support/core_ext/hash/transform_values'
 require 'active_support/hash_with_indifferent_access'
+require 'active_support/dependencies'
 
+require_dependency 'gitlab'
 require_dependency Gitlab.root.join('ee/spec/support/helpers/ee/stub_configuration')
 
 module StubConfiguration