Merge branch 'ajk-gql-base-resolver-improvements' into 'master'

[GQL] Support `#ready?` in resolvers

Closes #218289

See merge request gitlab-org/gitlab!32329

app/graphql/resolvers/base_resolver.rb

@@ -3,27 +3,33 @@
 module Resolvers
   class BaseResolver < GraphQL::Schema::Resolver
     extend ::Gitlab::Utils::Override
+    include ::Gitlab::Utils::StrongMemoize
 
     def self.single
       @single ||= Class.new(self) do
+        def ready?(**args)
+          ready, early_return = super
+          [ready, select_result(early_return)]
+        end
+
         def resolve(**args)
-          super.first
+          select_result(super)
         end
 
         def single?
           true
         end
+
+        def select_result(results)
+          results&.first
+        end
       end
     end
 
     def self.last
-      @last ||= Class.new(self) do
-        def resolve(**args)
-          super.last
-        end
-
-        def single?
-          true
+      @last ||= Class.new(self.single) do
+        def select_result(results)
+          results&.last
         end
       end
     end
@@ -59,6 +65,17 @@ module Resolvers
       end
     end
 
+    def synchronized_object
+      strong_memoize(:synchronized_object) do
+        case object
+        when BatchLoader::GraphQL
+          object.sync
+        else
+          object
+        end
+      end
+    end
+
     def single?
       false
     end

doc/development/api_graphql_styleguide.md

@@ -619,6 +619,31 @@ lot of dependent objects.
 
 To limit the amount of queries performed, we can use `BatchLoader`.
 
+### Correct use of `Resolver#ready?`
+
+Resolvers have two public API methods as part of the framework: `#ready?(**args)` and `#resolve(**args)`.
+We can use `#ready?` to perform set-up, validation or early-return without invoking `#resolve`.
+
+Good reasons to use `#ready?` include:
+
+- validating mutually exclusive arguments (see [validating arguments](#validating-arguments))
+- Returning `Relation.none` if we know before-hand that no results are possible
+- Performing setup such as initializing instance variables (although consider lazily initialized methods for this)
+
+Implementations of [`Resolver#ready?(**args)`](https://graphql-ruby.org/api-doc/1.10.9/GraphQL/Schema/Resolver#ready%3F-instance_method) should
+return `(Boolean, early_return_data)` as follows:
+
+```ruby
+def ready?(**args)
+  [false, 'have this instead']
+end
+```
+
+For this reason, whenever you call a resolver (mainly in tests - as framework
+abstractions Resolvers should not be considered re-usable, finders are to be
+preferred), remember to call the `ready?` method and check the boolean flag
+before calling `resolve`! An example can be seen in our [`GraphQLHelpers`](https://gitlab.com/gitlab-org/gitlab/-/blob/2d395f32d2efbb713f7bc861f96147a2a67e92f2/spec/support/helpers/graphql_helpers.rb#L20-27).
+
 ## Mutations
 
 Mutations are used to change any stored values, or to trigger
@@ -785,7 +810,7 @@ def ready?(**args)
   end
 
   # Always remember to call `#super`
-  super(args)
+  super
 end
 ```
 

spec/graphql/resolvers/base_resolver_spec.rb

@@ -41,9 +41,35 @@ describe Resolvers::BaseResolver do
     end
   end
 
+  context 'when the resolver returns early' do
+    let(:resolver) do
+      Class.new(described_class) do
+        def ready?(**args)
+          [false, %w(early return)]
+        end
+
+        def resolve(**args)
+          raise 'Should not get here'
+        end
+      end
+    end
+
+    it 'runs correctly in our test framework' do
+      expect(resolve(resolver)).to contain_exactly('early', 'return')
+    end
+
+    it 'single selects the first early return value' do
+      expect(resolve(resolver.single)).to eq('early')
+    end
+
+    it 'last selects the last early return value' do
+      expect(resolve(resolver.last)).to eq('return')
+    end
+  end
+
   describe '.last' do
     it 'returns a subclass from the resolver' do
-      expect(last_resolver.last.superclass).to eq(last_resolver)
+      expect(last_resolver.last.ancestors).to include(last_resolver)
     end
 
     it 'returns the same subclass every time' do
@@ -95,4 +121,28 @@ describe Resolvers::BaseResolver do
       end
     end
   end
+
+  describe '#synchronized_object' do
+    let(:object) { double(foo: :the_foo) }
+
+    let(:resolver) do
+      Class.new(described_class) do
+        def resolve(**args)
+          [synchronized_object.foo]
+        end
+      end
+    end
+
+    it 'handles raw objects' do
+      expect(resolve(resolver, obj: object)).to contain_exactly(:the_foo)
+    end
+
+    it 'handles lazy objects' do
+      delayed = BatchLoader::GraphQL.for(1).batch do |_, loader|
+        loader.call(1, object)
+      end
+
+      expect(resolve(resolver, obj: delayed)).to contain_exactly(:the_foo)
+    end
+  end
 end

spec/support/helpers/graphql_helpers.rb

@@ -11,9 +11,19 @@ module GraphqlHelpers
     underscored_field_name.to_s.camelize(:lower)
   end
 
-  # Run a loader's named resolver
+  # Run a loader's named resolver in a way that closely mimics the framework.
+  #
+  # First the `ready?` method is called. If it turns out that the resolver is not
+  # ready, then the early return is returned instead.
+  #
+  # Then the resolve method is called.
   def resolve(resolver_class, obj: nil, args: {}, ctx: {}, field: nil)
-    resolver_class.new(object: obj, context: ctx, field: field).resolve(args)
+    resolver = resolver_class.new(object: obj, context: ctx, field: field)
+    ready, early_return = sync_all { resolver.ready?(**args) }
+
+    return early_return unless ready
+
+    resolver.resolve(args)
   end
 
   # Eagerly run a loader's named resolver
@@ -51,12 +61,12 @@ module GraphqlHelpers
   # BatchLoader::GraphQL returns a wrapper, so we need to :sync in order
   # to get the actual values
   def batch_sync(max_queries: nil, &blk)
-    wrapper = proc do
-      lazy_vals = yield
-      lazy_vals.is_a?(Array) ? lazy_vals.map { |val| sync(val) } : sync(lazy_vals)
-    end
+    batch(max_queries: max_queries) { sync_all(&blk) }
+  end
 
-    batch(max_queries: max_queries, &wrapper)
+  def sync_all(&blk)
+    lazy_vals = yield
+    lazy_vals.is_a?(Array) ? lazy_vals.map { |val| sync(val) } : sync(lazy_vals)
   end
 
   def graphql_query_for(name, attributes = {}, fields = nil)

spec/support/matchers/graphql_matchers.rb

@@ -85,9 +85,16 @@ end
 RSpec::Matchers.define :have_graphql_arguments do |*expected|
   include GraphqlHelpers
 
+  def expected_names
+    @names ||= Array.wrap(expected).map { |name| GraphqlHelpers.fieldnamerize(name) }
+  end
+
   match do |field|
-    argument_names = expected.map { |name| GraphqlHelpers.fieldnamerize(name) }
-    expect(field.arguments.keys).to contain_exactly(*argument_names)
+    expect(field.arguments.keys).to contain_exactly(*expected_names)
+  end
+
+  failure_message do |field|
+    "expected that #{field.name} would have the following fields: #{expected_names.inspect}, but it has #{field.arguments.keys.inspect}."
   end
 end