Automatic merge of gitlab-org/gitlab-ce master

app/graphql/mutations/merge_requests/base.rb

@@ -25,7 +25,8 @@ module Mutations
 
       def find_object(project_path:, iid:)
         project = resolve_project(full_path: project_path)
-        resolver = Resolvers::MergeRequestResolver.new(object: project, context: context)
+        resolver = Resolvers::MergeRequestsResolver
+          .single.new(object: project, context: context)
 
         resolver.resolve(iid: iid)
       end

app/graphql/resolvers/base_resolver.rb

@@ -2,5 +2,12 @@
 
 module Resolvers
   class BaseResolver < GraphQL::Schema::Resolver
+    def self.single
+      @single ||= Class.new(self) do
+        def resolve(**args)
+          super.first
+        end
+      end
+    end
   end
 end

app/graphql/resolvers/issues_resolver.rb

@@ -2,7 +2,9 @@
 
 module Resolvers
   class IssuesResolver < BaseResolver
-    extend ActiveSupport::Concern
+    argument :iid, GraphQL::ID_TYPE,
+              required: false,
+              description: 'The IID of the issue, e.g., "1"'
 
     argument :iids, [GraphQL::ID_TYPE],
               required: false,
@@ -22,6 +24,7 @@ module Resolvers
       # Will need to be be made group & namespace aware with
       # https://gitlab.com/gitlab-org/gitlab-ce/issues/54520
       args[:project_id] = project.id
+      args[:iids] ||= [args[:iid]].compact
 
       IssuesFinder.new(context[:current_user], args).execute
     end

app/graphql/resolvers/merge_request_resolver.rb → app/graphql/resolvers/merge_requests_resolver.rb

 # frozen_string_literal: true
 
 module Resolvers
-  class MergeRequestResolver < BaseResolver
+  class MergeRequestsResolver < BaseResolver
     argument :iid, GraphQL::ID_TYPE,
-             required: true,
-             description: 'The IID of the merge request, e.g., "1"'
+              required: false,
+              description: 'The IID of the merge request, e.g., "1"'
+
+    argument :iids, [GraphQL::ID_TYPE],
+              required: false,
+              description: 'The list of IIDs of issues, e.g., [1, 2]'
 
     type Types::MergeRequestType, null: true
 
     alias_method :project, :object
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def resolve(iid:)
+    def resolve(**args)
       return unless project.present?
 
+      args[:iids] ||= [args[:iid]].compact
+
+      args[:iids].map { |iid| batch_load(iid) }
+        .select(&:itself) # .compact doesn't work on BatchLoader
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def batch_load(iid)
       BatchLoader.for(iid.to_s).batch(key: project) do |iids, loader, args|
         args[:key].merge_requests.where(iid: iids).each do |mr|
           loader.call(mr.iid.to_s, mr)

app/graphql/types/project_type.rb

@@ -66,10 +66,17 @@ module Types
     field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true
     field :printing_merge_request_link_enabled, GraphQL::BOOLEAN_TYPE, null: true
 
+    field :merge_requests,
+          Types::MergeRequestType.connection_type,
+          null: true,
+          resolver: Resolvers::MergeRequestsResolver do
+      authorize :read_merge_request
+    end
+
     field :merge_request,
           Types::MergeRequestType,
           null: true,
-          resolver: Resolvers::MergeRequestResolver do
+          resolver: Resolvers::MergeRequestsResolver.single do
       authorize :read_merge_request
     end
 
@@ -78,6 +85,11 @@ module Types
           null: true,
           resolver: Resolvers::IssuesResolver
 
+    field :issue,
+          Types::IssueType,
+          null: true,
+          resolver: Resolvers::IssuesResolver.single
+
     field :pipelines,
           Types::Ci::PipelineType.connection_type,
           null: false,

changelogs/unreleased/56485-implement-graphql-mergerequestsresolver.yml

+---
+title: Add field mergeRequests for project in GraphQL
+merge_request: 24805
+author:
+type: added

lib/gitlab/graphql/authorize/instrumentation.rb

@@ -35,10 +35,22 @@ module Gitlab
         private
 
         def build_checker(current_user, abilities)
-          proc do |obj|
+          lambda do |value|
             # 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) }
+            value = value.sync if value.respond_to?(:sync)
+
+            check = lambda do |object|
+              abilities.all? do |ability|
+                Ability.allowed?(current_user, ability, object)
+              end
+            end
+
+            case value
+            when Array
+              value.select(&check)
+            else
+              value if check.call(value)
+            end
           end
         end
       end

spec/graphql/resolvers/base_resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Resolvers::BaseResolver do
+  include GraphqlHelpers
+
+  let(:resolver) do
+    Class.new(described_class) do
+      def resolve(**args)
+        [args, args]
+      end
+    end
+  end
+
+  describe '.single' do
+    it 'returns a subclass from the resolver' do
+      expect(resolver.single.superclass).to eq(resolver)
+    end
+
+    it 'returns the same subclass every time' do
+      expect(resolver.single.object_id).to eq(resolver.single.object_id)
+    end
+
+    it 'returns a resolver that gives the first result from the original resolver' do
+      result = resolve(resolver.single, args: { test: 1 })
+
+      expect(result).to eq(test: 1)
+    end
+  end
+end

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -33,6 +33,10 @@ describe Resolvers::IssuesResolver do
       expect(resolve_issues).to contain_exactly(issue, issue2)
     end
 
+    it 'finds a specific issue with iid' do
+      expect(resolve_issues(iid: issue.iid)).to contain_exactly(issue)
+    end
+
     it 'finds a specific issue with iids' do
       expect(resolve_issues(iids: issue.iid)).to contain_exactly(issue)
     end

spec/graphql/resolvers/merge_request_resolver_spec.rb → spec/graphql/resolvers/merge_requests_resolver_spec.rb

 require 'spec_helper'
 
-describe Resolvers::MergeRequestResolver do
+describe Resolvers::MergeRequestsResolver do
   include GraphqlHelpers
 
   set(:project) { create(:project, :repository) }
@@ -16,9 +16,17 @@ describe Resolvers::MergeRequestResolver do
   let(:other_iid) { other_merge_request.iid }
 
   describe '#resolve' do
-    it 'batch-resolves merge requests by target project full path and IID' do
+    it 'batch-resolves by target project full path and individual IID' do
       result = batch(max_queries: 2) do
-        [resolve_mr(project, iid_1), resolve_mr(project, iid_2)]
+        resolve_mr(project, iid: iid_1) + resolve_mr(project, iid: 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' do
+      result = batch(max_queries: 2) do
+        resolve_mr(project, iids: [iid_1, iid_2])
       end
 
       expect(result).to contain_exactly(merge_request_1, merge_request_2)
@@ -26,20 +34,28 @@ describe Resolvers::MergeRequestResolver do
 
     it 'can batch-resolve merge requests from different projects' do
       result = batch(max_queries: 3) do
-        [resolve_mr(project, iid_1), resolve_mr(project, iid_2), resolve_mr(other_project, other_iid)]
+        resolve_mr(project, iid: iid_1) +
+          resolve_mr(project, iid: iid_2) +
+          resolve_mr(other_project, iid: other_iid)
       end
 
       expect(result).to contain_exactly(merge_request_1, merge_request_2, other_merge_request)
     end
 
-    it 'resolves an unknown iid to nil' do
-      result = batch { resolve_mr(project, -1) }
+    it 'resolves an unknown iid to be empty' do
+      result = batch { resolve_mr(project, iid: -1) }
+
+      expect(result).to be_empty
+    end
+
+    it 'resolves empty iids to be empty' do
+      result = batch { resolve_mr(project, iids: []) }
 
-      expect(result).to be_nil
+      expect(result).to be_empty
     end
   end
 
-  def resolve_mr(project, iid)
-    resolve(described_class, obj: project, args: { iid: iid })
+  def resolve_mr(project, args)
+    resolve(described_class, obj: project, args: args)
   end
 end

spec/graphql/types/project_type_spec.rb

@@ -6,12 +6,18 @@ describe GitlabSchema.types['Project'] do
   it { expect(described_class.graphql_name).to eq('Project') }
 
   describe 'nested merge request' do
+    it { expect(described_class).to have_graphql_field(:merge_requests) }
     it { expect(described_class).to have_graphql_field(:merge_request) }
 
     it 'authorizes the merge request' do
       expect(described_class.fields['mergeRequest'])
         .to require_graphql_authorizations(:read_merge_request)
     end
+
+    it 'authorizes the merge requests' do
+      expect(described_class.fields['mergeRequests'])
+        .to require_graphql_authorizations(:read_merge_request)
+    end
   end
 
   describe 'nested issues' do

spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Authorize::Instrumentation do
+  describe '#build_checker' do
+    let(:current_user) { double(:current_user) }
+    let(:abilities) { [double(:first_ability), double(:last_ability)] }
+
+    let(:checker) do
+      described_class.new.__send__(:build_checker, current_user, abilities)
+    end
+
+    it 'returns a checker which checks for a single object' do
+      object = double(:object)
+
+      abilities.each do |ability|
+        spy_ability_check_for(ability, object, passed: true)
+      end
+
+      expect(checker.call(object)).to eq(object)
+    end
+
+    it 'returns a checker which checks for all objects' do
+      objects = [double(:first), double(:last)]
+
+      abilities.each do |ability|
+        objects.each do |object|
+          spy_ability_check_for(ability, object, passed: true)
+        end
+      end
+
+      expect(checker.call(objects)).to eq(objects)
+    end
+
+    context 'when some objects would not pass the check' do
+      it 'returns nil when it is single object' do
+        disallowed = double(:object)
+
+        spy_ability_check_for(abilities.first, disallowed, passed: false)
+
+        expect(checker.call(disallowed)).to be_nil
+      end
+
+      it 'returns only objects which passed when there are more than one' do
+        allowed = double(:allowed)
+        disallowed = double(:disallowed)
+
+        spy_ability_check_for(abilities.first, disallowed, passed: false)
+
+        abilities.each do |ability|
+          spy_ability_check_for(ability, allowed, passed: true)
+        end
+
+        expect(checker.call([disallowed, allowed]))
+          .to contain_exactly(allowed)
+      end
+    end
+
+    def spy_ability_check_for(ability, object, passed: true)
+      expect(Ability)
+        .to receive(:allowed?)
+        .with(current_user, ability, object)
+        .and_return(passed)
+    end
+  end
+end

spec/requests/api/graphql/project/issues_spec.rb

@@ -56,4 +56,38 @@ describe 'getting an issue list for a project' do
       expect(issues_data).to eq []
     end
   end
+
+  context 'when there is a confidential issue' do
+    let!(:confidential_issue) do
+      create(:issue, :confidential, project: project)
+    end
+
+    context 'when the user cannot see confidential issues' do
+      it 'returns issues without confidential issues' do
+        post_graphql(query, current_user: current_user)
+
+        expect(issues_data.size).to eq(2)
+
+        issues_data.each do |issue|
+          expect(issue.dig('node', 'confidential')).to eq(false)
+        end
+      end
+    end
+
+    context 'when the user can see confidential issues' do
+      it 'returns issues with confidential issues' do
+        project.add_developer(current_user)
+
+        post_graphql(query, current_user: current_user)
+
+        expect(issues_data.size).to eq(3)
+
+        confidentials = issues_data.map do |issue|
+          issue.dig('node', 'confidential')
+        end
+
+        expect(confidentials).to eq([true, false, false])
+      end
+    end
+  end
 end

spec/support/helpers/graphql_helpers.rb

@@ -84,7 +84,7 @@ module GraphqlHelpers
     QUERY
   end
 
-  def all_graphql_fields_for(class_name)
+  def all_graphql_fields_for(class_name, parent_types = Set.new)
     type = GitlabSchema.types[class_name.to_s]
     return "" unless type
 
@@ -92,8 +92,17 @@ module GraphqlHelpers
       # We can't guess arguments, so skip fields that require them
       next if required_arguments?(field)
 
+      singular_field_type = field_type(field)
+
+      # If field type is the same as parent type, then we're hitting into
+      # mutual dependency. Break it from infinite recursion
+      next if parent_types.include?(singular_field_type)
+
       if nested_fields?(field)
-        "#{name} { #{all_graphql_fields_for(field_type(field))} }"
+        fields =
+          all_graphql_fields_for(singular_field_type, parent_types | [type])
+
+        "#{name} { #{fields} }"
       else
         name
       end