From f71fc9328c42bb67c0ad209dd6503de57fa2bcf8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 4 Jun 2018 22:54:50 +0800 Subject: [PATCH] Also verify if extending would override a class method Since extending a class means including on the singleton class of the class, this should now complain this: ``` ruby module M extend Gitlab::Utils::Override override :f def f super.succ end end class C extend M def self.f 0 end end ``` It should complain because `C.f` wasn't calling `M#f`. This should pass verification: ``` ruby module M extend Gitlab::Utils::Override override :f def f super.succ end end class B def self.f 0 end end class C < B extend M end ``` Because `C.f` would now call `M#f`, and `M#f` does override something. --- lib/gitlab/utils/override.rb | 16 ++- spec/lib/gitlab/utils/override_spec.rb | 170 ++++++++++++++++--------- 2 files changed, 121 insertions(+), 65 deletions(-) diff --git a/lib/gitlab/utils/override.rb b/lib/gitlab/utils/override.rb index 8bf6bcb1fe2..7b2a62fed48 100644 --- a/lib/gitlab/utils/override.rb +++ b/lib/gitlab/utils/override.rb @@ -87,18 +87,28 @@ module Gitlab end def included(base = nil) - return super if base.nil? # Rails concern, ignoring it + super + + queue_verification(base) + end + alias_method :prepended, :included + + def extended(mod) super + queue_verification(mod.singleton_class) + end + + def queue_verification(base) + return unless ENV['STATIC_VERIFICATION'] + if base.is_a?(Class) # We could check for Class in `override` # This could be `nil` if `override` was never called Override.extensions[self]&.add_class(base) end end - alias_method :prepended, :included - def self.extensions @extensions ||= {} end diff --git a/spec/lib/gitlab/utils/override_spec.rb b/spec/lib/gitlab/utils/override_spec.rb index 7c97cee982a..fc08ebcfc6d 100644 --- a/spec/lib/gitlab/utils/override_spec.rb +++ b/spec/lib/gitlab/utils/override_spec.rb @@ -1,7 +1,13 @@ -require 'spec_helper' +require 'fast_spec_helper' describe Gitlab::Utils::Override do - let(:base) { Struct.new(:good) } + let(:base) do + Struct.new(:good) do + def self.good + 0 + end + end + end let(:derived) { Class.new(base).tap { |m| m.extend described_class } } let(:extension) { Module.new.tap { |m| m.extend described_class } } @@ -9,6 +15,14 @@ describe Gitlab::Utils::Override do let(:prepending_class) { base.tap { |m| m.prepend extension } } let(:including_class) { base.tap { |m| m.include extension } } + let(:prepending_class_methods) do + base.tap { |m| m.singleton_class.prepend extension } + end + + let(:extending_class_methods) do + base.tap { |m| m.extend extension } + end + let(:klass) { subject } def good(mod) @@ -36,7 +50,7 @@ describe Gitlab::Utils::Override do shared_examples 'checking as intended' do it 'checks ok for overriding method' do good(subject) - result = klass.new(0).good + result = instance.good expect(result).to eq(1) described_class.verify! @@ -45,7 +59,25 @@ describe Gitlab::Utils::Override do it 'raises NotImplementedError when it is not overriding anything' do expect do bad(subject) - klass.new(0).bad + instance.bad + described_class.verify! + end.to raise_error(NotImplementedError) + end + end + + shared_examples 'checking as intended, nothing was overridden' do + it 'raises NotImplementedError because it is not overriding it' do + expect do + good(subject) + instance.good + described_class.verify! + end.to raise_error(NotImplementedError) + end + + it 'raises NotImplementedError when it is not overriding anything' do + expect do + bad(subject) + instance.bad described_class.verify! end.to raise_error(NotImplementedError) end @@ -54,7 +86,7 @@ describe Gitlab::Utils::Override do shared_examples 'nothing happened' do it 'does not complain when it is overriding something' do good(subject) - result = klass.new(0).good + result = instance.good expect(result).to eq(1) described_class.verify! @@ -62,7 +94,7 @@ describe Gitlab::Utils::Override do it 'does not complain when it is not overriding anything' do bad(subject) - result = klass.new(0).bad + result = instance.bad expect(result).to eq(true) described_class.verify! @@ -75,83 +107,97 @@ describe Gitlab::Utils::Override do end describe '#override' do - context 'when STATIC_VERIFICATION is set' do - before do - stub_env('STATIC_VERIFICATION', 'true') - end + context 'when instance is klass.new(0)' do + let(:instance) { klass.new(0) } - context 'when subject is a class' do - subject { derived } + context 'when STATIC_VERIFICATION is set' do + before do + stub_env('STATIC_VERIFICATION', 'true') + end - it_behaves_like 'checking as intended' - end + context 'when subject is a class' do + subject { derived } + + it_behaves_like 'checking as intended' + end + + context 'when subject is a module, and class is prepending it' do + subject { extension } + let(:klass) { prepending_class } + + it_behaves_like 'checking as intended' + end - context 'when subject is a module, and class is prepending it' do - subject { extension } - let(:klass) { prepending_class } + context 'when subject is a module, and class is including it' do + subject { extension } + let(:klass) { including_class } - it_behaves_like 'checking as intended' + it_behaves_like 'checking as intended, nothing was overridden' + end end - context 'when subject is a module, and class is including it' do - subject { extension } - let(:klass) { including_class } + context 'when STATIC_VERIFICATION is not set' do + before do + stub_env('STATIC_VERIFICATION', nil) + end - it 'raises NotImplementedError because it is not overriding it' do - expect do - good(subject) - klass.new(0).good - described_class.verify! - end.to raise_error(NotImplementedError) + context 'when subject is a class' do + subject { derived } + + it_behaves_like 'nothing happened' end - it 'raises NotImplementedError when it is not overriding anything' do - expect do - bad(subject) - klass.new(0).bad - described_class.verify! - end.to raise_error(NotImplementedError) + context 'when subject is a module, and class is prepending it' do + subject { extension } + let(:klass) { prepending_class } + + it_behaves_like 'nothing happened' end - end - end - end - context 'when STATIC_VERIFICATION is not set' do - before do - stub_env('STATIC_VERIFICATION', nil) - end + context 'when subject is a module, and class is including it' do + subject { extension } + let(:klass) { including_class } - context 'when subject is a class' do - subject { derived } + it 'does not complain when it is overriding something' do + good(subject) + result = instance.good - it_behaves_like 'nothing happened' - end + expect(result).to eq(0) + described_class.verify! + end - context 'when subject is a module, and class is prepending it' do - subject { extension } - let(:klass) { prepending_class } + it 'does not complain when it is not overriding anything' do + bad(subject) + result = instance.bad - it_behaves_like 'nothing happened' + expect(result).to eq(true) + described_class.verify! + end + end + end end - context 'when subject is a module, and class is including it' do - subject { extension } - let(:klass) { including_class } + context 'when instance is klass' do + let(:instance) { klass } - it 'does not complain when it is overriding something' do - good(subject) - result = klass.new(0).good + context 'when STATIC_VERIFICATION is set' do + before do + stub_env('STATIC_VERIFICATION', 'true') + end - expect(result).to eq(0) - described_class.verify! - end + context 'when subject is a module, and class is prepending it' do + subject { extension } + let(:klass) { prepending_class_methods } - it 'does not complain when it is not overriding anything' do - bad(subject) - result = klass.new(0).bad + it_behaves_like 'checking as intended' + end - expect(result).to eq(true) - described_class.verify! + context 'when subject is a module, and class is extending it' do + subject { extension } + let(:klass) { extending_class_methods } + + it_behaves_like 'checking as intended, nothing was overridden' + end end end end -- GitLab