Convert GQL presenter tool to field extension

This changes the presenter instrumentation to a field extension, which
is simpler, easier to maintain, and will allow us to make use of the
interpreter execution engine in the future.

This also makes is easier to test resolver and types that make use of
these facilities.

Other changes:

In the current user TODOs type, ensure we take the type of the unwrapped
object If we don't do this, the presented class name will be used
instead (for types that use `present_using`). This results in incorrect
todo_type values.

app/graphql/gitlab_schema.rb

@@ -13,7 +13,6 @@ class GitlabSchema < GraphQL::Schema
   use GraphQL::Pagination::Connections
   use BatchLoader::GraphQL
   use Gitlab::Graphql::Authorize
-  use Gitlab::Graphql::Present
   use Gitlab::Graphql::Pagination::Connections
   use Gitlab::Graphql::GenericTracing
   use Gitlab::Graphql::Timeout, max_seconds: Gitlab.config.gitlab.graphql_timeout

app/graphql/types/base_field.rb

@@ -22,6 +22,8 @@ module Types
 
       # We want to avoid the overhead of this in prod
       extension ::Gitlab::Graphql::CallsGitaly::FieldExtension if Gitlab.dev_or_test_env?
+
+      extension ::Gitlab::Graphql::Present::FieldExtension
     end
 
     def may_call_gitaly?

app/graphql/types/current_user_todos.rb

@@ -16,9 +16,10 @@ module Types
           end
 
     def current_user_todos(state: nil)
-      state ||= %i(done pending) # TodosFinder treats a `nil` state param as `pending`
+      state ||= %i[done pending] # TodosFinder treats a `nil` state param as `pending`
+      klass = unpresented.class
 
-      TodosFinder.new(current_user, state: state, type: object.class.name, target_id: object.id).execute
+      TodosFinder.new(current_user, state: state, type: klass.name, target_id: object.id).execute
     end
   end
 end

lib/gitlab/graphql/present.rb

@@ -12,11 +12,30 @@ module Gitlab
         def self.presenter_class
           @presenter_class
         end
+
+        def self.present(object, attrs)
+          klass = @presenter_class
+          return object if !klass || object.is_a?(klass)
+
+          @presenter_class.new(object, **attrs)
+        end
+      end
+
+      def unpresented
+        unwrapped || object
       end
 
-      def self.use(schema_definition)
-        schema_definition.instrument(:field, ::Gitlab::Graphql::Present::Instrumentation.new)
+      def present(object_type, attrs)
+        return unless object_type.respond_to?(:present)
+
+        self.unwrapped ||= object
+        # @object belongs to Schema::Object, which does not expose a writer.
+        @object = object_type.present(unwrapped, attrs) # rubocop: disable Gitlab/ModuleWithInstanceVariables
       end
+
+      private
+
+      attr_accessor :unwrapped
     end
   end
 end

lib/gitlab/graphql/present/field_extension.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Present
+      class FieldExtension < ::GraphQL::Schema::FieldExtension
+        SAFE_CONTEXT_KEYS = %i[current_user].freeze
+
+        def resolve(object:, arguments:, context:)
+          attrs = safe_context_values(context)
+
+          # We need to handle the object being either a Schema::Object or an
+          # inner Schema::Object#object. This depends on whether the field
+          # has a @resolver_proc or not.
+          if object.is_a?(::Types::BaseObject)
+            object.present(field.owner, attrs)
+            yield(object, arguments)
+          else
+            # This is the legacy code-path, hit if the field has a @resolver_proc
+            # TODO: remove this when resolve procs are removed from the
+            # graphql-ruby library, and all field instrumentation is removed.
+            presented = field.owner.try(:present, object, attrs) || object
+            yield(presented, arguments)
+          end
+        end
+
+        private
+
+        def safe_context_values(context)
+          context.to_h.slice(*SAFE_CONTEXT_KEYS)
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/present/instrumentation.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Graphql
-    module Present
-      class Instrumentation
-        SAFE_CONTEXT_KEYS = %i[current_user].freeze
-
-        def instrument(type, field)
-          return field unless field.metadata[:type_class]
-
-          presented_in = field.metadata[:type_class].owner
-          return field unless presented_in.respond_to?(:presenter_class)
-          return field unless presented_in.presenter_class
-
-          old_resolver = field.resolve_proc
-
-          resolve_with_presenter = -> (presented_type, args, context) do
-            # We need to wrap the original presentation type into a type that
-            # uses the presenter as an object.
-            object = presented_type.object
-
-            if object.is_a?(presented_in.presenter_class)
-              next old_resolver.call(presented_type, args, context)
-            end
-
-            attrs = safe_context_values(context)
-            presenter = presented_in.presenter_class.new(object, **attrs)
-
-            # we have to use the new `authorized_new` method, as `new` is protected
-            wrapped = presented_type.class.authorized_new(presenter, context)
-
-            old_resolver.call(wrapped, args, context)
-          end
-
-          field.redefine do
-            resolve(resolve_with_presenter)
-          end
-        end
-
-        private
-
-        def safe_context_values(context)
-          context.to_h.slice(*SAFE_CONTEXT_KEYS)
-        end
-      end
-    end
-  end
-end

spec/graphql/gitlab_schema_spec.rb

@@ -18,10 +18,6 @@ RSpec.describe GitlabSchema do
     expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Authorize::Instrumentation))
   end
 
-  it 'enables using presenters' do
-    expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Present::Instrumentation))
-  end
-
   it 'has the base mutation' do
     expect(described_class.mutation).to eq(::Types::MutationType)
   end

spec/lib/gitlab/graphql/present/field_extension_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Gitlab::Graphql::Present::FieldExtension do
+  include GraphqlHelpers
+
+  let_it_be(:user) { create(:user) }
+
+  let(:object) { double(value: 'foo') }
+  let(:owner) { fresh_object_type }
+  let(:field_name) { 'value' }
+  let(:field) do
+    ::Types::BaseField.new(name: field_name, type: GraphQL::STRING_TYPE, null: true, owner: owner)
+  end
+
+  let(:base_presenter) do
+    Class.new(SimpleDelegator) do
+      def initialize(object, **options)
+        super(object)
+        @object = object
+        @options = options
+      end
+    end
+  end
+
+  def resolve_value
+    resolve_field(field, object, current_user: user, object_type: owner)
+  end
+
+  context 'when the object does not declare a presenter' do
+    it 'does not affect normal resolution' do
+      expect(resolve_value).to eq 'foo'
+    end
+  end
+
+  describe 'interactions with inheritance' do
+    def parent
+      type = fresh_object_type('Parent')
+      type.present_using(provide_foo)
+      type.field :foo, ::GraphQL::INT_TYPE, null: true
+      type.field :value, ::GraphQL::STRING_TYPE, null: true
+      type
+    end
+
+    def child
+      type = Class.new(parent)
+      type.graphql_name 'Child'
+      type.present_using(provide_bar)
+      type.field :bar, ::GraphQL::INT_TYPE, null: true
+      type
+    end
+
+    def provide_foo
+      Class.new(base_presenter) do
+        def foo
+          100
+        end
+      end
+    end
+
+    def provide_bar
+      Class.new(base_presenter) do
+        def bar
+          101
+        end
+      end
+    end
+
+    it 'can resolve value, foo and bar' do
+      type = child
+      value = resolve_field(:value, object, object_type: type)
+      foo = resolve_field(:foo, object, object_type: type)
+      bar = resolve_field(:bar, object, object_type: type)
+
+      expect([value, foo, bar]).to eq ['foo', 100, 101]
+    end
+  end
+
+  shared_examples 'calling the presenter method' do
+    it 'calls the presenter method' do
+      expect(resolve_value).to eq presenter.new(object, current_user: user).send(field_name)
+    end
+  end
+
+  context 'when the object declares a presenter' do
+    before do
+      owner.present_using(presenter)
+    end
+
+    context 'when the presenter overrides the original method' do
+      def twice
+        Class.new(base_presenter) do
+          def value
+            @object.value * 2
+          end
+        end
+      end
+
+      let(:presenter) { twice }
+
+      it_behaves_like 'calling the presenter method'
+    end
+
+    # This is exercised here using an explicit `resolve:` proc, but
+    # @resolver_proc values are used in field instrumentation as well.
+    context 'when the field uses a resolve proc' do
+      let(:presenter) { base_presenter }
+      let(:field) do
+        ::Types::BaseField.new(
+          name: field_name,
+          type: GraphQL::STRING_TYPE,
+          null: true,
+          owner: owner,
+          resolve: ->(obj, args, ctx) { 'Hello from a proc' }
+        )
+      end
+
+      specify { expect(resolve_value).to eq 'Hello from a proc' }
+    end
+
+    context 'when the presenter provides a new method' do
+      def presenter
+        Class.new(base_presenter) do
+          def current_username
+            "Hello #{@options[:current_user]&.username} from the presenter!"
+          end
+        end
+      end
+
+      context 'when we select the original field' do
+        it 'is unaffected' do
+          expect(resolve_value).to eq 'foo'
+        end
+      end
+
+      context 'when we select the new field' do
+        let(:field_name) { 'current_username' }
+
+        it_behaves_like 'calling the presenter method'
+      end
+    end
+  end
+end

spec/requests/api/graphql/issue/issue_spec.rb

@@ -8,10 +8,9 @@ RSpec.describe 'Query.issue(id)' do
   let_it_be(:project) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
+  let_it_be(:issue_params) { { 'id' => issue.to_global_id.to_s } }
 
   let(:issue_data) { graphql_data['issue'] }
-
-  let_it_be(:issue_params) { { 'id' => issue.to_global_id.to_s } }
   let(:issue_fields) { all_graphql_fields_for('Issue'.classify) }
 
   let(:query) do
@@ -62,7 +61,7 @@ RSpec.describe 'Query.issue(id)' do
       )
     end
 
-    context 'selecting any single field' do
+    context 'when selecting any single field' do
       where(:field) do
         scalar_fields_of('Issue').map { |name| [name] }
       end
@@ -84,13 +83,13 @@ RSpec.describe 'Query.issue(id)' do
       end
     end
 
-    context 'selecting multiple fields' do
+    context 'when selecting multiple fields' do
       let(:issue_fields) { ['title', 'description', 'updatedBy { username }'] }
 
       it 'returns the Issue with the specified fields' do
         post_graphql(query, current_user: current_user)
 
-        expect(issue_data.keys).to eq( %w(title description updatedBy) )
+        expect(issue_data.keys).to eq %w[title description updatedBy]
         expect(issue_data['title']).to eq(issue.title)
         expect(issue_data['description']).to eq(issue.description)
         expect(issue_data['updatedBy']['username']).to eq(issue.author.username)
@@ -110,14 +109,14 @@ RSpec.describe 'Query.issue(id)' do
       it 'returns correct attributes' do
         post_graphql(query, current_user: current_user)
 
-        expect(issue_data.keys).to eq( %w(moved movedTo) )
+        expect(issue_data.keys).to eq %w[moved movedTo]
         expect(issue_data['moved']).to eq(true)
         expect(issue_data['movedTo']['title']).to eq(new_issue.title)
       end
     end
 
     context 'when passed a non-Issue gid' do
-      let(:mr) {create(:merge_request)}
+      let(:mr) { create(:merge_request) }
 
       it 'returns an error' do
         gid = mr.to_global_id.to_s