Merge branch 'sh-optimize-commit-is-ancestor-env' into 'master'

Reduce CommitIsAncestor RPCs with environments

Closes #29562

See merge request gitlab-org/gitlab!21778

app/controllers/projects/blame_controller.rb

@@ -17,6 +17,7 @@ class Projects::BlameController < Projects::ApplicationController
     end
 
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
+    environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
 
     @blame_groups = Gitlab::Blame.new(@blob, @commit).groups

app/controllers/projects/blob_controller.rb

@@ -205,6 +205,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   def show_html
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
+    environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
     @last_commit = @repository.last_commit_for_path(@commit.id, @blob.path)
 

app/controllers/projects/commit_controller.rb

@@ -151,7 +151,7 @@ class Projects::CommitController < Projects::ApplicationController
     @diffs = commit.diffs(opts)
     @notes_count = commit.notes.count
 
-    @environment = EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last
+    @environment = EnvironmentsFinder.new(@project, current_user, commit: @commit, find_latest: true).execute.last
   end
 
   # rubocop: disable CodeReuse/ActiveRecord

app/controllers/projects/compare_controller.rb

@@ -101,6 +101,7 @@ class Projects::CompareController < Projects::ApplicationController
   def define_environment
     if compare
       environment_params = @repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit }
+      environment_params[:find_latest] = true
       @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
     end
   end

app/controllers/projects/merge_requests/creations_controller.rb

@@ -52,7 +52,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
     @diff_notes_disabled = true
 
-    @environment = @merge_request.environments_for(current_user).last
+    @environment = @merge_request.environments_for(current_user, latest: true).last
 
     render json: { html: view_to_html_string('projects/merge_requests/creations/_diffs', diffs: @diffs, environment: @environment) }
   end

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -51,7 +51,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def render_diffs
     diffs = @compare.diffs(diff_options)
-    @environment = @merge_request.environments_for(current_user).last
+    @environment = @merge_request.environments_for(current_user, latest: true).last
 
     diffs.unfold_diff_files(note_positions.unfoldable)
     diffs.write_cache

app/finders/environments_finder.rb

@@ -25,25 +25,13 @@ class EnvironmentsFinder
       .select(:environment_id)
 
     environments = project.environments.available
-      .where(id: environment_ids).order_by_last_deployed_at.to_a
+      .where(id: environment_ids)
 
-    environments.select! do |environment|
-      Ability.allowed?(current_user, :read_environment, environment)
-    end
-
-    if ref && commit
-      environments.select! do |environment|
-        environment.includes_commit?(commit)
-      end
-    end
-
-    if ref && params[:recently_updated]
-      environments.select! do |environment|
-        environment.recently_updated_on_branch?(ref)
-      end
+    if params[:find_latest]
+      find_one(environments.order_by_last_deployed_at_desc)
+    else
+      find_all(environments.order_by_last_deployed_at.to_a)
     end
-
-    environments
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -62,6 +50,24 @@ class EnvironmentsFinder
 
   private
 
+  def find_one(environments)
+    [environments.find { |environment| valid_environment?(environment) }].compact
+  end
+
+  def find_all(environments)
+    environments.select { |environment| valid_environment?(environment) }
+  end
+
+  def valid_environment?(environment)
+    # Go in order of cost: SQL calls are cheaper than Gitaly calls
+    return false unless Ability.allowed?(current_user, :read_environment, environment)
+
+    return false if ref && params[:recently_updated] && !environment.recently_updated_on_branch?(ref)
+    return false if ref && commit && !environment.includes_commit?(commit)
+
+    true
+  end
+
   def ref
     params[:ref].try(:to_s)
   end

app/models/environment.rb

@@ -48,13 +48,14 @@ class Environment < ApplicationRecord
 
   scope :available, -> { with_state(:available) }
   scope :stopped, -> { with_state(:stopped) }
+
   scope :order_by_last_deployed_at, -> do
-    max_deployment_id_sql =
-      Deployment.select(Deployment.arel_table[:id].maximum)
-      .where(Deployment.arel_table[:environment_id].eq(arel_table[:id]))
-      .to_sql
     order(Gitlab::Database.nulls_first_order("(#{max_deployment_id_sql})", 'ASC'))
   end
+  scope :order_by_last_deployed_at_desc, -> do
+    order(Gitlab::Database.nulls_last_order("(#{max_deployment_id_sql})", 'DESC'))
+  end
+
   scope :in_review_folder, -> { where(environment_type: "review") }
   scope :for_name, -> (name) { where(name: name) }
   scope :preload_cluster, -> { preload(last_deployment: :cluster) }
@@ -90,6 +91,12 @@ class Environment < ApplicationRecord
     end
   end
 
+  def self.max_deployment_id_sql
+    Deployment.select(Deployment.arel_table[:id].maximum)
+    .where(Deployment.arel_table[:environment_id].eq(arel_table[:id]))
+    .to_sql
+  end
+
   def self.pluck_names
     pluck(:name)
   end

app/models/merge_request.rb

@@ -1122,22 +1122,18 @@ class MergeRequest < ApplicationRecord
     actual_head_pipeline.success?
   end
 
-  def environments_for(current_user)
+  def environments_for(current_user, latest: false)
     return [] unless diff_head_commit
 
-    @environments ||= Hash.new do |h, current_user|
     envs = EnvironmentsFinder.new(target_project, current_user,
-        ref: target_branch, commit: diff_head_commit, with_tags: true).execute
+      ref: target_branch, commit: diff_head_commit, with_tags: true, find_latest: latest).execute
 
     if source_project
       envs.concat EnvironmentsFinder.new(source_project, current_user,
-          ref: source_branch, commit: diff_head_commit).execute
+        ref: source_branch, commit: diff_head_commit, find_latest: latest).execute
     end
 
-      h[current_user] = envs.uniq
-    end
-
-    @environments[current_user]
+    envs.uniq
   end
 
   ##

changelogs/unreleased/sh-optimize-commit-is-ancestor-env.yml

+---
+title: Reduce CommitIsAncestor RPCs with environments
+merge_request: 21778
+author:
+type: performance

spec/finders/environments_finder_spec.rb

@@ -13,17 +13,22 @@ describe EnvironmentsFinder do
     end
 
     context 'tagged deployment' do
+      let(:environment_two) { create(:environment, project: project) }
+      # Environments need to include commits, so rewind two commits to fit
+      let(:commit) { project.commit('HEAD~2') }
+
       before do
-        create(:deployment, :success, environment: environment, ref: 'v1.1.0', tag: true, sha: project.commit.id)
+        create(:deployment, :success, environment: environment, ref: 'v1.0.0', tag: true, sha: project.commit.id)
+        create(:deployment, :success, environment: environment_two, ref: 'v1.1.0', tag: true, sha: project.commit('HEAD~1').id)
       end
 
       it 'returns environment when with_tags is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit, with_tags: true).execute)
-          .to contain_exactly(environment)
+        expect(described_class.new(project, user, ref: 'master', commit: commit, with_tags: true).execute)
+          .to contain_exactly(environment, environment_two)
       end
 
       it 'does not return environment when no with_tags is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit).execute)
+        expect(described_class.new(project, user, ref: 'master', commit: commit).execute)
           .to be_empty
       end
 
@@ -31,6 +36,21 @@ describe EnvironmentsFinder do
         expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
           .to be_empty
       end
+
+      it 'returns environment when with_tags is set' do
+        expect(described_class.new(project, user, ref: 'master', commit: commit, with_tags: true).execute)
+          .to contain_exactly(environment, environment_two)
+      end
+
+      # We expect two Gitaly calls: FindCommit, CommitIsAncestor
+      # This tests to ensure we don't call one CommitIsAncestor per environment
+      it 'only calls Gitaly twice when multiple environments are present', :request_store do
+        expect do
+          result = described_class.new(project, user, ref: 'master', commit: commit, with_tags: true, find_latest: true).execute
+
+          expect(result).to contain_exactly(environment_two)
+        end.to change { Gitlab::GitalyClient.get_request_count }.by(2)
+      end
     end
 
     context 'branch deployment' do

spec/models/environment_spec.rb

@@ -36,9 +36,13 @@ describe Environment, :use_clean_rails_memory_store_caching do
     let!(:deployment2) { create(:deployment, environment: environment2) }
     let!(:deployment3) { create(:deployment, environment: environment1) }
 
-    it 'returns the environments in order of having been last deployed' do
+    it 'returns the environments in ascending order of having been last deployed' do
       expect(project.environments.order_by_last_deployed_at.to_a).to eq([environment3, environment2, environment1])
     end
+
+    it 'returns the environments in descending order of having been last deployed' do
+      expect(project.environments.order_by_last_deployed_at_desc.to_a).to eq([environment1, environment2, environment3])
+    end
   end
 
   describe 'state machine' do

spec/models/merge_request_spec.rb

@@ -2322,6 +2322,10 @@ describe MergeRequest do
     let(:project)       { create(:project, :repository) }
     let(:user)          { project.creator }
     let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:source_branch) { merge_request.source_branch }
+    let(:target_branch) { merge_request.target_branch }
+    let(:source_oid) { project.commit(source_branch).id }
+    let(:target_oid) { project.commit(target_branch).id }
 
     before do
       merge_request.source_project.add_maintainer(user)
@@ -2332,13 +2336,21 @@ describe MergeRequest do
       let(:environments) { create_list(:environment, 3, project: project) }
 
       before do
-        create(:deployment, :success, environment: environments.first, ref: 'master', sha: project.commit('master').id)
-        create(:deployment, :success, environment: environments.second, ref: 'feature', sha: project.commit('feature').id)
+        create(:deployment, :success, environment: environments.first, ref: source_branch, sha: source_oid)
+        create(:deployment, :success, environment: environments.second, ref: target_branch, sha: target_oid)
       end
 
       it 'selects deployed environments' do
         expect(merge_request.environments_for(user)).to contain_exactly(environments.first)
       end
+
+      it 'selects latest deployed environment' do
+        latest_environment = create(:environment, project: project)
+        create(:deployment, :success, environment: latest_environment, ref: source_branch, sha: source_oid)
+
+        expect(merge_request.environments_for(user)).to eq([environments.first, latest_environment])
+        expect(merge_request.environments_for(user, latest: true)).to contain_exactly(latest_environment)
+      end
     end
 
     context 'with environments on source project' do