Convert from GraphQL::Batch to BatchLoader34754-graphql-start

11 files changed, 51 insertions, 97 deletions

diff --git a/Gemfile b/Gemfile
index 39982023cc6..dd76723f765 100644
--- a/Gemfile
+++ b/Gemfile
@@ -89,10 +89,9 @@ gem 'grape-entity', '~> 0.6.0'
 gem 'rack-cors', '~> 1.0.0', require: 'rack/cors'
 
 # GraphQL API
-gem 'graphql', '~> 1.6.7'
-gem 'graphql-batch', '~> 0.3.4'
-gem 'graphql-preload', '~> 1.0.3'
-gem 'graphiql-rails', '~> 1.4.4', group: :development
+gem 'graphql', '~> 1.7.12'
+gem 'graphql-preload', '~> 1.0.4'
+gem 'graphiql-rails', '~> 1.4.9', group: :development
 
 # Disable strong_params so that Mash does not respond to :permitted?
 gem 'hashie-forbidden_attributes'
diff --git a/Gemfile.lock b/Gemfile.lock
index 4ad70c0ae05..f1fef60365a 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -369,13 +369,13 @@ GEM
       rake
     grape_logging (1.7.0)
       grape
-    graphiql-rails (1.4.4)
+    graphiql-rails (1.4.9)
       rails
-    graphql (1.6.7)
-    graphql-batch (0.3.4)
+    graphql (1.7.12)
+    graphql-batch (0.3.9)
       graphql (>= 0.8, < 2)
       promise.rb (~> 0.7.2)
-    graphql-preload (1.0.3)
+    graphql-preload (1.0.4)
       activerecord (>= 3.2, < 6)
       graphql (>= 1.5, < 2)
       graphql-batch (~> 0.3)
@@ -648,7 +648,7 @@ GEM
       unparser
     procto (0.0.3)
     prometheus-client-mmap (0.9.1)
-    promise.rb (0.7.3)
+    promise.rb (0.7.4)
     pry (0.10.4)
       coderay (~> 1.1.0)
       method_source (~> 0.8.1)
@@ -1085,10 +1085,9 @@ DEPENDENCIES
   grape-entity (~> 0.6.0)
   grape-route-helpers (~> 2.1.0)
   grape_logging (~> 1.7)
-  graphiql-rails (~> 1.4.4)
-  graphql (~> 1.6.7)
-  graphql-batch (~> 0.3.4)
-  graphql-preload (~> 1.0.3)
+  graphiql-rails (~> 1.4.9)
+  graphql (~> 1.7.12)
+  graphql-preload (~> 1.0.4)
   haml_lint (~> 0.26.0)
   hamlit (~> 2.6.1)
   hashie-forbidden_attributes
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index 7392bf6f503..143ccf92c85 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -1,7 +1,7 @@
 Gitlab::Graphql::Authorize.register!
 
 GitlabSchema = GraphQL::Schema.define do
-  use GraphQL::Batch
+  use BatchLoader::GraphQL
 
   enable_preloading
   enable_authorization
diff --git a/app/graphql/loaders/base_loader.rb b/app/graphql/loaders/base_loader.rb
index c32c4daa91a..aad435ea09b 100644
--- a/app/graphql/loaders/base_loader.rb
+++ b/app/graphql/loaders/base_loader.rb
@@ -1,8 +1,10 @@
 # Helper methods for all loaders
-class Loaders::BaseLoader < GraphQL::Batch::Loader
-  # Convert a class method into a resolver proc. The method should follow the
-  # (obj, args, ctx) calling convention
-  class << self
+module Loaders::BaseLoader
+  extend ActiveSupport::Concern
+
+  class_methods do
+    # Convert a class method into a resolver proc. The method should follow the
+    # (obj, args, ctx) calling convention
     def [](sym)
       resolver = method(sym)
       raise ArgumentError.new("#{self}.#{sym} is not a resolver") unless resolver.arity == 3
@@ -10,15 +12,4 @@ class Loaders::BaseLoader < GraphQL::Batch::Loader
       resolver
     end
   end
-
-  # Fulfill all keys. Pass a block that converts each result into a key.
-  # Any keys not in results will be fulfilled with nil.
-  def fulfill_all(results, keys, &key_blk)
-    results.each do |result|
-      key = yield result
-      fulfill(key, result)
-    end
-
-    keys.each { |key| fulfill(key, nil) unless fulfilled?(key) }
-  end
 end
diff --git a/app/graphql/loaders/full_path_loader.rb b/app/graphql/loaders/full_path_loader.rb
index f99b487ce5d..27ada90b82a 100644
--- a/app/graphql/loaders/full_path_loader.rb
+++ b/app/graphql/loaders/full_path_loader.rb
@@ -1,23 +1,21 @@
-class Loaders::FullPathLoader < Loaders::BaseLoader
+module Loaders::FullPathLoader
+  include Loaders::BaseLoader
+
   class << self
     def project(obj, args, ctx)
       project_by_full_path(args[:full_path])
     end
 
     def project_by_full_path(full_path)
-      self.for(Project).load(full_path)
+      model_by_full_path(Project, full_path)
     end
-  end
-
-  attr_reader :model
 
-  def initialize(model)
-    @model = model
-  end
-
-  def perform(keys)
-    # `with_route` prevents relation.all.map(&:full_path)` from being N+1
-    relation = model.where_full_path_in(keys).with_route
-    fulfill_all(relation, keys, &:full_path)
+    def model_by_full_path(model, full_path)
+      BatchLoader.for(full_path).batch(key: "#{model.model_name.param_key}:full_path") do |full_paths, loader|
+        # `with_route` avoids an N+1 calculating full_path
+        results = model.where_full_path_in(full_paths).with_route
+        results.each { |project| loader.call(project.full_path, project) }
+      end
+    end
   end
 end
diff --git a/app/graphql/loaders/iid_loader.rb b/app/graphql/loaders/iid_loader.rb
index e89031da0c2..f9827765a92 100644
--- a/app/graphql/loaders/iid_loader.rb
+++ b/app/graphql/loaders/iid_loader.rb
@@ -1,35 +1,23 @@
-class Loaders::IidLoader < Loaders::BaseLoader
+class Loaders::IidLoader
+  include Loaders::BaseLoader
+
   class << self
     def merge_request(obj, args, ctx)
       iid = args[:iid]
-      promise = Loaders::FullPathLoader.project_by_full_path(args[:project])
+      project = Loaders::FullPathLoader.project_by_full_path(args[:project])
+      merge_request_by_project_and_iid(project, iid)
+    end
+
+    def merge_request_by_project_and_iid(project_loader, iid)
+      project_id = project_loader.__sync&.id
 
-      promise.then do |project|
-        if project
-          merge_request_by_project_and_iid(project.id, iid)
-        else
-          nil
+      # IIDs are represented as the GraphQL `id` type, which is a string
+      BatchLoader.for(iid.to_s).batch(key: "merge_request:target_project:#{project_id}:iid") do |iids, loader|
+        if project_id
+          results = MergeRequest.where(target_project_id: project_id, iid: iids)
+          results.each { |mr| loader.call(mr.iid.to_s, mr) }
         end
       end
     end
-
-    def merge_request_by_project_and_iid(project_id, iid)
-      self.for(MergeRequest, target_project_id: project_id.to_s).load(iid.to_s)
-    end
-  end
-
-  attr_reader :model, :restrictions
-
-  def initialize(model, restrictions = {})
-    @model = model
-    @restrictions = restrictions
-  end
-
-  def perform(keys)
-    relation = model.where(iid: keys)
-    relation = relation.where(restrictions) if restrictions.present?
-
-    # IIDs are represented as the GraphQL `id` type, which is a string
-    fulfill_all(relation, keys) { |instance| instance.iid.to_s }
   end
 end
diff --git a/config/dependency_decisions.yml b/config/dependency_decisions.yml
index 55a8c9dac9b..6616b85129e 100644
--- a/config/dependency_decisions.yml
+++ b/config/dependency_decisions.yml
@@ -534,9 +534,3 @@
     :why: https://github.com/squaremo/bitsyntax-js/blob/master/LICENSE-MIT
     :versions: []
     :when: 2018-02-20 22:20:25.958123000 Z
-  - promise.rb
-  - Unlicense
-  - :who: 
-    :why: 
-    :versions: []
-    :when: 2017-09-05 13:10:22.752422892 Z
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index 3582f297866..070b851b109 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe GitlabSchema do
   it 'uses batch loading' do
-    expect(described_class.instrumenters[:multiplex]).to include(GraphQL::Batch::SetupMultiplex)
+    expect(field_instrumenters).to include(BatchLoader::GraphQL)
   end
 
   it 'enables the preload instrumenter' do
diff --git a/spec/graphql/loaders/full_path_loader_spec.rb b/spec/graphql/loaders/full_path_loader_spec.rb
index 2a473239550..2732dd8c9da 100644
--- a/spec/graphql/loaders/full_path_loader_spec.rb
+++ b/spec/graphql/loaders/full_path_loader_spec.rb
@@ -24,12 +24,6 @@ describe Loaders::FullPathLoader do
 
       expect(result).to be_nil
     end
-
-    it 'returns a promise' do
-      batch do
-        expect(resolve_project(project1.full_path)).to be_a(Promise)
-      end
-    end
   end
 
   def resolve_project(full_path)
diff --git a/spec/graphql/loaders/iid_loader_spec.rb b/spec/graphql/loaders/iid_loader_spec.rb
index 8a0c1f0791a..0a57d7c4ed4 100644
--- a/spec/graphql/loaders/iid_loader_spec.rb
+++ b/spec/graphql/loaders/iid_loader_spec.rb
@@ -50,12 +50,6 @@ describe Loaders::IidLoader do
 
       expect(result).to be_nil
     end
-
-    it 'returns a promise' do
-      batch do
-        expect(resolve_mr(full_path, iid_1)).to be_a(Promise)
-      end
-    end
   end
 
   def resolve_mr(full_path, iid)
diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb
index 5bb2cf9dd9e..1eaa7603af0 100644
--- a/spec/support/helpers/graphql_helpers.rb
+++ b/spec/support/helpers/graphql_helpers.rb
@@ -4,20 +4,17 @@ module GraphqlHelpers
     kls[name].call(obj, args, ctx)
   end
 
-  # Runs a block inside a GraphQL::Batch wrapper
+  # Runs a block inside a BatchLoader::Executor wrapper
   def batch(max_queries: nil, &blk)
     wrapper = proc do
-      GraphQL::Batch.batch do
-        result = yield
-
-        if result.is_a?(Array)
-          Promise.all(result)
-        else
-          result
-        end
+      begin
+        BatchLoader::Executor.ensure_current
+        blk.call
+      ensure
+        BatchLoader::Executor.clear_current
       end
     end
-
+  
     if max_queries
       result = nil
       expect { result = wrapper.call }.not_to exceed_query_limit(max_queries)