From dab3ae39a2093fa924daf9c1902a2784002cca63 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 23 May 2018 13:29:21 +0200
Subject: [PATCH] Do not paginate pipelines active relation twice

---
 app/controllers/projects/pipelines_controller.rb    |  5 +----
 app/serializers/pipeline_serializer.rb              | 13 ++++++-------
 .../projects/pipelines_controller_spec.rb           |  2 +-
 spec/lib/gitlab/ci/pipeline/preloader_spec.rb       | 10 ++++++++++
 4 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index 2b758b960c2..768595ceeb4 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -15,7 +15,6 @@ class Projects::PipelinesController < Projects::ApplicationController
     @pipelines = PipelinesFinder
       .new(project, scope: @scope)
       .execute
-      .preload(:stages)
       .page(params[:page])
       .per(30)
 
@@ -24,8 +23,6 @@ class Projects::PipelinesController < Projects::ApplicationController
     @finished_count = limited_pipelines_count(project, 'finished')
     @pipelines_count = limited_pipelines_count(project)
 
-    Gitlab::Ci::Pipeline::Preloader.preload!(@pipelines)
-
     respond_to do |format|
       format.html
       format.json do
@@ -35,7 +32,7 @@ class Projects::PipelinesController < Projects::ApplicationController
           pipelines: PipelineSerializer
             .new(project: @project, current_user: @current_user)
             .with_pagination(request, response)
-            .represent(@pipelines, disable_coverage: true),
+            .represent(@pipelines, disable_coverage: true, preload: true),
           count: {
             all: @pipelines_count,
             running: @running_count,
diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb
index 7181f8a6b04..68325cbffa8 100644
--- a/app/serializers/pipeline_serializer.rb
+++ b/app/serializers/pipeline_serializer.rb
@@ -1,14 +1,12 @@
 class PipelineSerializer < BaseSerializer
   include WithPagination
-
-  InvalidResourceError = Class.new(StandardError)
-
   entity PipelineDetailsEntity
 
   def represent(resource, opts = {})
     if resource.is_a?(ActiveRecord::Relation)
 
       resource = resource.preload([
+        :stages,
         :retryable_builds,
         :cancelable_statuses,
         :trigger_requests,
@@ -19,11 +17,12 @@ class PipelineSerializer < BaseSerializer
       ])
     end
 
-    if paginated?
-      super(@paginator.paginate(resource), opts)
-    else
-      super(resource, opts)
+    if opts.delete(:preload)
+      resource = @paginator.paginate(resource) if paginated?
+      resource = Gitlab::Ci::Pipeline::Preloader.preload!(resource)
     end
+
+    super(resource, opts)
   end
 
   def represent_status(resource)
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index 2e0f9b00023..d7253c90ee5 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -36,7 +36,7 @@ describe Projects::PipelinesController do
       expect(json_response['count']['running']).to eq '1'
       expect(json_response['count']['pending']).to eq '1'
       expect(json_response['count']['finished']).to eq '2'
-      expect(queries.count).to be < 32
+      expect(queries.count).to be_within(2).of(29)
     end
 
     it 'does not include coverage data for the pipelines' do
diff --git a/spec/lib/gitlab/ci/pipeline/preloader_spec.rb b/spec/lib/gitlab/ci/pipeline/preloader_spec.rb
index 16d3631ec7c..7821d28ab06 100644
--- a/spec/lib/gitlab/ci/pipeline/preloader_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/preloader_spec.rb
@@ -18,5 +18,15 @@ describe Gitlab::Ci::Pipeline::Preloader do
 
       described_class.preload!([pipeline])
     end
+
+    it 'returns original collection' do
+      allow(commit).to receive(:lazy_author)
+      allow(pipeline).to receive(:number_of_warnings)
+      allow(stage).to receive(:number_of_warnings)
+
+      pipelines = [pipeline, pipeline]
+
+      expect(described_class.preload!(pipelines)).to eq pipelines
+    end
   end
 end