From 9403b1d951c47aca67d2bac1369b86c125c5119d Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 14 Jun 2018 15:06:53 +0200 Subject: [PATCH] Allow querying a single MR within a project This allows the user to get a single MR nested in a GraphQL project query. Since we need the full path and the iid anyway, this makes more sense than having a root query that needs the full path as well. --- .../resolvers/merge_request_resolver.rb | 11 ++-- app/graphql/types/project_type.rb | 7 +++ app/graphql/types/query_type.rb | 7 --- .../bvl-graphql-nested-merge-request.yml | 5 ++ doc/api/graphql/index.md | 4 +- .../resolvers/merge_request_resolver_spec.rb | 23 ++----- spec/graphql/types/project_type_spec.rb | 9 +++ spec/graphql/types/query_type_spec.rb | 16 +---- .../api/graphql/merge_request_query_spec.rb | 49 --------------- .../api/graphql/project_query_spec.rb | 63 ++++++++++++++++--- spec/support/helpers/graphql_helpers.rb | 15 ++++- spec/support/matchers/graphql_matchers.rb | 6 ++ 12 files changed, 107 insertions(+), 108 deletions(-) create mode 100644 changelogs/unreleased/bvl-graphql-nested-merge-request.yml delete mode 100644 spec/requests/api/graphql/merge_request_query_spec.rb diff --git a/app/graphql/resolvers/merge_request_resolver.rb b/app/graphql/resolvers/merge_request_resolver.rb index 93d48e2645c..9f2d348e95f 100644 --- a/app/graphql/resolvers/merge_request_resolver.rb +++ b/app/graphql/resolvers/merge_request_resolver.rb @@ -1,15 +1,14 @@ module Resolvers class MergeRequestResolver < BaseResolver - prepend FullPathResolver - - type Types::MergeRequestType, null: true - argument :iid, GraphQL::ID_TYPE, required: true, description: 'The IID of the merge request, e.g., "1"' - def resolve(full_path:, iid:) - project = model_by_full_path(Project, full_path) + type Types::MergeRequestType, null: true + + alias_method :project, :object + + def resolve(iid:) return unless project.present? BatchLoader.for(iid.to_s).batch(key: project.id) do |iids, loader| diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index 9e885d5845a..d9058ae7431 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -61,5 +61,12 @@ module Types field :request_access_enabled, GraphQL::BOOLEAN_TYPE, null: true 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_request, + Types::MergeRequestType, + null: true, + resolver: Resolvers::MergeRequestResolver do + authorize :read_merge_request + end end end diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index be79c78bf67..010ec2d7942 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -9,13 +9,6 @@ module Types authorize :read_project end - field :merge_request, Types::MergeRequestType, - null: true, - resolver: Resolvers::MergeRequestResolver, - description: "Find a merge request" do - authorize :read_merge_request - end - field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new end end diff --git a/changelogs/unreleased/bvl-graphql-nested-merge-request.yml b/changelogs/unreleased/bvl-graphql-nested-merge-request.yml new file mode 100644 index 00000000000..f0f0488d31a --- /dev/null +++ b/changelogs/unreleased/bvl-graphql-nested-merge-request.yml @@ -0,0 +1,5 @@ +--- +title: Allow querying a single merge request within a project +merge_request: 19853 +author: +type: changed diff --git a/doc/api/graphql/index.md b/doc/api/graphql/index.md index dcd5377284c..59e27922f64 100644 --- a/doc/api/graphql/index.md +++ b/doc/api/graphql/index.md @@ -29,9 +29,7 @@ curl --data "value=100" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://g ## Available queries -A first iteration of a GraphQL API includes only 2 queries: `project` and -`merge_request` and only returns scalar fields, or fields of the type `Project` -or `MergeRequest`. +A first iteration of a GraphQL API includes a query for: `project`. Within a project it is also possible to fetch a `mergeRequest` by IID. ## GraphiQL diff --git a/spec/graphql/resolvers/merge_request_resolver_spec.rb b/spec/graphql/resolvers/merge_request_resolver_spec.rb index af015533209..73993b3a039 100644 --- a/spec/graphql/resolvers/merge_request_resolver_spec.rb +++ b/spec/graphql/resolvers/merge_request_resolver_spec.rb @@ -10,49 +10,36 @@ describe Resolvers::MergeRequestResolver do set(:other_project) { create(:project, :repository) } set(:other_merge_request) { create(:merge_request, source_project: other_project, target_project: other_project) } - let(:full_path) { project.full_path } let(:iid_1) { merge_request_1.iid } let(:iid_2) { merge_request_2.iid } - let(:other_full_path) { other_project.full_path } let(:other_iid) { other_merge_request.iid } describe '#resolve' do it 'batch-resolves merge requests by target project full path and IID' do - path = full_path # avoid database query - result = batch(max_queries: 2) do - [resolve_mr(path, iid_1), resolve_mr(path, iid_2)] + [resolve_mr(project, iid_1), resolve_mr(project, iid_2)] end expect(result).to contain_exactly(merge_request_1, merge_request_2) end it 'can batch-resolve merge requests from different projects' do - path = project.full_path # avoid database queries - other_path = other_full_path - result = batch(max_queries: 3) do - [resolve_mr(path, iid_1), resolve_mr(path, iid_2), resolve_mr(other_path, other_iid)] + [resolve_mr(project, iid_1), resolve_mr(project, iid_2), resolve_mr(other_project, 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(full_path, -1) } - - expect(result).to be_nil - end - - it 'resolves a known iid for an unknown full_path to nil' do - result = batch { resolve_mr('unknown/project', iid_1) } + result = batch { resolve_mr(project, -1) } expect(result).to be_nil end end - def resolve_mr(full_path, iid) - resolve(described_class, args: { full_path: full_path, iid: iid }) + def resolve_mr(project, iid) + resolve(described_class, obj: project, args: { iid: iid }) end end diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index e0f89105b86..b4eeca2e3f1 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/spec/graphql/types/project_type_spec.rb @@ -2,4 +2,13 @@ require 'spec_helper' 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_request) } + + it 'authorizes the merge request' do + expect(described_class.fields['mergeRequest']) + .to require_graphql_authorizations(:read_merge_request) + end + end end diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb index 8488252fd59..e1df6f9811d 100644 --- a/spec/graphql/types/query_type_spec.rb +++ b/spec/graphql/types/query_type_spec.rb @@ -5,7 +5,7 @@ describe GitlabSchema.types['Query'] do expect(described_class.graphql_name).to eq('Query') end - it { is_expected.to have_graphql_fields(:project, :merge_request, :echo) } + it { is_expected.to have_graphql_fields(:project, :echo) } describe 'project field' do subject { described_class.fields['project'] } @@ -20,18 +20,4 @@ describe GitlabSchema.types['Query'] do is_expected.to require_graphql_authorizations(:read_project) end end - - describe 'merge_request field' do - subject { described_class.fields['mergeRequest'] } - - it 'finds MRs by project and IID' do - is_expected.to have_graphql_arguments(:full_path, :iid) - is_expected.to have_graphql_type(Types::MergeRequestType) - is_expected.to have_graphql_resolver(Resolvers::MergeRequestResolver) - end - - it 'authorizes with read_merge_request' do - is_expected.to require_graphql_authorizations(:read_merge_request) - end - end end diff --git a/spec/requests/api/graphql/merge_request_query_spec.rb b/spec/requests/api/graphql/merge_request_query_spec.rb deleted file mode 100644 index 12b1d5d18a2..00000000000 --- a/spec/requests/api/graphql/merge_request_query_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -require 'spec_helper' - -describe 'getting merge request information' do - include GraphqlHelpers - - let(:project) { create(:project, :repository) } - let(:merge_request) { create(:merge_request, source_project: project) } - let(:current_user) { create(:user) } - - let(:query) do - attributes = { - 'fullPath' => merge_request.project.full_path, - 'iid' => merge_request.iid - } - graphql_query_for('mergeRequest', attributes) - end - - context 'when the user has access to the merge request' do - before do - project.add_developer(current_user) - post_graphql(query, current_user: current_user) - end - - it 'returns the merge request' do - expect(graphql_data['mergeRequest']).not_to be_nil - end - - # This is a field coming from the `MergeRequestPresenter` - it 'includes a web_url' do - expect(graphql_data['mergeRequest']['webUrl']).to be_present - end - - it_behaves_like 'a working graphql query' - end - - context 'when the user does not have access to the merge request' do - before do - post_graphql(query, current_user: current_user) - end - - it 'returns an empty field' do - post_graphql(query, current_user: current_user) - - expect(graphql_data['mergeRequest']).to be_nil - end - - it_behaves_like 'a working graphql query' - end -end diff --git a/spec/requests/api/graphql/project_query_spec.rb b/spec/requests/api/graphql/project_query_spec.rb index 8196bcfa87c..796ffc9d569 100644 --- a/spec/requests/api/graphql/project_query_spec.rb +++ b/spec/requests/api/graphql/project_query_spec.rb @@ -13,27 +13,76 @@ describe 'getting project information' do context 'when the user has access to the project' do before do project.add_developer(current_user) - post_graphql(query, current_user: current_user) end it 'includes the project' do + post_graphql(query, current_user: current_user) + expect(graphql_data['project']).not_to be_nil end - it_behaves_like 'a working graphql query' + it_behaves_like 'a working graphql query' do + before do + post_graphql(query, current_user: current_user) + end + end + + context 'when requesting a nested merge request' do + let(:merge_request) { create(:merge_request, source_project: project) } + let(:merge_request_graphql_data) { graphql_data['project']['mergeRequest'] } + + let(:query) do + graphql_query_for( + 'project', + { 'fullPath' => project.full_path }, + query_graphql_field('mergeRequest', iid: merge_request.iid) + ) + end + + it_behaves_like 'a working graphql query' do + before do + post_graphql(query, current_user: current_user) + end + end + + it 'contains merge request information' do + post_graphql(query, current_user: current_user) + + expect(merge_request_graphql_data).not_to be_nil + end + + # This is a field coming from the `MergeRequestPresenter` + it 'includes a web_url' do + post_graphql(query, current_user: current_user) + + expect(merge_request_graphql_data['webUrl']).to be_present + end + + context 'when the user does not have access to the merge request' do + let(:project) { create(:project, :public, :repository) } + + it 'returns nil' do + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + + post_graphql(query) + + expect(merge_request_graphql_data).to be_nil + end + end + end end context 'when the user does not have access to the project' do - before do - post_graphql(query, current_user: current_user) - end - it 'returns an empty field' do post_graphql(query, current_user: current_user) expect(graphql_data['project']).to be_nil end - it_behaves_like 'a working graphql query' + it_behaves_like 'a working graphql query' do + before do + post_graphql(query, current_user: current_user) + end + end end end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 30ff9a1196a..0930b9da368 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -34,14 +34,20 @@ module GraphqlHelpers end def graphql_query_for(name, attributes = {}, fields = nil) + <<~QUERY + { + #{query_graphql_field(name, attributes, fields)} + } + QUERY + end + + def query_graphql_field(name, attributes = {}, fields = nil) fields ||= all_graphql_fields_for(name.classify) attributes = attributes_to_graphql(attributes) <<~QUERY - { #{name}(#{attributes}) { #{fields} } - } QUERY end @@ -50,12 +56,15 @@ module GraphqlHelpers return "" unless type type.fields.map do |name, field| + # We can't guess arguments, so skip fields that require them + next if field.arguments.any? + if scalar?(field) name else "#{name} { #{all_graphql_fields_for(field_type(field))} }" end - end.join("\n") + end.compact.join("\n") end def attributes_to_graphql(attributes) diff --git a/spec/support/matchers/graphql_matchers.rb b/spec/support/matchers/graphql_matchers.rb index ba7a1c8cde0..d23cbaf4beb 100644 --- a/spec/support/matchers/graphql_matchers.rb +++ b/spec/support/matchers/graphql_matchers.rb @@ -13,6 +13,12 @@ RSpec::Matchers.define :have_graphql_fields do |*expected| end end +RSpec::Matchers.define :have_graphql_field do |field_name| + match do |kls| + expect(kls.fields.keys).to include(GraphqlHelpers.fieldnamerize(field_name)) + end +end + RSpec::Matchers.define :have_graphql_arguments do |*expected| include GraphqlHelpers