Merge branch 'ajk-graphql-finders' into 'master'

Move issuable resolver logic to finders

See merge request gitlab-org/gitlab!31542

app/finders/issuable_finder/params.rb

@@ -110,7 +110,9 @@ class IssuableFinder
 
     def group
       strong_memoize(:group) do
-        if params[:group_id].present?
+        if params[:group_id].is_a?(Group)
+          params[:group_id]
+        elsif params[:group_id].present?
           Group.find(params[:group_id])
         else
           nil

app/finders/issues_finder/params.rb

@@ -27,18 +27,13 @@ class IssuesFinder
     end
 
     def user_can_see_all_confidential_issues?
-      return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues)
-
-      return @user_can_see_all_confidential_issues = false if current_user.blank?
-      return @user_can_see_all_confidential_issues = true if current_user.can_read_all_resources?
-
-      @user_can_see_all_confidential_issues =
-        if project? && project
-          project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
-        elsif group
-          group.max_member_access_for_user(current_user) >= CONFIDENTIAL_ACCESS_LEVEL
+      strong_memoize(:user_can_see_all_confidential_issues) do
+        parent = project? ? project : group
+        if parent
+          Ability.allowed?(current_user, :read_confidential_issues, parent)
         else
-          false
+          Ability.allowed?(current_user, :read_all_resources)
+        end
       end
     end
 

app/graphql/mutations/concerns/mutations/resolves_issuable.rb

@@ -9,30 +9,31 @@ module Mutations
     end
 
     def resolve_issuable(type:, parent_path:, iid:)
-      parent = resolve_issuable_parent(type, parent_path)
-      key = type == :merge_request ? :iids : :iid
-      args = { key => iid.to_s }
+      parent = ::Gitlab::Graphql::Lazy.force(resolve_issuable_parent(type, parent_path))
+      return unless parent.present?
 
-      resolver = issuable_resolver(type, parent, context)
-      ready, early_return = resolver.ready?(**args)
-
-      return early_return unless ready
-
-      resolver.resolve(**args)
+      finder = issuable_finder(type, iids: [iid])
+      Gitlab::Graphql::Loaders::IssuableLoader.new(parent, finder).find_all.first
     end
 
     private
 
-    def issuable_resolver(type, parent, context)
-      resolver_class = "Resolvers::#{type.to_s.classify.pluralize}Resolver".constantize
-
-      resolver_class.single.new(object: parent, context: context, field: nil)
+    def issuable_finder(type, args)
+      case type
+      when :merge_request
+        MergeRequestsFinder.new(current_user, args)
+      when :issue
+        IssuesFinder.new(current_user, args)
+      else
+        raise "Unsupported type: #{type}"
+      end
     end
 
     def resolve_issuable_parent(type, parent_path)
+      return unless parent_path.present?
       return unless type == :issue || type == :merge_request
 
-      resolve_project(full_path: parent_path) if parent_path.present?
+      resolve_project(full_path: parent_path)
     end
   end
 end

app/graphql/resolvers/base_resolver.rb

@@ -83,5 +83,10 @@ module Resolvers
     def current_user
       context[:current_user]
     end
+
+    # Overridden in sub-classes (see .single, .last)
+    def select_result(results)
+      results
+    end
   end
 end

app/graphql/resolvers/concerns/resolves_merge_requests.rb

@@ -11,16 +11,10 @@ module ResolvesMergeRequests
   end
 
   def resolve_with_lookahead(**args)
-    args[:iids] = Array.wrap(args[:iids]) if args[:iids]
-    args.compact!
+    mr_finder = MergeRequestsFinder.new(current_user, args.compact)
+    finder = Gitlab::Graphql::Loaders::IssuableLoader.new(project, mr_finder)
 
-    if project && args.keys == [:iids]
-      batch_load_merge_requests(args[:iids])
-    else
-      args[:project_id] ||= project
-
-      apply_lookahead(MergeRequestsFinder.new(current_user, args).execute)
-    end.then(&(single? ? :first : :itself))
+    select_result(finder.batching_find_all { |query| apply_lookahead(query) })
   end
 
   def ready?(**args)
@@ -35,22 +29,6 @@ module ResolvesMergeRequests
 
   private
 
-  def batch_load_merge_requests(iids)
-    iids.map { |iid| batch_load(iid) }.select(&:itself) # .compact doesn't work on BatchLoader
-  end
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def batch_load(iid)
-    BatchLoader::GraphQL.for(iid.to_s).batch(key: project) do |iids, loader, args|
-      query = args[:key].merge_requests.where(iid: iids)
-
-      apply_lookahead(query).each do |mr|
-        loader.call(mr.iid.to_s, mr)
-      end
-    end
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
   def unconditional_includes
     [:target_project]
   end

app/graphql/resolvers/issues_resolver.rb

@@ -63,18 +63,13 @@ module Resolvers
       parent = object.respond_to?(:sync) ? object.sync : object
       return Issue.none if parent.nil?
 
-      if parent.is_a?(Group)
-        args[:group_id] = parent.id
-      else
-        args[:project_id] = parent.id
-      end
-
       # Will need to be be made group & namespace aware with
       # https://gitlab.com/gitlab-org/gitlab-foss/issues/54520
-      args[:iids] ||= [args[:iid]].compact
-      args[:attempt_project_search_optimizations] = args[:search].present?
+      args[:iids] ||= [args.delete(:iid)].compact if args[:iid]
+      args[:attempt_project_search_optimizations] = true if args[:search].present?
 
-      issues = IssuesFinder.new(context[:current_user], args).execute
+      finder = IssuesFinder.new(current_user, args)
+      issues = Gitlab::Graphql::Loaders::IssuableLoader.new(parent, finder).batching_find_all
 
       if non_stable_cursor_sort?(args[:sort])
         # Certain complex sorts are not supported by the stable cursor pagination yet.

app/policies/group_policy.rb

@@ -67,6 +67,10 @@ class GroupPolicy < BasePolicy
     enable :update_max_artifacts_size
   end
 
+  rule { can?(:read_all_resources) }.policy do
+    enable :read_confidential_issues
+  end
+
   rule { has_projects }.policy do
     enable :read_group
   end

app/policies/project_policy.rb

@@ -176,6 +176,7 @@ class ProjectPolicy < BasePolicy
   rule { guest | admin }.enable :read_project_for_iids
 
   rule { admin }.enable :update_max_artifacts_size
+  rule { can?(:read_all_resources) }.enable :read_confidential_issues
 
   rule { guest }.enable :guest_access
   rule { reporter }.enable :reporter_access
@@ -257,6 +258,7 @@ class ProjectPolicy < BasePolicy
     enable :read_prometheus
     enable :read_metrics_dashboard_annotation
     enable :metrics_dashboard
+    enable :read_confidential_issues
   end
 
   # We define `:public_user_access` separately because there are cases in gitlab-ee

ee/app/finders/epics_finder.rb

@@ -22,6 +22,7 @@
 
 class EpicsFinder < IssuableFinder
   include TimeFrameFilter
+  include Gitlab::Utils::StrongMemoize
 
   IID_STARTS_WITH_PATTERN = %r{\A(\d)+\z}.freeze
 
@@ -128,10 +129,15 @@ class EpicsFinder < IssuableFinder
   end
 
   def group
-    return unless params[:group_id]
-    return @group if defined?(@group)
+    strong_memoize(:group) do
+      next unless params[:group_id]
 
-    @group = Group.find(params[:group_id])
+      if params[:group_id].is_a?(Group)
+        params[:group_id]
+      else
+        Group.find(params[:group_id])
+      end
+    end
   end
 
   def starts_with_iid(items)

ee/app/graphql/ee/mutations/resolves_issuable.rb

@@ -8,6 +8,12 @@ module EE
 
       private
 
+      def issuable_finder(type, args)
+        return EpicsFinder.new(current_user, args) if type == :epic
+
+        super
+      end
+
       override :resolve_issuable_parent
       def resolve_issuable_parent(type, parent_path)
         return super unless type == :epic

ee/spec/graphql/ee/mutations/concerns/mutations/resolves_issuable_spec.rb

@@ -12,11 +12,12 @@ RSpec.describe Mutations::ResolvesIssuable do
   let_it_be(:group)    { create(:group) }
   let_it_be(:user)     { create(:user) }
   let_it_be(:context)  { { current_user: user } }
-  let_it_be(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
+  let_it_be(:epic) { create(:epic, group: group) }
+  let(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
 
   context 'with epics' do
-    let_it_be(:issuable) { create(:epic, group: group) }
-    let_it_be(:parent)   { issuable.group }
+    let(:parent)   { issuable.group }
+    let(:issuable) { epic }
 
     before do
       stub_licensed_features(epics: true)

lib/gitlab/graphql/authorize/authorize_resource.rb

@@ -30,8 +30,7 @@ module Gitlab
         end
 
         def authorized_find!(*args)
-          object = find_object(*args)
-          object = object.sync if object.respond_to?(:sync)
+          object = Graphql::Lazy.force(find_object(*args))
 
           authorize!(object)
 

lib/gitlab/graphql/lazy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    class Lazy
+      # Force evaluation of a (possibly) lazy value
+      def self.force(value)
+        case value
+        when ::BatchLoader::GraphQL
+          value.sync
+        when ::Concurrent::Promise
+          value.execute.value
+        else
+          value
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/loaders/issuable_loader.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Loaders
+      class IssuableLoader
+        attr_reader :parent, :issuable_finder
+
+        BatchKey = Struct.new(:parent, :finder_class, :current_user)
+
+        def initialize(parent, issuable_finder)
+          @parent = parent
+          @issuable_finder = issuable_finder
+        end
+
+        def batching_find_all(&with_query)
+          if issuable_finder.params.keys == ['iids']
+            batch_load_issuables(issuable_finder.params[:iids], with_query)
+          else
+            post_process(find_all, with_query)
+          end
+        end
+
+        def find_all
+          issuable_finder.params[parent_param] = parent if parent
+
+          issuable_finder.execute
+        end
+
+        private
+
+        def parent_param
+          case parent
+          when Project
+            :project_id
+          when Group
+            :group_id
+          else
+            raise "Unexpected parent: #{parent.class}"
+          end
+        end
+
+        def post_process(query, with_query)
+          if with_query
+            with_query.call(query)
+          else
+            query
+          end
+        end
+
+        def batch_load_issuables(iids, with_query)
+          Array.wrap(iids).map { |iid| batch_load(iid, with_query) }
+        end
+
+        def batch_load(iid, with_query)
+          return if parent.nil?
+
+          BatchLoader::GraphQL
+            .for([parent_param, iid.to_s])
+            .batch(key: batch_key) do |params, loader, args|
+              batch_key = args[:key]
+              user = batch_key.current_user
+
+              params.group_by(&:first).each do |key, group|
+                iids = group.map(&:second).uniq
+                args = { key => batch_key.parent, iids: iids }
+                query = batch_key.finder_class.new(user, args).execute
+
+                post_process(query, with_query).each do |item|
+                  loader.call([key, item.iid.to_s], item)
+                end
+              end
+            end
+        end
+
+        def batch_key
+          BatchKey.new(parent, issuable_finder.class, issuable_finder.current_user)
+        end
+      end
+    end
+  end
+end

spec/graphql/mutations/concerns/mutations/resolves_issuable_spec.rb

@@ -3,26 +3,31 @@
 require 'spec_helper'
 
 RSpec.describe Mutations::ResolvesIssuable do
+  include GraphqlHelpers
+
   let_it_be(:mutation_class) do
     Class.new(Mutations::BaseMutation) do
       include Mutations::ResolvesIssuable
     end
   end
 
-  let_it_be(:project)  { create(:project) }
+  let_it_be(:project)  { create(:project, :empty_repo) }
   let_it_be(:user)     { create(:user) }
   let_it_be(:context)  { { current_user: user } }
-  let_it_be(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
+  let(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
   let(:parent) { issuable.project }
 
+  let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+
   context 'with issues' do
-    let(:issuable) { create(:issue, project: project) }
+    let(:issuable) { issue }
 
     it_behaves_like 'resolving an issuable in GraphQL', :issue
   end
 
   context 'with merge requests' do
-    let(:issuable) { create(:merge_request, source_project: project) }
+    let(:issuable) { merge_request }
 
     it_behaves_like 'resolving an issuable in GraphQL', :merge_request
   end

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -101,12 +101,10 @@ RSpec.describe Resolvers::IssuesResolver do
         end
 
         it 'uses project search optimization' do
-          expected_arguments = {
+          expected_arguments = a_hash_including(
             search: 'foo',
-            attempt_project_search_optimizations: true,
-            iids: [],
-            project_id: project.id
-          }
+            attempt_project_search_optimizations: true
+          )
           expect(IssuesFinder).to receive(:new).with(anything, expected_arguments).and_call_original
 
           resolve_issues(search: 'foo')
@@ -217,16 +215,32 @@ RSpec.describe Resolvers::IssuesResolver do
         expect(resolve_issues).to contain_exactly(issue1, issue2)
       end
 
-      it 'finds a specific issue with iid' do
-        expect(resolve_issues(iid: issue1.iid)).to contain_exactly(issue1)
+      it 'finds a specific issue with iid', :request_store do
+        result = batch_sync(max_queries: 2) { resolve_issues(iid: issue1.iid) }
+
+        expect(result).to contain_exactly(issue1)
+      end
+
+      it 'batches queries that only include IIDs', :request_store do
+        result = batch_sync(max_queries: 2) do
+          resolve_issues(iid: issue1.iid) + resolve_issues(iids: issue2.iid)
+        end
+
+        expect(result).to contain_exactly(issue1, issue2)
+      end
+
+      it 'finds a specific issue with iids', :request_store do
+        result = batch_sync(max_queries: 2) do
+          resolve_issues(iids: [issue1.iid])
         end
 
-      it 'finds a specific issue with iids' do
-        expect(resolve_issues(iids: issue1.iid)).to contain_exactly(issue1)
+        expect(result).to contain_exactly(issue1)
       end
 
       it 'finds multiple issues with iids' do
-        expect(resolve_issues(iids: [issue1.iid, issue2.iid]))
+        create(:issue, project: project, author: current_user)
+
+        expect(batch_sync { resolve_issues(iids: [issue1.iid, issue2.iid]) })
           .to contain_exactly(issue1, issue2)
       end
 
@@ -238,7 +252,7 @@ RSpec.describe Resolvers::IssuesResolver do
           create(:issue, project: another_project, iid: iid)
         end
 
-        expect(resolve_issues(iids: iids)).to contain_exactly(issue1, issue2)
+        expect(batch_sync { resolve_issues(iids: iids) }).to contain_exactly(issue1, issue2)
       end
     end
   end

spec/graphql/resolvers/merge_requests_resolver_spec.rb

@@ -22,9 +22,12 @@ RSpec.describe Resolvers::MergeRequestsResolver do
 
   before do
     project.add_developer(current_user)
+    other_project.add_developer(current_user)
   end
 
   describe '#resolve' do
+    let(:queries_per_project) { 3 }
+
     context 'no arguments' do
       it 'returns all merge requests' do
         result = resolve_mr(project, {})
@@ -40,24 +43,34 @@ RSpec.describe Resolvers::MergeRequestsResolver do
     end
 
     context 'by iid alone' do
-      it 'batch-resolves by target project full path and individual IID' do
-        result = batch_sync(max_queries: 2) do
+      it 'batch-resolves by target project full path and individual IID', :request_store do
+        # 1 query for project_authorizations, and 1 for merge_requests
+        result = batch_sync(max_queries: queries_per_project) do
           [iid_1, iid_2].map { |iid| resolve_mr_single(project, iid) }
         end
 
         expect(result).to contain_exactly(merge_request_1, merge_request_2)
       end
 
-      it 'batch-resolves by target project full path and IIDS' do
-        result = batch_sync(max_queries: 2) do
+      it 'batch-resolves by target project full path and IIDS', :request_store do
+        result = batch_sync(max_queries: queries_per_project) do
           resolve_mr(project, iids: [iid_1, iid_2])
         end
 
         expect(result).to contain_exactly(merge_request_1, merge_request_2)
       end
 
+      it 'batch-resolves by target project full path and IIDS, single or plural', :request_store do
+        result = batch_sync(max_queries: queries_per_project) do
+          [resolve_mr_single(project, merge_request_3.iid), *resolve_mr(project, iids: [iid_1, iid_2])]
+        end
+
+        expect(result).to contain_exactly(merge_request_1, merge_request_2, merge_request_3)
+      end
+
       it 'can batch-resolve merge requests from different projects' do
-        result = batch_sync(max_queries: 3) do
+        # 2 queries for project_authorizations, and 2 for merge_requests
+        result = batch_sync(max_queries: queries_per_project * 2) do
           resolve_mr(project, iids: iid_1) +
             resolve_mr(project, iids: iid_2) +
             resolve_mr(other_project, iids: other_iid)

spec/lib/gitlab/graphql/loaders/issuable_loader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Graphql::Loaders::IssuableLoader do
+  subject { described_class.new(parent, finder) }
+
+  let(:params) { HashWithIndifferentAccess.new }
+
+  describe '#find_all' do
+    let(:finder) { double(:finder, params: params, execute: %i[x y z]) }
+
+    where(:factory, :param_name) do
+      %i[project group].map { |thing| [thing, :"#{thing}_id"] }
+    end
+
+    with_them do
+      let(:parent) { build_stubbed(factory) }
+
+      it 'assignes the parent parameter, and batching_find_alls the finder' do
+        expect(subject.find_all).to contain_exactly(:x, :y, :z)
+        expect(params).to include(param_name => parent)
+      end
+    end
+
+    context 'the parent is of an unexpected type' do
+      let(:parent) { build(:merge_request) }
+
+      it 'raises an error if we pass an unexpected parent' do
+        expect { subject.find_all }.to raise_error(/Unexpected parent/)
+      end
+    end
+  end
+
+  describe '#batching_find_all' do
+    context 'the finder params are anything other than [iids]' do
+      let(:finder) { double(:finder, params: params, execute: [:foo]) }
+      let(:parent) { build_stubbed(:project) }
+
+      it 'batching_find_alls the finder, setting the correct parent parameter' do
+        expect(subject.batching_find_all).to eq([:foo])
+        expect(params[:project_id]).to eq(parent)
+      end
+
+      it 'allows a post-process block' do
+        expect(subject.batching_find_all(&:first)).to eq(:foo)
+      end
+    end
+
+    context 'the finder params are exactly [iids]' do
+      # Dumb finder class, that only implements what we need, and has
+      # predictable query counts.
+      let(:finder_class) do
+        Class.new do
+          attr_reader :current_user, :params
+
+          def initialize(user, args)
+            @current_user = user
+            @params = HashWithIndifferentAccess.new(args.to_h)
+          end
+
+          def execute
+            params[:project_id].issues.where(iid: params[:iids])
+          end
+        end
+      end
+
+      it 'batches requests' do
+        issue_a = create(:issue)
+        issue_b = create(:issue)
+        issue_c = create(:issue, project: issue_a.project)
+        proj_1 = issue_a.project
+        proj_2 = issue_b.project
+        user = create(:user, developer_projects: [proj_1, proj_2])
+
+        finder_a = finder_class.new(user, iids: [issue_a.iid])
+        finder_b = finder_class.new(user, iids: [issue_b.iid])
+        finder_c = finder_class.new(user, iids: [issue_c.iid])
+
+        results = []
+
+        expect do
+          results.concat(described_class.new(proj_1, finder_a).batching_find_all)
+          results.concat(described_class.new(proj_2, finder_b).batching_find_all)
+          results.concat(described_class.new(proj_1, finder_c).batching_find_all)
+        end.not_to exceed_query_limit(0)
+
+        expect do
+          results = results.map(&:sync)
+        end.not_to exceed_query_limit(2)
+
+        expect(results).to contain_exactly(issue_a, issue_b, issue_c)
+      end
+    end
+  end
+end

spec/policies/group_policy_spec.rb

@@ -154,7 +154,7 @@ RSpec.describe GroupPolicy do
   context 'admin' do
     let(:current_user) { admin }
 
-    it do
+    specify do
       expect_allowed(*read_group_permissions)
       expect_allowed(*guest_permissions)
       expect_allowed(*reporter_permissions)
@@ -162,6 +162,10 @@ RSpec.describe GroupPolicy do
       expect_allowed(*maintainer_permissions)
       expect_allowed(*owner_permissions)
     end
+
+    context 'with admin mode', :enable_admin_mode do
+      specify { expect_allowed(*admin_permissions) }
+    end
   end
 
   describe 'private nested group use the highest access level from the group and inherited permissions' do

spec/policies/project_policy_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe ProjectPolicy do
       admin_issue admin_label admin_list read_commit_status read_build
       read_container_image read_pipeline read_environment read_deployment
       read_merge_request download_wiki_code read_sentry_issue read_metrics_dashboard_annotation
-      metrics_dashboard
+      metrics_dashboard read_confidential_issues
     ]
   end
 

spec/support/shared_contexts/policies/group_policy_shared_context.rb

@@ -38,6 +38,7 @@ RSpec.shared_context 'GroupPolicy context' do
       :update_default_branch_protection
     ].compact
   end
+  let(:admin_permissions) { %i[read_confidential_issues] }
 
   before_all do
     group.add_guest(guest)

spec/support/shared_examples/graphql/resolves_issuable_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'resolving an issuable in GraphQL' do |type|
-  subject { mutation.resolve_issuable(type: type, parent_path: parent.full_path, iid: issuable.iid) }
+  include GraphqlHelpers
+
+  let(:parent_path) { parent.full_path }
+  let(:iid) { issuable.iid }
+
+  subject(:result) { mutation.resolve_issuable(type: type, parent_path: parent_path, iid: iid) }
 
   context 'when user has access' do
     before do
@@ -9,37 +14,23 @@ RSpec.shared_examples 'resolving an issuable in GraphQL' do |type|
     end
 
     it 'resolves issuable by iid' do
-      result = type == :merge_request ? subject.sync : subject
       expect(result).to eq(issuable)
     end
 
-    it 'uses the correct Resolver to resolve issuable' do
-      resolver_class = "Resolvers::#{type.to_s.classify.pluralize}Resolver".constantize
-      resolve_method = type == :epic ? :resolve_group : :resolve_project
-      resolved_parent = mutation.send(resolve_method, full_path: parent.full_path)
-
-      allow(mutation).to receive(resolve_method)
-        .with(full_path: parent.full_path)
-        .and_return(resolved_parent)
-
-      expect(resolver_class.single).to receive(:new)
-        .with(object: resolved_parent, context: context, field: nil)
-        .and_call_original
-
-      subject
-    end
-
-    it 'returns nil if issuable is not found' do
-      result = mutation.resolve_issuable(type: type, parent_path: parent.full_path, iid: "100")
-      result = result.respond_to?(:sync) ? result.sync : result
+    context 'the IID does not refer to a valid issuable' do
+      let(:iid) { '100' }
 
+      it 'returns nil' do
         expect(result).to be_nil
       end
+    end
 
-    it 'returns nil if parent path is not present' do
-      result = mutation.resolve_issuable(type: type, parent_path: "", iid: issuable.iid)
+    context 'the parent path is not present' do
+      let(:parent_path) { '' }
 
+      it 'returns nil' do
         expect(result).to be_nil
       end
     end
+  end
 end