Merge branch '20820-service-templates-performance' into 'master'

Reduce number of SQL queries for service templates

Closes #20820

See merge request gitlab-org/gitlab!27396

app/controllers/admin/services_controller.rb

@@ -3,11 +3,10 @@
 class Admin::ServicesController < Admin::ApplicationController
   include ServiceParams
 
-  before_action :whitelist_query_limiting, only: [:index]
   before_action :service, only: [:edit, :update]
 
   def index
-    @services = services_templates
+    @services = Service.find_or_create_templates
   end
 
   def edit
@@ -30,22 +29,9 @@ class Admin::ServicesController < Admin::ApplicationController
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def services_templates
-    Service.available_services_names.map do |service_name|
-      service_template = "#{service_name}_service".camelize.constantize
-      service_template.where(template: true).first_or_create
-    end
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-
   # rubocop: disable CodeReuse/ActiveRecord
   def service
     @service ||= Service.find_by(id: params[:id], template: true)
   end
   # rubocop: enable CodeReuse/ActiveRecord
-
-  def whitelist_query_limiting
-    Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42430')
-  end
 end

app/models/service.rb

@@ -46,6 +46,7 @@ class Service < ApplicationRecord
   scope :active, -> { where(active: true) }
   scope :without_defaults, -> { where(default: false) }
   scope :by_type, -> (type) { where(type: type) }
+  scope :templates, -> { where(template: true, type: available_services_types) }
 
   scope :push_hooks, -> { where(push_events: true, active: true) }
   scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) }
@@ -259,14 +260,32 @@ class Service < ApplicationRecord
     self.category == :issue_tracker
   end
 
+  # Find all service templates; if some of them do not exist, create them
+  # within a transaction to perform the lowest possible SQL queries.
+  def self.find_or_create_templates
+    create_nonexistent_templates
+    templates
+  end
+
+  private_class_method def self.create_nonexistent_templates
+    nonexistent_services = available_services_types - templates.map(&:type)
+    return if nonexistent_services.empty?
+
+    transaction do
+      nonexistent_services.each do |service_type|
+        service_type.constantize.create(template: true)
+      end
+    end
+  end
+
   def self.available_services_names
     service_names = %w[
       alerts
       asana
       assembla
       bamboo
-      buildkite
       bugzilla
+      buildkite
       campfire
       custom_issue_tracker
       discord
@@ -278,20 +297,20 @@ class Service < ApplicationRecord
       hipchat
       irker
       jira
-      mattermost_slash_commands
       mattermost
+      mattermost_slash_commands
+      microsoft_teams
       packagist
       pipelines_email
       pivotaltracker
       prometheus
       pushover
       redmine
-      youtrack
-      slack_slash_commands
       slack
+      slack_slash_commands
       teamcity
-      microsoft_teams
       unify_circuit
+      youtrack
     ]
 
     if Rails.env.development?
@@ -301,6 +320,10 @@ class Service < ApplicationRecord
     service_names.sort_by(&:downcase)
   end
 
+  def self.available_services_types
+    available_services_names.map { |service_name| "#{service_name}_service".camelize }
+  end
+
   def self.build_from_template(project_id, template)
     service = template.dup
 

changelogs/unreleased/20820-service-templates-performance.yml

+---
+title: Reduce number of SQL queries for service templates
+merge_request: 27396
+author:
+type: performance

spec/controllers/admin/services_controller_spec.rb

@@ -10,21 +10,14 @@ describe Admin::ServicesController do
   end
 
   describe 'GET #edit' do
-    let!(:project) { create(:project) }
-
-    Service.available_services_names.each do |service_name|
-      context "#{service_name}" do
-        let!(:service) do
-          service_template = "#{service_name}_service".camelize.constantize
-          service_template.where(template: true).first_or_create
-        end
+    let!(:service) do
+      create(:jira_service, :template)
+    end
 
-        it 'successfully displays the template' do
-          get :edit, params: { id: service.id }
+    it 'successfully displays the template' do
+      get :edit, params: { id: service.id }
 
-          expect(response).to have_gitlab_http_status(:ok)
-        end
-      end
+      expect(response).to have_gitlab_http_status(:ok)
     end
   end
 

spec/models/service_spec.rb

@@ -149,9 +149,58 @@ describe Service do
     end
   end
 
-  describe "Template" do
+  describe 'template' do
     let(:project) { create(:project) }
 
+    shared_examples 'retrieves service templates' do
+      it 'returns the available service templates' do
+        expect(Service.find_or_create_templates.pluck(:type)).to match_array(Service.available_services_types)
+      end
+    end
+
+    describe '.find_or_create_templates' do
+      it 'creates service templates' do
+        expect { Service.find_or_create_templates }.to change { Service.count }.from(0).to(Service.available_services_names.size)
+      end
+
+      it_behaves_like 'retrieves service templates'
+
+      context 'with all existing templates' do
+        before do
+          Service.insert_all(
+            Service.available_services_types.map { |type| { template: true, type: type } }
+          )
+        end
+
+        it 'does not create service templates' do
+          expect { Service.find_or_create_templates }.to change { Service.count }.by(0)
+        end
+
+        it_behaves_like 'retrieves service templates'
+
+        context 'with a previous existing service (Previous) and a new service (Asana)' do
+          before do
+            Service.insert(type: 'PreviousService', template: true)
+            Service.delete_by(type: 'AsanaService', template: true)
+          end
+
+          it_behaves_like 'retrieves service templates'
+        end
+      end
+
+      context 'with a few existing templates' do
+        before do
+          create(:jira_service, :template)
+        end
+
+        it 'creates the rest of the service templates' do
+          expect { Service.find_or_create_templates }.to change { Service.count }.from(1).to(Service.available_services_names.size)
+        end
+
+        it_behaves_like 'retrieves service templates'
+      end
+    end
+
     describe '.build_from_template' do
       context 'when template is invalid' do
         it 'sets service template to inactive when template is invalid' do