Merge branch 'improve-group-coverage-query-performance' into 'master'

Preload projects to prevent N+1 when populating project name

See merge request gitlab-org/gitlab!40769

app/models/ci/daily_build_group_report_result.rb

@@ -11,6 +11,8 @@ module Ci
 
     validates :data, json_schema: { filename: "daily_build_group_report_result_data" }
 
+    scope :with_included_projects, -> { includes(:project) }
+
     def self.upsert_reports(data)
       upsert_all(data, unique_by: :index_daily_build_group_report_results_unique_columns) if data.any?
     end

changelogs/unreleased/improve-group-coverage-query-performance.yml

+---
+title: Preload projects to prevent N+1 when populating project name
+merge_request: 40769
+author:
+type: performance

ee/app/finders/ci/daily_build_group_report_results_by_group_finder.rb

@@ -19,6 +19,13 @@ module Ci
 
     private
 
+    def query
+      Ci::DailyBuildGroupReportResult.with_included_projects.recent_results(
+        query_params,
+        limit: limit
+      )
+    end
+
     def query_allowed?
       can?(current_user, :read_group_build_report_results, @group)
     end

ee/spec/controllers/groups/analytics/coverage_reports_controller_spec.rb

@@ -110,6 +110,20 @@ RSpec.describe Groups::Analytics::CoverageReportsController do
         end
       end
 
+      it 'executes the same number of queries regardless of the number of records returned' do
+        control = ActiveRecord::QueryRecorder.new do
+          get :index, params: valid_request_params
+        end
+
+        expect(CSV.parse(response.body).length).to eq(3)
+
+        create_daily_coverage('rspec', project, 79.0, '2020-03-10')
+
+        expect { get :index, params: valid_request_params }.not_to exceed_query_limit(control)
+
+        expect(csv_response.length).to eq(4)
+      end
+
       context 'with an invalid format' do
         it 'responds 404' do
           get :index, params: valid_request_params.merge(format: :json)

ee/spec/finders/ci/daily_build_group_report_results_by_group_finder_spec.rb

@@ -4,20 +4,25 @@ require 'spec_helper'
 
 RSpec.describe Ci::DailyBuildGroupReportResultsByGroupFinder do
   describe '#execute' do
-    let(:user) { create(:user) }
+    let_it_be(:user) { create(:user) }
 
-    let(:project) { create(:project, :private) }
-    let!(:project_coverage) { create_daily_coverage('rspec', 95.0, '2020-03-10', project) }
+    let_it_be(:project) { create(:project, :private) }
+    let_it_be(:project_coverage) { create_daily_coverage('rspec', 95.0, '2020-03-10', project) }
 
-    let(:group) { create(:group, :private) }
-    let(:group_project) { create(:project, namespace: group) }
-    let!(:group_project_coverage) { create_daily_coverage('rspec', 79.0, '2020-03-09', group_project) }
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:rspec_project) { create(:project, namespace: group) }
+    let_it_be(:rspec_project_coverage) { create_daily_coverage('rspec', 79.0, '2020-03-09', rspec_project) }
 
-    let(:subgroup) { create(:group, :private, parent: group) }
-    let(:subgroup_project) { create(:project, namespace: subgroup) }
-    let!(:subgroup_project_coverage) { create_daily_coverage('rspec', 89.0, '2020-03-09', subgroup_project) }
+    let_it_be(:karma_project) { create(:project, namespace: group) }
+    let_it_be(:karma_project_coverage) { create_daily_coverage('karma', 89.0, '2020-03-09', karma_project) }
+
+    let_it_be(:generic_project) { create(:project, namespace: group) }
+    let_it_be(:generic_coverage) { create_daily_coverage('unreported', 95.0, '2020-03-10', generic_project) }
+
+    let_it_be(:subgroup) { create(:group, :private, parent: group) }
+    let_it_be(:subgroup_project) { create(:project, namespace: subgroup) }
+    let_it_be(:subgroup_project_coverage) { create_daily_coverage('rspec', 89.0, '2020-03-09', subgroup_project) }
 
-    let(:ref_path) { 'refs/heads/master' }
     let(:limit) { nil }
     let(:project_ids) { nil }
 
@@ -26,7 +31,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultsByGroupFinder do
         current_user: user,
         group: group,
         project_ids: project_ids,
-        ref_path: ref_path,
+        ref_path: 'refs/heads/master',
         start_date: '2020-03-09',
         end_date: '2020-03-10',
         limit: limit
@@ -38,13 +43,16 @@ RSpec.describe Ci::DailyBuildGroupReportResultsByGroupFinder do
     end
 
     context 'when current user is allowed to :read_group_build_report_results' do
-      let(:excluded_group_project) { create(:project, namespace: group) }
-      let!(:excluded_coverage) { create_daily_coverage('unreported', 95.0, '2020-03-10', excluded_group_project) }
-
       before do
         group.add_reporter(user)
       end
 
+      it 'returns only coverages belonging to the passed group' do
+        expect(subject).to include(rspec_project_coverage, karma_project_coverage)
+        expect(subject).not_to include(project_coverage)
+        expect(subject).not_to include(subgroup_project_coverage)
+      end
+
       context 'with a limit below 1000' do
         let(:limit) { 5 }
 
@@ -69,18 +77,18 @@ RSpec.describe Ci::DailyBuildGroupReportResultsByGroupFinder do
 
       context 'with nil project_ids' do
         it 'returns only coverages belonging to the passed group' do
-          expect(subject).to include(group_project_coverage)
+          expect(subject).to match_array([rspec_project_coverage, karma_project_coverage, generic_coverage])
           expect(subject).not_to include(project_coverage)
           expect(subject).not_to include(subgroup_project_coverage)
         end
       end
 
       context 'with passed project_ids' do
-        let(:project_ids) { [group_project.id] }
+        let(:project_ids) { [rspec_project.id] }
 
         it 'filters out non-specified projects' do
-          expect(subject).to include(group_project_coverage)
-          expect(subject).not_to include(excluded_coverage)
+          expect(subject).to match_array([rspec_project_coverage])
+          expect(subject).not_to include(karma_project_coverage, generic_coverage)
         end
       end
 
@@ -88,8 +96,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultsByGroupFinder do
         let(:project_ids) { [] }
 
         it 'returns all projects' do
-          expect(subject).to include(group_project_coverage)
-          expect(subject).to include(excluded_coverage)
+          expect(subject).to match_array([rspec_project_coverage, karma_project_coverage, generic_coverage])
         end
       end
     end
@@ -107,7 +114,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultsByGroupFinder do
     create(
       :ci_daily_build_group_report_result,
       project: project,
-      ref_path: ref_path,
+      ref_path: 'refs/heads/master',
       group_name: group_name,
       data: { 'coverage' => coverage },
       date: date