From 7ee8771c5cdedcffadee081358e1239ba71aede0 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Wed, 3 Oct 2018 11:39:42 -0500 Subject: [PATCH 1/2] Don't build project services unneccesarily --- .../projects/merge_requests_controller.rb | 4 +-- app/models/project.rb | 35 +++++++++---------- .../merge_requests_controller_spec.rb | 28 ++++++++++----- spec/models/project_spec.rb | 14 ++++++++ 4 files changed, 52 insertions(+), 29 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d691744d72a..6a5da9b8292 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -207,7 +207,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo environments = begin @merge_request.environments_for(current_user).map do |environment| - project = environment.project + project = environment.project deployment = environment.first_deployment_for(@merge_request.diff_head_sha) stop_url = @@ -217,7 +217,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo metrics_url = if can?(current_user, :read_environment, environment) && environment.has_metrics? - metrics_project_environment_deployment_path(environment.project, environment, deployment) + metrics_project_environment_deployment_path(project, environment, deployment) end metrics_monitoring_url = diff --git a/app/models/project.rb b/app/models/project.rb index 59f088156c7..6a9397a36d7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1082,26 +1082,10 @@ class Project < ActiveRecord::Base end def find_or_initialize_services(exceptions: []) - services_templates = Service.where(template: true) - available_services_names = Service.available_services_names - exceptions available_services = available_services_names.map do |service_name| - service = find_service(services, service_name) - - if service - service - else - # We should check if template for the service exists - template = find_service(services_templates, service_name) - - if template.nil? - # If no template, we should create an instance. Ex `build_gitlab_ci_service` - public_send("build_#{service_name}_service") # rubocop:disable GitlabSecurity/PublicSend - else - Service.build_from_template(id, template) - end - end + find_or_initialize_service(service_name) end available_services.reject do |service| @@ -1114,7 +1098,18 @@ class Project < ActiveRecord::Base end def find_or_initialize_service(name) - find_or_initialize_services.find { |service| service.to_param == name } + service = find_service(services, name) + return service if service + + # We should check if template for the service exists + template = find_service(services_templates, name) + + if template + Service.build_from_template(id, template) + else + # If no template, we should create an instance. Ex `build_gitlab_ci_service` + public_send("build_#{name}_service") # rubocop:disable GitlabSecurity/PublicSend + end end # rubocop: disable CodeReuse/ServiceClass @@ -2277,4 +2272,8 @@ class Project < ActiveRecord::Base check_access.call end end + + def services_templates + @services_templates ||= Service.where(template: true) + end end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 7446e0650f7..41c92a392aa 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -763,25 +763,35 @@ describe Projects::MergeRequestsController do describe 'GET ci_environments_status' do context 'the environment is from a forked project' do - let!(:forked) { fork_project(project, user, repository: true) } - let!(:environment) { create(:environment, project: forked) } - let!(:deployment) { create(:deployment, environment: environment, sha: forked.commit.id, ref: 'master') } - let(:admin) { create(:admin) } + let!(:forked) { fork_project(project, user, repository: true) } + let!(:environment) { create(:environment, project: forked) } + let!(:deployment) { create(:deployment, environment: environment, sha: forked.commit.id, ref: 'master') } + let(:admin) { create(:admin) } let(:merge_request) do create(:merge_request, source_project: forked, target_project: project) end - before do + it 'links to the environment on that project' do + get_ci_environments_status + + expect(json_response.first['url']).to match /#{forked.full_path}/ + end + + # we're trying to reduce the overall number of queries for this method. + # set a hard limit for now. https://gitlab.com/gitlab-org/gitlab-ce/issues/43109 + it 'keeps queries in check' do + control_count = ActiveRecord::QueryRecorder.new { get_ci_environments_status }.count + + expect(control_count).to be <= 137 + end + + def get_ci_environments_status get :ci_environments_status, namespace_id: merge_request.project.namespace.to_param, project_id: merge_request.project, id: merge_request.iid, format: 'json' end - - it 'links to the environment on that project' do - expect(json_response.first['url']).to match /#{forked.full_path}/ - end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8b71919544e..ff259dc12b3 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4037,6 +4037,20 @@ describe Project do expect(result).to be_empty end end + + describe "#find_or_initialize_service" do + subject { build(:project) } + + it 'avoids N+1 database queries' do + allow(Service).to receive(:available_services_names).and_return(%w(prometheus pushover)) + + control_count = ActiveRecord::QueryRecorder.new { project.find_or_initialize_service('prometheus') }.count + + allow(Service).to receive(:available_services_names).and_call_original + + expect { project.find_or_initialize_service('prometheus') }.not_to exceed_query_limit(control_count) + end + end end def rugged_config From cc339aa60877214820f9445336b5cf7766601176 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Fri, 5 Oct 2018 13:47:55 -0500 Subject: [PATCH 2/2] Update spec comment to point to correct issue --- spec/controllers/projects/merge_requests_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 41c92a392aa..f2467bfd525 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -779,7 +779,7 @@ describe Projects::MergeRequestsController do end # we're trying to reduce the overall number of queries for this method. - # set a hard limit for now. https://gitlab.com/gitlab-org/gitlab-ce/issues/43109 + # set a hard limit for now. https://gitlab.com/gitlab-org/gitlab-ce/issues/52287 it 'keeps queries in check' do control_count = ActiveRecord::QueryRecorder.new { get_ci_environments_status }.count