diff options
author | James Lopez <james@gitlab.com> | 2019-02-15 08:47:10 +0000 |
---|---|---|
committer | James Lopez <james@gitlab.com> | 2019-02-15 08:47:10 +0000 |
commit | 0328d4faeec093db7744ae5a018174ea4f558a42 (patch) | |
tree | 6d325974f2c6ef94a0bc783f8064a84a71c66bef | |
parent | b95cf314eddf18cd50ad02fb944909ed31c87881 (diff) | |
parent | f80f6bbcdc7f18d9154604602f3750abb0292020 (diff) | |
download | gitlab-ce-0328d4faeec093db7744ae5a018174ea4f558a42.tar.gz |
Merge branch '56485-implement-graphql-mergerequestsresolver' into 'master'
Resolve "Implement GraphQL MergeRequestsResolver"
Closes #56485
See merge request gitlab-org/gitlab-ce!24805
-rw-r--r-- | app/graphql/mutations/merge_requests/base.rb | 3 | ||||
-rw-r--r-- | app/graphql/resolvers/base_resolver.rb | 7 | ||||
-rw-r--r-- | app/graphql/resolvers/issues_resolver.rb | 5 | ||||
-rw-r--r-- | app/graphql/resolvers/merge_requests_resolver.rb (renamed from app/graphql/resolvers/merge_request_resolver.rb) | 21 | ||||
-rw-r--r-- | app/graphql/types/project_type.rb | 14 | ||||
-rw-r--r-- | changelogs/unreleased/56485-implement-graphql-mergerequestsresolver.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/graphql/authorize/instrumentation.rb | 18 | ||||
-rw-r--r-- | spec/graphql/resolvers/base_resolver_spec.rb | 31 | ||||
-rw-r--r-- | spec/graphql/resolvers/issues_resolver_spec.rb | 4 | ||||
-rw-r--r-- | spec/graphql/resolvers/merge_requests_resolver_spec.rb (renamed from spec/graphql/resolvers/merge_request_resolver_spec.rb) | 34 | ||||
-rw-r--r-- | spec/graphql/types/project_type_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb | 67 | ||||
-rw-r--r-- | spec/requests/api/graphql/project/issues_spec.rb | 34 | ||||
-rw-r--r-- | spec/support/helpers/graphql_helpers.rb | 13 |
14 files changed, 240 insertions, 22 deletions
diff --git a/app/graphql/mutations/merge_requests/base.rb b/app/graphql/mutations/merge_requests/base.rb index 54f01c99d78..7d0cb777ad1 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 459933af9d3..063def75d38 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 95e66fb3b7c..fd1b46ba860 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 index d047ce9e3a1..90795c797ac 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 050706f97be..d25c8c8bd90 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 00000000000..5362ac65038 --- /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 d638d2b43ee..2a3d790d67b 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 00000000000..e3a34762b62 --- /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 1a54ab540fc..66de372e9fe 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 index 73993b3a039..ab3c426b2cd 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 01d71abfac9..e8f1c84f8d6 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 00000000000..cf3a8bcc8b4 --- /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 355336ad7e2..c2934430821 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 ea3a03879c5..e468ee4676d 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 |