diff options
| -rw-r--r-- | Gemfile | 1 | ||||
| -rw-r--r-- | Gemfile.lock | 1 | ||||
| -rw-r--r-- | app/graphql/gitlab_schema.rb | 2 | ||||
| -rw-r--r-- | app/graphql/loaders/base_loader.rb | 21 | ||||
| -rw-r--r-- | app/graphql/loaders/full_path_loader.rb | 24 | ||||
| -rw-r--r-- | app/graphql/loaders/iid_loader.rb | 40 | ||||
| -rw-r--r-- | config/dependency_decisions.yml | 6 | ||||
| -rw-r--r-- | lib/gitlab/graphql/authorize.rb | 2 | ||||
| -rw-r--r-- | spec/graphql/gitlab_schema_spec.rb | 2 | ||||
| -rw-r--r-- | spec/graphql/loaders/full_path_loader_spec.rb | 6 | ||||
| -rw-r--r-- | spec/graphql/loaders/iid_loader_spec.rb | 6 | ||||
| -rw-r--r-- | spec/support/helpers/graphql_helpers.rb | 17 |
12 files changed, 42 insertions, 86 deletions
@@ -95,7 +95,6 @@ gem 'rack-cors', '~> 1.0.0', require: 'rack/cors' # GraphQL API gem 'graphql', '~> 1.7.14' -gem 'graphql-batch', '~> 0.3.9' gem 'graphql-preload', '~> 2.0.0' gem 'graphiql-rails', '~> 1.4.10' diff --git a/Gemfile.lock b/Gemfile.lock index 68dd0fba256..8fe2f5f06f8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1068,7 +1068,6 @@ DEPENDENCIES grape_logging (~> 1.7) graphiql-rails (~> 1.4.10) graphql (~> 1.7.14) - graphql-batch (~> 0.3.9) graphql-preload (~> 2.0.0) grpc (~> 1.11.0) haml_lint (~> 0.26.0) 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/lib/gitlab/graphql/authorize.rb b/lib/gitlab/graphql/authorize.rb index 1df130e519a..6f9de8c38b7 100644 --- a/lib/gitlab/graphql/authorize.rb +++ b/lib/gitlab/graphql/authorize.rb @@ -47,6 +47,8 @@ module Gitlab def build_checker(current_user, abilities) proc do |obj| + # Load the elements if they weren't loaded by BatchLoader yet + obj = obj.sync if obj.respond_to?(:sync) obj if abilities.all? { |ability| Ability.allowed?(current_user, ability, obj) } end end 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) |