From 6fefbf2e4b8e388470cfa8bb4eb7079817f85a5c Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 16 Jan 2018 16:56:05 +0000
Subject: Merge branch 'builds-api-nplusone' into 'master'

Resolve "N+1 queries with /projects/:project_id/builds API endpoint"

Closes #41957

See merge request gitlab-org/gitlab-ce!16445

(cherry picked from commit 191b5153c86964e59400fe086b88b89bb45fe7a9)

7e084984 Add n plus one spec for v3 builds api
c9a1a155 Fix N+1 in v3 builds API
c9840842 Eager load user, runner, pipeline and its creator
09118796 Add N+1 query spec for Jobs API
87cc0373 Fix N+1 builds query in Jobs list API
a4d7f155 Extend N+1 Builds and Jobs API specs
8cef1f4c Add missing newline in Jobs API spec
feb34497 Use preload instead of includes to avoid joins
---
 lib/api/jobs.rb                     |  1 +
 lib/api/v3/builds.rb                |  1 +
 spec/requests/api/jobs_spec.rb      | 27 +++++++++++++++++++++++++--
 spec/requests/api/v3/builds_spec.rb | 27 +++++++++++++++++++++++++--
 4 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb
index a116ab3c9bd..9c205514b3a 100644
--- a/lib/api/jobs.rb
+++ b/lib/api/jobs.rb
@@ -38,6 +38,7 @@ module API
         builds = user_project.builds.order('id DESC')
         builds = filter_builds(builds, params[:scope])
 
+        builds = builds.preload(:user, :job_artifacts_archive, :runner, pipeline: :project)
         present paginate(builds), with: Entities::Job
       end
 
diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb
index fa0bef39602..ac76fece931 100644
--- a/lib/api/v3/builds.rb
+++ b/lib/api/v3/builds.rb
@@ -36,6 +36,7 @@ module API
           builds = user_project.builds.order('id DESC')
           builds = filter_builds(builds, params[:scope])
 
+          builds = builds.preload(:user, :job_artifacts_archive, :runner, pipeline: :project)
           present paginate(builds), with: ::API::V3::Entities::Build
         end
 
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 805496e4a54..e211c347266 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -25,8 +25,10 @@ describe API::Jobs do
   describe 'GET /projects/:id/jobs' do
     let(:query) { Hash.new }
 
-    before do
-      get api("/projects/#{project.id}/jobs", api_user), query
+    before do |example|
+      unless example.metadata[:skip_before_request]
+        get api("/projects/#{project.id}/jobs", api_user), query
+      end
     end
 
     context 'authorized user' do
@@ -51,6 +53,23 @@ describe API::Jobs do
         expect(json_job['pipeline']['status']).to eq job.pipeline.status
       end
 
+      it 'avoids N+1 queries', skip_before_request: true do
+        first_build = create(:ci_build, :artifacts, pipeline: pipeline)
+        first_build.runner = create(:ci_runner)
+        first_build.user = create(:user)
+        first_build.save
+
+        control_count = ActiveRecord::QueryRecorder.new { go }.count
+
+        second_pipeline = create(:ci_empty_pipeline, project: project, sha: project.commit.id, ref: project.default_branch)
+        second_build = create(:ci_build, :artifacts, pipeline: second_pipeline)
+        second_build.runner = create(:ci_runner)
+        second_build.user = create(:user)
+        second_build.save
+
+        expect { go }.not_to exceed_query_limit(control_count)
+      end
+
       context 'filter project with one scope element' do
         let(:query) { { 'scope' => 'pending' } }
 
@@ -83,6 +102,10 @@ describe API::Jobs do
         expect(response).to have_gitlab_http_status(401)
       end
     end
+
+    def go
+      get api("/projects/#{project.id}/jobs", api_user), query
+    end
   end
 
   describe 'GET /projects/:id/pipelines/:pipeline_id/jobs' do
diff --git a/spec/requests/api/v3/builds_spec.rb b/spec/requests/api/v3/builds_spec.rb
index a73bb456b52..cdbc5692e19 100644
--- a/spec/requests/api/v3/builds_spec.rb
+++ b/spec/requests/api/v3/builds_spec.rb
@@ -13,10 +13,12 @@ describe API::V3::Builds do
   describe 'GET /projects/:id/builds ' do
     let(:query) { '' }
 
-    before do
+    before do |example|
       create(:ci_build, :skipped, pipeline: pipeline)
 
-      get v3_api("/projects/#{project.id}/builds?#{query}", api_user)
+      unless example.metadata[:skip_before_request]
+        get v3_api("/projects/#{project.id}/builds?#{query}", api_user)
+      end
     end
 
     context 'authorized user' do
@@ -40,6 +42,23 @@ describe API::V3::Builds do
         expect(json_build['pipeline']['status']).to eq build.pipeline.status
       end
 
+      it 'avoids N+1 queries', skip_before_request: true do
+        first_build = create(:ci_build, :artifacts, pipeline: pipeline)
+        first_build.runner = create(:ci_runner)
+        first_build.user = create(:user)
+        first_build.save
+
+        control_count = ActiveRecord::QueryRecorder.new { go }.count
+
+        second_pipeline = create(:ci_empty_pipeline, project: project, sha: project.commit.id, ref: project.default_branch)
+        second_build = create(:ci_build, :artifacts, pipeline: second_pipeline)
+        second_build.runner = create(:ci_runner)
+        second_build.user = create(:user)
+        second_build.save
+
+        expect { go }.not_to exceed_query_limit(control_count)
+      end
+
       context 'filter project with one scope element' do
         let(:query) { 'scope=pending' }
 
@@ -84,6 +103,10 @@ describe API::V3::Builds do
         expect(response).to have_gitlab_http_status(401)
       end
     end
+
+    def go
+      get v3_api("/projects/#{project.id}/builds?#{query}", api_user)
+    end
   end
 
   describe 'GET /projects/:id/repository/commits/:sha/builds' do
-- 
cgit v1.2.1