Commit c6590e57 authored by Lin Jen-Shin's avatar Lin Jen-Shin

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:

``` ruby
module Base
  extend ActiveSupport::Concern

  class_methods do
    def f
    end
  end
end

module Derived
  include Base
end
```

What would you expect for this?

``` ruby
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: https://gitlab.com/gitlab-org/gitlab/-/issues/23932
parent ed29856f
......@@ -109,6 +109,48 @@ Refer to [`override.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gi
Because only a class or prepended module can actually override a method.
Including or extending a module into another cannot override anything.
### Interactions with `ActiveSupport::Concern`, `prepend`, and `class_methods`
When you use `ActiveSupport::Concern` that includes class methods, you do not
get expected results because `ActiveSupport::Concern` doesn't work like a
regular Ruby module.
Since we already have `Prependable` as a patch for `ActiveSupport::Concern`
to enable `prepend`, it has consequences with how it would interact with
`override` and `class_methods`. We add a workaround directly into
`Prependable` to resolve the problem, by `extend`ing `ClassMethods` into the
defining module.
This allows us to use `override` to verify `class_methods` used in the
context mentioned above. This workaround only applies when we run the
verification, not when running the application itself.
Here are example code blocks that demonstrate the effect of this workaround:
following codes:
```ruby
module Base
extend ActiveSupport::Concern
class_methods do
def f
end
end
end
module Derived
include Base
end
# Without the workaround
Base.f # => NoMethodError
Derived.f # => nil
# With the workaround
Base.f # => nil
Derived.f # => nil
```
## `StrongMemoize`
Refer to [`strong_memoize.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/strong_memoize.rb):
......
......@@ -6,7 +6,9 @@ module EE
class_methods do
include ::Gitlab::Utils::StrongMemoize
extend ::Gitlab::Utils::Override
override :available_features_for_issue_types
def available_features_for_issue_types
strong_memoize(:available_features_for_issue_types) do
super.merge(epics: %w(issue), sla: %w(incident))
......
......@@ -39,9 +39,14 @@ module Gitlab
def class_methods
super
class_methods_module = const_get(:ClassMethods, false)
if instance_variable_defined?(:@_prepended_class_methods)
const_get(:ClassMethods, false).prepend @_prepended_class_methods
class_methods_module.prepend @_prepended_class_methods
end
# Hack to resolve https://gitlab.com/gitlab-org/gitlab/-/issues/23932
extend class_methods_module if ENV['STATIC_VERIFICATION']
end
def prepended(base = nil, &block)
......
......@@ -153,7 +153,13 @@ module Gitlab
def extended(mod = nil)
super
queue_verification(mod.singleton_class) if mod
# Hack to resolve https://gitlab.com/gitlab-org/gitlab/-/issues/23932
is_not_concern_hack =
(mod.is_a?(Class) || !name&.end_with?('::ClassMethods'))
if mod && is_not_concern_hack
queue_verification(mod.singleton_class)
end
end
def queue_verification(base, verify: false)
......@@ -174,7 +180,7 @@ module Gitlab
end
def self.verify!
extensions.values.each(&:verify!)
extensions.each_value(&:verify!)
end
end
end
......
......@@ -231,4 +231,22 @@ RSpec.describe Gitlab::Patch::Prependable do
.to raise_error(described_class::MultiplePrependedBlocks)
end
end
describe 'the extra hack for override verification' do
context 'when ENV["STATIC_VERIFICATION"] is not defined' do
it 'does not extend ClassMethods onto the defining module' do
expect(ee).not_to respond_to(:class_name)
end
end
context 'when ENV["STATIC_VERIFICATION"] is defined' do
before do
stub_env('STATIC_VERIFICATION', 'true')
end
it 'does extend ClassMethods onto the defining module' do
expect(ee).to respond_to(:class_name)
end
end
end
end
......@@ -2,6 +2,9 @@
require 'fast_spec_helper'
# Patching ActiveSupport::Concern
require_relative '../../../../config/initializers/0_as_concern'
RSpec.describe Gitlab::Utils::Override do
let(:base) do
Struct.new(:good) do
......@@ -164,6 +167,70 @@ RSpec.describe Gitlab::Utils::Override do
it_behaves_like 'checking as intended, nothing was overridden'
end
context 'when ActiveSupport::Concern and class_methods are used' do
# We need to give module names before using Override
let(:base) { stub_const('Base', Module.new) }
let(:extension) { stub_const('Extension', Module.new) }
def define_base(method_name:)
base.module_eval do
extend ActiveSupport::Concern
class_methods do
define_method(method_name) do
:f
end
end
end
end
def define_extension(method_name:)
extension.module_eval do
extend ActiveSupport::Concern
class_methods do
extend Gitlab::Utils::Override
override method_name
define_method(method_name) do
:g
end
end
end
end
context 'when it is defining a overriding method' do
before do
define_base(method_name: :f)
define_extension(method_name: :f)
base.prepend(extension)
end
it 'verifies' do
expect(base.f).to eq(:g)
described_class.verify!
end
end
context 'when it is not defining a overriding method' do
before do
define_base(method_name: :f)
define_extension(method_name: :g)
base.prepend(extension)
end
it 'raises NotImplementedError' do
expect(base.f).to eq(:f)
expect { described_class.verify! }
.to raise_error(NotImplementedError)
end
end
end
end
context 'when STATIC_VERIFICATION is not set' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment