Read about our upcoming Code of Conduct on this issue

Commit c8b63a28 authored by Douwe Maan's avatar Douwe Maan
Browse files

Improve performance of finding last deployed environment

parent 41d40a8e79af
......@@ -31,7 +31,7 @@ def create
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 @@ def define_commit_vars
@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 @@ def define_diff_vars
@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
......
......@@ -103,7 +103,7 @@ def diffs
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|
......@@ -248,7 +248,7 @@ def new_diffs
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) }
......
......@@ -9,6 +9,7 @@ class Build < CommitStatus
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
......@@ -183,10 +184,6 @@ def outdated_deployment?
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 @@ def recently_updated_on_branch?(ref)
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 @@ def includes_commit?(commit)
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
......
......@@ -720,21 +720,15 @@ def environments
@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"
......
......@@ -1306,7 +1306,7 @@ def reset_pushes_since_gc
Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
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 = ?'
......@@ -1322,22 +1322,17 @@ def environments_for(ref, commit: nil, with_tags: false)
.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
......
......@@ -1190,6 +1190,7 @@ def check_tree_entry_for_dir(branch_name, path)
def route_map_for(sha)
blob = blob_at(sha, ROUTE_MAP_PATH)
return unless blob
blob.load_all_data!(self)
blob.data
end
......@@ -1197,6 +1198,7 @@ def route_map_for(sha)
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 @@ def self.nulls_last_order(field, direction = 'ASC')
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 @@ class MigrationTest
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 @@
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
......
......@@ -1069,30 +1069,6 @@
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) }
......
......@@ -1726,17 +1726,17 @@ def create_build(new_pipeline = pipeline, name = 'test')
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
......@@ -1747,22 +1747,22 @@ def create_build(new_pipeline = pipeline, name = 'test')
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
......@@ -1773,48 +1773,12 @@ def create_build(new_pipeline = pipeline, name = 'test')
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