From 6ecf16b8f70335e1e6868b7c918cd031f5eb2f8d Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 5 May 2017 18:01:33 +0200 Subject: refactor code based on feedback --- app/controllers/admin/services_controller.rb | 2 +- app/services/projects/propagate_service.rb | 94 ------------------- .../projects/propagate_service_template.rb | 103 +++++++++++++++++++++ app/workers/propagate_project_service_worker.rb | 23 ----- app/workers/propagate_service_template_worker.rb | 23 +++++ config/sidekiq_queues.yml | 2 +- lib/gitlab/sql/bulk_insert.rb | 21 ----- spec/controllers/admin/services_controller_spec.rb | 8 +- spec/services/projects/propagate_service_spec.rb | 100 -------------------- .../projects/propagate_service_template_spec.rb | 99 ++++++++++++++++++++ .../propagate_project_service_worker_spec.rb | 29 ------ .../propagate_service_template_worker_spec.rb | 29 ++++++ 12 files changed, 260 insertions(+), 273 deletions(-) delete mode 100644 app/services/projects/propagate_service.rb create mode 100644 app/services/projects/propagate_service_template.rb delete mode 100644 app/workers/propagate_project_service_worker.rb create mode 100644 app/workers/propagate_service_template_worker.rb delete mode 100644 lib/gitlab/sql/bulk_insert.rb delete mode 100644 spec/services/projects/propagate_service_spec.rb create mode 100644 spec/services/projects/propagate_service_template_spec.rb delete mode 100644 spec/workers/propagate_project_service_worker_spec.rb create mode 100644 spec/workers/propagate_service_template_worker_spec.rb diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb index e335fbfffed..a40ce3c2418 100644 --- a/app/controllers/admin/services_controller.rb +++ b/app/controllers/admin/services_controller.rb @@ -16,7 +16,7 @@ class Admin::ServicesController < Admin::ApplicationController def update if service.update_attributes(service_params[:service]) - PropagateProjectServiceWorker.perform_async(service.id) if service.active? + PropagateServiceTemplateWorker.perform_async(service.id) if service.active? redirect_to admin_application_settings_services_path, notice: 'Application settings saved successfully' diff --git a/app/services/projects/propagate_service.rb b/app/services/projects/propagate_service.rb deleted file mode 100644 index f4fae478609..00000000000 --- a/app/services/projects/propagate_service.rb +++ /dev/null @@ -1,94 +0,0 @@ -module Projects - class PropagateService - BATCH_SIZE = 100 - - def self.propagate(*args) - new(*args).propagate - end - - def initialize(template) - @template = template - end - - def propagate - return unless @template&.active - - Rails.logger.info("Propagating services for template #{@template.id}") - - propagate_projects_with_template - end - - private - - def propagate_projects_with_template - loop do - batch = project_ids_batch - - bulk_create_from_template(batch) unless batch.empty? - - break if batch.size < BATCH_SIZE - end - end - - def bulk_create_from_template(batch) - service_list = batch.map do |project_id| - service_hash.merge('project_id' => project_id).values - end - - Project.transaction do - Gitlab::SQL::BulkInsert.new(service_hash.keys + ['project_id'], - service_list, - 'services').execute - run_callbacks(batch) - end - end - - def project_ids_batch - Project.connection.select_values( - <<-SQL - SELECT id - FROM projects - WHERE NOT EXISTS ( - SELECT true - FROM services - WHERE services.project_id = projects.id - AND services.type = '#{@template.type}' - ) - LIMIT #{BATCH_SIZE} - SQL - ) - end - - def service_hash - @service_hash ||= - begin - template_hash = @template.as_json(methods: :type).except('id', 'template', 'project_id') - - template_hash.each_with_object({}) do |(key, value), service_hash| - value = value.is_a?(Hash) ? value.to_json : value - key = Gitlab::Database.postgresql? ? "\"#{key}\"" : "`#{key}`" - - service_hash[key] = ActiveRecord::Base.sanitize(value) - end - end - end - - def run_callbacks(batch) - if active_external_issue_tracker? - Project.where(id: batch).update_all(has_external_issue_tracker: true) - end - - if active_external_wiki? - Project.where(id: batch).update_all(has_external_wiki: true) - end - end - - def active_external_issue_tracker? - @template['category'] == 'issue_tracker' && @template['active'] && !@template['default'] - end - - def active_external_wiki? - @template['type'] == 'ExternalWikiService' && @template['active'] - end - end -end diff --git a/app/services/projects/propagate_service_template.rb b/app/services/projects/propagate_service_template.rb new file mode 100644 index 00000000000..32ad68673ac --- /dev/null +++ b/app/services/projects/propagate_service_template.rb @@ -0,0 +1,103 @@ +module Projects + class PropagateServiceTemplate + BATCH_SIZE = 100 + + def self.propagate(*args) + new(*args).propagate + end + + def initialize(template) + @template = template + end + + def propagate + return unless @template&.active + + Rails.logger.info("Propagating services for template #{@template.id}") + + propagate_projects_with_template + end + + private + + def propagate_projects_with_template + loop do + batch = project_ids_batch + + bulk_create_from_template(batch) unless batch.empty? + + break if batch.size < BATCH_SIZE + end + end + + def bulk_create_from_template(batch) + service_list = batch.map do |project_id| + service_hash.merge('project_id' => project_id).values + end + + Project.transaction do + bulk_insert_services(service_hash.keys + ['project_id'], service_list) + run_callbacks(batch) + end + end + + def project_ids_batch + Project.connection.select_values( + <<-SQL + SELECT id + FROM projects + WHERE NOT EXISTS ( + SELECT true + FROM services + WHERE services.project_id = projects.id + AND services.type = '#{@template.type}' + ) + AND projects.pending_delete = false + AND projects.archived = false + LIMIT #{BATCH_SIZE} + SQL + ) + end + + def bulk_insert_services(columns, values_array) + ActiveRecord::Base.connection.execute( + <<-SQL.strip_heredoc + INSERT INTO services (#{columns.join(', ')}) + VALUES #{values_array.map { |tuple| "(#{tuple.join(', ')})" }.join(', ')} + SQL + ) + end + + def service_hash + @service_hash ||= + begin + template_hash = @template.as_json(methods: :type).except('id', 'template', 'project_id') + + template_hash.each_with_object({}) do |(key, value), service_hash| + value = value.is_a?(Hash) ? value.to_json : value + key = Gitlab::Database.postgresql? ? "\"#{key}\"" : "`#{key}`" + + service_hash[key] = ActiveRecord::Base.sanitize(value) + end + end + end + + def run_callbacks(batch) + if active_external_issue_tracker? + Project.where(id: batch).update_all(has_external_issue_tracker: true) + end + + if active_external_wiki? + Project.where(id: batch).update_all(has_external_wiki: true) + end + end + + def active_external_issue_tracker? + @template.category == :issue_tracker && !@template.default + end + + def active_external_wiki? + @template.type == 'ExternalWikiService' + end + end +end diff --git a/app/workers/propagate_project_service_worker.rb b/app/workers/propagate_project_service_worker.rb deleted file mode 100644 index 5eabe4eaecd..00000000000 --- a/app/workers/propagate_project_service_worker.rb +++ /dev/null @@ -1,23 +0,0 @@ -# Worker for updating any project specific caches. -class PropagateProjectServiceWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue - - sidekiq_options retry: 3 - - LEASE_TIMEOUT = 4.hours.to_i - - def perform(template_id) - return unless try_obtain_lease_for(template_id) - - Projects::PropagateService.propagate(Service.find_by(id: template_id)) - end - - private - - def try_obtain_lease_for(template_id) - Gitlab::ExclusiveLease. - new("propagate_project_service_worker:#{template_id}", timeout: LEASE_TIMEOUT). - try_obtain - end -end diff --git a/app/workers/propagate_service_template_worker.rb b/app/workers/propagate_service_template_worker.rb new file mode 100644 index 00000000000..f1fc7ccb955 --- /dev/null +++ b/app/workers/propagate_service_template_worker.rb @@ -0,0 +1,23 @@ +# Worker for updating any project specific caches. +class PropagateServiceTemplateWorker + include Sidekiq::Worker + include DedicatedSidekiqQueue + + sidekiq_options retry: 3 + + LEASE_TIMEOUT = 4.hours.to_i + + def perform(template_id) + return unless try_obtain_lease_for(template_id) + + Projects::PropagateServiceTemplate.propagate(Service.find_by(id: template_id)) + end + + private + + def try_obtain_lease_for(template_id) + Gitlab::ExclusiveLease. + new("propagate_service_template_worker:#{template_id}", timeout: LEASE_TIMEOUT). + try_obtain + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 91ea1c0f779..433381e79d3 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -53,4 +53,4 @@ - [pages, 1] - [system_hook_push, 1] - [update_user_activity, 1] - - [propagate_project_service, 1] + - [propagate_service_template, 1] diff --git a/lib/gitlab/sql/bulk_insert.rb b/lib/gitlab/sql/bulk_insert.rb deleted file mode 100644 index 097f9ff237b..00000000000 --- a/lib/gitlab/sql/bulk_insert.rb +++ /dev/null @@ -1,21 +0,0 @@ -module Gitlab - module SQL - # Class for building SQL bulk inserts - class BulkInsert - def initialize(columns, values_array, table) - @columns = columns - @values_array = values_array - @table = table - end - - def execute - ActiveRecord::Base.connection.execute( - <<-SQL.strip_heredoc - INSERT INTO #{@table} (#{@columns.join(', ')}) - VALUES #{@values_array.map { |tuple| "(#{tuple.join(', ')})" }.join(', ')} - SQL - ) - end - end - end -end diff --git a/spec/controllers/admin/services_controller_spec.rb b/spec/controllers/admin/services_controller_spec.rb index 808c98edb7f..c94616d8508 100644 --- a/spec/controllers/admin/services_controller_spec.rb +++ b/spec/controllers/admin/services_controller_spec.rb @@ -39,16 +39,16 @@ describe Admin::ServicesController do ) end - it 'updates the service params successfully and calls the propagation worker' do - expect(PropagateProjectServiceWorker).to receive(:perform_async).with(service.id) + it 'calls the propagation worker when service is active' do + expect(PropagateServiceTemplateWorker).to receive(:perform_async).with(service.id) put :update, id: service.id, service: { active: true } expect(response).to have_http_status(302) end - it 'updates the service params successfully' do - expect(PropagateProjectServiceWorker).not_to receive(:perform_async) + it 'does not call the propagation worker when service is not active' do + expect(PropagateServiceTemplateWorker).not_to receive(:perform_async) put :update, id: service.id, service: { properties: {} } diff --git a/spec/services/projects/propagate_service_spec.rb b/spec/services/projects/propagate_service_spec.rb deleted file mode 100644 index b8aa4de5bd1..00000000000 --- a/spec/services/projects/propagate_service_spec.rb +++ /dev/null @@ -1,100 +0,0 @@ -require 'spec_helper' - -describe Projects::PropagateService, services: true do - describe '.propagate!' do - let!(:service_template) do - PushoverService.create( - template: true, - active: true, - properties: { - device: 'MyDevice', - sound: 'mic', - priority: 4, - user_key: 'asdf', - api_key: '123456789' - }) - end - - let!(:project) { create(:empty_project) } - - it 'creates services for projects' do - expect { described_class.propagate(service_template) }. - to change { Service.count }.by(1) - end - - it 'creates services for a project that has another service' do - other_service = BambooService.create( - template: true, - active: true, - properties: { - bamboo_url: 'http://gitlab.com', - username: 'mic', - password: "password", - build_key: 'build' - } - ) - - Service.build_from_template(project.id, other_service).save! - - expect { described_class.propagate(service_template) }. - to change { Service.count }.by(1) - end - - it 'does not create the service if it exists already' do - other_service = BambooService.create( - template: true, - active: true, - properties: { - bamboo_url: 'http://gitlab.com', - username: 'mic', - password: "password", - build_key: 'build' - } - ) - - Service.build_from_template(project.id, service_template).save! - Service.build_from_template(project.id, other_service).save! - - expect { described_class.propagate(service_template) }. - not_to change { Service.count } - end - - it 'creates the service containing the template attributes' do - described_class.propagate(service_template) - - service = Service.find_by(type: service_template.type, template: false) - - expect(service.properties).to eq(service_template.properties) - end - - describe 'bulk update' do - it 'creates services for all projects' do - project_total = 5 - stub_const 'Projects::PropagateService::BATCH_SIZE', 3 - - project_total.times { create(:empty_project) } - - expect { described_class.propagate(service_template) }. - to change { Service.count }.by(project_total + 1) - end - end - - describe 'external tracker' do - it 'updates the project external tracker' do - service_template.update(category: 'issue_tracker', default: false) - - expect { described_class.propagate(service_template) }. - to change { project.reload.has_external_issue_tracker }.to(true) - end - end - - describe 'external wiki' do - it 'updates the project external tracker' do - service_template.update(type: 'ExternalWikiService') - - expect { described_class.propagate(service_template) }. - to change { project.reload.has_external_wiki }.to(true) - end - end - end -end diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb new file mode 100644 index 00000000000..331fb3c5ac5 --- /dev/null +++ b/spec/services/projects/propagate_service_template_spec.rb @@ -0,0 +1,99 @@ +require 'spec_helper' + +describe Projects::PropagateServiceTemplate, services: true do + describe '.propagate' do + let!(:service_template) do + PushoverService.create( + template: true, + active: true, + properties: { + device: 'MyDevice', + sound: 'mic', + priority: 4, + user_key: 'asdf', + api_key: '123456789' + }) + end + + let!(:project) { create(:empty_project) } + + it 'creates services for projects' do + expect { described_class.propagate(service_template) }. + to change { Service.count }.by(1) + end + + it 'creates services for a project that has another service' do + BambooService.create( + template: true, + active: true, + project: project, + properties: { + bamboo_url: 'http://gitlab.com', + username: 'mic', + password: "password", + build_key: 'build' + } + ) + + expect { described_class.propagate(service_template) }. + to change { Service.count }.by(1) + end + + it 'does not create the service if it exists already' do + other_service = BambooService.create( + template: true, + active: true, + properties: { + bamboo_url: 'http://gitlab.com', + username: 'mic', + password: "password", + build_key: 'build' + } + ) + + Service.build_from_template(project.id, service_template).save! + Service.build_from_template(project.id, other_service).save! + + expect { described_class.propagate(service_template) }. + not_to change { Service.count } + end + + it 'creates the service containing the template attributes' do + described_class.propagate(service_template) + + service = Service.find_by!(type: service_template.type, template: false) + + expect(service.properties).to eq(service_template.properties) + end + + describe 'bulk update' do + it 'creates services for all projects' do + project_total = 5 + stub_const 'Projects::PropagateServiceTemplate::BATCH_SIZE', 3 + + project_total.times { create(:empty_project) } + + expect { described_class.propagate(service_template) }. + to change { Service.count }.by(project_total + 1) + end + end + + describe 'external tracker' do + it 'updates the project external tracker' do + service_template.update!(category: 'issue_tracker', default: false) + + expect { described_class.propagate(service_template) }. + to change { project.reload.has_external_issue_tracker }.to(true) + end + end + + describe 'external wiki' do + it 'updates the project external tracker' do + service_template.update!(type: 'ExternalWikiService') + + expect { described_class.propagate(service_template) }. + to change { project.reload.has_external_wiki }.to(true) + end + end + end +end diff --git a/spec/workers/propagate_project_service_worker_spec.rb b/spec/workers/propagate_project_service_worker_spec.rb deleted file mode 100644 index 4c7edbfcd3e..00000000000 --- a/spec/workers/propagate_project_service_worker_spec.rb +++ /dev/null @@ -1,29 +0,0 @@ -require 'spec_helper' - -describe PropagateProjectServiceWorker do - let!(:service_template) do - PushoverService.create( - template: true, - active: true, - properties: { - device: 'MyDevice', - sound: 'mic', - priority: 4, - user_key: 'asdf', - api_key: '123456789' - }) - end - - before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain). - and_return(true) - end - - describe '#perform' do - it 'calls the propagate service with the template' do - expect(Projects::PropagateService).to receive(:propagate).with(service_template) - - subject.perform(service_template.id) - end - end -end diff --git a/spec/workers/propagate_service_template_worker_spec.rb b/spec/workers/propagate_service_template_worker_spec.rb new file mode 100644 index 00000000000..7040d5ef81c --- /dev/null +++ b/spec/workers/propagate_service_template_worker_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe PropagateServiceTemplateWorker do + let!(:service_template) do + PushoverService.create( + template: true, + active: true, + properties: { + device: 'MyDevice', + sound: 'mic', + priority: 4, + user_key: 'asdf', + api_key: '123456789' + }) + end + + before do + allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain). + and_return(true) + end + + describe '#perform' do + it 'calls the propagate service with the template' do + expect(Projects::PropagateServiceTemplate).to receive(:propagate).with(service_template) + + subject.perform(service_template.id) + end + end +end -- cgit v1.2.1