Skip to content

ActiveSupport::Concern with prepend and class_methods doesn't interact well with Override

This is discovered in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7079#note_98106411

The following code should pass verification but it didn't:

module EE
  module Abstract
    extend ActiveSupport::Concern

    class_methods do
      extend ::Gitlab::Utils::Override

      override :name
      def name
        super.tr('c', 'e')
      end
    end
  end
end

module Abstract
  extend ActiveSupport::Concern
  prepend EE::Abstract

  class_methods do
    def name
      'ce'
    end
  end
end

class Concrete
  include Abstract
end

Concrete.name # => 'ee'

This is very complicated. We do get what we want: (with https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7079 applied)

Concrete.singleton_class.ancestors.take 4
# => [#<Class:Concrete>, EE::Abstract::ClassMethods, Abstract::ClassMethods, #<Class:Object>]

But Override verification won't pass because we're not verifying on Concrete.singleton_class, but on Abstract.singleton_class, which has:

>> Abstract.singleton_class.ancestors.take 2
# => [EE::Abstract::ClassMethods, #<Class:Abstract>]

This can't pass because there's no Abstract::ClassMethods and that's where super was defined. This is actually not following Ruby's regular inheritance chain! In order to fix this, we'll need to run something like:

Abstract.extend Abstract::ClassMethods

Then we'll get:

>> Abstract.singleton_class.ancestors.take 3
# => [EE::Abstract::ClassMethods, Abstract::ClassMethods, #<Class:Abstract>]

Now the verification will pass. However we can't do this because if we do this it will break the other verification! One example is:

module CacheableAttributes
  extend ActiveSupport::Concern

  class_methods do
    def defaults
    end
  end
end

module EE
  module ApplicationSetting
    extend ActiveSupport::Concern

    class_methods do
      extend ::Gitlab::Utils::Override

      override :defaults
      def defaults
      end
    end
  end
end

class ApplicationSetting < ActiveRecord::Base
  include CacheableAttributes
  prepend EE::ApplicationSetting
end

Here we'll see that EE::ApplicationSetting has completely nothing to do with CacheableAttributes, therefore EE::ApplicationSetting::ClassMethods surely won't know anything about CacheableAttributes::ClassMethods.

Where we really want to verify it will be ApplicationSetting.singleton_class, which knows EE::ApplicationSetting::ClassMethods and CacheableAttributes::ClassMethods, and that's also how it's working right now!

This is currently working because ActiveSupport::Concern delayed the time where it extend the ClassMethods. It didn't try to build the connection between each concerns, but on the actual concrete class where ActiveSupport::Concern is not used at all (i.e. ApplicationSetting here)

Unfortunately, we can't do the same with prepend! Otherwise we'll end up with weird ancestor chain mentioned in https://gitlab.com/gitlab-org/gitlab-ce/issues/50824#note_97942857

Concrete.singleton_class.ancestors.take 4
# => [EE::Abstract::ClassMethods, #<Class:Concrete>, Abstract::ClassMethods, #<Class:Object>]

This is surely not what we're looking for.


Given all above, I am not sure if this could be fixed at all. I still want to create this issue to record my findings though. This is unfortunate and I think we'll hit into this again in the future while we're using a similar pattern.