Merge branch 'dz-refactor-environments-finder' into 'master'

Split EnvironmentsFinder into two

See merge request gitlab-org/gitlab!58271

.rubocop_manual_todo.yml

@@ -2010,6 +2010,7 @@ Gitlab/NamespacedClass:
     - 'app/finders/deployments_finder.rb'
     - 'app/finders/environment_names_finder.rb'
     - 'app/finders/environments_finder.rb'
+    - 'app/finders/environments_by_deployments_finder.rb'
     - 'app/finders/events_finder.rb'
     - 'app/finders/feature_flags_finder.rb'
     - 'app/finders/feature_flags_user_lists_finder.rb'

app/controllers/projects/blame_controller.rb

@@ -20,7 +20,7 @@ class Projects::BlameController < Projects::ApplicationController
 
     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
+    @environment = EnvironmentsByDeploymentsFinder.new(@project, current_user, environment_params).execute.last
 
     @blame = Gitlab::Blame.new(@blob, @commit)
     @blame = Gitlab::View::Presenter::Factory.new(@blame, project: @project, path: @path).fabricate!

app/controllers/projects/blob_controller.rb

@@ -214,7 +214,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
+    @environment = EnvironmentsByDeploymentsFinder.new(@project, current_user, environment_params).execute.last
     @last_commit = @repository.last_commit_for_path(@commit.id, @blob.path, literal_pathspec: true)
     @code_navigation_path = Gitlab::CodeNavigationPath.new(@project, @blob.commit_id).full_json_path_for(@blob.path)
 

app/controllers/projects/commit_controller.rb

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

app/controllers/projects/compare_controller.rb

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

app/finders/environments_by_deployments_finder.rb

+# frozen_string_literal: true
+
+class EnvironmentsByDeploymentsFinder
+  attr_reader :project, :current_user, :params
+
+  def initialize(project, current_user, params = {})
+    @project, @current_user, @params = project, current_user, params
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def execute
+    deployments = project.deployments
+    deployments =
+      if ref
+        deployments_query = params[:with_tags] ? 'ref = :ref OR tag IS TRUE' : 'ref = :ref'
+        deployments.where(deployments_query, ref: ref.to_s)
+      elsif commit
+        deployments.where(sha: commit.sha)
+      else
+        deployments.none
+      end
+
+    environment_ids = deployments
+      .group(:environment_id)
+      .select(:environment_id)
+
+    environments = project.environments.available
+      .where(id: environment_ids)
+
+    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
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
+  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
+
+  def commit
+    params[:commit]
+  end
+end

app/finders/environments_finder.rb

@@ -9,34 +9,6 @@ class EnvironmentsFinder
     @project, @current_user, @params = project, current_user, params
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def execute
-    deployments = project.deployments
-    deployments =
-      if ref
-        deployments_query = params[:with_tags] ? 'ref = :ref OR tag IS TRUE' : 'ref = :ref'
-        deployments.where(deployments_query, ref: ref.to_s)
-      elsif commit
-        deployments.where(sha: commit.sha)
-      else
-        deployments.none
-      end
-
-    environment_ids = deployments
-      .group(:environment_id)
-      .select(:environment_id)
-
-    environments = project.environments.available
-      .where(id: environment_ids)
-
-    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
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
   # This method will eventually take the place of `#execute` as an
   # efficient way to get relevant environment entries.
   # Currently, `#execute` method has a serious technical debt and
@@ -55,32 +27,6 @@ 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
-
-  def commit
-    params[:commit]
-  end
-
   def by_name(environments)
     if params[:name].present?
       environments.for_name(params[:name])

app/models/merge_request.rb

@@ -1367,11 +1367,11 @@ class MergeRequest < ApplicationRecord
   def environments_for(current_user, latest: false)
     return [] unless diff_head_commit
 
-    envs = EnvironmentsFinder.new(target_project, current_user,
+    envs = EnvironmentsByDeploymentsFinder.new(target_project, current_user,
       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,
+      envs.concat EnvironmentsByDeploymentsFinder.new(source_project, current_user,
         ref: source_branch, commit: diff_head_commit, find_latest: latest).execute
     end
 

app/services/ci/stop_environments_service.rb

@@ -35,7 +35,7 @@ module Ci
     private
 
     def environments
-      @environments ||= EnvironmentsFinder
+      @environments ||= EnvironmentsByDeploymentsFinder
         .new(project, current_user, ref: @ref, recently_updated: true)
         .execute
     end

spec/finders/environments_by_deployments_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EnvironmentsByDeploymentsFinder do
+  let(:project) { create(:project, :repository) }
+  let(:user) { project.creator }
+  let(:environment) { create(:environment, :available, project: project) }
+
+  before do
+    project.add_maintainer(user)
+  end
+
+  describe '#execute' do
+    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.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: 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: commit).execute)
+          .to be_empty
+      end
+
+      it 'does not return environment when commit is not part of deployment' do
+        expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
+          .to be_empty
+      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
+      before do
+        create(:deployment, :success, environment: environment, ref: 'master', sha: project.commit.id)
+      end
+
+      it 'returns environment when ref is set' do
+        expect(described_class.new(project, user, ref: 'master', commit: project.commit).execute)
+          .to contain_exactly(environment)
+      end
+
+      it 'does not environment when ref is different' do
+        expect(described_class.new(project, user, ref: 'feature', commit: project.commit).execute)
+          .to be_empty
+      end
+
+      it 'does not return environment when commit is not part of deployment' do
+        expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
+          .to be_empty
+      end
+
+      it 'returns environment when commit constraint is not set' do
+        expect(described_class.new(project, user, ref: 'master').execute)
+          .to contain_exactly(environment)
+      end
+    end
+
+    context 'commit deployment' do
+      before do
+        create(:deployment, :success, environment: environment, ref: 'master', sha: project.commit.id)
+      end
+
+      it 'returns environment' do
+        expect(described_class.new(project, user, commit: project.commit).execute)
+          .to contain_exactly(environment)
+      end
+    end
+
+    context 'recently updated' do
+      context 'when last deployment to environment is the most recent one' do
+        before do
+          create(:deployment, :success, environment: environment, ref: 'feature')
+        end
+
+        it 'finds recently updated environment' do
+          expect(described_class.new(project, user, ref: 'feature', recently_updated: true).execute)
+            .to contain_exactly(environment)
+        end
+      end
+
+      context 'when last deployment to environment is not the most recent' do
+        before do
+          create(:deployment, :success, environment: environment, ref: 'feature')
+          create(:deployment, :success, environment: environment, ref: 'master')
+        end
+
+        it 'does not find environment' do
+          expect(described_class.new(project, user, ref: 'feature', recently_updated: true).execute)
+            .to be_empty
+        end
+      end
+
+      context 'when there are two environments that deploy to the same branch' do
+        let(:second_environment) { create(:environment, project: project) }
+
+        before do
+          create(:deployment, :success, environment: environment, ref: 'feature')
+          create(:deployment, :success, environment: second_environment, ref: 'feature')
+        end
+
+        it 'finds both environments' do
+          expect(described_class.new(project, user, ref: 'feature', recently_updated: true).execute)
+            .to contain_exactly(environment, second_environment)
+        end
+      end
+    end
+  end
+end

spec/finders/environments_finder_spec.rb

@@ -11,120 +11,6 @@ RSpec.describe EnvironmentsFinder do
     project.add_maintainer(user)
   end
 
-  describe '#execute' do
-    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.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: 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: commit).execute)
-          .to be_empty
-      end
-
-      it 'does not return environment when commit is not part of deployment' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
-          .to be_empty
-      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
-      before do
-        create(:deployment, :success, environment: environment, ref: 'master', sha: project.commit.id)
-      end
-
-      it 'returns environment when ref is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit).execute)
-          .to contain_exactly(environment)
-      end
-
-      it 'does not environment when ref is different' do
-        expect(described_class.new(project, user, ref: 'feature', commit: project.commit).execute)
-          .to be_empty
-      end
-
-      it 'does not return environment when commit is not part of deployment' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
-          .to be_empty
-      end
-
-      it 'returns environment when commit constraint is not set' do
-        expect(described_class.new(project, user, ref: 'master').execute)
-          .to contain_exactly(environment)
-      end
-    end
-
-    context 'commit deployment' do
-      before do
-        create(:deployment, :success, environment: environment, ref: 'master', sha: project.commit.id)
-      end
-
-      it 'returns environment' do
-        expect(described_class.new(project, user, commit: project.commit).execute)
-          .to contain_exactly(environment)
-      end
-    end
-
-    context 'recently updated' do
-      context 'when last deployment to environment is the most recent one' do
-        before do
-          create(:deployment, :success, environment: environment, ref: 'feature')
-        end
-
-        it 'finds recently updated environment' do
-          expect(described_class.new(project, user, ref: 'feature', recently_updated: true).execute)
-            .to contain_exactly(environment)
-        end
-      end
-
-      context 'when last deployment to environment is not the most recent' do
-        before do
-          create(:deployment, :success, environment: environment, ref: 'feature')
-          create(:deployment, :success, environment: environment, ref: 'master')
-        end
-
-        it 'does not find environment' do
-          expect(described_class.new(project, user, ref: 'feature', recently_updated: true).execute)
-            .to be_empty
-        end
-      end
-
-      context 'when there are two environments that deploy to the same branch' do
-        let(:second_environment) { create(:environment, project: project) }
-
-        before do
-          create(:deployment, :success, environment: environment, ref: 'feature')
-          create(:deployment, :success, environment: second_environment, ref: 'feature')
-        end
-
-        it 'finds both environments' do
-          expect(described_class.new(project, user, ref: 'feature', recently_updated: true).execute)
-            .to contain_exactly(environment, second_environment)
-        end
-      end
-    end
-  end
-
   describe '#find' do
     context 'with states parameter' do
       let(:stopped_environment) { create(:environment, :stopped, project: project) }