diff options
4 files changed, 67 insertions, 12 deletions
diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb index 465a42759df..d7dc64190d6 100644 --- a/app/models/environment_status.rb +++ b/app/models/environment_status.rb @@ -40,7 +40,7 @@ class EnvironmentStatus end def changes - return [] if project.route_map_for(sha).nil? + return [] unless has_route_map? changed_files.map { |file| build_change(file) }.compact end @@ -50,6 +50,10 @@ class EnvironmentStatus .merge_request_diff_files.where(deleted_file: false) end + def has_route_map? + project.route_map_for(sha).present? + end + private PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze diff --git a/app/models/project.rb b/app/models/project.rb index bfc35b77b8f..3648025b8fa 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1914,9 +1914,8 @@ class Project < ApplicationRecord @route_maps_by_commit ||= Hash.new do |h, sha| h[sha] = begin data = repository.route_map_for(sha) - next unless data - Gitlab::RouteMap.new(data) + Gitlab::RouteMap.new(data) if data rescue Gitlab::RouteMap::FormatError nil end diff --git a/changelogs/unreleased/backstage-gb-improve-performance-environment-statuses-endpoint.yml b/changelogs/unreleased/backstage-gb-improve-performance-environment-statuses-endpoint.yml new file mode 100644 index 00000000000..f614e076268 --- /dev/null +++ b/changelogs/unreleased/backstage-gb-improve-performance-environment-statuses-endpoint.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance of fetching environments statuses +merge_request: 30560 +author: +type: performance diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 9878f88a395..cc6adc0a6c6 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -878,7 +878,7 @@ describe Projects::MergeRequestsController do expect(control_count).to be <= 137 end - it 'has no N+1 issues for environments', :request_store, retry: 0 do + it 'has no N+1 SQL issues for environments', :request_store, retry: 0 do # First run to insert test data from lets, which does take up some 30 queries get_ci_environments_status @@ -893,18 +893,65 @@ describe Projects::MergeRequestsController do leeway = 11 expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway) end + end - def get_ci_environments_status(extra_params = {}) - params = { - namespace_id: merge_request.project.namespace.to_param, - project_id: merge_request.project, - id: merge_request.iid, - format: 'json' - } + context 'when a merge request has multiple environments with deployments' do + let(:sha) { merge_request.diff_head_sha } + let(:ref) { merge_request.source_branch } + + let!(:build) { create(:ci_build, pipeline: pipeline) } + let!(:pipeline) { create(:ci_pipeline, sha: sha, project: project) } + let!(:environment) { create(:environment, name: 'env_a', project: project) } + let!(:another_environment) { create(:environment, name: 'env_b', project: project) } + + before do + merge_request.update_head_pipeline + + create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) + create(:deployment, :succeed, environment: another_environment, sha: sha, ref: ref, deployable: build) + end + + it 'exposes multiple environment statuses' do + get_ci_environments_status + + expect(json_response.count).to eq 2 + end + + context 'when route map is not present in the project' do + it 'does not have N+1 Gitaly requests for environments', :request_store do + expect(merge_request).to be_present + + expect { get_ci_environments_status } + .to change { Gitlab::GitalyClient.get_request_count }.by_at_most(1) + end + end - get :ci_environments_status, params: params.merge(extra_params) + context 'when there is route map present in a project' do + before do + allow_any_instance_of(EnvironmentStatus) + .to receive(:has_route_map?) + .and_return(true) + end + + it 'does not have N+1 Gitaly requests for diff files', :request_store do + expect(merge_request.merge_request_diff.merge_request_diff_files).to be_many + + expect { get_ci_environments_status } + .to change { Gitlab::GitalyClient.get_request_count }.by_at_most(1) + end end end + + def get_ci_environments_status(extra_params = {}) + params = { + namespace_id: merge_request.project.namespace.to_param, + project_id: merge_request.project, + id: merge_request.iid, + format: 'json' + } + + get :ci_environments_status, params: params.merge(extra_params) + end end describe 'GET pipeline_status.json' do |