diff --git a/app/graphql/mutations/merge_requests/base.rb b/app/graphql/mutations/merge_requests/base.rb index 54f01c99d782c2804a99a0d9bc5894bde96133da..7d0cb777ad1c4f8e832776d28420f064d656f6f6 100644 --- a/app/graphql/mutations/merge_requests/base.rb +++ b/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 diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb index 459933af9d379b20e074e0feea70f22365ef24a2..063def75d38a691187f396ec01b447a5b6a3da0d 100644 --- a/app/graphql/resolvers/base_resolver.rb +++ b/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 diff --git a/app/graphql/resolvers/issues_resolver.rb b/app/graphql/resolvers/issues_resolver.rb index 95e66fb3b7c7699482e520ab84ccc6d69f3e23a3..fd1b46ba86056d67ed6424cc4c7b28619a5884ba 100644 --- a/app/graphql/resolvers/issues_resolver.rb +++ b/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 diff --git a/app/graphql/resolvers/merge_request_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb similarity index 50% rename from app/graphql/resolvers/merge_request_resolver.rb rename to app/graphql/resolvers/merge_requests_resolver.rb index d047ce9e3a1ead4c5119560ad66d37d191ae41f7..90795c797ac658e6dec121998d66fbee2dcff011 100644 --- a/app/graphql/resolvers/merge_request_resolver.rb +++ b/app/graphql/resolvers/merge_requests_resolver.rb @@ -1,19 +1,30 @@ # 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) diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index 050706f97bee46a900223ac85b831797c9b2d4bb..d25c8c8bd900b7aa68a6e9200d077893006c01cc 100644 --- a/app/graphql/types/project_type.rb +++ b/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, diff --git a/changelogs/unreleased/56485-implement-graphql-mergerequestsresolver.yml b/changelogs/unreleased/56485-implement-graphql-mergerequestsresolver.yml new file mode 100644 index 0000000000000000000000000000000000000000..5362ac6503811153d25b391ed0c63b10208b02c2 --- /dev/null +++ b/changelogs/unreleased/56485-implement-graphql-mergerequestsresolver.yml @@ -0,0 +1,5 @@ +--- +title: Add field mergeRequests for project in GraphQL +merge_request: 24805 +author: +type: added diff --git a/lib/gitlab/graphql/authorize/instrumentation.rb b/lib/gitlab/graphql/authorize/instrumentation.rb index d638d2b43eebc5ed62266ef9596043309f11bbd2..2a3d790d67bd1cb2b9eb9bbe03f00e4a5bb95685 100644 --- a/lib/gitlab/graphql/authorize/instrumentation.rb +++ b/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 diff --git a/spec/graphql/resolvers/base_resolver_spec.rb b/spec/graphql/resolvers/base_resolver_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e3a34762b622ff646c657adb50f348cbd7d551bf --- /dev/null +++ b/spec/graphql/resolvers/base_resolver_spec.rb @@ -0,0 +1,31 @@ +# 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 diff --git a/spec/graphql/resolvers/issues_resolver_spec.rb b/spec/graphql/resolvers/issues_resolver_spec.rb index 1a54ab540fc4af0458fddebaa8945d31cced2794..66de372e9fe5748993380c1e4a7b871a137c4f04 100644 --- a/spec/graphql/resolvers/issues_resolver_spec.rb +++ b/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 diff --git a/spec/graphql/resolvers/merge_request_resolver_spec.rb b/spec/graphql/resolvers/merge_requests_resolver_spec.rb similarity index 52% rename from spec/graphql/resolvers/merge_request_resolver_spec.rb rename to spec/graphql/resolvers/merge_requests_resolver_spec.rb index 73993b3a0399e83ec7997f08573d9a640c61c494..ab3c426b2cd608b9f2bf60472e37fe0507cf6e9c 100644 --- a/spec/graphql/resolvers/merge_request_resolver_spec.rb +++ b/spec/graphql/resolvers/merge_requests_resolver_spec.rb @@ -1,6 +1,6 @@ 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 diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index 01d71abfac97464597bdc96b8177058ea28dbd31..e8f1c84f8d61895e306598b20f79e1e9afd72af7 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb b/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cf3a8bcc8b4ec3ee54e79480605e29869c28bdb7 --- /dev/null +++ b/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb @@ -0,0 +1,67 @@ +# 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 diff --git a/spec/requests/api/graphql/project/issues_spec.rb b/spec/requests/api/graphql/project/issues_spec.rb index 355336ad7e2dd15690f915b10f8e1565cb6a23e8..c293443082187de9ea2b69356d1326fd07ef7b82 100644 --- a/spec/requests/api/graphql/project/issues_spec.rb +++ b/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 diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index ea3a03879c5426673361fb106e761fd24de4a7f2..e468ee4676d4128b5ec4ab893eb34dc4caca845d 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/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