summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBrett Walker <bwalker@gitlab.com>2018-10-03 11:39:42 -0500
committerBrett Walker <bwalker@gitlab.com>2018-10-04 13:28:52 -0500
commit7ee8771c5cdedcffadee081358e1239ba71aede0 (patch)
tree477f871b9afe5b21069068ca51e440ae05ee68b0
parent1973d0c183218e7ca826d627eaea5ce91a17bed3 (diff)
downloadgitlab-ce-7ee8771c5cdedcffadee081358e1239ba71aede0.tar.gz
Don't build project services unneccesarily
-rw-r--r--app/controllers/projects/merge_requests_controller.rb4
-rw-r--r--app/models/project.rb35
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb28
-rw-r--r--spec/models/project_spec.rb14
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