From b890b7b7742a32ae2dcfd696c2c44a866bdc385c Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 23 Feb 2018 15:36:40 +0000 Subject: Convert from GraphQL::Batch to BatchLoader --- Gemfile | 7 ++--- Gemfile.lock | 17 ++++++------ app/graphql/gitlab_schema.rb | 2 +- app/graphql/loaders/base_loader.rb | 21 ++++---------- app/graphql/loaders/full_path_loader.rb | 24 ++++++++-------- app/graphql/loaders/iid_loader.rb | 40 ++++++++++----------------- config/dependency_decisions.yml | 6 ---- spec/graphql/gitlab_schema_spec.rb | 2 +- spec/graphql/loaders/full_path_loader_spec.rb | 6 ---- spec/graphql/loaders/iid_loader_spec.rb | 6 ---- spec/support/helpers/graphql_helpers.rb | 17 +++++------- 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) -- cgit v1.2.1