Merge branch 'fix-n-1-in-environments-dashboard-list' into 'master'

Fix N+1 in environments dashboard list

See merge request gitlab-org/gitlab!74834

ee/app/serializers/dashboard_environments_serializer.rb

@@ -4,4 +4,38 @@ class DashboardEnvironmentsSerializer < BaseSerializer
   include WithPagination
 
   entity DashboardEnvironmentsProjectEntity
+
+  def represent(resource, opts = {})
+    resource = @paginator.paginate(resource) if paginated?
+
+    super(batch_load(resource), opts)
+  end
+
+  private
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def batch_load(projects)
+    ActiveRecord::Associations::Preloader.new.preload(projects, [
+      :route,
+      environments_for_dashboard: [
+        last_visible_pipeline: [
+          :user,
+          project: [:route, :group, :project_feature, namespace: :route]
+        ],
+        last_visible_deployment: [
+          deployable: [
+            :metadata,
+            :pipeline,
+            project: [:project_feature, :group, :route, namespace: :route]
+          ],
+          project: [:route, namespace: :route]
+        ],
+        project: [:project_feature, :group, namespace: :route]
+      ],
+      namespace: [:route, :owner]
+    ])
+
+    projects
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
 end

ee/app/services/dashboard/environments/list_service.rb

@@ -3,47 +3,17 @@
 module Dashboard
   module Environments
     class ListService
+      attr_reader :user
+
       def initialize(user)
         @user = user
       end
 
       def execute
-        load_projects(user)
-      end
-
-      private
-
-      attr_reader :user
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def load_projects(user)
-        projects = ::Dashboard::Projects::ListService
+        ::Dashboard::Projects::ListService
           .new(user, feature: :operations_dashboard)
           .execute(user.ops_dashboard_projects)
-
-        ActiveRecord::Associations::Preloader.new.preload(projects, [
-          :route,
-          environments_for_dashboard: [
-            last_visible_pipeline: [
-              :user,
-              project: [:route, :group, :project_feature, namespace: :route]
-            ],
-            last_visible_deployment: [
-              deployable: [
-                :metadata,
-                :pipeline,
-                project: [:project_feature, :group, :route, namespace: :route]
-              ],
-              project: [:route, namespace: :route]
-            ],
-            project: [:project_feature, :group, namespace: :route]
-          ],
-          namespace: [:route, :owner]
-        ])
-
-        projects
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

ee/spec/controllers/operations_controller_spec.rb

@@ -322,6 +322,23 @@ RSpec.describe OperationsController do
             expect(project2_json['environments'].map { |e| e['id'] }).to eq([environment3.id])
           end
 
+          it 'does not make N+1 queries with multiple environments' do
+            project2 = create(:project)
+            project2.add_developer(user)
+            user.update!(ops_dashboard_projects: [project, project2])
+
+            create(:environment, project: project)
+            create(:environment, project: project)
+            create(:environment, project: project2)
+
+            control = ActiveRecord::QueryRecorder.new { get_environments(:json) }
+
+            create(:environment, project: project)
+            create(:environment, project: project2)
+
+            expect { get_environments(:json) }.not_to exceed_query_limit(control)
+          end
+
           it 'does not return environments that would be grouped into a folder' do
             create(:environment, project: project, name: 'review/test-feature')
             create(:environment, project: project, name: 'review/another-feature')

ee/spec/serializers/dashboard_environments_serializer_spec.rb

@@ -23,5 +23,37 @@ RSpec.describe DashboardEnvironmentsSerializer do
 
       expect(result.first.keys.sort).to eq([:avatar_url, :environments, :id, :name, :namespace, :remove_path, :web_url])
     end
+
+    it 'preloads only relevant ci_builds and does not result in N+1' do
+      current_user = create(:user)
+      project = create(:project, :repository)
+
+      pipeline = create(:ci_pipeline, user: current_user, project: project, sha: project.commit.sha)
+
+      ci_build_a = create(:ci_build, user: current_user, project: project, pipeline: pipeline)
+      ci_build_b = create(:ci_build, user: current_user, project: project, pipeline: pipeline)
+      ci_build_c = create(:ci_build, user: current_user, project: project, pipeline: pipeline)
+
+      environment_a = create(:environment, project: project, state: :available)
+      environment_b = create(:environment, project: project, state: :available)
+
+      projects = [project]
+
+      create(:deployment, :success, project: project, environment: environment_a, deployable: ci_build_a)
+      create(:deployment, :success, project: project, environment: environment_a, deployable: ci_build_b)
+      create(:deployment, :success, project: project, environment: environment_b, deployable: ci_build_c)
+
+      expect(CommitStatus).to receive(:instantiate)
+        .with(a_hash_including("id" => ci_build_b.id), anything)
+        .at_least(:once)
+        .and_call_original
+
+      expect(CommitStatus).to receive(:instantiate)
+        .with(a_hash_including("id" => ci_build_c.id), anything)
+        .at_least(:once)
+        .and_call_original
+
+      described_class.new(current_user: current_user).represent(projects)
+    end
   end
 end

ee/spec/services/dashboard/environments/list_service_spec.rb

@@ -25,33 +25,6 @@ RSpec.describe Dashboard::Environments::ListService do
       expect(projects_with_environments).to eq([project])
     end
 
-    it 'preloads only relevant ci_builds' do
-      user, project = setup
-
-      ci_build_a = create(:ci_build, project: project)
-      ci_build_b = create(:ci_build, project: project)
-      ci_build_c = create(:ci_build, project: project)
-
-      environment_a = create(:environment, project: project)
-      environment_b = create(:environment, project: project)
-
-      create(:deployment, :success, project: project, environment: environment_a, deployable: ci_build_a)
-      create(:deployment, :success, project: project, environment: environment_a, deployable: ci_build_b)
-      create(:deployment, :success, project: project, environment: environment_b, deployable: ci_build_c)
-
-      expect(CommitStatus).to receive(:instantiate)
-        .with(a_hash_including("id" => ci_build_b.id), anything)
-        .at_least(:once)
-        .and_call_original
-
-      expect(CommitStatus).to receive(:instantiate)
-        .with(a_hash_including("id" => ci_build_c.id), anything)
-        .at_least(:once)
-        .and_call_original
-
-      described_class.new(user).execute
-    end
-
     context 'when unlicensed' do
       before do
         stub_licensed_features(operations_dashboard: false)