Add latest changes from gitlab-org/gitlab@13-8-stable-eev13.8.0-rc42

6 files changed, 276 insertions, 53 deletions

diff --git a/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb b/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb
index dd001a73349..9ab94f1d749 100644
--- a/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb
+++ b/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb
@@ -60,19 +60,35 @@ RSpec.describe 'getting Alert Management Alert Assignees' do
     expect(second_assignees).to be_empty
   end
 
-  it 'avoids N+1 queries' do
-    base_count = ActiveRecord::QueryRecorder.new do
-      post_graphql(query, current_user: current_user)
+  describe 'performance' do
+    let(:first_n) { var('Int') }
+    let(:params) { { first: first_n } }
+    let(:limited_query) { with_signature([first_n], query) }
+
+    before do
+      create(:alert_management_alert, project: project, assignees: [current_user])
+    end
+
+    it 'can limit results' do
+      post_graphql(limited_query, current_user: current_user, variables: first_n.with(1))
+
+      expect(alerts.size).to eq 1
+      expect(alerts).not_to include(a_hash_including('iid' => first_alert.iid.to_s))
     end
 
-    # An N+1 would mean a new alert would increase the query count
-    third_alert = create(:alert_management_alert, project: project, assignees: [current_user])
+    it 'can include all results' do
+      post_graphql(limited_query, current_user: current_user)
 
-    expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(base_count)
+      expect(alerts.size).to be > 1
+      expect(alerts).to include(a_hash_including('iid' => first_alert.iid.to_s))
+    end
 
-    third_assignees = assignees[third_alert.iid.to_s]
+    it 'avoids N+1 queries' do
+      base_count = ActiveRecord::QueryRecorder.new do
+        post_graphql(limited_query, current_user: current_user, variables: first_n.with(1))
+      end
 
-    expect(third_assignees.length).to eq(1)
-    expect(third_assignees.first).to include('username' => current_user.username)
+      expect { post_graphql(limited_query, current_user: current_user) }.not_to exceed_query_limit(base_count)
+    end
   end
 end
diff --git a/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb b/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb
index 1350cba119b..5d46f370756 100644
--- a/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb
+++ b/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb
@@ -65,16 +65,28 @@ RSpec.describe 'getting Alert Management Alert Notes' do
     expect(second_notes_result).to be_empty
   end
 
-  it 'avoids N+1 queries' do
-    base_count = ActiveRecord::QueryRecorder.new do
-      post_graphql(query, current_user: current_user)
+  describe 'performance' do
+    let(:first_n) { var('Int') }
+    let(:params) { { first: first_n } }
+
+    before do
+      # An N+1 would mean a new alert would increase the query count
+      create(:alert_management_alert, project: project)
     end
 
-    # An N+1 would mean a new alert would increase the query count
-    create(:alert_management_alert, project: project)
+    it 'avoids N+1 queries' do
+      q = with_signature([first_n], query)
 
-    expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(base_count)
-    expect(alerts_result.length).to eq(3)
+      base_count = ActiveRecord::QueryRecorder.new do
+        post_graphql(q, current_user: current_user, variables: first_n.with(1))
+        expect(alerts_result.length).to eq(1)
+      end
+
+      expect do
+        post_graphql(q, current_user: current_user, variables: first_n.with(3))
+        expect(alerts_result.length).to eq(3)
+      end.not_to exceed_query_limit(base_count)
+    end
   end
 
   context 'for non-system notes' do
diff --git a/spec/requests/api/graphql/project/merge_request_spec.rb b/spec/requests/api/graphql/project/merge_request_spec.rb
index fae52fe814d..e1b867ad097 100644
--- a/spec/requests/api/graphql/project/merge_request_spec.rb
+++ b/spec/requests/api/graphql/project/merge_request_spec.rb
@@ -9,12 +9,13 @@ RSpec.describe 'getting merge request information nested in a project' do
   let(:current_user) { create(:user) }
   let(:merge_request_graphql_data) { graphql_data['project']['mergeRequest'] }
   let!(:merge_request) { create(:merge_request, source_project: project) }
+  let(:mr_fields) { all_graphql_fields_for('MergeRequest') }
 
   let(:query) do
     graphql_query_for(
       'project',
       { 'fullPath' => project.full_path },
-      query_graphql_field('mergeRequest', iid: merge_request.iid.to_s)
+      query_graphql_field('mergeRequest', { iid: merge_request.iid.to_s }, mr_fields)
     )
   end
 
@@ -43,6 +44,38 @@ RSpec.describe 'getting merge request information nested in a project' do
     expect(merge_request_graphql_data['author']['username']).to eq(merge_request.author.username)
   end
 
+  context 'the merge_request has reviewers' do
+    let(:mr_fields) do
+      <<~SELECT
+      reviewers { nodes { id username } }
+      participants { nodes { id username } }
+      SELECT
+    end
+
+    before do
+      merge_request.reviewers << create_list(:user, 2)
+    end
+
+    it 'includes reviewers' do
+      expected = merge_request.reviewers.map do |r|
+        a_hash_including('id' => global_id_of(r), 'username' => r.username)
+      end
+
+      post_graphql(query, current_user: current_user)
+
+      expect(graphql_data_at(:project, :merge_request, :reviewers, :nodes)).to match_array(expected)
+      expect(graphql_data_at(:project, :merge_request, :participants, :nodes)).to include(*expected)
+    end
+
+    it 'suppresses reviewers if reviewers are not allowed' do
+      stub_feature_flags(merge_request_reviewers: false)
+
+      post_graphql(query, current_user: current_user)
+
+      expect(graphql_data_at(:project, :merge_request, :reviewers)).to be_nil
+    end
+  end
+
   it 'includes diff stats' do
     be_natural = an_instance_of(Integer).and(be >= 0)
 
@@ -219,4 +252,41 @@ RSpec.describe 'getting merge request information nested in a project' do
       expect(merge_request_graphql_data['mergeStatus']).to eq('checking')
     end
   end
+
+  # see: https://gitlab.com/gitlab-org/gitlab/-/issues/297358
+  context 'when the notes have been preloaded (by participants)' do
+    let(:query) do
+      <<~GQL
+      query($path: ID!) {
+        project(fullPath: $path) {
+          mrs: mergeRequests(first: 1) {
+            nodes {
+              participants { nodes { id } }
+              notes(first: 1) {
+                pageInfo { endCursor hasPreviousPage hasNextPage }
+                nodes { id }
+              }
+            }
+          }
+        }
+      }
+      GQL
+    end
+
+    before do
+      create_list(:note_on_merge_request, 3, project: project, noteable: merge_request)
+    end
+
+    it 'does not error' do
+      post_graphql(query,
+                   current_user: current_user,
+                   variables: { path: project.full_path })
+
+      expect(graphql_data_at(:project, :mrs, :nodes, :notes, :pageInfo)).to contain_exactly a_hash_including(
+        'endCursor' => String,
+        'hasNextPage' => true,
+        'hasPreviousPage' => false
+      )
+    end
+  end
 end
diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb
index c05a620bb62..c85cb8b2ffe 100644
--- a/spec/requests/api/graphql/project/merge_requests_spec.rb
+++ b/spec/requests/api/graphql/project/merge_requests_spec.rb
@@ -23,9 +23,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
     graphql_query_for(
       :project,
       { full_path: project.full_path },
-      query_graphql_field(:merge_requests, search_params, [
-        query_graphql_field(:nodes, nil, fields)
-      ])
+      query_nodes(:merge_requests, fields, args: search_params)
     )
   end
 
@@ -50,24 +48,22 @@ RSpec.describe 'getting merge request listings nested in a project' do
       project.repository.expire_branches_cache
     end
 
+    let(:graphql_data) do
+      GitlabSchema.execute(query, context: { current_user: current_user }).to_h['data']
+    end
+
     context 'selecting any single scalar field' do
       where(:field) do
         scalar_fields_of('MergeRequest').map { |name| [name] }
       end
 
       with_them do
-        it_behaves_like 'a working graphql query' do
-          let(:query) do
-            query_merge_requests([:iid, field].uniq)
-          end
-
-          before do
-            post_graphql(query, current_user: current_user)
-          end
-
-          it 'selects the correct MR' do
-            expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
-          end
+        let(:query) do
+          query_merge_requests([:iid, field].uniq)
+        end
+
+        it 'selects the correct MR' do
+          expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
         end
       end
     end
@@ -87,19 +83,13 @@ RSpec.describe 'getting merge request listings nested in a project' do
       end
 
       with_them do
-        it_behaves_like 'a working graphql query' do
-          let(:query) do
-            fld = is_connection ? query_graphql_field(:nodes, nil, [subfield]) : subfield
-            query_merge_requests([:iid, query_graphql_field(field, nil, [fld])])
-          end
-
-          before do
-            post_graphql(query, current_user: current_user)
-          end
-
-          it 'selects the correct MR' do
-            expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
-          end
+        let(:query) do
+          fld = is_connection ? query_graphql_field(:nodes, nil, [subfield]) : subfield
+          query_merge_requests([:iid, query_graphql_field(field, nil, [fld])])
+        end
+
+        it 'selects the correct MR' do
+          expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s))
         end
       end
     end
@@ -254,6 +244,115 @@ RSpec.describe 'getting merge request listings nested in a project' do
 
       include_examples 'N+1 query check'
     end
+
+    context 'when requesting reviewers' do
+      let(:requested_fields) { ['reviewers { nodes { username } }'] }
+
+      before do
+        merge_request_a.reviewers << create(:user)
+        merge_request_a.reviewers << create(:user)
+        merge_request_c.reviewers << create(:user)
+      end
+
+      it 'returns the reviewers' do
+        execute_query
+
+        expect(results).to include a_hash_including('reviewers' => {
+          'nodes' => match_array(merge_request_a.reviewers.map do |r|
+            a_hash_including('username' => r.username)
+          end)
+        })
+      end
+
+      context 'the feature flag is disabled' do
+        before do
+          stub_feature_flags(merge_request_reviewers: false)
+        end
+
+        it 'does not return reviewers' do
+          execute_query
+
+          expect(results).to all(match a_hash_including('reviewers' => be_nil))
+        end
+      end
+
+      include_examples 'N+1 query check'
+    end
+  end
+
+  describe 'performance' do
+    let(:mr_fields) do
+      <<~SELECT
+      assignees { nodes { username } }
+      reviewers { nodes { username } }
+      participants { nodes { username } }
+      headPipeline { status }
+      SELECT
+    end
+
+    let(:query) do
+      <<~GQL
+        query($first: Int) {
+          project(fullPath: "#{project.full_path}") {
+            mergeRequests(first: $first) {
+              nodes { #{mr_fields} }
+            }
+          }
+        }
+      GQL
+    end
+
+    before_all do
+      project.add_developer(current_user)
+      mrs = create_list(:merge_request, 10, :closed, :with_head_pipeline,
+                        source_project: project,
+                        author: current_user)
+      mrs.each do |mr|
+        mr.assignees << create(:user)
+        mr.assignees << current_user
+        mr.reviewers << create(:user)
+        mr.reviewers << current_user
+      end
+    end
+
+    before do
+      # Confounding factor: makes DB calls in EE
+      allow(Gitlab::Database).to receive(:read_only?).and_return(false)
+    end
+
+    def run_query(number)
+      # Ensure that we have a fresh request store and batch-context between runs
+      result = run_with_clean_state(query,
+        context: { current_user: current_user },
+        variables: { first: number }
+      )
+
+      graphql_dig_at(result.to_h, :data, :project, :merge_requests, :nodes)
+    end
+
+    def user_collection
+      { 'nodes' => all(match(a_hash_including('username' => be_present))) }
+    end
+
+    it 'returns appropriate results' do
+      mrs = run_query(2)
+
+      expect(mrs.size).to eq(2)
+      expect(mrs).to all(
+        match(
+          a_hash_including(
+            'assignees' => user_collection,
+            'reviewers' => user_collection,
+            'participants' => user_collection,
+            'headPipeline' => { 'status' => be_present }
+          )))
+    end
+
+    it 'can lookahead to eliminate N+1 queries' do
+      baseline = ActiveRecord::QueryRecorder.new { run_query(1) }
+
+      expect { run_query(10) }.not_to exceed_query_limit(baseline)
+    end
   end
 
   describe 'sorting and pagination' do
diff --git a/spec/requests/api/graphql/project/project_members_spec.rb b/spec/requests/api/graphql/project/project_members_spec.rb
index cb937432ef7..984a0adb8c6 100644
--- a/spec/requests/api/graphql/project/project_members_spec.rb
+++ b/spec/requests/api/graphql/project/project_members_spec.rb
@@ -11,15 +11,13 @@ RSpec.describe 'getting project members information' do
   let_it_be(:user_1) { create(:user, username: 'user') }
   let_it_be(:user_2) { create(:user, username: 'test') }
 
-  let(:member_data) { graphql_data['project']['projectMembers']['edges'] }
-
   before_all do
     [user_1, user_2].each { |user| parent_group.add_guest(user) }
   end
 
   context 'when the request is correct' do
     it_behaves_like 'a working graphql query' do
-      before_all do
+      before do
         fetch_members(project: parent_project)
       end
     end
@@ -114,8 +112,7 @@ RSpec.describe 'getting project members information' do
 
   def expect_array_response(*items)
     expect(response).to have_gitlab_http_status(:success)
-    expect(member_data).to be_an Array
-    expect(member_data.map { |node| node['node']['user']['id'] })
-      .to match_array(items.map { |u| global_id_of(u) })
+    member_gids = graphql_data_at(:project, :project_members, :edges, :node, :user, :id)
+    expect(member_gids).to match_array(items.map { |u| global_id_of(u) })
   end
 end
diff --git a/spec/requests/api/graphql/project/release_spec.rb b/spec/requests/api/graphql/project/release_spec.rb
index 99b15ff00b1..ccc2825da25 100644
--- a/spec/requests/api/graphql/project/release_spec.rb
+++ b/spec/requests/api/graphql/project/release_spec.rb
@@ -76,11 +76,11 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       it 'finds all milestones associated to a release' do
         post_query
 
-        expected = release.milestones.map do |milestone|
+        expected = release.milestones.order_by_dates_and_title.map do |milestone|
           { 'id' => global_id_of(milestone), 'title' => milestone.title }
         end
 
-        expect(data).to match_array(expected)
+        expect(data).to eq(expected)
       end
     end
 
@@ -427,4 +427,33 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       end
     end
   end
+
+  describe 'milestone order' do
+    let(:path) { path_prefix }
+    let(:current_user) { stranger }
+    let_it_be(:project) { create(:project, :public) }
+    let_it_be_with_reload(:release) { create(:release, project: project) }
+
+    let(:release_fields) do
+      query_graphql_field(%{
+        milestones {
+          nodes {
+            title
+          }
+        }
+      })
+    end
+
+    let(:actual_milestone_title_order) do
+      post_query
+
+      data.dig('milestones', 'nodes').map { |m| m['title'] }
+    end
+
+    before do
+      release.update!(milestones: [milestone_2, milestone_1])
+    end
+
+    it_behaves_like 'correct release milestone order'
+  end
 end