Skip to content

Hack `Prependable` class methods for `override`

Lin Jen-Shin requested to merge 23932-hack-override-class-concern into master

What does this MR do?

Hack Prependable class methods for override

ActiveSupport::Concern uses an unusual way to chain class methods, which does not exactly follow how Ruby chains the classes.

In particular, it's like Concern reinvents the object model and builds the ancestors chain in runtime, rather than at the compile time. This for sure introduced a lot of culprits.

This fix is aiming for restoring the class methods for the module which is defining the class methods. To be specific, consider this case:

module Base
  extend ActiveSupport::Concern

  class_methods do
    def f
    end
  end
end

module Derived
  include Base
end

What would you expect for this?

Base.f    # => NoMethodError
Derived.f # => nil

With this hack, it'll allow Base.f to work, which can make override check for class methods. Before this hack it'll not work due to this disparity.

Since so far the only place we really need this is when we're checking override with those class methods, we don't have to take the risk to change how it works in production, but just how we check override.

This hack is needed for override because we're checking prepend at where it's defined, not at where it's eventually included into a class, and that's where Concern does the magic.

If one day we can stop using ActiveSupport::Concern, and just do plain old good Ruby, we'll be free from all those wild hacks.

See original bug report: #23932 (closed)

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Related to #23932 (closed)

Merge request reports