Skip to content

Fix N+1 in environments dashboard list

The below test which checks for a query that would not happen as we have preloading actually fails.

This failure seems to indicate that there is a bug in the Serializer Paginator#paginate as the resource object passed seems to be reloaded somewhere inside the call.

diff --git a/app/serializers/concerns/with_pagination.rb b/app/serializers/concerns/with_pagination.rb
index c8ffae355e8..4e8559a7520 100644
--- a/app/serializers/concerns/with_pagination.rb
+++ b/app/serializers/concerns/with_pagination.rb
@@ -16,6 +16,7 @@ def paginated?
   # we shouldn't try to paginate single resources
   def represent(resource, opts = {})
     if paginated? && resource.respond_to?(:page)
+      # Bug: `paginator.paginate(resource)` is reloading the resource.
       super(paginator.paginate(resource), opts)
     else
       super(resource, opts)
diff --git a/ee/spec/serializers/dashboard_environments_serializer_spec.rb b/ee/spec/serializers/dashboard_environments_serializer_spec.rb
index 0886c9c3c38..5c5f13bc98d 100644
--- a/ee/spec/serializers/dashboard_environments_serializer_spec.rb
+++ b/ee/spec/serializers/dashboard_environments_serializer_spec.rb
@@ -4,6 +4,19 @@

 RSpec.describe DashboardEnvironmentsSerializer do
   describe '.represent' do
+    def setup
+      user = create(:user)
+      project = create(:project, :repository)
+      project.add_developer(user)
+      user.update!(ops_dashboard_projects: [project])
+
+      [user, project]
+    end
+
+    before do
+      stub_licensed_features(operations_dashboard: true)
+    end
+
     it 'returns an empty array when there are no projects' do
       current_user = create(:user)
       projects = []
@@ -23,5 +36,47 @@

       expect(result.first.keys.sort).to eq([:avatar_url, :environments, :id, :name, :namespace, :remove_path, :web_url
])
     end
+
+    context 'Pagination vs Non Pagination' do
+      let(:request) { double(url: "#{Gitlab.config.gitlab.url}:8080/-/operations/environments?#{query.to_query}", quer
y_parameters: query) }
+      let(:response) { spy('response') }
+      let(:query) { { page: 1, per_page: 1 } }
+
+      it 'makes same number of queries with and without pagination' do
+        current_user, project = setup
+
+        pipeline = create(:ci_pipeline, user: current_user, project: project)
+
+        build_a = create(:ci_build, user: current_user, project: project, pipeline: pipeline)
+        build_b = create(:ci_build, user: current_user, project: project, pipeline: pipeline)
+        build_c = create(:ci_build, user: current_user, project: project, pipeline: pipeline)
+
+        environment_a = create(:environment, name: "a", project: project, state: :available)
+        environment_b = create(:environment, name: "b", project: project, state: :available)
+
+        create(:deployment, project: project, environment: environment_a, status: :success, deployable: build_a)
+        create(:deployment, project: project, environment: environment_b, status: :success, deployable: build_b)
+        create(:deployment, project: project, environment: environment_a, status: :failed, deployable: build_c)
+
+        preloaded_projects = Dashboard::Environments::ListService.new(current_user).execute
+
+        non_paginated_case = ActiveRecord::QueryRecorder.new do
+          described_class.new(current_user: current_user).represent(preloaded_projects)
+        end
+
+        paginated_case = ActiveRecord::QueryRecorder.new do
+          described_class.new(current_user: current_user).with_pagination(request, response).represent(preloaded_proje
cts)
+        end
+
+        validation_query_substring = 'SELECT "ci_pipelines".* FROM "ci_pipelines" INNER JOIN "ci_builds" ON "ci_pipeli
nes"."id" = "ci_builds"."commit_id" INNER JOIN "deployments" ON "ci_builds"."id" = "deployments"."deployable_id"'
+
+        # expect(non_paginated_case.count).eq paginated_case.count
+
+        expect(non_paginated_case.occurrences.keys.any? { |query| query.include?(validation_query_substring) }).to be_falsey
+
+        # Below case will fail because it is not using the preloaded associations and makes a new association call.
+        expect(paginated_case.occurrences.keys.any? { |query| query.include?(validation_query_substring) }).to be_falsey
+      end
+    end
   end
 end
Edited by Bala Kumar