Commit 3098b1cc authored by Rémy Coutable's avatar Rémy Coutable

Merge branch '1819-override' into 'master'

Override module to specify that we're overriding

Closes #1819

See merge request gitlab-org/gitlab-ee!3876
parents 9ba995fc dafeab1a
......@@ -3,6 +3,7 @@ module ShaAttribute
module ClassMethods
def sha_attribute(name)
return if ENV['STATIC_VERIFICATION']
return unless table_exists?
column = columns.find { |c| c.name == name.to_s }
......
......@@ -87,9 +87,9 @@ still having access the class's implementation with `super`.
There are a few gotchas with it:
- you should always add a `raise NotImplementedError unless defined?(super)`
guard clause in the "overrider" method to ensure that if the method gets
renamed in CE, the EE override won't be silently forgotten.
- you should always [`extend ::Gitlab::Utils::Override`] and use `override` to
guard the "overrider" method to ensure that if the method gets renamed in
CE, the EE override won't be silently forgotten.
- when the "overrider" would add a line in the middle of the CE
implementation, you should refactor the CE method and split it in
smaller methods. Or create a "hook" method that is empty in CE,
......@@ -134,6 +134,9 @@ There are a few gotchas with it:
guards:
``` ruby
module EE::Base
extend ::Gitlab::Utils::Override
override :do_something
def do_something
# Follow the above pattern to call super and extend it
end
......@@ -174,10 +177,11 @@ implementation:
```ruby
module EE
class ApplicationController
def after_sign_out_path_for(resource)
raise NotImplementedError unless defined?(super)
module ApplicationController
extend ::Gitlab::Utils::Override
override :after_sign_out_path_for
def after_sign_out_path_for(resource)
if Gitlab::Geo.secondary?
Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state)
else
......@@ -188,6 +192,8 @@ module EE
end
```
[`extend ::Gitlab::Utils::Override`]: utilities.md#override
#### Use self-descriptive wrapper methods
When it's not possible/logical to modify the implementation of a
......@@ -208,8 +214,8 @@ end
In EE, the implementation `ee/app/models/ee/users.rb` would be:
```ruby
override :full_private_access?
def full_private_access?
raise NotImplementedError unless defined?(super)
super || auditor?
end
```
......
......@@ -45,6 +45,51 @@ We developed a number of utilities to ease development.
[:hello, "world", :this, :crushes, "an entire", "hash"]
```
## [`Override`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/utils/override.rb)
* This utility could help us check if a particular method would override
another method or not. It has the same idea of Java's `@Override` annotation
or Scala's `override` keyword. However we only do this check when
`ENV['STATIC_VERIFICATION']` is set to avoid production runtime overhead.
This is useful to check:
* If we have typos in overriding methods.
* If we renamed the overridden methods, making original overriding methods
overrides nothing.
Here's a simple example:
``` ruby
class Base
def execute
end
end
class Derived < Base
extend ::Gitlab::Utils::Override
override :execute # Override check happens here
def execute
end
end
```
This also works on modules:
``` ruby
module Extension
extend ::Gitlab::Utils::Override
override :execute # Modules do not check this immediately
def execute
end
end
class Derived < Base
prepend Extension # Override check happens here, not in the module
end
```
## [`StrongMemoize`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/utils/strong_memoize.rb)
* Memoize the value even if it is `nil` or `false`.
......
module EE
module Admin
module ApplicationsController
extend ::Gitlab::Utils::Override
protected
override :redirect_to_admin_page
def redirect_to_admin_page
raise NotImplementedError unless defined?(super)
log_audit_event
super
......
module EE::Admin::LogsController
include ::Gitlab::Utils::StrongMemoize
extend ::Gitlab::Utils::Override
override :loggers
def loggers
raise NotImplementedError unless defined?(super)
strong_memoize(:loggers) do
super + [
Gitlab::GeoLogger
......
module EE
module ConfirmationsController
include EE::Audit::Changes
extend ::Gitlab::Utils::Override
protected
override :after_sign_in
def after_sign_in(resource)
raise NotImplementedError unless defined?(super)
audit_changes(:email, as: 'email address', model: resource)
super(resource)
......
module EE
module Projects
module GitHttpController
def render_ok
raise NotImplementedError.new unless defined?(super)
extend ::Gitlab::Utils::Override
override :render_ok
def render_ok
set_workhorse_internal_api_content_type
render json: ::Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name, show_all_refs: geo_request?)
end
......@@ -18,17 +19,15 @@ module EE
authentication_result.geo?(project)
end
override :access_actor
def access_actor
raise NotImplementedError.new unless defined?(super)
return :geo if geo?
super
end
override :authenticate_user
def authenticate_user
raise NotImplementedError.new unless defined?(super)
return super unless geo_request?
payload = ::Gitlab::Geo::JwtRequestDecoder.new(request.headers['Authorization']).decode
......
module EE
module Projects
module LfsApiController
def lfs_read_only_message
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :lfs_read_only_message
def lfs_read_only_message
return super unless ::Gitlab::Geo.secondary_with_primary?
(_('You cannot write to a read-only secondary GitLab Geo instance. Please use %{link_to_primary_node} instead.') % { link_to_primary_node: geo_primary_default_url_to_repo(project) }).html_safe
......
module EE
module SessionsController
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
prepended do
before_action :gitlab_geo_login, only: [:new]
......@@ -36,9 +37,8 @@ module EE
super
end
override :redirect_allowed_to?
def redirect_allowed_to?(uri)
raise NotImplementedError unless defined?(super)
# Redirect is not only allowed to current host, but also to other Geo
# nodes. relative_url_root *must* be ignored here as we don't know what
# is root and what is path
......
module EE
module ApplicationSettingsHelper
def visible_attributes
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :visible_attributes
def visible_attributes
super + [
:check_namespace_plan,
:elasticsearch_aws,
......
......@@ -8,6 +8,7 @@
module EE
module ProtectedRefAccess
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
included do
belongs_to :user
......@@ -40,12 +41,12 @@ module EE
end
# Is this a role-based access level?
override :role?
def role?
raise NotImplementedError unless defined?(super)
type == :role
end
override :humanize
def humanize
return self.user.name if self.user.present?
return self.group.name if self.group.present?
......@@ -53,6 +54,7 @@ module EE
super
end
override :check_access
def check_access(user)
return true if user.admin?
return user.id == self.user_id if self.user.present?
......
......@@ -5,6 +5,7 @@ module EE
# and be prepended in the `Namespace` model
module Namespace
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
include ::Gitlab::Utils::StrongMemoize
FREE_PLAN = 'free'.freeze
......@@ -44,9 +45,8 @@ module EE
end
end
override :move_dir
def move_dir
raise NotImplementedError unless defined?(super)
succeeded = super
if succeeded
......
......@@ -5,6 +5,7 @@ module EE
# and be prepended in the `Project` model
module Project
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
include ::Gitlab::Utils::StrongMemoize
prepended do
......@@ -415,9 +416,8 @@ module EE
end
alias_method :merge_requests_ff_only_enabled?, :merge_requests_ff_only_enabled
override :rename_repo
def rename_repo
raise NotImplementedError unless defined?(super)
super
path_was = previous_changes['path'].first
......
module EE
module ProjectTeam
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
override :add_users
def add_users(users, access_level, current_user: nil, expires_at: nil)
raise NotImplementedError unless defined?(super)
return false if group_member_lock
super
end
override :add_user
def add_user(user, access_level, current_user: nil, expires_at: nil)
raise NotImplementedError unless defined?(super)
return false if group_member_lock
super
......
module EE
module BaseCountService
extend ::Gitlab::Utils::Override
# geo secondary cache should expire quicker than primary, otherwise various counts
# could be incorrect for 2 weeks.
override :cache_options
def cache_options
raise NotImplementedError.new unless defined?(super)
value = super
value[:expires_in] = 20.minutes if ::Gitlab::Geo.secondary?
value
......
module EE
module Boards
module CreateService
def can_create_board?
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :can_create_board?
def can_create_board?
parent.feature_available?(:multiple_issue_boards) || super
end
end
......
module EE
module Boards
module ListService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
if parent.multiple_issue_boards_available?(current_user)
super
else
......
module EE
module Groups
module CreateService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
super.tap { |group| log_audit_event if group&.persisted? }
end
......
module EE
module Groups
module DestroyService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
super.tap { |group| log_audit_event unless group&.persisted? }
end
......
module EE
module Groups
module UpdateService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
super.tap { |success| log_audit_event if success }
end
......
module EE
module MergeRequests
module MergeService
def error_check!
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :error_check!
def error_check!
check_size_limit
super
......
module EE
module Projects
module CreateService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
limit = params.delete(:repository_size_limit)
mirror = params.delete(:mirror)
mirror_user_id = params.delete(:mirror_user_id)
......@@ -34,9 +35,8 @@ module EE
::Geo::RepositoryCreatedEventStore.new(project).create
end
override :after_create_actions
def after_create_actions
raise NotImplementedError unless defined?(super)
super
create_predefined_push_rule
......
module EE
module Projects
module DestroyService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
succeeded = super
if succeeded
......
......@@ -2,9 +2,10 @@ module EE
module Projects
module GroupLinks
module CreateService
def execute(group)
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute(group)
super.tap { |link| log_audit_event(link) if link && link&.persisted? }
end
......
......@@ -2,9 +2,10 @@ module EE
module Projects
module GroupLinks
module DestroyService
def execute(group_link)
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute(group_link)
super.tap { |link| log_audit_event(link) if link && !link&.persisted? }
end
......
......@@ -2,9 +2,10 @@ module EE
module Projects
module HashedStorage
module MigrateAttachmentsService
def execute
raise NotImplementedError.new unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
super do
::Geo::HashedStorageAttachmentsEventStore.new(
project,
......
......@@ -2,9 +2,10 @@ module EE
module Projects
module HashedStorage
module MigrateRepositoryService
def execute
raise NotImplementedError.new unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
super do
::Geo::HashedStorageMigratedEventStore.new(
project,
......
module EE
module Projects
module TransferService
extend ::Gitlab::Utils::Override
private
override :execute_system_hooks
def execute_system_hooks
raise NotImplementedError unless defined?(super)
super
EE::Audit::ProjectChangesAuditor.new(current_user, project).execute
......
module EE
module Projects
module UpdateService
def execute
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :execute
def execute
unless project.feature_available?(:repository_mirrors)
params.delete(:mirror)
params.delete(:mirror_user_id)
......
module EE
module ProtectedBranches
module AccessLevelParams
def access_levels
raise NotImplementedError unless defined?(super)
extend ::Gitlab::Utils::Override
override :access_levels
def access_levels
ce_style_access_level + ee_style_access_levels
end
......@@ -17,9 +18,8 @@ module EE
private
override :use_default_access_level?
def use_default_access_level?(params)
raise NotImplementedError unless defined?(super)
params[:"allowed_to_#{type}"].blank?
end
......
module EE
module ProtectedBranches
module ApiService
extend ::Gitlab::Utils::Override
GroupsNotAccessibleError = Class.new(StandardError)
UsersNotAccessibleError = Class.new(StandardError)
override :create
def create
raise NotImplementedError unless defined?(super)
super
rescue ::EE::ProtectedBranches::ApiService::GroupsNotAccessibleError,
::EE::ProtectedBranches::ApiService::UsersNotAccessibleError
......@@ -18,9 +19,8 @@ module EE
private
override :verify_params!
def verify_params!
raise NotImplementedError unless defined?(super)
raise GroupsNotAccessibleError.new unless groups_accessible?
raise UsersNotAccessibleError.new unless users_accessible?
end
......
......@@ -2,9 +2,10 @@ module EE
module Gitlab
module Auth
module Result
def success?
raise NotImplementedError.new unless defined?(super)
extend ::Gitlab::Utils::Override
override :success?
def success?
type == :geo || super
end
......
......@@ -2,18 +2,17 @@ module EE
module Gitlab
module GitAccess
prepend GeoGitAccess
extend ::Gitlab::Utils::Override
override :check
def check(cmd, changes)
raise NotImplementedError.new unless defined?(super)
check_geo_license!
super
end
override :can_read_project?
def can_read_project?
raise NotImplementedError.new unless defined?(super)
return true if geo?
super
......@@ -21,9 +20,8 @@ module EE
protected
override :user
def user
raise NotImplementedError.new unless defined?(super)
return nil if geo?
super
......@@ -31,17 +29,15 @@ module EE
private
override :check_download_access!
def check_download_access!
raise NotImplementedError.new unless defined?(super)
return if geo?
super
end
override :check_active_user!
def check_active_user!
raise NotImplementedError.new unless defined?(super)
return if geo?
super
......
module Gitlab
module Utils
module Override
class Extension
def self.verify_class!(klass, method_name)
instance_method_defined?(klass, method_name) ||
raise(
NotImplementedError.new(
"#{klass}\##{method_name} doesn't exist!"))
end
def self.instance_method_defined?(klass, name, include_super: true)
klass.instance_methods(include_super).include?(name) ||
klass.private_instance_methods(include_super).include?(name)
end
attr_reader :subject
def initialize(subject)
@subject = subject
end
def add_method_name(method_name)
method_names << method_name
end
def add_class(klass)
classes << klass
end
def verify!
classes.each do |klass|
index = klass.ancestors.index(subject)
parents = klass.ancestors.drop(index + 1)
method_names.each do |method_name|
parents.any? do |parent|
self.class.instance_method_defined?(
parent, method_name, include_super: false)
end ||
raise(
NotImplementedError.new(
"#{klass}\##{method_name} doesn't exist!"))
end
end
end
private
def method_names
@method_names ||= []
end
def classes
@classes ||= []
end
end
# Instead of writing patterns like this:
#
# def f
# raise NotImplementedError unless defined?(super)
#
# true
# end
#
# We could write it like:
#
# extend ::Gitlab::Utils::Override
#
# override :f
# def f
# true
# end
#
# This would make sure we're overriding something. See:
# https://gitlab.com/gitlab-org/gitlab-ee/issues/1819
def override(method_name)
return unless ENV['STATIC_VERIFICATION']
if is_a?(Class)
Extension.verify_class!(self, method_name)
else # We delay the check for modules
Override.extensions[self] ||= Extension.new(self)
Override.extensions[self].add_method_name(method_name)
end
end
def included(base = nil)
return super if base.nil? # Rails concern, ignoring it
super
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
def self.verify!
extensions.values.each(&:verify!)
end
end
end
end
......@@ -7,4 +7,9 @@ namespace :dev do
Rake::Task["gitlab:setup"].invoke
Rake::Task["gitlab:shell:setup"].invoke
end
desc "GitLab | Eager load application"
task load: :environment do
Rails.application.eager_load!
end
end
unless Rails.env.production?
namespace :lint do
task :static_verification_env do
ENV['STATIC_VERIFICATION'] = 'true'
end
desc "GitLab | lint | Static verification"
task static_verification: %w[
lint:static_verification_env
dev:load
] do
Gitlab::Utils::Override.verify!
end
desc "GitLab | lint | Lint JavaScript files using ESLint"
task :javascript do
Rake::Task['eslint'].invoke
......
......@@ -10,9 +10,10 @@ tasks = [
%w[bundle exec license_finder],
%w[yarn run eslint],
%w[bundle exec rubocop --parallel],
%w[scripts/lint-conflicts.sh],
%w[bundle exec rake gettext:lint],
%w[scripts/lint-changelog-yaml]
%w[bundle exec rake lint:static_verification],
%w[scripts/lint-changelog-yaml],
%w[scripts/lint-conflicts.sh]
]
failed_tasks = tasks.reduce({}) do |failures, task|
......
require 'spec_helper'
describe Gitlab::Utils::Override do
let(:base) { Struct.new(:good) }
let(:derived) { Class.new(base).tap { |m| m.extend described_class } }
let(:extension) { Module.new.tap { |m| m.extend described_class } }
let(:prepending_class) { base.tap { |m| m.prepend extension } }
let(:including_class) { base.tap { |m| m.include extension } }
let(:klass) { subject }
def good(mod)
mod.module_eval do
override :good
def good
super.succ
end
end
mod
end
def bad(mod)
mod.module_eval do
override :bad
def bad
true
end
end
mod
end
shared_examples 'checking as intended' do
it 'checks ok for overriding method' do
good(subject)
result = klass.new(0).good
expect(result).to eq(1)
described_class.verify!
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)
end
end
shared_examples 'nothing happened' do
it 'does not complain when it is overriding something' do
good(subject)
result = klass.new(0).good
expect(result).to eq(1)
described_class.verify!
end
it 'does not complain when it is not overriding anything' do
bad(subject)
result = klass.new(0).bad
expect(result).to eq(true)
described_class.verify!
end
end
before do
# Make sure we're not touching the internal cache
allow(described_class).to receive(:extensions).and_return({})
end
describe '#override' do
context 'when STATIC_VERIFICATION is set' do
before do
stub_env('STATIC_VERIFICATION', 'true')
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 including it' do
subject { extension }
let(:klass) { including_class }
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)
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)
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 class' do
subject { derived }
it_behaves_like 'nothing happened'
end
context 'when subject is a module, and class is prepending it' do
subject { extension }
let(:klass) { prepending_class }
it_behaves_like 'nothing happened'
end
context 'when subject is a module, and class is including it' do
subject { extension }
let(:klass) { including_class }
it 'does not complain when it is overriding something' do
good(subject)
result = klass.new(0).good
expect(result).to eq(0)
described_class.verify!
end
it 'does not complain when it is not overriding anything' do
bad(subject)
result = klass.new(0).bad
expect(result).to eq(true)
described_class.verify!
end
end
end
end
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