Commit b735a2c7 authored by Adam Hegyi's avatar Adam Hegyi

Expose fields in GraphQL for MR analytics

- Avoid N+1, preload `metrics`
- Exposes `commit_count`
- Exposes `approvers`
- Exposes pipeline count
parent 60659e69
......@@ -38,6 +38,9 @@ module ResolvesMergeRequests
assignees: [:assignees],
labels: [:labels],
author: [:author],
merged_at: [:metrics],
commit_count: [:metrics],
approved_by: [:approver_users],
milestone: [:milestone],
head_pipeline: [:merge_request_diff, { head_pipeline: [:merge_request] }]
}
......
......@@ -5,6 +5,8 @@ module Types
class PipelineType < BaseObject
graphql_name 'Pipeline'
connection_type_class(Types::CountableConnectionType)
authorize :read_pipeline
expose_permissions Types::PermissionTypes::Ci::Pipeline
......
......@@ -2,13 +2,14 @@
module Types
# rubocop: disable Graphql/AuthorizeTypes
class IssuableConnectionType < GraphQL::Types::Relay::BaseConnection
class CountableConnectionType < GraphQL::Types::Relay::BaseConnection
field :count, Integer, null: false,
description: 'Total count of collection'
def count
# rubocop: disable CodeReuse/ActiveRecord
relation = object.items
# sometimes relation is an Array
relation = relation.reorder(nil) if relation.respond_to?(:reorder)
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -4,7 +4,7 @@ module Types
class IssueType < BaseObject
graphql_name 'Issue'
connection_type_class(Types::IssuableConnectionType)
connection_type_class(Types::CountableConnectionType)
implements(Types::Notes::NoteableType)
......
......@@ -4,7 +4,7 @@ module Types
class MergeRequestType < BaseObject
graphql_name 'MergeRequest'
connection_type_class(Types::IssuableConnectionType)
connection_type_class(Types::CountableConnectionType)
implements(Types::Notes::NoteableType)
......@@ -143,6 +143,8 @@ module Types
end
field :task_completion_status, Types::TaskCompletionStatus, null: false,
description: Types::TaskCompletionStatus.description
field :commit_count, GraphQL::INT_TYPE, null: true,
description: 'Number of commits in the merge request'
def diff_stats(path: nil)
stats = Array.wrap(object.diff_stats&.to_a)
......@@ -162,5 +164,14 @@ module Types
hash.merge!(additions: status.additions, deletions: status.deletions, file_count: 1) { |_, x, y| x + y }
end
end
def commit_count
object&.metrics&.commits_count
end
def approvers
object.approver_users
end
end
end
Types::MergeRequestType.prepend_if_ee('::EE::Types::MergeRequestType')
---
title: Expose counts (pipeline, commits) and approvers for a merge request in GraphQL
merge_request: 39086
author:
type: added
......@@ -8240,6 +8240,31 @@ type MergeRequest implements Noteable {
"""
allowCollaboration: Boolean
"""
Users who approved the merge request
"""
approvedBy(
"""
Returns the elements in the list that come after the specified cursor.
"""
after: String
"""
Returns the elements in the list that come before the specified cursor.
"""
before: String
"""
Returns the first _n_ elements from the list.
"""
first: Int
"""
Returns the last _n_ elements from the list.
"""
last: Int
): UserConnection
"""
Assignees of the merge request
"""
......@@ -8270,6 +8295,11 @@ type MergeRequest implements Noteable {
"""
author: User
"""
Number of commits in the merge request
"""
commitCount: Int
"""
Timestamp of when the merge request was created
"""
......@@ -10249,6 +10279,11 @@ type Pipeline {
The connection type for Pipeline.
"""
type PipelineConnection {
"""
Total count of collection
"""
count: Int!
"""
A list of edges.
"""
......
......@@ -22922,6 +22922,59 @@
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "approvedBy",
"description": "Users who approved the merge request",
"args": [
{
"name": "after",
"description": "Returns the elements in the list that come after the specified cursor.",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": null
},
{
"name": "before",
"description": "Returns the elements in the list that come before the specified cursor.",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": null
},
{
"name": "first",
"description": "Returns the first _n_ elements from the list.",
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"defaultValue": null
},
{
"name": "last",
"description": "Returns the last _n_ elements from the list.",
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"defaultValue": null
}
],
"type": {
"kind": "OBJECT",
"name": "UserConnection",
"ofType": null
},
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "assignees",
"description": "Assignees of the merge request",
......@@ -22989,6 +23042,20 @@
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "commitCount",
"description": "Number of commits in the merge request",
"args": [
],
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "createdAt",
"description": "Timestamp of when the merge request was created",
......@@ -30665,6 +30732,24 @@
"name": "PipelineConnection",
"description": "The connection type for Pipeline.",
"fields": [
{
"name": "count",
"description": "Total count of collection",
"args": [
],
"type": {
"kind": "NON_NULL",
"name": null,
"ofType": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
}
},
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "edges",
"description": "A list of edges.",
......@@ -1257,6 +1257,7 @@ Autogenerated return type of MarkAsSpamSnippet
| --- | ---- | ---------- |
| `allowCollaboration` | Boolean | Indicates if members of the target project can push to the fork |
| `author` | User | User who created this merge request |
| `commitCount` | Int | Number of commits in the merge request |
| `createdAt` | Time! | Timestamp of when the merge request was created |
| `defaultMergeCommitMessage` | String | Default merge commit message of the merge request |
| `description` | String | Description of the merge request (Markdown rendered as HTML for caching) |
......
# frozen_string_literal: true
module EE
module Types
module MergeRequestType
extend ActiveSupport::Concern
prepended do
field :approved_by, ::Types::UserType.connection_type, null: true,
description: 'Users who approved the merge request'
def approved_by
object.approver_users
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe 'getting merge request listings (EE) nested in a project' do
include GraphqlHelpers
let_it_be(:project) { create(:project, :repository, :public) }
let_it_be(:current_user) { create(:user) }
let_it_be(:merge_request_a) { create(:merge_request, :unique_branches, source_project: project) }
let_it_be(:merge_request_b) { create(:merge_request, :closed, :unique_branches, source_project: project) }
let_it_be(:merge_request_c) { create(:merge_request, :closed, :unique_branches, source_project: project) }
let(:results) { graphql_data.dig('project', 'mergeRequests', 'nodes') }
let(:query) do
query_merge_requests(all_graphql_fields_for('MergeRequest', max_depth: 1))
end
def query_merge_requests(fields)
graphql_query_for(
:project,
{ full_path: project.full_path },
query_graphql_field(:merge_requests, search_params, [
query_graphql_field(:nodes, nil, fields)
])
)
end
def execute_query
query = query_merge_requests(requested_fields)
post_graphql(query, current_user: current_user)
end
context 'when requesting `approved_by`' do
let(:search_params) { { iids: [merge_request_a.iid.to_s, merge_request_b.iid.to_s] } }
let(:extra_iid_for_second_query) { merge_request_c.iid.to_s }
let(:requested_fields) { query_graphql_field(:approved_by, nil, query_graphql_field(:nodes, nil, [:username])) }
it 'exposes approver username' do
merge_request_a.approver_users << current_user
execute_query
user_data = { 'username' => current_user.username }
expect(results).to include(a_hash_including('approvedBy' => { 'nodes' => array_including(user_data) }))
end
include_examples 'N+1 query check'
end
end
......@@ -24,9 +24,11 @@ RSpec.describe GitlabSchema.types['MergeRequest'] do
source_branch_exists target_branch_exists
upvotes downvotes head_pipeline pipelines task_completion_status
milestone assignees participants subscribed labels discussion_locked time_estimate
total_time_spent reference author merged_at
total_time_spent reference author merged_at commit_count
]
expected_fields << 'approved_by' if Gitlab.ee?
expect(described_class).to have_graphql_fields(*expected_fields)
end
end
......@@ -171,4 +171,43 @@ RSpec.describe 'getting merge request listings nested in a project' do
it_behaves_like 'searching with parameters'
end
describe 'fields' do
let(:requested_fields) { nil }
let(:extra_iid_for_second_query) { merge_request_c.iid.to_s }
let(:search_params) { { iids: [merge_request_a.iid.to_s, merge_request_b.iid.to_s] } }
def execute_query
query = query_merge_requests(requested_fields)
post_graphql(query, current_user: current_user)
end
context 'when requesting `commit_count`' do
let(:requested_fields) { [:commit_count] }
it 'exposes `commit_count`' do
merge_request_a.metrics.update!(commits_count: 5)
execute_query
expect(results).to include(a_hash_including('commitCount' => 5))
end
include_examples 'N+1 query check'
end
context 'when requesting `merged_at`' do
let(:requested_fields) { [:merged_at] }
before do
# make the MRs "merged"
[merge_request_a, merge_request_b, merge_request_c].each do |mr|
mr.update_column(:state_id, MergeRequest.available_states[:merged])
mr.metrics.update_column(:merged_at, Time.now)
end
end
include_examples 'N+1 query check'
end
end
end
# frozen_string_literal: true
shared_examples 'N+1 query check' do
it 'prevents N+1 queries' do
execute_query # "warm up" to prevent undeterministic counts
control_count = ActiveRecord::QueryRecorder.new { execute_query }.count
search_params[:iids] << extra_iid_for_second_query
expect { execute_query }.not_to exceed_query_limit(control_count)
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment