Merge branch 'optimize-environments-serializer' into 'master'

Optimize environment serializer by preloading associated entities [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!58748

app/models/ci/build.rb

@@ -377,11 +377,11 @@ module Ci
     end
 
     def other_manual_actions
-      pipeline.manual_actions.where.not(name: name)
+      pipeline.manual_actions.reject { |action| action.name == self.name }
     end
 
     def other_scheduled_actions
-      pipeline.scheduled_actions.where.not(name: name)
+      pipeline.scheduled_actions.reject { |action| action.name == self.name }
     end
 
     def pages_generator?

app/models/deployment.rb

@@ -171,7 +171,7 @@ class Deployment < ApplicationRecord
   end
 
   def commit
-    project.commit(sha)
+    @commit ||= project.commit(sha)
   end
 
   def commit_title
@@ -250,7 +250,7 @@ class Deployment < ApplicationRecord
     return unless on_stop.present?
     return unless manual_actions
 
-    @stop_action ||= manual_actions.find_by(name: on_stop)
+    @stop_action ||= manual_actions.find { |action| action.name == self.on_stop }
   end
 
   def finished_at

app/models/environment.rb

@@ -24,13 +24,13 @@ class Environment < ApplicationRecord
   has_many :self_managed_prometheus_alert_events, inverse_of: :environment
   has_many :alert_management_alerts, class_name: 'AlertManagement::Alert', inverse_of: :environment
 
-  has_one :last_deployment, -> { success.order('deployments.id DESC') }, class_name: 'Deployment'
+  has_one :last_deployment, -> { success.order('deployments.id DESC') }, class_name: 'Deployment', inverse_of: :environment
   has_one :last_deployable, through: :last_deployment, source: 'deployable', source_type: 'CommitStatus'
   has_one :last_pipeline, through: :last_deployable, source: 'pipeline'
   has_one :last_visible_deployment, -> { visible.distinct_on_environment }, inverse_of: :environment, class_name: 'Deployment'
   has_one :last_visible_deployable, through: :last_visible_deployment, source: 'deployable', source_type: 'CommitStatus'
   has_one :last_visible_pipeline, through: :last_visible_deployable, source: 'pipeline'
-  has_one :upcoming_deployment, -> { running.order('deployments.id DESC') }, class_name: 'Deployment'
+  has_one :upcoming_deployment, -> { running.order('deployments.id DESC') }, class_name: 'Deployment', inverse_of: :environment
   has_one :latest_opened_most_severe_alert, -> { order_severity_with_open_prometheus_alert }, class_name: 'AlertManagement::Alert', inverse_of: :environment
 
   before_validation :nullify_external_url

app/serializers/environment_serializer.rb

@@ -23,7 +23,7 @@ class EnvironmentSerializer < BaseSerializer
           latest: super(item.latest, opts) }
       end
     else
-      super(resource, opts)
+      super(batch_load(resource), opts)
     end
   end
 
@@ -41,11 +41,59 @@ class EnvironmentSerializer < BaseSerializer
     # immediately.
     items = @paginator.paginate(items) if paginated?
 
-    environments = resource.where(id: items.map(&:last_id)).index_by(&:id)
+    environments = batch_load(resource.where(id: items.map(&:last_id)))
+    environments_by_id = environments.index_by(&:id)
 
     items.map do |item|
-      Item.new(item.folder, item.size, environments[item.last_id])
+      Item.new(item.folder, item.size, environments_by_id[item.last_id])
     end
   end
+
+  def batch_load(resource)
+    resource = resource.preload(environment_associations)
+
+    resource.all.tap do |environments|
+      environments.each do |environment|
+        # Batch loading the commits of the deployments
+        environment.last_deployment&.commit&.try(:lazy_author)
+        environment.upcoming_deployment&.commit&.try(:lazy_author)
+      end
+    end
+  end
+
+  def environment_associations
+    {
+      last_deployment: deployment_associations,
+      upcoming_deployment: deployment_associations,
+      project: project_associations
+    }
+  end
+
+  def deployment_associations
+    {
+      user: [],
+      cluster: [],
+      project: [],
+      deployable: {
+        user: [],
+        metadata: [],
+        pipeline: {
+          manual_actions: [],
+          scheduled_actions: []
+        },
+        project: project_associations
+      }
+    }
+  end
+
+  def project_associations
+    {
+      project_feature: [],
+      route: [],
+      namespace: :route
+    }
+  end
   # rubocop: enable CodeReuse/ActiveRecord
 end
+
+EnvironmentSerializer.prepend_if_ee('EE::EnvironmentSerializer')

changelogs/unreleased/optimize-environments-serializer.yml

+---
+title: Optimize environment serializer to reduce N+1 problems
+merge_request: 58748
+author:
+type: performance

ee/app/models/ee/project.rb

@@ -672,7 +672,7 @@ module EE
       key = "protected_environment_by_name:#{id}:#{environment_name}"
 
       ::Gitlab::SafeRequestStore.fetch(key) do
-        protected_environments.find_by(name: environment_name)
+        protected_environments.find { |pe| pe.name == environment_name }
       end
     end
 

ee/app/serializers/ee/environment_serializer.rb

+# frozen_string_literal: true
+
+module EE
+  module EnvironmentSerializer
+    extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
+
+    override :environment_associations
+    def environment_associations
+      super.deep_merge(latest_opened_most_severe_alert: [])
+    end
+
+    override :project_associations
+    def project_associations
+      super.deep_merge(protected_environments: [])
+    end
+  end
+end

ee/spec/models/project_spec.rb

@@ -1840,7 +1840,7 @@ RSpec.describe Project do
   end
 
   describe '#protected_environment_by_name' do
-    let_it_be(:project) { create(:project) }
+    let_it_be(:project, reload: true) { create(:project) }
 
     subject { project.protected_environment_by_name('production') }
 

ee/spec/serializers/ee/environment_serializer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::EnvironmentSerializer do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project, reload: true) { create(:project, :repository) }
+
+  before_all do
+    project.add_developer(user)
+  end
+
+  before do
+    stub_licensed_features(environment_alerts: true)
+  end
+
+  it_behaves_like 'avoid N+1 on environments serialization'
+
+  def create_environment_with_associations(project)
+    create(:environment, project: project).tap do |environment|
+      create(:deployment, :success, environment: environment, project: project)
+      create(:deployment, :running, environment: environment, project: project)
+      create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project)
+      prometheus_alert = create(:prometheus_alert, project: project, environment: environment)
+      create(:alert_management_alert, :triggered, :prometheus, project: project, environment: environment, prometheus_alert: prometheus_alert)
+    end
+  end
+end

spec/serializers/environment_serializer_spec.rb

@@ -3,8 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe EnvironmentSerializer do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
+  include CreateEnvironmentsHelpers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project, reload: true) { create(:project, :repository) }
 
   let(:json) do
     described_class
@@ -12,43 +14,18 @@ RSpec.describe EnvironmentSerializer do
       .represent(resource)
   end
 
-  before do
+  before_all do
     project.add_developer(user)
   end
 
-  context 'when there is a single object provided' do
-    let(:project) { create(:project, :repository) }
-    let(:deployable) { create(:ci_build) }
-    let(:deployment) do
-      create(:deployment, :success,
-                          deployable: deployable,
-                          user: user,
-                          project: project,
-                          sha: project.commit.id)
-    end
+  it_behaves_like 'avoid N+1 on environments serialization'
 
-    let(:resource) { deployment.environment }
-
-    before do
-      create(:ci_build, :manual, name: 'manual1', pipeline: deployable.pipeline)
-    end
-
-    it 'contains important elements of environment' do
-      expect(json)
-        .to include(:name, :external_url, :environment_path, :last_deployment)
-    end
-
-    it 'contains relevant information about last deployment' do
-      last_deployment = json.fetch(:last_deployment)
+  context 'when there is a collection of objects provided' do
+    let(:resource) { project.environments }
 
-      expect(last_deployment)
-        .to include(:ref, :user, :commit, :deployable, :manual_actions)
+    before_all do
+      create_list(:environment, 2, project: project)
     end
-  end
-
-  context 'when there is a collection of objects provided' do
-    let(:project) { create(:project) }
-    let(:resource) { create_list(:environment, 2) }
 
     it 'contains important elements of environment' do
       expect(json.first)
@@ -207,4 +184,11 @@ RSpec.describe EnvironmentSerializer do
       end
     end
   end
+
+  def create_environment_with_associations(project)
+    create(:environment, project: project).tap do |environment|
+      create(:deployment, :success, environment: environment, project: project)
+      create(:deployment, :running, environment: environment, project: project)
+    end
+  end
 end

spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb

+# frozen_string_literal: true
+RSpec.shared_examples 'avoid N+1 on environments serialization' do
+  it 'avoids N+1 database queries with grouping', :request_store do
+    create_environment_with_associations(project)
+
+    control = ActiveRecord::QueryRecorder.new { serialize(grouping: true) }
+
+    create_environment_with_associations(project)
+
+    expect { serialize(grouping: true) }.not_to exceed_query_limit(control.count)
+  end
+
+  it 'avoids N+1 database queries without grouping', :request_store do
+    create_environment_with_associations(project)
+
+    control = ActiveRecord::QueryRecorder.new { serialize(grouping: false) }
+
+    create_environment_with_associations(project)
+
+    expect { serialize(grouping: false) }.not_to exceed_query_limit(control.count)
+  end
+
+  def serialize(grouping:)
+    EnvironmentSerializer.new(current_user: user, project: project).yield_self do |serializer|
+      serializer.within_folders if grouping
+      serializer.represent(Environment.where(project: project))
+    end
+  end
+end