Commit 8d6fb34c authored by Douwe Maan's avatar Douwe Maan

Improve performance of finding last deployed environment

parent efe4ac6f
......@@ -31,7 +31,7 @@ class Projects::BlobController < Projects::ApplicationController
def show
branch_name = @ref if @repository.branch_exists?(@ref)
@environment = @project.latest_environment_for(@commit, ref: branch_name)
@environment = @project.environments_for(commit: @commit, ref: branch_name).last
@environment = nil unless can?(current_user, :read_environment, @environment)
end
......
......@@ -96,7 +96,7 @@ class Projects::CommitController < Projects::ApplicationController
@diffs = commit.diffs(opts)
@notes_count = commit.notes.count
@environment = @project.latest_environment_for(@commit)
@environment = @project.environments_for(commit: @commit).last
@environment = nil unless can?(current_user, :read_environment, @environment)
end
......
......@@ -58,7 +58,7 @@ class Projects::CompareController < Projects::ApplicationController
@diffs = @compare.diffs(diff_options)
branch_name = @head_ref if @repository.branch_exists?(@head_ref)
@environment = @project.latest_environment_for(@commit, ref: branch_name)
@environment = @project.environments_for(commit: @commit, ref: branch_name).last
@environment = nil unless can?(current_user, :read_environment, @environment)
@diff_notes_disabled = true
......
......@@ -10,7 +10,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
def index
@scope = params[:scope]
@environments = project.environments
@environments = project.environments.includes(:last_deployment)
respond_to do |format|
format.html
......
......@@ -106,7 +106,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
end
@environment = @merge_request.latest_environment
@environment = @merge_request.environments.last
@environment = nil unless can?(current_user, :read_environment, @environment)
respond_to do |format|
......@@ -251,7 +251,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
@diff_notes_disabled = true
@environment = @merge_request.latest_environment
@environment = @merge_request.environments.last
@environment = nil unless can?(current_user, :read_environment, @environment)
render json: { html: view_to_html_string('projects/merge_requests/_new_diffs', diffs: @diffs, environment: @environment) }
......
......@@ -10,6 +10,7 @@ module Ci
belongs_to :erased_by, class_name: 'User'
has_many :deployments, as: :deployable
has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment'
# The "environment" field for builds is a String, and is the unexpanded name
def persisted_environment
......@@ -184,10 +185,6 @@ module Ci
success? && !last_deployment.try(:last?)
end
def last_deployment
deployments.last
end
def depends_on_builds
# Get builds of the same type
latest_builds = self.pipeline.builds.latest
......
......@@ -6,7 +6,8 @@ class Environment < ActiveRecord::Base
belongs_to :project, required: true, validate: true
has_many :deployments
has_many :deployments, dependent: :destroy
has_one :last_deployment, -> { order('deployments.id DESC') }, class_name: 'Deployment'
before_validation :nullify_external_url
before_validation :generate_slug, if: ->(env) { env.slug.blank? }
......@@ -37,6 +38,7 @@ class Environment < ActiveRecord::Base
scope :available, -> { with_state(:available) }
scope :stopped, -> { with_state(:stopped) }
scope :order_by_last_deployed_at, -> { order(Gitlab::Database.nulls_first_order('(SELECT MAX(id) FROM deployments WHERE deployments.environment_id = environments.id)', 'ASC')) }
state_machine :state, initial: :available do
event :start do
......@@ -51,14 +53,6 @@ class Environment < ActiveRecord::Base
state :stopped
end
def self.latest_for_commit(environments, commit)
environments.sort_by do |environment|
deployment = environment.first_deployment_for(commit)
deployment.try(:created_at) || DateTime.parse('1970-01-01')
end.last
end
def predefined_variables
[
{ key: 'CI_ENVIRONMENT_NAME', value: name, public: true },
......@@ -70,10 +64,6 @@ class Environment < ActiveRecord::Base
ref.to_s == last_deployment.try(:ref)
end
def last_deployment
deployments.last
end
def nullify_external_url
self.external_url = nil if self.external_url.blank?
end
......@@ -95,6 +85,10 @@ class Environment < ActiveRecord::Base
last_deployment.includes_commit?(commit)
end
def last_deployed_at
last_deployment.try(:created_at)
end
def update_merge_request_metrics?
(environment_type || name) == "production"
end
......
......@@ -743,21 +743,15 @@ class MergeRequest < ActiveRecord::Base
@environments ||= begin
target_envs = target_project.environments_for(
target_branch, commit: diff_head_commit, with_tags: true)
ref: target_branch, commit: diff_head_commit, with_tags: true)
source_envs = source_project.environments_for(
source_branch, commit: diff_head_commit) if source_project
ref: source_branch, commit: diff_head_commit) if source_project
(target_envs.to_a + source_envs.to_a).uniq
end
end
def latest_environment
return @latest_environment if defined?(@latest_environment)
@latest_environment = Environment.latest_for_commit(environments, diff_head_commit)
end
def state_human_name
if merged?
"Merged"
......
......@@ -1543,7 +1543,7 @@ class Project < ActiveRecord::Base
size_in_bytes + repository_and_lfs_size > actual_size_limit)
end
def environments_for(ref, commit: nil, with_tags: false)
def environments_for(ref: nil, commit: nil, with_tags: false)
deps =
if ref
deployments_query = with_tags ? 'ref = ? OR tag IS TRUE' : 'ref = ?'
......@@ -1559,22 +1559,17 @@ class Project < ActiveRecord::Base
.select(:environment_id)
environments_found = environments.available
.where(id: environment_ids).to_a
.where(id: environment_ids).order_by_last_deployed_at.to_a
return environments_found unless commit
return environments_found unless ref && commit
environments_found.select do |environment|
environment.includes_commit?(commit)
end
end
def latest_environment_for(commit, ref: nil)
environments = environments_for(ref, commit: commit)
Environment.latest_for_commit(environments, commit)
end
def environments_recently_updated_on_branch(branch)
environments_for(branch).select do |environment|
environments_for(ref: branch).select do |environment|
environment.recently_updated_on_branch?(branch)
end
end
......
......@@ -1343,6 +1343,7 @@ class Repository
def route_map_for(sha)
blob = blob_at(sha, ROUTE_MAP_PATH)
return unless blob
blob.load_all_data!(self)
blob.data
end
......@@ -1350,6 +1351,7 @@ class Repository
def gitlab_ci_yml_for(sha)
blob = blob_at(sha, GITLAB_CI_YML_PATH)
return unless blob
blob.load_all_data!(self)
blob.data
end
......
......@@ -35,6 +35,20 @@ module Gitlab
order
end
def self.nulls_first_order(field, direction = 'ASC')
order = "#{field} #{direction}"
if Gitlab::Database.postgresql?
order << ' NULLS FIRST'
else
# `field IS NULL` will be `0` for non-NULL columns and `1` for NULL
# columns. In the (default) ascending order, `0` comes first.
order.prepend("#{field} IS NULL, ") if direction == 'DESC'
end
order
end
def self.random
Gitlab::Database.postgresql? ? "RANDOM()" : "RAND()"
end
......
......@@ -55,6 +55,22 @@ describe Gitlab::Database, lib: true do
end
end
describe '.nulls_first_order' do
context 'when using PostgreSQL' do
before { expect(described_class).to receive(:postgresql?).and_return(true) }
it { expect(described_class.nulls_first_order('column', 'ASC')).to eq 'column ASC NULLS FIRST'}
it { expect(described_class.nulls_first_order('column', 'DESC')).to eq 'column DESC NULLS FIRST'}
end
context 'when using MySQL' do
before { expect(described_class).to receive(:postgresql?).and_return(false) }
it { expect(described_class.nulls_first_order('column', 'ASC')).to eq 'column ASC'}
it { expect(described_class.nulls_first_order('column', 'DESC')).to eq 'column IS NULL, column DESC'}
end
end
describe '#true_value' do
it 'returns correct value for PostgreSQL' do
expect(described_class).to receive(:postgresql?).and_return(true)
......
......@@ -22,22 +22,17 @@ describe Environment, models: true do
it { is_expected.to validate_length_of(:external_url).is_at_most(255) }
it { is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id) }
describe '.latest_for_commit' do
describe '.order_by_last_deployed_at' do
let(:project) { create(:project) }
let!(:environment1) { create(:environment, project: project) }
let!(:environment2) { create(:environment, project: project) }
let!(:environment3) { create(:environment, project: project) }
let!(:deployment1) { create(:deployment, environment: environment1) }
let!(:deployment2) { create(:deployment, environment: environment2) }
let(:commit) { RepoHelpers.sample_commit }
before do
allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1)
allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2)
allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil)
end
let!(:deployment1) { create(:deployment, environment: environment1) }
it 'returns the environment that the commit was last deployed to' do
expect(Environment.latest_for_commit([environment1, environment2, environment3], commit)).to be(environment2)
it 'returns the environments in order of having been last deployed' do
# byebug
expect(project.environments.order_by_last_deployed_at.to_a).to eq([environment3, environment2, environment1])
end
end
......
......@@ -1326,30 +1326,6 @@ describe MergeRequest, models: true do
end
end
describe '#latest_environment' do
let(:project) { subject.project }
let!(:environment1) { create(:environment, project: project) }
let!(:environment2) { create(:environment, project: project) }
let!(:environment3) { create(:environment, project: project) }
let!(:deployment1) { create(:deployment, environment: environment1, ref: 'master', sha: commit.id) }
let!(:deployment2) { create(:deployment, environment: environment2, ref: 'feature', sha: commit.id) }
let(:commit) { subject.diff_head_commit }
before do
allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1)
allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2)
allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil)
end
before do
allow(subject).to receive(:environments).and_return([environment1, environment2, environment3])
end
it 'returns the environment that the commit was last deployed to' do
expect(subject.latest_environment).to eq(environment2)
end
end
describe "#reload_diff" do
let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
......
......@@ -2077,17 +2077,17 @@ describe Project, models: true do
end
it 'returns environment when with_tags is set' do
expect(project.environments_for('master', commit: project.commit, with_tags: true))
expect(project.environments_for(ref: 'master', commit: project.commit, with_tags: true))
.to contain_exactly(environment)
end
it 'does not return environment when no with_tags is set' do
expect(project.environments_for('master', commit: project.commit))
expect(project.environments_for(ref: 'master', commit: project.commit))
.to be_empty
end
it 'does not return environment when commit is not part of deployment' do
expect(project.environments_for('master', commit: project.commit('feature')))
expect(project.environments_for(ref: 'master', commit: project.commit('feature')))
.to be_empty
end
end
......@@ -2098,22 +2098,22 @@ describe Project, models: true do
end
it 'returns environment when ref is set' do
expect(project.environments_for('master', commit: project.commit))
expect(project.environments_for(ref: 'master', commit: project.commit))
.to contain_exactly(environment)
end
it 'does not environment when ref is different' do
expect(project.environments_for('feature', commit: project.commit))
expect(project.environments_for(ref: 'feature', commit: project.commit))
.to be_empty
end
it 'does not return environment when commit is not part of deployment' do
expect(project.environments_for('master', commit: project.commit('feature')))
expect(project.environments_for(ref: 'master', commit: project.commit('feature')))
.to be_empty
end
it 'returns environment when commit constraint is not set' do
expect(project.environments_for('master'))
expect(project.environments_for(ref: 'master'))
.to contain_exactly(environment)
end
end
......@@ -2124,48 +2124,12 @@ describe Project, models: true do
end
it 'returns environment' do
expect(project.environments_for(nil, commit: project.commit))
expect(project.environments_for(commit: project.commit))
.to contain_exactly(environment)
end
end
end
describe '#latest_environment_for' do
let(:project) { create(:project) }
let!(:environment1) { create(:environment, project: project) }
let!(:environment2) { create(:environment, project: project) }
let!(:environment3) { create(:environment, project: project) }
let!(:deployment1) { create(:deployment, environment: environment1, ref: 'master', sha: commit.id) }
let!(:deployment2) { create(:deployment, environment: environment2, ref: 'feature', sha: commit.id) }
let(:commit) { project.commit }
before do
allow(environment1).to receive(:first_deployment_for).with(commit).and_return(deployment1)
allow(environment2).to receive(:first_deployment_for).with(commit).and_return(deployment2)
allow(environment3).to receive(:first_deployment_for).with(commit).and_return(nil)
end
context 'when specifying a ref' do
before do
allow(project).to receive(:environments_for).with('master', commit: commit).and_return([environment1])
end
it 'returns the environment that the commit was last deployed to from that ref' do
expect(project.latest_environment_for(commit, ref: 'master')).to eq(environment1)
end
end
context 'when not specifying a ref' do
before do
allow(project).to receive(:environments_for).with(nil, commit: commit).and_return([environment1, environment2])
end
it 'returns the environment that the commit was last deployed to' do
expect(project.latest_environment_for(commit)).to eq(environment2)
end
end
end
describe '#environments_recently_updated_on_branch' do
let(:project) { create(:project, :repository) }
let(:environment) { create(:environment, project: project) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment