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.