Enforce specifying target class in the presenters

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

The following discussion from !69823 (merged) should be addressed:

• @godfat-gitlab started a discussion: (+4 comments)

  I see that target_klass might not be set for prepended EE module, which makes sense. However, I feel not setting this should be an error.

  What do you think about:

  • Setting the EE module target automatically when we're expanding on the ancestors
  • Make sure all the presenter does define a target class

  I think making sure target_klass is set is pretty crucial, because our verification is based on it being present. If someone forgot to set it, it completely by-passes everything.

  Here's the patch to show this check. This is also based on expanding on ancestors. Sorry that I didn't try to split the patches, so it's kinda mixed up but I hope this is clear enough:

  diff --git a/lib/gitlab/utils/delegator_override.rb b/lib/gitlab/utils/delegator_override.rb
  index 9870acb987f..9b3cda938fb 100644
  --- a/lib/gitlab/utils/delegator_override.rb
  +++ b/lib/gitlab/utils/delegator_override.rb
  @@ -5,13 +5,22 @@ module Utils
       # This module is to validate that delegator classes (`SimpleDelegator`) do not
       # accidentally override important logic on the fabricated object.
       module DelegatorOverride
  +      def self.extended(presenter)
  +        presenters << presenter
  +      end
  +
  +      def self.presenters
  +        @presenters ||= Set.new
  +      end
  +
  +      def close_delegator_target
  +        DelegatorOverride.presenters.delete(self)
  +      end
  +
         def delegator_target(target_klass)
           return unless ENV['STATIC_VERIFICATION']
   
  -        unless self.ancestors.include?(::SimpleDelegator)
  -          raise ArgumentError, "'#{self}' is not a subclass of 'Delegator' class."
  -        end
  -
  +        close_delegator_target
           DelegatorOverride.validator(self).set_target(target_klass)
         end
   
  @@ -39,9 +48,12 @@ def self.validators
   
         def self.verify!
           validators.each_value do |validator|
  -          # This takes prepended modules into account e.g. `EE::` modules
  -          ancestor_validators = validators.values_at(*validator.delegator_klass.ancestors).compact
  -          validator.with(ancestor_validators).validate_overrides! # rubocop: disable CodeReuse/ActiveRecord
  +          validator.expand_on_ancestors(validators)
  +          validator.validate_overrides!
  +        end
  +
  +        if presenters.any?
  +          raise "#{presenters} do not define a delegator_target yet!"
           end
         end
       end
  diff --git a/lib/gitlab/utils/delegator_override/validator.rb b/lib/gitlab/utils/delegator_override/validator.rb
  index c188456f3c2..956ec25bca4 100644
  --- a/lib/gitlab/utils/delegator_override/validator.rb
  +++ b/lib/gitlab/utils/delegator_override/validator.rb
  @@ -6,7 +6,7 @@ module DelegatorOverride
         UnexpectedDelegatorOverrideError = Class.new(StandardError)
   
         class Validator
  -        attr_reader :delegator_klass, :target_klass, :error_header, :ancestor_validators
  +        attr_reader :delegator_klass, :target_klass, :error_header
   
           OVERRIDE_ERROR_MESSAGE = <<~EOS
             We've detected that a presetner is overriding a specific method(s) on a subject model.
  @@ -17,7 +17,6 @@ class Validator
   
           def initialize(delegator_klass)
             @delegator_klass = delegator_klass
  -          @ancestor_validators = []
           end
   
           def add_allowlist(names)
  @@ -36,8 +35,18 @@ def set_error_header(error_header)
             @error_header = error_header
           end
   
  -        def with(ancestor_validators)
  -          tap { @ancestor_validators = ancestor_validators }
  +        # This will make sure allowlist we put into ancestors are all included
  +        def expand_on_ancestors(validators)
  +          delegator_klass.ancestors.each do |ancestor|
  +            next if delegator_klass == ancestor # ancestor includes itself
  +
  +            validator_ancestor = validators[ancestor]
  +
  +            next unless validator_ancestor
  +
  +            ancestor.delegator_target(target_klass)
  +            add_allowlist(validator_ancestor.method_names)
  +          end
           end
   
           def validate_overrides!
  @@ -77,7 +86,6 @@ def extract_location(klass, name)
           def allowlist
             allowed = []
             allowed.concat(method_names)
  -          allowed.concat(ancestor_validators.flat_map(&:method_names))
             allowed.concat(Object.instance_methods)
             allowed.concat(::Delegator.instance_methods)
             allowed.concat(::Presentable.instance_methods)
  diff --git a/lib/gitlab/view/presenter/base.rb b/lib/gitlab/view/presenter/base.rb
  index 76d3da265e4..6fc6bcecf43 100644
  --- a/lib/gitlab/view/presenter/base.rb
  +++ b/lib/gitlab/view/presenter/base.rb
  @@ -8,6 +8,7 @@ module Presenter
         module Base
           extend ActiveSupport::Concern
           extend ::Gitlab::Utils::DelegatorOverride
  +        close_delegator_target
   
           include Gitlab::Routing
           include Gitlab::Allowable
  diff --git a/lib/gitlab/view/presenter/delegated.rb b/lib/gitlab/view/presenter/delegated.rb
  index 1178a8f5a58..5336dab864f 100644
  --- a/lib/gitlab/view/presenter/delegated.rb
  +++ b/lib/gitlab/view/presenter/delegated.rb
  @@ -5,6 +5,7 @@ module View
       module Presenter
         class Delegated < SimpleDelegator
           extend ::Gitlab::Utils::DelegatorOverride
  +        close_delegator_target
   
           # TODO: Stop including `Presenter::Base` in `Presenter::Delegated` as
           # it overrides many methods in the Active Record models.

  What do you think?