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