diff options
Diffstat (limited to 'app/services')
313 files changed, 2432 insertions, 917 deletions
diff --git a/app/services/admin/propagate_integration_service.rb b/app/services/admin/propagate_integration_service.rb index 253c3a84fef..f7a4bf1a9f9 100644 --- a/app/services/admin/propagate_integration_service.rb +++ b/app/services/admin/propagate_integration_service.rb @@ -5,7 +5,7 @@ module Admin include PropagateService def propagate - if integration.instance? + if integration.instance_level? update_inherited_integrations create_integration_for_groups_without_integration create_integration_for_projects_without_integration @@ -20,14 +20,14 @@ module Admin def update_inherited_integrations propagate_integrations( - Service.by_type(integration.type).inherit_from_id(integration.id), + Integration.by_type(integration.type).inherit_from_id(integration.id), PropagateIntegrationInheritWorker ) end def update_inherited_descendant_integrations propagate_integrations( - Service.inherited_descendants_from_self_or_ancestors_from(integration), + Integration.inherited_descendants_from_self_or_ancestors_from(integration), PropagateIntegrationInheritDescendantWorker ) end diff --git a/app/services/alert_management/http_integrations/create_service.rb b/app/services/alert_management/http_integrations/create_service.rb index e7f1084ce5c..1abe0548c45 100644 --- a/app/services/alert_management/http_integrations/create_service.rb +++ b/app/services/alert_management/http_integrations/create_service.rb @@ -66,4 +66,4 @@ module AlertManagement end end -::AlertManagement::HttpIntegrations::CreateService.prepend_if_ee('::EE::AlertManagement::HttpIntegrations::CreateService') +::AlertManagement::HttpIntegrations::CreateService.prepend_mod_with('AlertManagement::HttpIntegrations::CreateService') diff --git a/app/services/alert_management/http_integrations/update_service.rb b/app/services/alert_management/http_integrations/update_service.rb index af079f670b8..8662f966a2e 100644 --- a/app/services/alert_management/http_integrations/update_service.rb +++ b/app/services/alert_management/http_integrations/update_service.rb @@ -56,4 +56,4 @@ module AlertManagement end end -::AlertManagement::HttpIntegrations::UpdateService.prepend_if_ee('::EE::AlertManagement::HttpIntegrations::UpdateService') +::AlertManagement::HttpIntegrations::UpdateService.prepend_mod_with('AlertManagement::HttpIntegrations::UpdateService') diff --git a/app/services/alert_management/process_prometheus_alert_service.rb b/app/services/alert_management/process_prometheus_alert_service.rb index 0591376bcdf..605ab7a1869 100644 --- a/app/services/alert_management/process_prometheus_alert_service.rb +++ b/app/services/alert_management/process_prometheus_alert_service.rb @@ -25,13 +25,6 @@ module AlertManagement attr_reader :project, :payload - override :process_new_alert - def process_new_alert - return if resolving_alert? - - super - end - override :incoming_payload def incoming_payload strong_memoize(:incoming_payload) do diff --git a/app/services/analytics/cycle_analytics/stages/base_service.rb b/app/services/analytics/cycle_analytics/stages/base_service.rb new file mode 100644 index 00000000000..b676eff0a0b --- /dev/null +++ b/app/services/analytics/cycle_analytics/stages/base_service.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Analytics + module CycleAnalytics + module Stages + class BaseService + include Gitlab::Allowable + + DEFAULT_VALUE_STREAM_NAME = 'default' + + def initialize(parent:, current_user:, params: {}) + @parent = parent + @current_user = current_user + @params = params + end + + def execute + raise NotImplementedError + end + + private + + attr_reader :parent, :current_user, :params + + def success(stage, http_status = :created) + ServiceResponse.success(payload: { stage: stage }, http_status: http_status) + end + + def forbidden + ServiceResponse.error(message: 'Forbidden', payload: {}, http_status: :forbidden) + end + + def build_default_stages + Gitlab::Analytics::CycleAnalytics::DefaultStages.all.map do |stage_params| + parent.cycle_analytics_stages.build(stage_params.merge(value_stream: value_stream)) + end + end + + def value_stream + @value_stream ||= params[:value_stream] + end + end + end + end +end + +Analytics::CycleAnalytics::Stages::BaseService.prepend_mod_with('Analytics::CycleAnalytics::Stages::BaseService') diff --git a/app/services/analytics/cycle_analytics/stages/list_service.rb b/app/services/analytics/cycle_analytics/stages/list_service.rb new file mode 100644 index 00000000000..a6b94ef8295 --- /dev/null +++ b/app/services/analytics/cycle_analytics/stages/list_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Analytics + module CycleAnalytics + module Stages + class ListService < Analytics::CycleAnalytics::Stages::BaseService + def execute + return forbidden unless allowed? + + success(build_default_stages) + end + + private + + def allowed? + can?(current_user, :read_cycle_analytics, parent) + end + + def success(stages) + ServiceResponse.success(payload: { stages: stages }) + end + end + end + end +end + +Analytics::CycleAnalytics::Stages::ListService.prepend_mod_with('Analytics::CycleAnalytics::Stages::ListService') diff --git a/app/services/application_settings/update_service.rb b/app/services/application_settings/update_service.rb index 7792b811b4e..7728982779e 100644 --- a/app/services/application_settings/update_service.rb +++ b/app/services/application_settings/update_service.rb @@ -120,4 +120,4 @@ module ApplicationSettings end end -ApplicationSettings::UpdateService.prepend_if_ee('EE::ApplicationSettings::UpdateService') +ApplicationSettings::UpdateService.prepend_mod_with('ApplicationSettings::UpdateService') diff --git a/app/services/applications/create_service.rb b/app/services/applications/create_service.rb index 92500fbc254..96cde9057c7 100644 --- a/app/services/applications/create_service.rb +++ b/app/services/applications/create_service.rb @@ -25,4 +25,4 @@ module Applications end end -Applications::CreateService.prepend_if_ee('EE::Applications::CreateService') +Applications::CreateService.prepend_mod_with('Applications::CreateService') diff --git a/app/services/audit_event_service.rb b/app/services/audit_event_service.rb index d1558c60c3d..60421f61007 100644 --- a/app/services/audit_event_service.rb +++ b/app/services/audit_event_service.rb @@ -131,9 +131,9 @@ class AuditEventService def save_or_track(event) event.save! - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, audit_event_type: event.class.to_s) end end -AuditEventService.prepend_if_ee('EE::AuditEventService') +AuditEventService.prepend_mod_with('AuditEventService') diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index d74f20511bd..5fde346c4ab 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -251,4 +251,4 @@ module Auth end end -Auth::ContainerRegistryAuthenticationService.prepend_if_ee('EE::Auth::ContainerRegistryAuthenticationService') +Auth::ContainerRegistryAuthenticationService.prepend_mod_with('Auth::ContainerRegistryAuthenticationService') diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb index 41236286d23..142eebca2e3 100644 --- a/app/services/auto_merge/base_service.rb +++ b/app/services/auto_merge/base_service.rb @@ -15,7 +15,7 @@ module AutoMerge AutoMergeProcessWorker.perform_async(merge_request.id) strategy.to_sym - rescue => e + rescue StandardError => e track_exception(e, merge_request) :failed end @@ -35,7 +35,7 @@ module AutoMerge end success - rescue => e + rescue StandardError => e track_exception(e, merge_request) error("Can't cancel the automatic merge", 406) end @@ -47,7 +47,7 @@ module AutoMerge end success - rescue => e + rescue StandardError => e track_exception(e, merge_request) error("Can't abort the automatic merge", 406) end diff --git a/app/services/auto_merge_service.rb b/app/services/auto_merge_service.rb index c5cbcc7c93b..912248d3a06 100644 --- a/app/services/auto_merge_service.rb +++ b/app/services/auto_merge_service.rb @@ -74,4 +74,4 @@ class AutoMergeService < BaseService end end -AutoMergeService.prepend_if_ee('EE::AutoMergeService') +AutoMergeService.prepend_mod_with('AutoMergeService') diff --git a/app/services/award_emojis/add_service.rb b/app/services/award_emojis/add_service.rb index ceb7a38cead..f45a4330c09 100644 --- a/app/services/award_emojis/add_service.rb +++ b/app/services/award_emojis/add_service.rb @@ -46,4 +46,4 @@ module AwardEmojis end end -AwardEmojis::AddService.prepend_if_ee('EE::AwardEmojis::AddService') +AwardEmojis::AddService.prepend_mod_with('AwardEmojis::AddService') diff --git a/app/services/award_emojis/destroy_service.rb b/app/services/award_emojis/destroy_service.rb index cfd194262f9..47dc8418e07 100644 --- a/app/services/award_emojis/destroy_service.rb +++ b/app/services/award_emojis/destroy_service.rb @@ -26,4 +26,4 @@ module AwardEmojis end end -AwardEmojis::DestroyService.prepend_if_ee('EE::AwardEmojis::DestroyService') +AwardEmojis::DestroyService.prepend_mod_with('AwardEmojis::DestroyService') diff --git a/app/services/base_container_service.rb b/app/services/base_container_service.rb index 6852237dc25..ee15763ce65 100644 --- a/app/services/base_container_service.rb +++ b/app/services/base_container_service.rb @@ -1,6 +1,13 @@ # frozen_string_literal: true -# Base class, scoped by container (project or group) +# Base class, scoped by container (project or group). +# +# New or existing services which only require project as a container +# should subclass BaseProjectService. +# +# If you require a different but specific, non-polymorphic container (such +# as group), consider creating a new subclass such as BaseGroupService, +# and update the related comment at the top of the original BaseService. class BaseContainerService include BaseServiceUtility diff --git a/app/services/base_count_service.rb b/app/services/base_count_service.rb index 2936bdae16e..c316c488148 100644 --- a/app/services/base_count_service.rb +++ b/app/services/base_count_service.rb @@ -49,4 +49,4 @@ class BaseCountService end end -BaseCountService.prepend_if_ee('EE::BaseCountService') +BaseCountService.prepend_mod_with('BaseCountService') diff --git a/app/services/base_project_service.rb b/app/services/base_project_service.rb new file mode 100644 index 00000000000..fb466e61673 --- /dev/null +++ b/app/services/base_project_service.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +# Base class, scoped by project +class BaseProjectService < ::BaseContainerService + attr_accessor :project + + def initialize(project:, current_user: nil, params: {}) + super(container: project, current_user: current_user, params: params) + + @project = project + end + + delegate :repository, to: :project +end diff --git a/app/services/base_service.rb b/app/services/base_service.rb index 20dfeb67815..7ab87a1af09 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -6,9 +6,12 @@ # and existing service will use these one by one. # After all are migrated, we can remove this class. # -# TODO: New services should consider inheriting from -# BaseContainerService, or create new base class: -# https://gitlab.com/gitlab-org/gitlab/-/issues/216672 +# New services should consider inheriting from: +# +# - BaseContainerService for services scoped by container (project or group) +# - BaseProjectService for services scoped to projects +# +# or, create a new base class and update this comment. class BaseService include BaseServiceUtility diff --git a/app/services/boards/base_items_list_service.rb b/app/services/boards/base_items_list_service.rb index 5aebf216460..cbc7a332cbe 100644 --- a/app/services/boards/base_items_list_service.rb +++ b/app/services/boards/base_items_list_service.rb @@ -129,7 +129,7 @@ module Boards # rubocop: disable CodeReuse/ActiveRecord def label_links(label_ids) LabelLink - .where('label_links.target_type = ?', item_model) + .where(label_links: { target_type: item_model }) .where(item_model.arel_table[:id].eq(LabelLink.arel_table[:target_id]).to_sql) .where(label_id: label_ids) end diff --git a/app/services/boards/base_service.rb b/app/services/boards/base_service.rb index 83bb69b3822..f371f88d44b 100644 --- a/app/services/boards/base_service.rb +++ b/app/services/boards/base_service.rb @@ -13,4 +13,4 @@ module Boards end end -Boards::BaseService.prepend_if_ee('EE::Boards::BaseService') +Boards::BaseService.prepend_mod_with('Boards::BaseService') diff --git a/app/services/boards/create_service.rb b/app/services/boards/create_service.rb index 54dab581686..5f014abe071 100644 --- a/app/services/boards/create_service.rb +++ b/app/services/boards/create_service.rb @@ -37,4 +37,4 @@ module Boards end end -Boards::CreateService.prepend_if_ee('EE::Boards::CreateService') +Boards::CreateService.prepend_mod_with('Boards::CreateService') diff --git a/app/services/boards/issues/create_service.rb b/app/services/boards/issues/create_service.rb index 1769966a049..0639acfb399 100644 --- a/app/services/boards/issues/create_service.rb +++ b/app/services/boards/issues/create_service.rb @@ -30,10 +30,10 @@ module Boards end def create_issue(params) - ::Issues::CreateService.new(project, current_user, params).execute + ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute end end end end -Boards::Issues::CreateService.prepend_if_ee('EE::Boards::Issues::CreateService') +Boards::Issues::CreateService.prepend_mod_with('Boards::Issues::CreateService') diff --git a/app/services/boards/issues/list_service.rb b/app/services/boards/issues/list_service.rb index c6855f29af0..6284e454561 100644 --- a/app/services/boards/issues/list_service.rb +++ b/app/services/boards/issues/list_service.rb @@ -50,4 +50,4 @@ module Boards end end -Boards::Issues::ListService.prepend_if_ee('EE::Boards::Issues::ListService') +Boards::Issues::ListService.prepend_mod_with('Boards::Issues::ListService') diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb index 76ea57968b2..959a7fa3ad2 100644 --- a/app/services/boards/issues/move_service.rb +++ b/app/services/boards/issues/move_service.rb @@ -52,7 +52,7 @@ module Boards end def update(issue, issue_modification_params) - ::Issues::UpdateService.new(issue.project, current_user, issue_modification_params).execute(issue) + ::Issues::UpdateService.new(project: issue.project, current_user: current_user, params: issue_modification_params).execute(issue) end def reposition_parent @@ -62,4 +62,4 @@ module Boards end end -Boards::Issues::MoveService.prepend_if_ee('EE::Boards::Issues::MoveService') +Boards::Issues::MoveService.prepend_mod_with('Boards::Issues::MoveService') diff --git a/app/services/boards/lists/base_destroy_service.rb b/app/services/boards/lists/base_destroy_service.rb new file mode 100644 index 00000000000..dc0247c40b0 --- /dev/null +++ b/app/services/boards/lists/base_destroy_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Boards + module Lists + # This class is used by issue and epic board lists + # for destroying a single list + class BaseDestroyService < Boards::BaseService + def execute(list) + unless list.destroyable? + return ServiceResponse.error(message: "Open and closed lists on a board cannot be destroyed.") + end + + list.with_lock do + decrement_higher_lists(list) + list.destroy! + end + + ServiceResponse.success + rescue StandardError => e + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) + ServiceResponse.error(message: "List destroy failed.") + end + + private + + # rubocop: disable CodeReuse/ActiveRecord + def decrement_higher_lists(list) + list.board.lists.movable.where('position > ?', list.position) + .update_all('position = position - 1') + end + # rubocop: enable CodeReuse/ActiveRecord + end + end +end diff --git a/app/services/boards/lists/base_update_service.rb b/app/services/boards/lists/base_update_service.rb index faf58e405fc..bcb7d6c8504 100644 --- a/app/services/boards/lists/base_update_service.rb +++ b/app/services/boards/lists/base_update_service.rb @@ -3,16 +3,30 @@ module Boards module Lists class BaseUpdateService < Boards::BaseService + extend ::Gitlab::Utils::Override + def execute(list) if execute_by_params(list) success(list: list) else - error(list.errors.messages, 422) + message = list.errors.empty? ? 'The update was not successful.' : list.errors.messages + + error(message, { list: list }) end end private + override :error + def error(message, pass_back = {}) + ServiceResponse.error(message: message, http_status: :unprocessable_entity, payload: pass_back) + end + + override :success + def success(pass_back = {}) + ServiceResponse.success(payload: pass_back) + end + def execute_by_params(list) update_preferences_result = update_preferences(list) if can_read?(list) update_position_result = update_position(list) if can_admin?(list) diff --git a/app/services/boards/lists/create_service.rb b/app/services/boards/lists/create_service.rb index 37fe0a815bd..3ee0b6d8821 100644 --- a/app/services/boards/lists/create_service.rb +++ b/app/services/boards/lists/create_service.rb @@ -7,4 +7,4 @@ module Boards end end -Boards::Lists::CreateService.prepend_if_ee('EE::Boards::Lists::CreateService') +Boards::Lists::CreateService.prepend_mod_with('Boards::Lists::CreateService') diff --git a/app/services/boards/lists/destroy_service.rb b/app/services/boards/lists/destroy_service.rb index ebac0f07fe1..10d9f275d3f 100644 --- a/app/services/boards/lists/destroy_service.rb +++ b/app/services/boards/lists/destroy_service.rb @@ -2,36 +2,8 @@ module Boards module Lists - class DestroyService < Boards::BaseService - def execute(list) - unless list.destroyable? - return ServiceResponse.error(message: "The list cannot be destroyed. Only label lists can be destroyed.") - end - - @board = list.board - - list.with_lock do - decrement_higher_lists(list) - remove_list(list) - end - - ServiceResponse.success - end - - private - - attr_reader :board - - # rubocop: disable CodeReuse/ActiveRecord - def decrement_higher_lists(list) - board.lists.movable.where('position > ?', list.position) - .update_all('position = position - 1') - end - # rubocop: enable CodeReuse/ActiveRecord - - def remove_list(list) - list.destroy! - end + # overridden in EE for board lists and also for epic board lists. + class DestroyService < Boards::Lists::BaseDestroyService end end end diff --git a/app/services/boards/lists/list_service.rb b/app/services/boards/lists/list_service.rb index 03d54a8c74c..e81ef467a4e 100644 --- a/app/services/boards/lists/list_service.rb +++ b/app/services/boards/lists/list_service.rb @@ -32,4 +32,4 @@ module Boards end end -Boards::Lists::ListService.prepend_if_ee('EE::Boards::Lists::ListService') +Boards::Lists::ListService.prepend_mod_with('Boards::Lists::ListService') diff --git a/app/services/boards/lists/update_service.rb b/app/services/boards/lists/update_service.rb index 2e1a6592cd9..5c24c0daa73 100644 --- a/app/services/boards/lists/update_service.rb +++ b/app/services/boards/lists/update_service.rb @@ -14,4 +14,4 @@ module Boards end end -Boards::Lists::UpdateService.prepend_if_ee('EE::Boards::Lists::UpdateService') +Boards::Lists::UpdateService.prepend_mod_with('Boards::Lists::UpdateService') diff --git a/app/services/boards/update_service.rb b/app/services/boards/update_service.rb index 48c6e44d55e..6ba8f68a4cb 100644 --- a/app/services/boards/update_service.rb +++ b/app/services/boards/update_service.rb @@ -19,4 +19,4 @@ module Boards end end -Boards::UpdateService.prepend_if_ee('EE::Boards::UpdateService') +Boards::UpdateService.prepend_mod_with('Boards::UpdateService') diff --git a/app/services/boards/visits/create_service.rb b/app/services/boards/visits/create_service.rb index 428ed1a8bcc..4d659596803 100644 --- a/app/services/boards/visits/create_service.rb +++ b/app/services/boards/visits/create_service.rb @@ -5,13 +5,17 @@ module Boards class CreateService < Boards::BaseService def execute(board) return unless current_user && Gitlab::Database.read_write? - return unless board.is_a?(Board) # other board types do not support board visits yet + return unless board - if parent.is_a?(Group) - BoardGroupRecentVisit.visited!(current_user, board) - else - BoardProjectRecentVisit.visited!(current_user, board) - end + model.visited!(current_user, board) + end + + private + + def model + return BoardGroupRecentVisit if parent.is_a?(Group) + + BoardProjectRecentVisit end end end diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index ae756d0856e..adb989be218 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -10,7 +10,7 @@ class BulkCreateIntegrationService def execute service_list = ServiceList.new(batch, service_hash, association).to_array - Service.transaction do + Integration.transaction do results = bulk_insert(*service_list) if integration.data_fields_present? diff --git a/app/services/bulk_imports/export_service.rb b/app/services/bulk_imports/export_service.rb new file mode 100644 index 00000000000..33b3a8e187f --- /dev/null +++ b/app/services/bulk_imports/export_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module BulkImports + class ExportService + def initialize(portable:, user:) + @portable = portable + @current_user = user + end + + def execute + FileTransfer.config_for(portable).portable_relations.each do |relation| + RelationExportWorker.perform_async(current_user.id, portable.id, portable.class.name, relation) + end + + ServiceResponse.success + rescue StandardError => e + ServiceResponse.error( + message: e.class, + http_status: :unprocessable_entity + ) + end + + private + + attr_reader :portable, :current_user + end +end diff --git a/app/services/bulk_imports/relation_export_service.rb b/app/services/bulk_imports/relation_export_service.rb new file mode 100644 index 00000000000..53952a33b5f --- /dev/null +++ b/app/services/bulk_imports/relation_export_service.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +module BulkImports + class RelationExportService + include Gitlab::ImportExport::CommandLineUtil + + def initialize(user, portable, relation, jid) + @user = user + @portable = portable + @relation = relation + @jid = jid + end + + def execute + find_or_create_export! do |export| + remove_existing_export_file!(export) + serialize_relation_to_file(export.relation_definition) + compress_exported_relation + upload_compressed_file(export) + end + end + + private + + attr_reader :user, :portable, :relation, :jid + + def find_or_create_export! + validate_user_permissions! + + export = portable.bulk_import_exports.safe_find_or_create_by!(relation: relation) + export.update!(status_event: 'start', jid: jid) + + yield export + + export.update!(status_event: 'finish', error: nil) + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, portable_id: portable.id, portable_type: portable.class.name) + + export&.update(status_event: 'fail_op', error: e.class) + end + + def validate_user_permissions! + ability = "admin_#{portable.to_ability_name}" + + user.can?(ability, portable) || + raise(::Gitlab::ImportExport::Error.permission_error(user, portable)) + end + + def remove_existing_export_file!(export) + upload = export.upload + + return unless upload&.export_file&.file + + upload.remove_export_file! + upload.save! + end + + def serialize_relation_to_file(relation_definition) + serializer.serialize_relation(relation_definition) + end + + def compress_exported_relation + gzip(dir: export_path, filename: ndjson_filename) + end + + def upload_compressed_file(export) + compressed_filename = File.join(export_path, "#{ndjson_filename}.gz") + upload = ExportUpload.find_or_initialize_by(export_id: export.id) # rubocop: disable CodeReuse/ActiveRecord + + File.open(compressed_filename) { |file| upload.export_file = file } + + upload.save! + end + + def config + @config ||= FileTransfer.config_for(portable) + end + + def export_path + @export_path ||= config.export_path + end + + def portable_tree + @portable_tree ||= config.portable_tree + end + + # rubocop: disable CodeReuse/Serializer + def serializer + @serializer ||= ::Gitlab::ImportExport::JSON::StreamingSerializer.new( + portable, + portable_tree, + json_writer, + exportable_path: '' + ) + end + # rubocop: enable CodeReuse/Serializer + + def json_writer + @json_writer ||= ::Gitlab::ImportExport::JSON::NdjsonWriter.new(export_path) + end + + def ndjson_filename + @ndjson_filename ||= "#{relation}.ndjson" + end + end +end diff --git a/app/services/bulk_update_integration_service.rb b/app/services/bulk_update_integration_service.rb index 5ddfdd359c2..29cfd824c12 100644 --- a/app/services/bulk_update_integration_service.rb +++ b/app/services/bulk_update_integration_service.rb @@ -8,8 +8,8 @@ class BulkUpdateIntegrationService # rubocop: disable CodeReuse/ActiveRecord def execute - Service.transaction do - Service.where(id: batch.select(:id)).update_all(service_hash) + Integration.transaction do + Integration.where(id: batch.select(:id)).update_all(service_hash) if integration.data_fields_present? integration.data_fields.class.where(service_id: batch.select(:id)).update_all(data_fields_hash) diff --git a/app/services/chat_names/find_user_service.rb b/app/services/chat_names/find_user_service.rb index c91738fa4c7..3dd3ba7f01c 100644 --- a/app/services/chat_names/find_user_service.rb +++ b/app/services/chat_names/find_user_service.rb @@ -2,8 +2,8 @@ module ChatNames class FindUserService - def initialize(service, params) - @service = service + def initialize(integration, params) + @integration = integration @params = params end @@ -20,7 +20,7 @@ module ChatNames # rubocop: disable CodeReuse/ActiveRecord def find_chat_name ChatName.find_by( - service: @service, + integration: @integration, team_id: @params[:team_id], chat_id: @params[:user_id] ) diff --git a/app/services/ci/after_requeue_job_service.rb b/app/services/ci/after_requeue_job_service.rb index 3858ee9d550..2b611c857c7 100644 --- a/app/services/ci/after_requeue_job_service.rb +++ b/app/services/ci/after_requeue_job_service.rb @@ -4,7 +4,7 @@ module Ci class AfterRequeueJobService < ::BaseService def execute(processable) process_subsequent_jobs(processable) - reset_ancestor_bridges(processable) + reset_source_bridge(processable) end private @@ -15,8 +15,8 @@ module Ci end end - def reset_ancestor_bridges(processable) - processable.pipeline.reset_ancestor_bridges! + def reset_source_bridge(processable) + processable.pipeline.reset_source_bridge!(current_user) end def process(processable) diff --git a/app/services/ci/archive_trace_service.rb b/app/services/ci/archive_trace_service.rb index 9b2c7788897..bc3219fbd79 100644 --- a/app/services/ci/archive_trace_service.rb +++ b/app/services/ci/archive_trace_service.rb @@ -24,7 +24,7 @@ module Ci end rescue ::Gitlab::Ci::Trace::AlreadyArchivedError # It's already archived, thus we can safely ignore this exception. - rescue => e + rescue StandardError => e # Tracks this error with application logs, Sentry, and Prometheus. # If `archive!` keeps failing for over a week, that could incur data loss. # (See more https://docs.gitlab.com/ee/administration/job_logs.html#new-incremental-logging-architecture) diff --git a/app/services/ci/change_variable_service.rb b/app/services/ci/change_variable_service.rb index f515a335d54..83cd6aae14b 100644 --- a/app/services/ci/change_variable_service.rb +++ b/app/services/ci/change_variable_service.rb @@ -30,4 +30,4 @@ module Ci end end -::Ci::ChangeVariableService.prepend_if_ee('EE::Ci::ChangeVariableService') +::Ci::ChangeVariableService.prepend_mod_with('Ci::ChangeVariableService') diff --git a/app/services/ci/change_variables_service.rb b/app/services/ci/change_variables_service.rb index 3337eb09411..7a68bd2e2b3 100644 --- a/app/services/ci/change_variables_service.rb +++ b/app/services/ci/change_variables_service.rb @@ -8,4 +8,4 @@ module Ci end end -::Ci::ChangeVariablesService.prepend_if_ee('EE::Ci::ChangeVariablesService') +::Ci::ChangeVariablesService.prepend_mod_with('Ci::ChangeVariablesService') diff --git a/app/services/ci/create_downstream_pipeline_service.rb b/app/services/ci/create_downstream_pipeline_service.rb index 93f0338fcba..64a99e404c6 100644 --- a/app/services/ci/create_downstream_pipeline_service.rb +++ b/app/services/ci/create_downstream_pipeline_service.rb @@ -85,6 +85,12 @@ module Ci return false end + if has_cyclic_dependency? + @bridge.drop!(:pipeline_loop_detected) + + return false + end + true end @@ -109,11 +115,24 @@ module Ci end end + def has_cyclic_dependency? + return false if @bridge.triggers_child_pipeline? + + if Feature.enabled?(:ci_drop_cyclical_triggered_pipelines, @bridge.project, default_enabled: :yaml) + checksums = @bridge.pipeline.base_and_ancestors.map { |pipeline| config_checksum(pipeline) } + checksums.uniq.length != checksums.length + end + end + def has_max_descendants_depth? return false unless @bridge.triggers_child_pipeline? ancestors_of_new_child = @bridge.pipeline.base_and_ancestors(same_project: true) ancestors_of_new_child.count > MAX_DESCENDANTS_DEPTH end + + def config_checksum(pipeline) + [pipeline.project_id, pipeline.ref].hash + end end end diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index ca936307acc..fd333e24860 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -10,6 +10,7 @@ module Ci Gitlab::Ci::Pipeline::Chain::Build::Associations, Gitlab::Ci::Pipeline::Chain::Validate::Abilities, Gitlab::Ci::Pipeline::Chain::Validate::Repository, + Gitlab::Ci::Pipeline::Chain::Validate::SecurityOrchestrationPolicy, Gitlab::Ci::Pipeline::Chain::Config::Content, Gitlab::Ci::Pipeline::Chain::Config::Process, Gitlab::Ci::Pipeline::Chain::RemoveUnwantedChatJobs, @@ -84,7 +85,6 @@ module Ci if pipeline.persisted? schedule_head_pipeline_update - record_conversion_event create_namespace_onboarding_action end @@ -122,12 +122,6 @@ module Ci end end - def record_conversion_event - return unless project.namespace.recent? - - Experiments::RecordConversionEventWorker.perform_async(:ci_syntax_templates_b, current_user.id) - end - def create_namespace_onboarding_action Namespaces::OnboardingPipelineCreatedWorker.perform_async(project.namespace_id) end @@ -138,4 +132,4 @@ module Ci end end -Ci::CreatePipelineService.prepend_if_ee('EE::Ci::CreatePipelineService') +Ci::CreatePipelineService.prepend_mod_with('Ci::CreatePipelineService') diff --git a/app/services/ci/create_web_ide_terminal_service.rb b/app/services/ci/create_web_ide_terminal_service.rb index 3b89a599180..db8f61c81fa 100644 --- a/app/services/ci/create_web_ide_terminal_service.rb +++ b/app/services/ci/create_web_ide_terminal_service.rb @@ -28,6 +28,11 @@ module Ci def create_pipeline! build_pipeline.tap do |pipeline| pipeline.stages << terminal_stage_seed(pipeline).to_resource + + # Project iid must be called outside a transaction, so we ensure it is set here + # otherwise it may be set within the save! which it will lock the InternalId row for the whole transaction + pipeline.ensure_project_iid! + pipeline.save! Ci::ProcessPipelineService diff --git a/app/services/ci/delete_unit_tests_service.rb b/app/services/ci/delete_unit_tests_service.rb new file mode 100644 index 00000000000..28f96351175 --- /dev/null +++ b/app/services/ci/delete_unit_tests_service.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Ci + class DeleteUnitTestsService + include EachBatch + + BATCH_SIZE = 100 + + def execute + purge_data!(Ci::UnitTestFailure) + purge_data!(Ci::UnitTest) + end + + private + + def purge_data!(klass) + loop do + break unless delete_batch!(klass) + end + end + + # rubocop: disable CodeReuse/ActiveRecord + def delete_batch!(klass) + deleted = 0 + + ActiveRecord::Base.transaction do + ids = klass.deletable.lock('FOR UPDATE SKIP LOCKED').limit(BATCH_SIZE).pluck(:id) + break if ids.empty? + + deleted = klass.where(id: ids).delete_all + end + + deleted > 0 + end + # rubocop: enable CodeReuse/ActiveRecord + end +end diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb index 2ae60907dab..80c83818d0b 100644 --- a/app/services/ci/expire_pipeline_cache_service.rb +++ b/app/services/ci/expire_pipeline_cache_service.rb @@ -56,6 +56,10 @@ module Ci url_helpers.graphql_etag_pipeline_path(pipeline) end + def graphql_pipeline_sha_path(sha) + url_helpers.graphql_etag_pipeline_sha_path(sha) + end + # Updates ETag caches of a pipeline. # # This logic resides in a separate method so that EE can more easily extend @@ -76,6 +80,7 @@ module Ci pipeline.self_with_ancestors_and_descendants.each do |relative_pipeline| store.touch(project_pipeline_path(relative_pipeline.project, relative_pipeline)) store.touch(graphql_pipeline_path(relative_pipeline)) + store.touch(graphql_pipeline_sha_path(relative_pipeline.sha)) end end diff --git a/app/services/ci/generate_codequality_mr_diff_report_service.rb b/app/services/ci/generate_codequality_mr_diff_report_service.rb index 3b1bd319a4f..117b0a21eaa 100644 --- a/app/services/ci/generate_codequality_mr_diff_report_service.rb +++ b/app/services/ci/generate_codequality_mr_diff_report_service.rb @@ -12,9 +12,9 @@ module Ci { status: :parsed, key: key(base_pipeline, head_pipeline), - data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_quality_mr_diff).present.for_files(merge_request.new_paths) + data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_quality_mr_diff).present.for_files(merge_request) } - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, project_id: project.id) { status: :error, diff --git a/app/services/ci/generate_coverage_reports_service.rb b/app/services/ci/generate_coverage_reports_service.rb index 4e6fbc5462a..12b1f19f4b5 100644 --- a/app/services/ci/generate_coverage_reports_service.rb +++ b/app/services/ci/generate_coverage_reports_service.rb @@ -14,7 +14,7 @@ module Ci key: key(base_pipeline, head_pipeline), data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_coverage).present.for_files(merge_request.new_paths) } - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception( e, project_id: project.id, diff --git a/app/services/ci/generate_exposed_artifacts_report_service.rb b/app/services/ci/generate_exposed_artifacts_report_service.rb index 1dbcd192279..dfa7cbd7d98 100644 --- a/app/services/ci/generate_exposed_artifacts_report_service.rb +++ b/app/services/ci/generate_exposed_artifacts_report_service.rb @@ -14,7 +14,7 @@ module Ci key: key(base_pipeline, head_pipeline), data: data } - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, project_id: project.id) { status: :error, diff --git a/app/services/ci/generate_terraform_reports_service.rb b/app/services/ci/generate_terraform_reports_service.rb index d768ce777d4..0ffb2d7e34a 100644 --- a/app/services/ci/generate_terraform_reports_service.rb +++ b/app/services/ci/generate_terraform_reports_service.rb @@ -13,7 +13,7 @@ module Ci key: key(base_pipeline, head_pipeline), data: head_pipeline.terraform_reports.plans } - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, project_id: project.id) { status: :error, diff --git a/app/services/ci/job_artifacts/create_service.rb b/app/services/ci/job_artifacts/create_service.rb index 65752e56c64..a22ac87f660 100644 --- a/app/services/ci/job_artifacts/create_service.rb +++ b/app/services/ci/job_artifacts/create_service.rb @@ -136,7 +136,7 @@ module Ci rescue *OBJECT_STORAGE_ERRORS => error track_exception(error, params) error(error.message, :service_unavailable) - rescue => error + rescue StandardError => error track_exception(error, params) error(error.message, :bad_request) end diff --git a/app/services/ci/job_artifacts/destroy_associations_service.rb b/app/services/ci/job_artifacts/destroy_associations_service.rb new file mode 100644 index 00000000000..794d24eadf2 --- /dev/null +++ b/app/services/ci/job_artifacts/destroy_associations_service.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Ci + module JobArtifacts + class DestroyAssociationsService + BATCH_SIZE = 100 + + def initialize(job_artifacts_relation) + @job_artifacts_relation = job_artifacts_relation + @statistics = {} + end + + def destroy_records + @job_artifacts_relation.each_batch(of: BATCH_SIZE) do |relation| + service = Ci::JobArtifacts::DestroyBatchService.new(relation, pick_up_at: Time.current) + result = service.execute(update_stats: false) + updates = result[:statistics_updates] + + @statistics.merge!(updates) { |_key, oldval, newval| newval + oldval } + end + end + + def update_statistics + @statistics.each do |project, delta| + project.increment_statistic_value(Ci::JobArtifact.project_statistics_name, delta) + end + end + end + end +end diff --git a/app/services/ci/job_artifacts/destroy_batch_service.rb b/app/services/ci/job_artifacts/destroy_batch_service.rb index 95315dd11ec..8536b88ccc0 100644 --- a/app/services/ci/job_artifacts/destroy_batch_service.rb +++ b/app/services/ci/job_artifacts/destroy_batch_service.rb @@ -23,8 +23,8 @@ module Ci end # rubocop: disable CodeReuse/ActiveRecord - def execute - return success(destroyed_artifacts_count: artifacts_count) if @job_artifacts.empty? + def execute(update_stats: true) + return success(destroyed_artifacts_count: 0, statistics_updates: {}) if @job_artifacts.empty? Ci::DeletedObject.transaction do Ci::DeletedObject.bulk_import(@job_artifacts, @pick_up_at) @@ -33,10 +33,11 @@ module Ci end # This is executed outside of the transaction because it depends on Redis - update_project_statistics + update_project_statistics! if update_stats increment_monitoring_statistics(artifacts_count) - success(destroyed_artifacts_count: artifacts_count) + success(destroyed_artifacts_count: artifacts_count, + statistics_updates: affected_project_statistics) end # rubocop: enable CodeReuse/ActiveRecord @@ -45,12 +46,20 @@ module Ci # This method is implemented in EE and it must do only database work def destroy_related_records(artifacts); end - def update_project_statistics - artifacts_by_project = @job_artifacts.group_by(&:project) - artifacts_by_project.each do |project, artifacts| - delta = -artifacts.sum { |artifact| artifact.size.to_i } - ProjectStatistics.increment_statistic( - project, Ci::JobArtifact.project_statistics_name, delta) + # using ! here since this can't be called inside a transaction + def update_project_statistics! + affected_project_statistics.each do |project, delta| + project.increment_statistic_value(Ci::JobArtifact.project_statistics_name, delta) + end + end + + def affected_project_statistics + strong_memoize(:affected_project_statistics) do + artifacts_by_project = @job_artifacts.group_by(&:project) + artifacts_by_project.each.with_object({}) do |(project, artifacts), accumulator| + delta = -artifacts.sum { |artifact| artifact.size.to_i } + accumulator[project] = delta + end end end @@ -71,4 +80,4 @@ module Ci end end -Ci::JobArtifacts::DestroyBatchService.prepend_if_ee('EE::Ci::JobArtifacts::DestroyBatchService') +Ci::JobArtifacts::DestroyBatchService.prepend_mod_with('Ci::JobArtifacts::DestroyBatchService') diff --git a/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb b/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb index 5c52eef7ba6..d6865efac9f 100644 --- a/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb +++ b/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb @@ -2,11 +2,18 @@ module Ci module PipelineArtifacts class CreateCodeQualityMrDiffReportService - def execute(pipeline) + include Gitlab::Utils::StrongMemoize + + def initialize(pipeline) + @pipeline = pipeline + end + + def execute return unless pipeline.can_generate_codequality_reports? return if pipeline.has_codequality_mr_diff_report? + return unless new_errors_introduced? - file = build_carrierwave_file(pipeline) + file = build_carrierwave_file! pipeline.pipeline_artifacts.create!( project_id: pipeline.project_id, @@ -20,18 +27,54 @@ module Ci private - def build_carrierwave_file(pipeline) + attr_reader :pipeline + + def merge_requests + strong_memoize(:merge_requests) do + pipeline.merge_requests_as_head_pipeline + end + end + + def head_report + strong_memoize(:head_report) do + pipeline.codequality_reports + end + end + + def base_report(merge_request) + strong_memoize(:base_report) do + merge_request&.base_pipeline&.codequality_reports + end + end + + def mr_diff_report_by_merge_requests + strong_memoize(:mr_diff_report_by_merge_requests) do + merge_requests.each_with_object({}) do |merge_request, hash| + key = "merge_request_#{merge_request.id}" + new_errors = Gitlab::Ci::Reports::CodequalityReportsComparer.new(base_report(merge_request), head_report).new_errors + next if new_errors.empty? + + hash[key] = Gitlab::Ci::Reports::CodequalityMrDiff.new(new_errors) + end + end + end + + def new_errors_introduced? + mr_diff_report_by_merge_requests.present? + end + + def build_carrierwave_file! CarrierWaveStringFile.new_file( - file_content: build_quality_mr_diff_report(pipeline), + file_content: build_quality_mr_diff_report(mr_diff_report_by_merge_requests), filename: Ci::PipelineArtifact::DEFAULT_FILE_NAMES.fetch(:code_quality_mr_diff), content_type: 'application/json' ) end - def build_quality_mr_diff_report(pipeline) - mr_diff_report = Gitlab::Ci::Reports::CodequalityMrDiff.new(pipeline.codequality_reports) - - Ci::CodequalityMrDiffReportSerializer.new.represent(mr_diff_report).to_json # rubocop: disable CodeReuse/Serializer + def build_quality_mr_diff_report(mr_diff_report) + mr_diff_report.each_with_object({}) do |diff_report, hash| + hash[diff_report.first] = Ci::CodequalityMrDiffReportSerializer.new.represent(diff_report.second) # rubocop: disable CodeReuse/Serializer + end.to_json end end end diff --git a/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb b/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb index fed40aef697..7b6590a117c 100644 --- a/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb +++ b/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb @@ -25,7 +25,7 @@ module Ci private def destroy_artifacts_batch - artifacts = ::Ci::PipelineArtifact.expired(BATCH_SIZE).to_a + artifacts = ::Ci::PipelineArtifact.unlocked.expired(BATCH_SIZE).to_a return false if artifacts.empty? artifacts.each(&:destroy!) diff --git a/app/services/ci/pipeline_bridge_status_service.rb b/app/services/ci/pipeline_bridge_status_service.rb index e2e5dd386f2..aeac43588f7 100644 --- a/app/services/ci/pipeline_bridge_status_service.rb +++ b/app/services/ci/pipeline_bridge_status_service.rb @@ -17,4 +17,4 @@ module Ci end end -Ci::PipelineBridgeStatusService.prepend_if_ee('EE::Ci::PipelineBridgeStatusService') +Ci::PipelineBridgeStatusService.prepend_mod_with('Ci::PipelineBridgeStatusService') diff --git a/app/services/ci/pipeline_trigger_service.rb b/app/services/ci/pipeline_trigger_service.rb index a5f70d62e13..62c4d6b4599 100644 --- a/app/services/ci/pipeline_trigger_service.rb +++ b/app/services/ci/pipeline_trigger_service.rb @@ -3,6 +3,7 @@ module Ci class PipelineTriggerService < BaseService include Gitlab::Utils::StrongMemoize + include Services::ReturnServiceResponses def execute if trigger_from_token @@ -20,7 +21,7 @@ module Ci private PAYLOAD_VARIABLE_KEY = 'TRIGGER_PAYLOAD' - PAYLOAD_VARIABLE_HIDDEN_PARAMS = %i(token).freeze + PAYLOAD_VARIABLE_HIDDEN_PARAMS = %i[token].freeze def create_pipeline_from_trigger(trigger) # this check is to not leak the presence of the project if user cannot read it @@ -32,10 +33,17 @@ module Ci pipeline.trigger_requests.build(trigger: trigger) end - if pipeline.persisted? + pipeline_service_response(pipeline) + end + + def pipeline_service_response(pipeline) + if pipeline.created_successfully? success(pipeline: pipeline) + elsif pipeline.persisted? + err = pipeline.errors.messages.presence || pipeline.failure_reason.presence || 'Could not create pipeline' + error(err, :unprocessable_entity) else - error(pipeline.errors.messages, 400) + error(pipeline.errors.messages, :bad_request) end end @@ -61,11 +69,7 @@ module Ci pipeline.source_pipeline = source end - if pipeline.persisted? - success(pipeline: pipeline) - else - error(pipeline.errors.messages, 400) - end + pipeline_service_response(pipeline) end def job_from_token diff --git a/app/services/ci/prepare_build_service.rb b/app/services/ci/prepare_build_service.rb index 3f87c711270..ec61c43cce9 100644 --- a/app/services/ci/prepare_build_service.rb +++ b/app/services/ci/prepare_build_service.rb @@ -12,7 +12,7 @@ module Ci prerequisites.each(&:complete!) build.enqueue_preparing! - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, build_id: build.id) build.drop(:unmet_prerequisites) diff --git a/app/services/ci/process_build_service.rb b/app/services/ci/process_build_service.rb index 73cf3308fe7..5271c0fe93d 100644 --- a/app/services/ci/process_build_service.rb +++ b/app/services/ci/process_build_service.rb @@ -40,4 +40,4 @@ module Ci end end -Ci::ProcessBuildService.prepend_if_ee('EE::Ci::ProcessBuildService') +Ci::ProcessBuildService.prepend_mod_with('Ci::ProcessBuildService') diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb index 6c69df0c616..fb26d5d3356 100644 --- a/app/services/ci/process_pipeline_service.rb +++ b/app/services/ci/process_pipeline_service.rb @@ -48,7 +48,13 @@ module Ci # This counter is temporary. It will be used to check whether if we still use this method or not # after setting correct value of `GenericCommitStatus#retried`. # More info: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50465#note_491657115 - metrics.legacy_update_jobs_counter.increment if updated_count > 0 + if updated_count > 0 + Gitlab::AppJsonLogger.info(event: 'update_retried_is_used', + project_id: pipeline.project.id, + pipeline_id: pipeline.id) + + metrics.legacy_update_jobs_counter.increment + end end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/services/ci/prometheus_metrics/observe_histograms_service.rb b/app/services/ci/prometheus_metrics/observe_histograms_service.rb index 527d87f19c2..6bd3d2121ba 100644 --- a/app/services/ci/prometheus_metrics/observe_histograms_service.rb +++ b/app/services/ci/prometheus_metrics/observe_histograms_service.rb @@ -25,8 +25,6 @@ module Ci end def execute - return ServiceResponse.success(http_status: :accepted) unless enabled? - params .fetch(:histograms, []) .each(&method(:observe)) @@ -48,10 +46,6 @@ module Ci .fetch(name) { raise ActiveRecord::RecordNotFound } .call end - - def enabled? - ::Feature.enabled?(:ci_accept_frontend_prometheus_metrics, project, default_enabled: :yaml) - end end end end diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 90341b26fd6..461647ffccc 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -169,7 +169,7 @@ module Ci @metrics.increment_queue_operation(:build_conflict_transition) Result.new(nil, nil, false) - rescue => ex + rescue StandardError => ex @metrics.increment_queue_operation(:build_conflict_exception) # If an error (e.g. GRPC::DeadlineExceeded) occurred constructing @@ -233,7 +233,7 @@ module Ci Gitlab::OptimisticLocking.retry_lock(build, 3, name: 'register_job_scheduler_failure') do |subject| subject.drop!(:scheduler_failure) end - rescue => ex + rescue StandardError => ex build.doom! # This requires extra exception, otherwise we would loose information @@ -253,17 +253,23 @@ module Ci # rubocop: disable CodeReuse/ActiveRecord def builds_for_shared_runner - new_builds. + relation = new_builds. # don't run projects which have not enabled shared runners and builds joins(:project).where(projects: { shared_runners_enabled: true, pending_delete: false }) .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id') - .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0'). + .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') - # Implement fair scheduling - # this returns builds that are ordered by number of running builds - # we prefer projects that don't use shared runners at all - joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") - .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC') + if Feature.enabled?(:ci_queueing_disaster_recovery, runner, type: :ops, default_enabled: :yaml) + # if disaster recovery is enabled, we fallback to FIFO scheduling + relation.order('ci_builds.id ASC') + else + # Implement fair scheduling + # this returns builds that are ordered by number of running builds + # we prefer projects that don't use shared runners at all + relation + .joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") + .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC') + end end # rubocop: enable CodeReuse/ActiveRecord @@ -310,4 +316,4 @@ module Ci end end -Ci::RegisterJobService.prepend_if_ee('EE::Ci::RegisterJobService') +Ci::RegisterJobService.prepend_mod_with('Ci::RegisterJobService') diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index e3de7f43fda..e03f2ae3d52 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -18,16 +18,14 @@ module Ci AfterRequeueJobService.new(project, current_user).execute(build) ::MergeRequests::AddTodoWhenBuildFailsService - .new(project, current_user) + .new(project: project, current_user: current_user) .close(new_build) end end # rubocop: disable CodeReuse/ActiveRecord def reprocess!(build) - unless can?(current_user, :update_build, build) - raise Gitlab::Access::AccessDeniedError - end + check_access!(build) attributes = self.class.clone_accessors.to_h do |attribute| [attribute, build.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend @@ -52,6 +50,12 @@ module Ci private + def check_access!(build) + unless can?(current_user, :update_build, build) + raise Gitlab::Access::AccessDeniedError + end + end + def create_build!(attributes) build = project.builds.new(attributes) build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(build)) @@ -64,4 +68,4 @@ module Ci end end -Ci::RetryBuildService.prepend_if_ee('EE::Ci::RetryBuildService') +Ci::RetryBuildService.prepend_mod_with('Ci::RetryBuildService') diff --git a/app/services/ci/retry_pipeline_service.rb b/app/services/ci/retry_pipeline_service.rb index bb8590a769c..5cc6b89bfef 100644 --- a/app/services/ci/retry_pipeline_service.rb +++ b/app/services/ci/retry_pipeline_service.rb @@ -26,10 +26,10 @@ module Ci retry_optimistic_lock(skipped, name: 'ci_retry_pipeline') { |build| build.process(current_user) } end - pipeline.reset_ancestor_bridges! + pipeline.reset_source_bridge!(current_user) ::MergeRequests::AddTodoWhenBuildFailsService - .new(project, current_user) + .new(project: project, current_user: current_user) .close_all(pipeline) Ci::ProcessPipelineService diff --git a/app/services/ci/stop_environments_service.rb b/app/services/ci/stop_environments_service.rb index 81457130fa0..7c9fc44e7f4 100644 --- a/app/services/ci/stop_environments_service.rb +++ b/app/services/ci/stop_environments_service.rb @@ -27,7 +27,7 @@ module Ci stop_actions.each do |stop_action| stop_action.play(stop_action.user) - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, deployable_id: stop_action.id) end end @@ -35,7 +35,7 @@ module Ci private def environments - @environments ||= EnvironmentsByDeploymentsFinder + @environments ||= Environments::EnvironmentsByDeploymentsFinder .new(project, current_user, ref: @ref, recently_updated: true) .execute end diff --git a/app/services/ci/test_failure_history_service.rb b/app/services/ci/test_failure_history_service.rb index 58bbc716ff0..a3f45c1b9cd 100644 --- a/app/services/ci/test_failure_history_service.rb +++ b/app/services/ci/test_failure_history_service.rb @@ -30,7 +30,7 @@ module Ci end def should_track_failures? - return false unless project.default_branch_or_master == pipeline.ref + return false unless project.default_branch_or_main == pipeline.ref # We fetch for up to MAX_TRACKABLE_FAILURES + 1 builds. So if ever we get # 201 total number of builds with the assumption that each job has at least diff --git a/app/services/clusters/applications/check_installation_progress_service.rb b/app/services/clusters/applications/check_installation_progress_service.rb index 249abd3ff9d..10a12f30956 100644 --- a/app/services/clusters/applications/check_installation_progress_service.rb +++ b/app/services/clusters/applications/check_installation_progress_service.rb @@ -19,9 +19,7 @@ module Clusters def check_timeout if timed_out? - begin - app.make_errored!("Operation timed out. Check pod logs for #{pod_name} for more details.") - end + app.make_errored!("Operation timed out. Check pod logs for #{pod_name} for more details.") else ClusterWaitForAppInstallationWorker.perform_in( ClusterWaitForAppInstallationWorker::INTERVAL, app.name, app.id) diff --git a/app/services/clusters/applications/check_upgrade_progress_service.rb b/app/services/clusters/applications/check_upgrade_progress_service.rb index bc161218618..c4fd234b302 100644 --- a/app/services/clusters/applications/check_upgrade_progress_service.rb +++ b/app/services/clusters/applications/check_upgrade_progress_service.rb @@ -51,7 +51,7 @@ module Clusters def remove_pod helm_api.delete_pod!(pod_name) - rescue + rescue StandardError # no-op end diff --git a/app/services/clusters/applications/prometheus_config_service.rb b/app/services/clusters/applications/prometheus_config_service.rb index 50c4e26b0d0..d39d63c874f 100644 --- a/app/services/clusters/applications/prometheus_config_service.rb +++ b/app/services/clusters/applications/prometheus_config_service.rb @@ -96,8 +96,6 @@ module Clusters end def alert_manager_token - app.generate_alert_manager_token! - app.alert_manager_token end diff --git a/app/services/clusters/applications/prometheus_update_service.rb b/app/services/clusters/applications/prometheus_update_service.rb index 437f6ab1202..b8b50f06d72 100644 --- a/app/services/clusters/applications/prometheus_update_service.rb +++ b/app/services/clusters/applications/prometheus_update_service.rb @@ -2,6 +2,7 @@ module Clusters module Applications + # Deprecated, to be removed in %14.0 as part of https://gitlab.com/groups/gitlab-org/-/epics/4280 class PrometheusUpdateService < BaseHelmService attr_accessor :project @@ -11,6 +12,8 @@ module Clusters end def execute + raise NotImplementedError, 'Externally installed prometheus should not be modified!' unless app.managed_prometheus? + app.make_updating! helm_api.update(patch_command(values)) diff --git a/app/services/clusters/applications/schedule_update_service.rb b/app/services/clusters/applications/schedule_update_service.rb index 41718df9a98..4f130f76b87 100644 --- a/app/services/clusters/applications/schedule_update_service.rb +++ b/app/services/clusters/applications/schedule_update_service.rb @@ -14,6 +14,7 @@ module Clusters def execute return unless application + return unless application.managed_prometheus? if recently_scheduled? worker_class.perform_in(BACKOFF_DELAY, application.name, application.id, project.id, Time.current) diff --git a/app/services/clusters/aws/fetch_credentials_service.rb b/app/services/clusters/aws/fetch_credentials_service.rb index 497e676f549..e38852c7ec7 100644 --- a/app/services/clusters/aws/fetch_credentials_service.rb +++ b/app/services/clusters/aws/fetch_credentials_service.rb @@ -14,7 +14,7 @@ module Clusters end def execute - raise MissingRoleError.new('AWS provisioning role not configured') unless provision_role.present? + raise MissingRoleError, 'AWS provisioning role not configured' unless provision_role.present? ::Aws::AssumeRoleCredentials.new( client: client, @@ -54,7 +54,7 @@ module Clusters ## # If we haven't created a provider record yet, - # we restrict ourselves to read only access so + # we restrict ourselves to read-only access so # that we can safely expose credentials to the # frontend (to be used when populating the # creation form). diff --git a/app/services/clusters/integrations/create_service.rb b/app/services/clusters/integrations/create_service.rb index f9e9dd3e457..142f731a7d3 100644 --- a/app/services/clusters/integrations/create_service.rb +++ b/app/services/clusters/integrations/create_service.rb @@ -27,12 +27,15 @@ module Clusters private def integration - case params[:application_type] - when 'prometheus' - cluster.find_or_build_integration_prometheus - else - raise ArgumentError, "invalid application_type: #{params[:application_type]}" - end + @integration ||= \ + case params[:application_type] + when 'prometheus' + cluster.find_or_build_integration_prometheus + when 'elastic_stack' + cluster.find_or_build_integration_elastic_stack + else + raise ArgumentError, "invalid application_type: #{params[:application_type]}" + end end def authorized? diff --git a/app/services/clusters/management/create_project_service.rb b/app/services/clusters/management/create_project_service.rb deleted file mode 100644 index 5a0176edd12..00000000000 --- a/app/services/clusters/management/create_project_service.rb +++ /dev/null @@ -1,84 +0,0 @@ -# frozen_string_literal: true - -module Clusters - module Management - class CreateProjectService - CreateError = Class.new(StandardError) - - attr_reader :cluster, :current_user - - def initialize(cluster, current_user:) - @cluster = cluster - @current_user = current_user - end - - def execute - return unless management_project_required? - - project = create_management_project! - update_cluster!(project) - end - - private - - def management_project_required? - Feature.enabled?(:auto_create_cluster_management_project) && cluster.management_project.nil? - end - - def project_params - { - name: project_name, - description: project_description, - namespace_id: namespace.id, - visibility_level: Gitlab::VisibilityLevel::PRIVATE - } - end - - def project_name - "#{cluster.name} Cluster Management" - end - - def project_description - "This project is automatically generated and will be used to manage your Kubernetes cluster. [More information](#{docs_path})" - end - - def docs_path - Rails.application.routes.url_helpers.help_page_path('user/clusters/management_project') - end - - def create_management_project! - ::Projects::CreateService.new(current_user, project_params).execute.tap do |project| - errors = project.errors.full_messages - - if errors.any? - raise CreateError.new("Failed to create project: #{errors}") - end - end - end - - def update_cluster!(project) - unless cluster.update(management_project: project) - raise CreateError.new("Failed to update cluster: #{cluster.errors.full_messages}") - end - end - - def namespace - case cluster.cluster_type - when 'project_type' - cluster.project.namespace - when 'group_type' - cluster.group - when 'instance_type' - instance_administrators_group - else - raise NotImplementedError - end - end - - def instance_administrators_group - Gitlab::CurrentSettings.instance_administrators_group || - raise(CreateError.new('Instance administrators group not found')) - end - end - end -end diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb index a1498da302e..fc18420f6e4 100644 --- a/app/services/commits/create_service.rb +++ b/app/services/commits/create_service.rb @@ -113,4 +113,4 @@ module Commits end end -Commits::CreateService.prepend_if_ee('EE::Commits::CreateService') +Commits::CreateService.prepend_mod_with('Commits::CreateService') diff --git a/app/services/concerns/alert_management/alert_processing.rb b/app/services/concerns/alert_management/alert_processing.rb index 7b6f681fe3e..98d255dec27 100644 --- a/app/services/concerns/alert_management/alert_processing.rb +++ b/app/services/concerns/alert_management/alert_processing.rb @@ -19,11 +19,7 @@ module AlertManagement # Updates or creates alert from payload for project # including system notes def process_alert - if alert.persisted? - process_existing_alert - else - process_new_alert - end + alert.persisted? ? process_existing_alert : process_new_alert end # Creates or closes issue for alert and notifies stakeholders @@ -33,22 +29,16 @@ module AlertManagement end def process_existing_alert - if resolving_alert? - process_resolved_alert - else - process_firing_alert - end + resolving_alert? ? process_resolved_alert : process_firing_alert end def process_resolved_alert SystemNoteService.log_resolving_alert(alert, alert_source) - return unless auto_close_incident? - if alert.resolve(incoming_payload.ends_at) SystemNoteService.change_alert_status(alert, User.alert_bot) - close_issue(alert.issue) + close_issue(alert.issue) if auto_close_incident? else logger.warn( message: 'Unable to update AlertManagement::Alert status to resolved', @@ -66,7 +56,7 @@ module AlertManagement return if issue.blank? || issue.closed? ::Issues::CloseService - .new(project, User.alert_bot) + .new(project: project, current_user: User.alert_bot) .execute(issue, system_note: false) SystemNoteService.auto_resolve_prometheus_alert(issue, project, User.alert_bot) if issue.reset.closed? @@ -76,6 +66,8 @@ module AlertManagement if alert.save alert.execute_services SystemNoteService.create_new_alert(alert, alert_source) + + process_resolved_alert if resolving_alert? else logger.warn( message: "Unable to create AlertManagement::Alert from #{alert_source}", @@ -88,7 +80,7 @@ module AlertManagement def process_incident_issues return if alert.issue || alert.resolved? - ::IncidentManagement::ProcessAlertWorker.perform_async(nil, nil, alert.id) + ::IncidentManagement::ProcessAlertWorkerV2.perform_async(alert.id) end def send_alert_email @@ -128,7 +120,7 @@ module AlertManagement end def alert_source - alert.monitoring_tool + incoming_payload.monitoring_tool end def logger @@ -137,4 +129,4 @@ module AlertManagement end end -AlertManagement::AlertProcessing.prepend_ee_mod +AlertManagement::AlertProcessing.prepend_mod diff --git a/app/services/concerns/integrations/project_test_data.rb b/app/services/concerns/integrations/project_test_data.rb index 5968b90f8fe..acaa773fd49 100644 --- a/app/services/concerns/integrations/project_test_data.rb +++ b/app/services/concerns/integrations/project_test_data.rb @@ -63,7 +63,7 @@ module Integrations return { error: s_('TestHooks|Ensure the project has deployments.') } unless deployment.present? - Gitlab::DataBuilder::Deployment.build(deployment) + Gitlab::DataBuilder::Deployment.build(deployment, Time.current) end def releases_events_data diff --git a/app/services/concerns/measurable.rb b/app/services/concerns/measurable.rb index fcb3022a1dc..ebce8a0667a 100644 --- a/app/services/concerns/measurable.rb +++ b/app/services/concerns/measurable.rb @@ -23,7 +23,7 @@ # end # end # -# DummyService.prepend_if_ee('EE::DummyService') +# DummyService.prepend_mod_with('DummyService') # DummyService.prepend(Measurable) # ``` # diff --git a/app/services/concerns/services/return_service_responses.rb b/app/services/concerns/services/return_service_responses.rb new file mode 100644 index 00000000000..75432b76033 --- /dev/null +++ b/app/services/concerns/services/return_service_responses.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Services + # adapter for existing services over BaseServiceUtility - add error and + # success methods returning ServiceResponse objects + module ReturnServiceResponses + def error(message, http_status, pass_back: {}) + ServiceResponse.error(message: message, http_status: http_status, payload: pass_back) + end + + def success(payload) + ServiceResponse.success(payload: payload) + end + end +end diff --git a/app/services/container_expiration_policies/cleanup_service.rb b/app/services/container_expiration_policies/cleanup_service.rb index 69e5620d986..38a3fc231c6 100644 --- a/app/services/container_expiration_policies/cleanup_service.rb +++ b/app/services/container_expiration_policies/cleanup_service.rb @@ -13,13 +13,20 @@ module ContainerExpirationPolicies def execute return ServiceResponse.error(message: 'no repository') unless repository + unless policy.valid? + disable_policy! + + return ServiceResponse.error(message: 'invalid policy') + end + repository.start_expiration_policy! + schedule_next_run_if_needed begin service_result = Projects::ContainerRepository::CleanupTagsService .new(project, nil, policy_params.merge('container_expiration_policy' => true)) .execute(repository) - rescue + rescue StandardError repository.cleanup_unfinished! raise @@ -28,7 +35,6 @@ module ContainerExpirationPolicies if service_result[:status] == :success repository.update!( expiration_policy_cleanup_status: :cleanup_unscheduled, - expiration_policy_started_at: nil, expiration_policy_completed_at: Time.zone.now ) @@ -42,6 +48,27 @@ module ContainerExpirationPolicies private + def schedule_next_run_if_needed + return unless Feature.enabled?(:container_registry_expiration_policies_loopless) + return if policy.next_run_at.future? + + repos_before_next_run = ::ContainerRepository.for_project_id(policy.project_id) + .expiration_policy_started_at_nil_or_before(policy.next_run_at) + return if repos_before_next_run.exists? + + policy.schedule_next_run! + end + + def disable_policy! + policy.disable! + repository.cleanup_unscheduled! + + Gitlab::ErrorTracking.log_exception( + ::ContainerExpirationPolicyWorker::InvalidPolicyError.new, + container_expiration_policy_id: policy.id + ) + end + def success(cleanup_status, service_result) payload = { cleanup_status: cleanup_status, diff --git a/app/services/deploy_keys/create_service.rb b/app/services/deploy_keys/create_service.rb index 2dac94c7ade..3245e749164 100644 --- a/app/services/deploy_keys/create_service.rb +++ b/app/services/deploy_keys/create_service.rb @@ -8,4 +8,4 @@ module DeployKeys end end -DeployKeys::CreateService.prepend_if_ee('::EE::DeployKeys::CreateService') +DeployKeys::CreateService.prepend_mod_with('DeployKeys::CreateService') diff --git a/app/services/deployments/older_deployments_drop_service.rb b/app/services/deployments/older_deployments_drop_service.rb index 9283a5c1279..100d1267848 100644 --- a/app/services/deployments/older_deployments_drop_service.rb +++ b/app/services/deployments/older_deployments_drop_service.rb @@ -15,7 +15,7 @@ module Deployments Gitlab::OptimisticLocking.retry_lock(older_deployment.deployable, name: 'older_deployments_drop') do |deployable| deployable.drop(:forward_deployment_failure) end - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_exception(e, subject_id: @deployment.id, deployment_id: older_deployment.id) end end diff --git a/app/services/deployments/update_environment_service.rb b/app/services/deployments/update_environment_service.rb index 98fedb9f699..9e862d6fa52 100644 --- a/app/services/deployments/update_environment_service.rb +++ b/app/services/deployments/update_environment_service.rb @@ -77,4 +77,4 @@ module Deployments end end -Deployments::UpdateEnvironmentService.prepend_if_ee('EE::Deployments::UpdateEnvironmentService') +Deployments::UpdateEnvironmentService.prepend_mod_with('Deployments::UpdateEnvironmentService') diff --git a/app/services/design_management/copy_design_collection/copy_service.rb b/app/services/design_management/copy_design_collection/copy_service.rb index c0b32e1e9ae..496103f9e58 100644 --- a/app/services/design_management/copy_design_collection/copy_service.rb +++ b/app/services/design_management/copy_design_collection/copy_service.rb @@ -47,7 +47,7 @@ module DesignManagement end ServiceResponse.success - rescue => error + rescue StandardError => error log_exception(error) target_design_collection.error_copy! diff --git a/app/services/design_management/delete_designs_service.rb b/app/services/design_management/delete_designs_service.rb index a90c34d4e34..7f76bcc5626 100644 --- a/app/services/design_management/delete_designs_service.rb +++ b/app/services/design_management/delete_designs_service.rb @@ -67,4 +67,4 @@ module DesignManagement end end -DesignManagement::DeleteDesignsService.prepend_if_ee('EE::DesignManagement::DeleteDesignsService') +DesignManagement::DeleteDesignsService.prepend_mod_with('DesignManagement::DeleteDesignsService') diff --git a/app/services/design_management/save_designs_service.rb b/app/services/design_management/save_designs_service.rb index c26d2e7ab47..44ebd45f76e 100644 --- a/app/services/design_management/save_designs_service.rb +++ b/app/services/design_management/save_designs_service.rb @@ -141,4 +141,4 @@ module DesignManagement end end -DesignManagement::SaveDesignsService.prepend_if_ee('EE::DesignManagement::SaveDesignsService') +DesignManagement::SaveDesignsService.prepend_mod_with('DesignManagement::SaveDesignsService') diff --git a/app/services/discussions/resolve_service.rb b/app/services/discussions/resolve_service.rb index 91c3cf136a4..3b733023eae 100644 --- a/app/services/discussions/resolve_service.rb +++ b/app/services/discussions/resolve_service.rb @@ -44,7 +44,7 @@ module Discussions Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter .track_resolve_thread_action(user: current_user) - MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request) end SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb index 82917241347..d73c3417a8b 100644 --- a/app/services/draft_notes/publish_service.rb +++ b/app/services/draft_notes/publish_service.rb @@ -24,7 +24,7 @@ module DraftNotes create_note_from_draft(draft) draft.delete - MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request) end def publish_draft_notes @@ -41,7 +41,7 @@ module DraftNotes set_reviewed notification_service.async.new_review(review) - MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request) end def create_note_from_draft(draft) @@ -68,7 +68,7 @@ module DraftNotes end def set_reviewed - ::MergeRequests::MarkReviewerReviewedService.new(project, current_user).execute(merge_request) + ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user).execute(merge_request) end end end diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index c94505b2068..58fc9799673 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -12,4 +12,4 @@ module Emails end end -Emails::BaseService.prepend_if_ee('::EE::Emails::BaseService') +Emails::BaseService.prepend_mod_with('Emails::BaseService') diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb index 473256d9c6f..011978ba76a 100644 --- a/app/services/emails/create_service.rb +++ b/app/services/emails/create_service.rb @@ -12,4 +12,4 @@ module Emails end end -Emails::CreateService.prepend_if_ee('EE::Emails::CreateService') +Emails::CreateService.prepend_mod_with('Emails::CreateService') diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index 6e671f52d57..288bee84ef7 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -18,4 +18,4 @@ module Emails end end -Emails::DestroyService.prepend_if_ee('EE::Emails::DestroyService') +Emails::DestroyService.prepend_mod_with('Emails::DestroyService') diff --git a/app/services/error_tracking/issue_update_service.rb b/app/services/error_tracking/issue_update_service.rb index b8235678d1d..2f8bbfddef0 100644 --- a/app/services/error_tracking/issue_update_service.rb +++ b/app/services/error_tracking/issue_update_service.rb @@ -35,7 +35,7 @@ module ErrorTracking def close_issue(issue) Issues::CloseService - .new(project, current_user) + .new(project: project, current_user: current_user) .execute(issue, system_note: false) end diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb index 85658598afc..01a40fc6473 100644 --- a/app/services/event_create_service.rb +++ b/app/services/event_create_service.rb @@ -222,4 +222,4 @@ class EventCreateService end end -EventCreateService.prepend_if_ee('EE::EventCreateService') +EventCreateService.prepend_mod_with('EventCreateService') diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb index 825faf59c13..a49b981c680 100644 --- a/app/services/git/branch_hooks_service.rb +++ b/app/services/git/branch_hooks_service.rb @@ -96,7 +96,6 @@ module Git def track_ci_config_change_event return unless Gitlab::CurrentSettings.usage_ping_enabled? - return unless ::Feature.enabled?(:usage_data_unique_users_committing_ciconfigfile, project, default_enabled: :yaml) return unless default_branch? commits_changing_ci_config.each do |commit| @@ -227,4 +226,4 @@ module Git end end -Git::BranchHooksService.prepend_if_ee('::EE::Git::BranchHooksService') +Git::BranchHooksService.prepend_mod_with('Git::BranchHooksService') diff --git a/app/services/git/branch_push_service.rb b/app/services/git/branch_push_service.rb index d250bca7bf2..5dcc2de456c 100644 --- a/app/services/git/branch_push_service.rb +++ b/app/services/git/branch_push_service.rb @@ -94,4 +94,4 @@ module Git end end -Git::BranchPushService.prepend_if_ee('::EE::Git::BranchPushService') +Git::BranchPushService.prepend_mod_with('Git::BranchPushService') diff --git a/app/services/git/process_ref_changes_service.rb b/app/services/git/process_ref_changes_service.rb index d039ed00efc..6f348ff9e0b 100644 --- a/app/services/git/process_ref_changes_service.rb +++ b/app/services/git/process_ref_changes_service.rb @@ -77,7 +77,7 @@ module Git def merge_request_branches_for(ref_type, changes) return [] if ref_type == :tag - MergeRequests::PushedBranchesService.new(project, current_user, changes: changes).execute + MergeRequests::PushedBranchesService.new(project: project, current_user: current_user, params: { changes: changes }).execute end end end diff --git a/app/services/git/tag_hooks_service.rb b/app/services/git/tag_hooks_service.rb index 0e5e1bbc992..d83924fec28 100644 --- a/app/services/git/tag_hooks_service.rb +++ b/app/services/git/tag_hooks_service.rb @@ -35,4 +35,4 @@ module Git end end -Git::TagHooksService.prepend_if_ee('::EE::Git::TagHooksService') +Git::TagHooksService.prepend_mod_with('Git::TagHooksService') diff --git a/app/services/git/wiki_push_service.rb b/app/services/git/wiki_push_service.rb index 82958abfe6e..11aeb650a5b 100644 --- a/app/services/git/wiki_push_service.rb +++ b/app/services/git/wiki_push_service.rb @@ -86,4 +86,4 @@ module Git end end -Git::WikiPushService.prepend_if_ee('EE::Git::WikiPushService') +Git::WikiPushService.prepend_mod_with('Git::WikiPushService') diff --git a/app/services/groups/autocomplete_service.rb b/app/services/groups/autocomplete_service.rb new file mode 100644 index 00000000000..92b05d9ac08 --- /dev/null +++ b/app/services/groups/autocomplete_service.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Groups + class AutocompleteService < Groups::BaseService + include LabelsAsHash + + # rubocop: disable CodeReuse/ActiveRecord + def issues(confidential_only: false, issue_types: nil) + finder_params = { group_id: group.id, include_subgroups: true, state: 'opened' } + finder_params[:confidential] = true if confidential_only.present? + finder_params[:issue_types] = issue_types if issue_types.present? + + IssuesFinder.new(current_user, finder_params) + .execute + .preload(project: :namespace) + .select(:iid, :title, :project_id) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def merge_requests + MergeRequestsFinder.new(current_user, group_id: group.id, include_subgroups: true, state: 'opened') + .execute + .preload(target_project: :namespace) + .select(:iid, :title, :target_project_id) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def milestones + group_ids = group.self_and_ancestors.public_or_visible_to_user(current_user).pluck(:id) + + MilestonesFinder.new(group_ids: group_ids).execute.select(:iid, :title, :due_date) + end + # rubocop: enable CodeReuse/ActiveRecord + + def labels_as_hash(target) + super(target, group_id: group.id, only_group_labels: true, include_ancestor_groups: true) + end + + def commands(noteable) + return [] unless noteable + + QuickActions::InterpretService.new(nil, current_user).available_commands(noteable) + end + end +end + +Groups::AutocompleteService.prepend_mod diff --git a/app/services/groups/count_service.rb b/app/services/groups/count_service.rb index 2a15ae3bc57..735acddb025 100644 --- a/app/services/groups/count_service.rb +++ b/app/services/groups/count_service.rb @@ -19,13 +19,26 @@ module Groups cached_count = Rails.cache.read(cache_key) return cached_count unless cached_count.blank? - refreshed_count = uncached_count - update_cache_for_key(cache_key) { refreshed_count } if refreshed_count > CACHED_COUNT_THRESHOLD - refreshed_count + refresh_cache_over_threshold end - def cache_key - ['groups', "#{issuable_key}_count_service", VERSION, group.id, cache_key_name] + def refresh_cache_over_threshold(key = nil) + key ||= cache_key + new_count = uncached_count + + if new_count > CACHED_COUNT_THRESHOLD + update_cache_for_key(key) { new_count } + else + delete_cache + end + + new_count + end + + def cache_key(key_name = nil) + key_name ||= cache_key_name + + ['groups', "#{issuable_key}_count_service", VERSION, group.id, key_name] end private diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 9ddb8ae7695..8e8efe7d555 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -37,7 +37,7 @@ module Groups Group.transaction do if @group.save @group.add_owner(current_user) - Service.create_from_active_default_integrations(@group, :group_id) + Integration.create_from_active_default_integrations(@group, :group_id) OnboardingProgress.onboard(@group) end end @@ -103,4 +103,4 @@ module Groups end end -Groups::CreateService.prepend_if_ee('EE::Groups::CreateService') +Groups::CreateService.prepend_mod_with('Groups::CreateService') diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index a27330d1104..08c4e0231e7 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -58,4 +58,4 @@ module Groups end end -Groups::DestroyService.prepend_if_ee('EE::Groups::DestroyService') +Groups::DestroyService.prepend_mod_with('Groups::DestroyService') diff --git a/app/services/groups/import_export/export_service.rb b/app/services/groups/import_export/export_service.rb index a436aec1b39..ea26ebec20b 100644 --- a/app/services/groups/import_export/export_service.rb +++ b/app/services/groups/import_export/export_service.rb @@ -96,7 +96,7 @@ module Groups def notify_error! notify_error - raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence) + raise Gitlab::ImportExport::Error, shared.errors.to_sentence end def notify_success @@ -127,4 +127,4 @@ module Groups end end -Groups::ImportExport::ExportService.prepend_if_ee('EE::Groups::ImportExport::ExportService') +Groups::ImportExport::ExportService.prepend_mod_with('Groups::ImportExport::ExportService') diff --git a/app/services/groups/import_export/import_service.rb b/app/services/groups/import_export/import_service.rb index bf3f09f22d4..f9db552f743 100644 --- a/app/services/groups/import_export/import_service.rb +++ b/app/services/groups/import_export/import_service.rb @@ -114,7 +114,7 @@ module Groups def notify_error! notify_error - raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence) + raise Gitlab::ImportExport::Error, shared.errors.to_sentence end def remove_base_tmp_dir @@ -124,4 +124,4 @@ module Groups end end -Groups::ImportExport::ImportService.prepend_if_ee('EE::Groups::ImportExport::ImportService') +Groups::ImportExport::ImportService.prepend_mod_with('Groups::ImportExport::ImportService') diff --git a/app/services/groups/open_issues_count_service.rb b/app/services/groups/open_issues_count_service.rb index ef787a04315..17cf3d38987 100644 --- a/app/services/groups/open_issues_count_service.rb +++ b/app/services/groups/open_issues_count_service.rb @@ -6,6 +6,12 @@ module Groups PUBLIC_COUNT_KEY = 'group_public_open_issues_count' TOTAL_COUNT_KEY = 'group_total_open_issues_count' + def clear_all_cache_keys + [cache_key(PUBLIC_COUNT_KEY), cache_key(TOTAL_COUNT_KEY)].each do |key| + Rails.cache.delete(key) + end + end + private def cache_key_name @@ -23,7 +29,14 @@ module Groups end def relation_for_count - IssuesFinder.new(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: public_only?).execute + IssuesFinder.new( + user, + group_id: group.id, + state: 'opened', + non_archived: true, + include_subgroups: true, + public_only: public_only? + ).execute end def issuable_key diff --git a/app/services/groups/participants_service.rb b/app/services/groups/participants_service.rb new file mode 100644 index 00000000000..0844c98dd6a --- /dev/null +++ b/app/services/groups/participants_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Groups + class ParticipantsService < Groups::BaseService + include Users::ParticipableService + + def execute(noteable) + @noteable = noteable + + participants = + noteable_owner + + participants_in_noteable + + all_members + + groups + + group_members + + render_participants_as_hash(participants.uniq) + end + + def all_members + count = group_members.count + [{ username: "all", name: "All Group Members", count: count }] + end + + def group_members + return [] unless noteable + + @group_members ||= sorted(noteable.group.direct_and_indirect_users) + end + end +end diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index e800e546a45..56ff1310def 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -200,16 +200,16 @@ module Groups end def update_integrations - @group.services.inherit.delete_all - Service.create_from_active_default_integrations(@group, :group_id) + @group.integrations.inherit.delete_all + Integration.create_from_active_default_integrations(@group, :group_id) end def propagate_integrations - @group.services.inherit.each do |integration| + @group.integrations.inherit.each do |integration| PropagateIntegrationWorker.perform_async(integration.id) end end end end -Groups::TransferService.prepend_if_ee('EE::Groups::TransferService') +Groups::TransferService.prepend_mod_with('Groups::TransferService') diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index ff369d01efc..1ad43b051be 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -147,4 +147,4 @@ module Groups end end -Groups::UpdateService.prepend_if_ee('EE::Groups::UpdateService') +Groups::UpdateService.prepend_mod_with('Groups::UpdateService') diff --git a/app/services/ide/schemas_config_service.rb b/app/services/ide/schemas_config_service.rb index 8d2ce97103d..a013a4679b5 100644 --- a/app/services/ide/schemas_config_service.rb +++ b/app/services/ide/schemas_config_service.rb @@ -10,7 +10,7 @@ module Ide def execute schema = predefined_schema_for(params[:filename]) || {} success(schema: schema) - rescue => e + rescue StandardError => e error(e.message) end @@ -46,4 +46,4 @@ module Ide end end -Ide::SchemasConfigService.prepend_if_ee('::EE::Ide::SchemasConfigService') +Ide::SchemasConfigService.prepend_mod_with('Ide::SchemasConfigService') diff --git a/app/services/import/base_service.rb b/app/services/import/base_service.rb index 2683c75e41f..4a43b2f7425 100644 --- a/app/services/import/base_service.rb +++ b/app/services/import/base_service.rb @@ -18,7 +18,7 @@ module Import group = Groups::NestedCreateService.new(current_user, group_path: namespace).execute group.errors.any? ? current_user.namespace : group - rescue => e + rescue StandardError => e Gitlab::AppLogger.error(e) current_user.namespace diff --git a/app/services/import/github_service.rb b/app/services/import/github_service.rb index 3ee5a185f42..2f808d45ffd 100644 --- a/app/services/import/github_service.rb +++ b/app/services/import/github_service.rb @@ -122,4 +122,4 @@ module Import end end -Import::GithubService.prepend_if_ee('EE::Import::GithubService') +Import::GithubService.prepend_mod_with('Import::GithubService') diff --git a/app/services/import/gitlab_projects/create_project_from_remote_file_service.rb b/app/services/import/gitlab_projects/create_project_from_remote_file_service.rb new file mode 100644 index 00000000000..bbfdaf692f9 --- /dev/null +++ b/app/services/import/gitlab_projects/create_project_from_remote_file_service.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +module Import + module GitlabProjects + class CreateProjectFromRemoteFileService < CreateProjectFromUploadedFileService + FILE_SIZE_LIMIT = 10.gigabytes + ALLOWED_CONTENT_TYPES = ['application/gzip'].freeze + + validate :valid_remote_import_url? + validate :validate_file_size + validate :validate_content_type + + private + + def required_params + [:path, :namespace, :remote_import_url] + end + + def project_params + super + .except(:file) + .merge(import_export_upload: ::ImportExportUpload.new( + remote_import_url: params[:remote_import_url] + )) + end + + def valid_remote_import_url? + ::Gitlab::UrlBlocker.validate!( + params[:remote_import_url], + allow_localhost: allow_local_requests?, + allow_local_network: allow_local_requests?, + schemes: %w(http https) + ) + + true + rescue ::Gitlab::UrlBlocker::BlockedUrlError => e + errors.add(:base, e.message) + + false + end + + def allow_local_requests? + ::Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? + end + + def validate_content_type + if headers['content-type'].blank? + errors.add(:base, "Missing 'ContentType' header") + elsif !ALLOWED_CONTENT_TYPES.include?(headers['content-type']) + errors.add(:base, "Remote file content type '%{content_type}' not allowed. (Allowed content types: %{allowed})" % { + content_type: headers['content-type'], + allowed: ALLOWED_CONTENT_TYPES.join(',') + }) + end + end + + def validate_file_size + if headers['content-length'].to_i == 0 + errors.add(:base, "Missing 'ContentLength' header") + elsif headers['content-length'].to_i > FILE_SIZE_LIMIT + errors.add(:base, 'Remote file larger than limit. (limit %{limit})' % { + limit: ActiveSupport::NumberHelper.number_to_human_size(FILE_SIZE_LIMIT) + }) + end + end + + def headers + return {} if params[:remote_import_url].blank? || !valid_remote_import_url? + + @headers ||= Gitlab::HTTP.head(params[:remote_import_url]).headers + end + end + end +end diff --git a/app/services/import/gitlab_projects/create_project_from_uploaded_file_service.rb b/app/services/import/gitlab_projects/create_project_from_uploaded_file_service.rb new file mode 100644 index 00000000000..35d52a11288 --- /dev/null +++ b/app/services/import/gitlab_projects/create_project_from_uploaded_file_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Import + module GitlabProjects + class CreateProjectFromUploadedFileService + include ActiveModel::Validations + include ::Services::ReturnServiceResponses + + validate :required_params_presence + + def initialize(current_user, params = {}) + @current_user = current_user + @params = params.dup + end + + def execute + return error(errors.full_messages.first) unless valid? + return error(project.errors.full_messages&.first) unless project.saved? + + success(project) + rescue StandardError => e + error(e.message) + end + + private + + attr_reader :current_user, :params + + def error(message) + super(message, :bad_request) + end + + def project + @project ||= ::Projects::GitlabProjectsImportService.new( + current_user, + project_params, + params[:override] + ).execute + end + + def project_params + { + name: params[:name], + path: params[:path], + namespace_id: params[:namespace].id, + file: params[:file], + overwrite: params[:overwrite], + import_type: 'gitlab_project' + } + end + + def required_params + [:path, :namespace, :file] + end + + def required_params_presence + required_params + .select { |key| params[key].blank? } + .each do |missing_parameter| + errors.add(:base, "Parameter '#{missing_parameter}' is required") + end + end + end + end +end diff --git a/app/services/incident_management/incidents/create_service.rb b/app/services/incident_management/incidents/create_service.rb index cff288d602b..7497ee00d74 100644 --- a/app/services/incident_management/incidents/create_service.rb +++ b/app/services/incident_management/incidents/create_service.rb @@ -15,11 +15,13 @@ module IncidentManagement def execute issue = Issues::CreateService.new( - project, - current_user, - title: title, - description: description, - issue_type: ISSUE_TYPE + project: project, + current_user: current_user, + params: { + title: title, + description: description, + issue_type: ISSUE_TYPE + } ).execute return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid? diff --git a/app/services/integrations/test/project_service.rb b/app/services/integrations/test/project_service.rb index d72ca928c34..31c8f02c7b6 100644 --- a/app/services/integrations/test/project_service.rb +++ b/app/services/integrations/test/project_service.rb @@ -42,4 +42,4 @@ module Integrations end end -Integrations::Test::ProjectService.prepend_if_ee('::EE::Integrations::Test::ProjectService') +Integrations::Test::ProjectService.prepend_mod_with('Integrations::Test::ProjectService') diff --git a/app/services/issuable/bulk_update_service.rb b/app/services/issuable/bulk_update_service.rb index 8bcbb92cd0e..cd32cd78728 100644 --- a/app/services/issuable/bulk_update_service.rb +++ b/app/services/issuable/bulk_update_service.rb @@ -15,9 +15,13 @@ module Issuable def execute(type) ids = params.delete(:issuable_ids).split(",") set_update_params(type) - items = update_issuables(type, ids) + updated_issuables = update_issuables(type, ids) - response_success(payload: { count: items.size }) + if updated_issuables.present? && requires_count_cache_reset?(type) + schedule_group_issues_count_reset(updated_issuables) + end + + response_success(payload: { count: updated_issuables.size }) rescue ArgumentError => e response_error(e.message, 422) end @@ -53,7 +57,7 @@ module Issuable items.each do |issuable| next unless can?(current_user, :"update_#{type}", issuable) - update_class.new(issuable.issuing_parent, current_user, params).execute(issuable) + update_class.new(**update_class.constructor_container_arg(issuable.issuing_parent), current_user: current_user, params: params).execute(issuable) end items @@ -81,7 +85,18 @@ module Issuable def response_error(message, http_status) ServiceResponse.error(message: message, http_status: http_status) end + + def requires_count_cache_reset?(type) + type.to_sym == :issue && params.include?(:state_event) + end + + def schedule_group_issues_count_reset(updated_issuables) + group_ids = updated_issuables.map(&:project).map(&:namespace_id) + return if group_ids.empty? + + Issuables::ClearGroupsIssueCounterWorker.perform_async(group_ids) + end end end -Issuable::BulkUpdateService.prepend_if_ee('EE::Issuable::BulkUpdateService') +Issuable::BulkUpdateService.prepend_mod_with('Issuable::BulkUpdateService') diff --git a/app/services/issuable/clone/attributes_rewriter.rb b/app/services/issuable/clone/attributes_rewriter.rb index 3861d88bce9..e1b4613726d 100644 --- a/app/services/issuable/clone/attributes_rewriter.rb +++ b/app/services/issuable/clone/attributes_rewriter.rb @@ -73,12 +73,17 @@ module Issuable copy_events(ResourceStateEvent.table_name, original_entity.resource_state_events) do |event| event.attributes - .except('id') + .except(*blocked_state_event_attributes) .merge(entity_key => new_entity.id, 'state' => ResourceStateEvent.states[event.state]) end end + # Overriden on EE::Issuable::Clone::AttributesRewriter + def blocked_state_event_attributes + ['id'] + end + def event_attributes_with_milestone(event, milestone) event.attributes .except('id') @@ -118,4 +123,4 @@ module Issuable end end -Issuable::Clone::AttributesRewriter.prepend_if_ee('EE::Issuable::Clone::AttributesRewriter') +Issuable::Clone::AttributesRewriter.prepend_mod_with('Issuable::Clone::AttributesRewriter') diff --git a/app/services/issuable/clone/base_service.rb b/app/services/issuable/clone/base_service.rb index 3c2bc527b12..f8a9eb3ece5 100644 --- a/app/services/issuable/clone/base_service.rb +++ b/app/services/issuable/clone/base_service.rb @@ -65,7 +65,7 @@ module Issuable end def close_issue - close_service = Issues::CloseService.new(old_project, current_user) + close_service = Issues::CloseService.new(project: old_project, current_user: current_user) close_service.execute(original_entity, notifications: false, system_note: false) end @@ -88,4 +88,4 @@ module Issuable end end -Issuable::Clone::BaseService.prepend_if_ee('EE::Issuable::Clone::BaseService') +Issuable::Clone::BaseService.prepend_mod_with('Issuable::Clone::BaseService') diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb index fd2dc3787c2..aedd0c377c6 100644 --- a/app/services/issuable/common_system_notes_service.rb +++ b/app/services/issuable/common_system_notes_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Issuable - class CommonSystemNotesService < ::BaseService + class CommonSystemNotesService < ::BaseProjectService attr_reader :issuable def execute(issuable, old_labels: [], old_milestone: nil, is_update: true) @@ -109,4 +109,4 @@ module Issuable end end -Issuable::CommonSystemNotesService.prepend_if_ee('EE::Issuable::CommonSystemNotesService') +Issuable::CommonSystemNotesService.prepend_mod_with('Issuable::CommonSystemNotesService') diff --git a/app/services/issuable/destroy_label_links_service.rb b/app/services/issuable/destroy_label_links_service.rb new file mode 100644 index 00000000000..6fff9b5e8d2 --- /dev/null +++ b/app/services/issuable/destroy_label_links_service.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Issuable + class DestroyLabelLinksService + BATCH_SIZE = 100 + + def initialize(target_id, target_type) + @target_id = target_id + @target_type = target_type + end + + def execute + inner_query = + LabelLink + .select(:id) + .for_target(target_id, target_type) + .limit(BATCH_SIZE) + + delete_query = <<~SQL + DELETE FROM "#{LabelLink.table_name}" + WHERE id IN (#{inner_query.to_sql}) + SQL + + loop do + result = ActiveRecord::Base.connection.execute(delete_query) + + break if result.cmd_tuples == 0 + end + end + + private + + attr_reader :target_id, :target_type + end +end diff --git a/app/services/issuable/destroy_service.rb b/app/services/issuable/destroy_service.rb index d5aa84d8d6c..b75905fb5b0 100644 --- a/app/services/issuable/destroy_service.rb +++ b/app/services/issuable/destroy_service.rb @@ -3,15 +3,13 @@ module Issuable class DestroyService < IssuableBaseService def execute(issuable) - if issuable.destroy - after_destroy(issuable) - end + after_destroy(issuable) if issuable.destroy end private def after_destroy(issuable) - delete_todos(issuable) + delete_associated_records(issuable) issuable.update_project_counter_caches issuable.assignees.each(&:invalidate_cache_counts) end @@ -20,19 +18,23 @@ module Issuable issuable.resource_parent.group end - def delete_todos(issuable) + def delete_associated_records(issuable) actor = group_for(issuable) - if Feature.enabled?(:destroy_issuable_todos_async, actor, default_enabled: :yaml) - TodosDestroyer::DestroyedIssuableWorker - .perform_async(issuable.id, issuable.class.name) - else - TodosDestroyer::DestroyedIssuableWorker - .new - .perform(issuable.id, issuable.class.name) - end + delete_todos(actor, issuable) + delete_label_links(actor, issuable) + end + + def delete_todos(actor, issuable) + TodosDestroyer::DestroyedIssuableWorker + .perform_async(issuable.id, issuable.class.name) + end + + def delete_label_links(actor, issuable) + Issuable::LabelLinksDestroyWorker + .perform_async(issuable.id, issuable.class.name) end end end -Issuable::DestroyService.prepend_if_ee('EE::Issuable::DestroyService') +Issuable::DestroyService.prepend_mod_with('Issuable::DestroyService') diff --git a/app/services/issuable/import_csv/base_service.rb b/app/services/issuable/import_csv/base_service.rb index 5a2665285de..27dbc8b3cc4 100644 --- a/app/services/issuable/import_csv/base_service.rb +++ b/app/services/issuable/import_csv/base_service.rb @@ -68,7 +68,7 @@ module Issuable end def create_issuable(attributes) - create_issuable_class.new(@project, @user, attributes).execute + create_issuable_class.new(project: @project, current_user: @user, params: attributes).execute end def email_results_to_user diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index add53bc6267..099e0d81bc9 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -1,11 +1,21 @@ # frozen_string_literal: true -class IssuableBaseService < BaseService +class IssuableBaseService < ::BaseProjectService private + def self.constructor_container_arg(value) + # TODO: Dynamically determining the type of a constructor arg based on the class is an antipattern, + # but the root cause is that Epics::BaseService has some issues that inheritance may not be the + # appropriate pattern. See more details in comments at the top of Epics::BaseService#initialize. + # Follow on issue to address this: + # https://gitlab.com/gitlab-org/gitlab/-/issues/328438 + + { project: value } + end + attr_accessor :params, :skip_milestone_email - def initialize(project, user = nil, params = {}) + def initialize(project:, current_user: nil, params: {}) super @skip_milestone_email = @params.delete(:skip_milestone_email) @@ -343,9 +353,13 @@ class IssuableBaseService < BaseService def change_state(issuable) case params.delete(:state_event) when 'reopen' - reopen_service.new(project, current_user, {}).execute(issuable) + service_class = reopen_service when 'close' - close_service.new(project, current_user, {}).execute(issuable) + service_class = close_service + end + + if service_class + service_class.new(**service_class.constructor_container_arg(project), current_user: current_user).execute(issuable) end end @@ -406,7 +420,7 @@ class IssuableBaseService < BaseService end def create_system_notes(issuable, **options) - Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, **options) + Issuable::CommonSystemNotesService.new(project: project, current_user: current_user).execute(issuable, **options) end def associations_before_update(issuable) @@ -493,4 +507,4 @@ class IssuableBaseService < BaseService end end -IssuableBaseService.prepend_if_ee('EE::IssuableBaseService') +IssuableBaseService.prepend_mod_with('IssuableBaseService') diff --git a/app/services/issuable_links/create_service.rb b/app/services/issuable_links/create_service.rb index cbb81f1f521..81685f81afa 100644 --- a/app/services/issuable_links/create_service.rb +++ b/app/services/issuable_links/create_service.rb @@ -118,4 +118,4 @@ module IssuableLinks end end -IssuableLinks::CreateService.prepend_if_ee('EE::IssuableLinks::CreateService') +IssuableLinks::CreateService.prepend_mod_with('IssuableLinks::CreateService') diff --git a/app/services/issue_links/create_service.rb b/app/services/issue_links/create_service.rb index 63762b1af79..a022d3e0bcf 100644 --- a/app/services/issue_links/create_service.rb +++ b/app/services/issue_links/create_service.rb @@ -43,4 +43,4 @@ module IssueLinks end end -IssueLinks::CreateService.prepend_if_ee('EE::IssueLinks::CreateService') +IssueLinks::CreateService.prepend_mod_with('IssueLinks::CreateService') diff --git a/app/services/issue_rebalancing_service.rb b/app/services/issue_rebalancing_service.rb index f9c3388204f..6a8d45b92b2 100644 --- a/app/services/issue_rebalancing_service.rb +++ b/app/services/issue_rebalancing_service.rb @@ -3,8 +3,18 @@ class IssueRebalancingService MAX_ISSUE_COUNT = 10_000 BATCH_SIZE = 100 + SMALLEST_BATCH_SIZE = 5 + RETRIES_LIMIT = 3 TooManyIssues = Class.new(StandardError) + TIMING_CONFIGURATION = [ + [0.1.seconds, 0.05.seconds], # short timings, lock_timeout: 100ms, sleep after LockWaitTimeout: 50ms + [0.5.seconds, 0.05.seconds], + [1.second, 0.5.seconds], + [1.second, 0.5.seconds], + [5.seconds, 1.second] + ].freeze + def initialize(issue) @issue = issue @base = Issue.relative_positioning_query_base(issue) @@ -23,14 +33,23 @@ class IssueRebalancingService assign_positions(start, indexed_ids) .sort_by(&:first) .each_slice(BATCH_SIZE) do |pairs_with_position| - update_positions(pairs_with_position, 'rebalance issue positions in batches ordered by id') + if Feature.enabled?(:issue_rebalancing_with_retry) + update_positions_with_retry(pairs_with_position, 'rebalance issue positions in batches ordered by id') + else + update_positions(pairs_with_position, 'rebalance issue positions in batches ordered by id') + end end end else Issue.transaction do indexed_ids.each_slice(BATCH_SIZE) do |pairs| pairs_with_position = assign_positions(start, pairs) - update_positions(pairs_with_position, 'rebalance issue positions') + + if Feature.enabled?(:issue_rebalancing_with_retry) + update_positions_with_retry(pairs_with_position, 'rebalance issue positions') + else + update_positions(pairs_with_position, 'rebalance issue positions') + end end end end @@ -52,12 +71,37 @@ class IssueRebalancingService end end + def update_positions_with_retry(pairs_with_position, query_name) + retries = 0 + batch_size = pairs_with_position.size + + until pairs_with_position.empty? + begin + update_positions(pairs_with_position.first(batch_size), query_name) + pairs_with_position = pairs_with_position.drop(batch_size) + retries = 0 + rescue ActiveRecord::StatementTimeout, ActiveRecord::QueryCanceled => ex + raise ex if batch_size < SMALLEST_BATCH_SIZE + + if (retries += 1) == RETRIES_LIMIT + # shrink the batch size in half when RETRIES limit is reached and update still fails perhaps because batch size is still too big + batch_size = (batch_size / 2).to_i + retries = 0 + end + + retry + end + end + end + def update_positions(pairs_with_position, query_name) values = pairs_with_position.map do |id, index| "(#{id}, #{index})" end.join(', ') - run_update_query(values, query_name) + Gitlab::Database::WithLockRetries.new(timing_configuration: TIMING_CONFIGURATION, klass: self.class).run do + run_update_query(values, query_name) + end end def run_update_query(values, query_name) diff --git a/app/services/issues/after_create_service.rb b/app/services/issues/after_create_service.rb index 0c6ec65f0e2..5d10eca2979 100644 --- a/app/services/issues/after_create_service.rb +++ b/app/services/issues/after_create_service.rb @@ -10,4 +10,4 @@ module Issues end end -Issues::AfterCreateService.prepend_ee_mod +Issues::AfterCreateService.prepend_mod diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 07e4a10708e..72e906e20f1 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -37,6 +37,8 @@ module Issues def filter_params(issue) super + params.delete(:issue_type) unless issue_type_allowed?(issue) + moved_issue = params.delete(:moved_issue) # Setting created_at, updated_at and iid is allowed only for admins and owners or @@ -75,7 +77,12 @@ module Issues Milestones::IssuesCountService.new(milestone).delete_cache end + + # @param object [Issue, Project] + def issue_type_allowed?(object) + can?(current_user, :"create_#{params[:issue_type]}", object) + end end end -Issues::BaseService.prepend_if_ee('EE::Issues::BaseService') +Issues::BaseService.prepend_mod_with('Issues::BaseService') diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb index 3145739fe91..5cb138946d7 100644 --- a/app/services/issues/build_service.rb +++ b/app/services/issues/build_service.rb @@ -64,20 +64,17 @@ module Issues private - def allowed_issue_base_params - [:title, :description, :confidential, :issue_type] - end + def allowed_issue_params + allowed_params = [ + :title, + :description, + :confidential + ] - def allowed_issue_admin_params - [:milestone_id] - end + allowed_params << :milestone_id if can?(current_user, :admin_issue, project) + allowed_params << :issue_type if issue_type_allowed?(project) - def allowed_issue_params - if can?(current_user, :admin_issue, project) - params.slice(*(allowed_issue_base_params + allowed_issue_admin_params)) - else - params.slice(*allowed_issue_base_params) - end + params.slice(*allowed_params) end def build_issue_params @@ -88,4 +85,4 @@ module Issues end end -Issues::BuildService.prepend_if_ee('EE::Issues::BuildService') +Issues::BuildService.prepend_mod_with('Issues::BuildService') diff --git a/app/services/issues/clone_service.rb b/app/services/issues/clone_service.rb index b64e4687a87..6df32f1104c 100644 --- a/app/services/issues/clone_service.rb +++ b/app/services/issues/clone_service.rb @@ -57,7 +57,7 @@ module Issues # Skip creation of system notes for existing attributes of the issue. The system notes of the old # issue are copied over so we don't want to end up with duplicate notes. - CreateService.new(target_project, current_user, new_params).execute(skip_system_notes: true) + CreateService.new(project: target_project, current_user: current_user, params: new_params).execute(skip_system_notes: true) end def queue_copy_designs @@ -90,4 +90,4 @@ module Issues end end -Issues::CloneService.prepend_if_ee('EE::Issues::CloneService') +Issues::CloneService.prepend_mod_with('Issues::CloneService') diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index 746f7d1f4c1..1700d1d8586 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -24,8 +24,8 @@ module Issues return issue end - if project.issues_enabled? && issue.close - issue.update(closed_by: current_user) + if project.issues_enabled? && issue.close(current_user) + remove_on_close_labels_from(issue) event_service.close_issue(issue, current_user) create_note(issue, closed_via) if system_note @@ -52,6 +52,18 @@ module Issues private + def remove_on_close_labels_from(issue) + old_labels = issue.labels.to_a + + issue.label_links.with_remove_on_close_labels.delete_all + issue.labels.reset + + Issuable::CommonSystemNotesService.new(project: project, current_user: current_user).execute( + issue, + old_labels: old_labels + ) + end + def close_external_issue(issue, closed_via) return unless project.external_issue_tracker&.support_close_issue? diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 68660b35bee..1f4efeb1a8a 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -8,7 +8,7 @@ module Issues @request = params.delete(:request) @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) - @issue = BuildService.new(project, current_user, params).execute + @issue = BuildService.new(project: project, current_user: current_user, params: params).execute filter_resolve_discussion_params @@ -75,4 +75,4 @@ module Issues end end -Issues::CreateService.prepend_ee_mod +Issues::CreateService.prepend_mod diff --git a/app/services/issues/duplicate_service.rb b/app/services/issues/duplicate_service.rb index feb496542c8..d150f0e5917 100644 --- a/app/services/issues/duplicate_service.rb +++ b/app/services/issues/duplicate_service.rb @@ -10,7 +10,7 @@ module Issues create_issue_duplicate_note(duplicate_issue, canonical_issue) create_issue_canonical_note(canonical_issue, duplicate_issue) - close_service.new(project, current_user, {}).execute(duplicate_issue) + close_service.new(project: project, current_user: current_user).execute(duplicate_issue) duplicate_issue.update(duplicated_to: canonical_issue) relate_two_issues(duplicate_issue, canonical_issue) diff --git a/app/services/issues/export_csv_service.rb b/app/services/issues/export_csv_service.rb index dd43c77adfa..3809d8bc347 100644 --- a/app/services/issues/export_csv_service.rb +++ b/app/services/issues/export_csv_service.rb @@ -58,4 +58,4 @@ module Issues end end -Issues::ExportCsvService.prepend_if_ee('EE::Issues::ExportCsvService') +Issues::ExportCsvService.prepend_mod_with('Issues::ExportCsvService') diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb index c1afb8f456d..e49123a2993 100644 --- a/app/services/issues/move_service.rb +++ b/app/services/issues/move_service.rb @@ -61,7 +61,7 @@ module Issues # Skip creation of system notes for existing attributes of the issue. The system notes of the old # issue are copied over so we don't want to end up with duplicate notes. - CreateService.new(@target_project, @current_user, new_params).execute(skip_system_notes: true) + CreateService.new(project: @target_project, current_user: @current_user, params: new_params).execute(skip_system_notes: true) end def queue_copy_designs @@ -106,4 +106,4 @@ module Issues end end -Issues::MoveService.prepend_if_ee('EE::Issues::MoveService') +Issues::MoveService.prepend_mod_with('Issues::MoveService') diff --git a/app/services/issues/related_branches_service.rb b/app/services/issues/related_branches_service.rb index 98d8412102f..8b08c1f8ddb 100644 --- a/app/services/issues/related_branches_service.rb +++ b/app/services/issues/related_branches_service.rb @@ -30,7 +30,7 @@ module Issues def branches_with_merge_request_for(issue) Issues::ReferencedMergeRequestsService - .new(project, current_user) + .new(project: project, current_user: current_user) .referenced_merge_requests(issue) .map(&:source_branch) end diff --git a/app/services/issues/reorder_service.rb b/app/services/issues/reorder_service.rb index c82ad6ea501..9c5fbec7d8e 100644 --- a/app/services/issues/reorder_service.rb +++ b/app/services/issues/reorder_service.rb @@ -21,7 +21,7 @@ module Issues end def update(issue, attrs) - ::Issues::UpdateService.new(project, current_user, attrs).execute(issue) + ::Issues::UpdateService.new(project: project, current_user: current_user, params: attrs).execute(issue) rescue ActiveRecord::RecordNotFound false end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 702527d80a7..af5029f8364 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -24,7 +24,7 @@ module Issues def filter_params(issue) super - # filter confidential in `Issues::UpdateService` and not in `IssuableBaseService#filtr_params` + # filter confidential in `Issues::UpdateService` and not in `IssuableBaseService#filter_params` # because we do allow users that cannot admin issues to set confidential flag when creating an issue unless can_admin_issuable?(issue) params.delete(:confidential) @@ -42,10 +42,6 @@ module Issues ).execute(spam_params: spam_params) end - def after_update(issue) - IssuesChannel.broadcast_to(issue, event: 'updated') if Gitlab::ActionCable::Config.in_app? || Feature.enabled?(:broadcast_issue_updates, issue.project) - end - def handle_changes(issue, options) old_associations = options.fetch(:old_associations, {}) old_labels = old_associations.fetch(:labels, []) @@ -61,12 +57,7 @@ module Issues todo_service.update_issue(issue, current_user, old_mentioned_users) end - if issue.assignees != old_assignees - create_assignee_note(issue, old_assignees) - notification_service.async.reassigned_issue(issue, current_user, old_assignees) - todo_service.reassigned_assignable(issue, current_user, old_assignees) - track_incident_action(current_user, issue, :incident_assigned) - end + handle_assignee_changes(issue, old_assignees) if issue.previous_changes.include?('confidential') # don't enqueue immediately to prevent todos removal in case of a mistake @@ -90,12 +81,27 @@ module Issues end end + def handle_assignee_changes(issue, old_assignees) + return if issue.assignees == old_assignees + + create_assignee_note(issue, old_assignees) + notification_service.async.reassigned_issue(issue, current_user, old_assignees) + todo_service.reassigned_assignable(issue, current_user, old_assignees) + track_incident_action(current_user, issue, :incident_assigned) + + if Gitlab::ActionCable::Config.in_app? || Feature.enabled?(:broadcast_issue_updates, issue.project) + GraphqlTriggers.issuable_assignees_updated(issue) + end + end + def handle_task_changes(issuable) todo_service.resolve_todos_for_target(issuable, current_user) todo_service.update_issue(issuable, current_user) end def handle_move_between_ids(issue) + issue.check_repositioning_allowed! if params[:move_between_ids] + super rebalance_if_needed(issue) @@ -113,7 +119,7 @@ module Issues canonical_issue = IssuesFinder.new(current_user).find_by(id: canonical_issue_id) if canonical_issue - Issues::DuplicateService.new(project, current_user).execute(issue, canonical_issue) + Issues::DuplicateService.new(project: project, current_user: current_user).execute(issue, canonical_issue) end end # rubocop: enable CodeReuse/ActiveRecord @@ -126,7 +132,7 @@ module Issues target_project != issue.project update(issue) - Issues::MoveService.new(project, current_user).execute(issue, target_project) + Issues::MoveService.new(project: project, current_user: current_user).execute(issue, target_project) end private @@ -142,14 +148,14 @@ module Issues # we've pre-empted this from running in #execute, so let's go ahead and update the Issue now. update(issue) - Issues::CloneService.new(project, current_user).execute(issue, target_project, with_notes: with_notes) + Issues::CloneService.new(project: project, current_user: current_user).execute(issue, target_project, with_notes: with_notes) end def create_merge_request_from_quick_action create_merge_request_params = params.delete(:create_merge_request) return unless create_merge_request_params - MergeRequests::CreateFromIssueService.new(project, current_user, create_merge_request_params).execute + MergeRequests::CreateFromIssueService.new(project: project, current_user: current_user, mr_params: create_merge_request_params).execute end def handle_milestone_change(issue) @@ -201,4 +207,4 @@ module Issues end end -Issues::UpdateService.prepend_if_ee('EE::Issues::UpdateService') +Issues::UpdateService.prepend_mod_with('Issues::UpdateService') diff --git a/app/services/issues/zoom_link_service.rb b/app/services/issues/zoom_link_service.rb index 1384e2f83b2..ef48134dec4 100644 --- a/app/services/issues/zoom_link_service.rb +++ b/app/services/issues/zoom_link_service.rb @@ -2,10 +2,10 @@ module Issues class ZoomLinkService < Issues::BaseService - def initialize(issue, user) - super(issue.project, user) + def initialize(project:, current_user:, params:) + super - @issue = issue + @issue = params.fetch(:issue) @added_meeting = ZoomMeeting.canonical_meeting(@issue) end diff --git a/app/services/jira_import/start_import_service.rb b/app/services/jira_import/start_import_service.rb index 88cfe684125..c9ffdeb2a16 100644 --- a/app/services/jira_import/start_import_service.rb +++ b/app/services/jira_import/start_import_service.rb @@ -41,7 +41,7 @@ module JiraImport project.save! && jira_import.schedule! ServiceResponse.success(payload: { import_data: jira_import } ) - rescue => ex + rescue StandardError => ex # in case project.save! raises an error Gitlab::ErrorTracking.track_exception(ex, project_id: project.id) jira_import&.do_fail!(error_message: ex.message) diff --git a/app/services/keys/create_service.rb b/app/services/keys/create_service.rb index c1c3ef8792f..507537391ed 100644 --- a/app/services/keys/create_service.rb +++ b/app/services/keys/create_service.rb @@ -19,4 +19,4 @@ module Keys end end -Keys::CreateService.prepend_if_ee('EE::Keys::CreateService') +Keys::CreateService.prepend_mod_with('Keys::CreateService') diff --git a/app/services/keys/destroy_service.rb b/app/services/keys/destroy_service.rb index 4552c5cf9a2..eaf5eb35f58 100644 --- a/app/services/keys/destroy_service.rb +++ b/app/services/keys/destroy_service.rb @@ -13,4 +13,4 @@ module Keys end end -Keys::DestroyService.prepend_if_ee('EE::Keys::DestroyService') +Keys::DestroyService.prepend_mod_with('Keys::DestroyService') diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb index 1d022740c44..ff29358df86 100644 --- a/app/services/labels/available_labels_service.rb +++ b/app/services/labels/available_labels_service.rb @@ -14,15 +14,16 @@ module Labels return [] unless labels - labels = labels.split(',') if labels.is_a?(String) + labels = labels.split(',').map(&:strip) if labels.is_a?(String) + existing_labels = LabelsFinder.new(current_user, finder_params(labels)).execute.index_by(&:title) labels.map do |label_name| label = Labels::FindOrCreateService.new( current_user, parent, include_ancestor_groups: true, - title: label_name.strip, - available_labels: available_labels + title: label_name, + existing_labels_by_title: existing_labels ).execute(find_only: find_only) label @@ -45,18 +46,19 @@ module Labels private - def finder_params - params = { include_ancestor_groups: true } + def finder_params(titles = nil) + finder_params = { include_ancestor_groups: true } + finder_params[:title] = titles if titles case parent when Group - params[:group_id] = parent.id - params[:only_group_labels] = true + finder_params[:group_id] = parent.id + finder_params[:only_group_labels] = true when Project - params[:project_id] = parent.id + finder_params[:project_id] = parent.id end - params + finder_params end end end diff --git a/app/services/labels/create_service.rb b/app/services/labels/create_service.rb index 665d1035b2b..6c070d15cdb 100644 --- a/app/services/labels/create_service.rb +++ b/app/services/labels/create_service.rb @@ -26,4 +26,4 @@ module Labels end end -Labels::CreateService.prepend_if_ee('EE::Labels::CreateService') +Labels::CreateService.prepend_mod_with('Labels::CreateService') diff --git a/app/services/labels/find_or_create_service.rb b/app/services/labels/find_or_create_service.rb index a47dd42aea0..112d156c214 100644 --- a/app/services/labels/find_or_create_service.rb +++ b/app/services/labels/find_or_create_service.rb @@ -6,6 +6,7 @@ module Labels @current_user = current_user @parent = parent @available_labels = params.delete(:available_labels) + @existing_labels_by_title = params.delete(:existing_labels_by_title) @params = params.dup.with_indifferent_access end @@ -16,7 +17,7 @@ module Labels private - attr_reader :current_user, :parent, :params, :skip_authorization + attr_reader :current_user, :parent, :params, :skip_authorization, :existing_labels_by_title def available_labels @available_labels ||= LabelsFinder.new( @@ -29,9 +30,8 @@ module Labels # Only creates the label if current_user can do so, if the label does not exist # and the user can not create the label, nil is returned - # rubocop: disable CodeReuse/ActiveRecord def find_or_create_label(find_only: false) - new_label = available_labels.find_by(title: title) + new_label = find_existing_label(title) return new_label if find_only @@ -42,6 +42,13 @@ module Labels new_label end + + # rubocop: disable CodeReuse/ActiveRecord + def find_existing_label(title) + return existing_labels_by_title[title] if existing_labels_by_title + + available_labels.find_by(title: title) + end # rubocop: enable CodeReuse/ActiveRecord def title diff --git a/app/services/labels/promote_service.rb b/app/services/labels/promote_service.rb index fdf2cf13f92..e3b110f8f26 100644 --- a/app/services/labels/promote_service.rb +++ b/app/services/labels/promote_service.rb @@ -101,4 +101,4 @@ module Labels end end -Labels::PromoteService.prepend_if_ee('EE::Labels::PromoteService') +Labels::PromoteService.prepend_mod_with('Labels::PromoteService') diff --git a/app/services/lfs/lock_file_service.rb b/app/services/lfs/lock_file_service.rb index 1b283018c16..54f193c86e6 100644 --- a/app/services/lfs/lock_file_service.rb +++ b/app/services/lfs/lock_file_service.rb @@ -12,7 +12,7 @@ module Lfs error('already locked', 409, current_lock) rescue Gitlab::GitAccess::ForbiddenError => ex error(ex.message, 403) - rescue => ex + rescue StandardError => ex error(ex.message, 500) end @@ -42,4 +42,4 @@ module Lfs end end -Lfs::LockFileService.prepend_if_ee('EE::Lfs::LockFileService') +Lfs::LockFileService.prepend_mod_with('Lfs::LockFileService') diff --git a/app/services/lfs/locks_finder_service.rb b/app/services/lfs/locks_finder_service.rb index 192ce3d3c2a..a77be643478 100644 --- a/app/services/lfs/locks_finder_service.rb +++ b/app/services/lfs/locks_finder_service.rb @@ -4,7 +4,7 @@ module Lfs class LocksFinderService < BaseService def execute success(locks: find_locks) - rescue => ex + rescue StandardError => ex error(ex.message, 500) end diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb index 9b947fbed07..e21988aa561 100644 --- a/app/services/lfs/push_service.rb +++ b/app/services/lfs/push_service.rb @@ -16,12 +16,17 @@ module Lfs end success - rescue => err + rescue StandardError => err + Gitlab::ErrorTracking.log_exception(err, extra_context) error(err.message) end private + def extra_context + { project_id: project.id, user_id: current_user&.id }.compact + end + # Currently we only set repository_type for design repository objects, so # push mirroring must send objects with a `nil` repository type - but if the # wiki repository uses LFS, its objects will also be sent. This will be diff --git a/app/services/lfs/unlock_file_service.rb b/app/services/lfs/unlock_file_service.rb index a13e89904a0..7a3025ee7ea 100644 --- a/app/services/lfs/unlock_file_service.rb +++ b/app/services/lfs/unlock_file_service.rb @@ -12,7 +12,7 @@ module Lfs error(ex.message, 403) rescue ActiveRecord::RecordNotFound error(_('Lock not found'), 404) - rescue => ex + rescue StandardError => ex error(ex.message, 500) end @@ -46,4 +46,4 @@ module Lfs end end -Lfs::UnlockFileService.prepend_if_ee('EE::Lfs::UnlockFileService') +Lfs::UnlockFileService.prepend_mod_with('Lfs::UnlockFileService') diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb index e79c5f69a30..919c22894c1 100644 --- a/app/services/members/approve_access_request_service.rb +++ b/app/services/members/approve_access_request_service.rb @@ -21,4 +21,4 @@ module Members end end -Members::ApproveAccessRequestService.prepend_if_ee('EE::Members::ApproveAccessRequestService') +Members::ApproveAccessRequestService.prepend_mod_with('Members::ApproveAccessRequestService') diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb index 953cf7f5bf6..7b81cc27635 100644 --- a/app/services/members/create_service.rb +++ b/app/services/members/create_service.rb @@ -98,4 +98,4 @@ module Members end end -Members::CreateService.prepend_if_ee('EE::Members::CreateService') +Members::CreateService.prepend_mod_with('Members::CreateService') diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index 8cad065e6cc..bb2d419c046 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -117,4 +117,4 @@ module Members end end -Members::DestroyService.prepend_if_ee('EE::Members::DestroyService') +Members::DestroyService.prepend_mod_with('Members::DestroyService') diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index 5c6e51201c2..257698f65ae 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -31,4 +31,4 @@ module Members end end -Members::UpdateService.prepend_if_ee('EE::Members::UpdateService') +Members::UpdateService.prepend_mod_with('Members::UpdateService') diff --git a/app/services/merge_request_metrics_service.rb b/app/services/merge_request_metrics_service.rb index 9ea71838011..d86bcca8892 100644 --- a/app/services/merge_request_metrics_service.rb +++ b/app/services/merge_request_metrics_service.rb @@ -20,4 +20,4 @@ class MergeRequestMetricsService end end -MergeRequestMetricsService.prepend_if_ee('EE::MergeRequestMetricsService') +MergeRequestMetricsService.prepend_mod_with('MergeRequestMetricsService') diff --git a/app/services/merge_requests/add_context_service.rb b/app/services/merge_requests/add_context_service.rb index 77b00f645c9..7b441ddf5e4 100644 --- a/app/services/merge_requests/add_context_service.rb +++ b/app/services/merge_requests/add_context_service.rb @@ -50,7 +50,7 @@ module MergeRequests def duplicates existing_oids = merge_request.merge_request_context_commits.map { |commit| commit.sha.to_s } existing_oids.select do |existing_oid| - commit_ids.select { |commit_id| existing_oid.start_with?(commit_id) }.count > 0 + commit_ids.count { |commit_id| existing_oid.start_with?(commit_id) } > 0 end end diff --git a/app/services/merge_requests/add_spent_time_service.rb b/app/services/merge_requests/add_spent_time_service.rb new file mode 100644 index 00000000000..ae79645a96a --- /dev/null +++ b/app/services/merge_requests/add_spent_time_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module MergeRequests + class AddSpentTimeService < UpdateService + def execute(merge_request) + old_associations = { total_time_spent: merge_request.total_time_spent } + + merge_request.spend_time(params[:spend_time]) + + merge_request_saved = merge_request.with_transaction_returning_status do + merge_request.save + end + + if merge_request_saved + create_system_notes(merge_request) + + # track usage + track_time_spend_edits(merge_request, old_associations[:total_time_spent]) + + execute_hooks(merge_request, 'update', old_associations: old_associations) + end + + merge_request + end + + private + + def track_time_spend_edits(merge_request, old_total_time_spent) + if old_total_time_spent != merge_request.total_time_spent + merge_request_activity_counter.track_time_spent_changed_action(user: current_user) + end + end + end +end diff --git a/app/services/merge_requests/after_create_service.rb b/app/services/merge_requests/after_create_service.rb index ed9747a8c99..77564521d45 100644 --- a/app/services/merge_requests/after_create_service.rb +++ b/app/services/merge_requests/after_create_service.rb @@ -35,9 +35,9 @@ module MergeRequests end def link_lfs_objects(merge_request) - LinkLfsObjectsService.new(merge_request.target_project).execute(merge_request) + LinkLfsObjectsService.new(project: merge_request.target_project).execute(merge_request) end end end -MergeRequests::AfterCreateService.prepend_if_ee('EE::MergeRequests::AfterCreateService') +MergeRequests::AfterCreateService.prepend_mod_with('MergeRequests::AfterCreateService') diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 59d8f553eff..62e599e3e27 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -54,4 +54,4 @@ module MergeRequests end end -MergeRequests::ApprovalService.prepend_if_ee('EE::MergeRequests::ApprovalService') +MergeRequests::ApprovalService.prepend_mod_with('MergeRequests::ApprovalService') diff --git a/app/services/merge_requests/assign_issues_service.rb b/app/services/merge_requests/assign_issues_service.rb index e9107b9998e..f016c16e816 100644 --- a/app/services/merge_requests/assign_issues_service.rb +++ b/app/services/merge_requests/assign_issues_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module MergeRequests - class AssignIssuesService < BaseService + class AssignIssuesService < BaseProjectService def assignable_issues @assignable_issues ||= begin if current_user == merge_request.author @@ -16,7 +16,7 @@ module MergeRequests def execute assignable_issues.each do |issue| - Issues::UpdateService.new(issue.project, current_user, assignee_ids: [current_user.id]).execute(issue) + Issues::UpdateService.new(project: issue.project, current_user: current_user, params: { assignee_ids: [current_user.id] }).execute(issue) end { diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 3a3765355d8..e94274aff9d 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -147,7 +147,7 @@ module MergeRequests if async MergeRequests::CreatePipelineWorker.perform_async(project.id, user.id, merge_request.id) else - MergeRequests::CreatePipelineService.new(project, user).execute(merge_request) + MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request) end end @@ -208,4 +208,4 @@ module MergeRequests end end -MergeRequests::BaseService.prepend_if_ee('EE::MergeRequests::BaseService') +MergeRequests::BaseService.prepend_mod_with('MergeRequests::BaseService') diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index ecc55eae5de..878e42172b7 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -300,4 +300,4 @@ module MergeRequests end end -MergeRequests::BuildService.prepend_if_ee('EE::MergeRequests::BuildService') +MergeRequests::BuildService.prepend_mod_with('MergeRequests::BuildService') diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb index b43e697d3ab..12fc828b194 100644 --- a/app/services/merge_requests/create_from_issue_service.rb +++ b/app/services/merge_requests/create_from_issue_service.rb @@ -2,16 +2,28 @@ module MergeRequests class CreateFromIssueService < MergeRequests::CreateService - def initialize(project, user, params) + # TODO: This constructor does not use the "params:" argument from the superclass, + # but instead has a custom "mr_params:" argument. This is because historically, + # prior to named arguments being introduced to the constructor, it never passed + # along the third positional argument when calling `super`. + # This should be changed, in order to be consistent (all subclasses should pass + # along all of the arguments to the superclass, otherwise it is probably not an + # "is a" relationship). However, we need to be sure that passing the params + # argument to `super` (especially target_project_id) will not cause any unexpected + # behavior in the superclass. Since the addition of the named arguments is + # intended to be a low-risk pure refactor, we will defer this fix + # to this follow-on issue: + # https://gitlab.com/gitlab-org/gitlab/-/issues/328726 + def initialize(project:, current_user:, mr_params: {}) # branch - the name of new branch # ref - the source of new branch. - @branch_name = params[:branch_name] - @issue_iid = params[:issue_iid] - @ref = params[:ref] - @target_project_id = params[:target_project_id] + @branch_name = mr_params[:branch_name] + @issue_iid = mr_params[:issue_iid] + @ref = mr_params[:ref] + @target_project_id = mr_params[:target_project_id] - super(project, user) + super(project: project, current_user: current_user) end def execute @@ -73,11 +85,11 @@ module MergeRequests end def default_branch - target_project.default_branch || 'master' + target_project.default_branch_or_main end def merge_request - MergeRequests::BuildService.new(target_project, current_user, merge_request_params).execute + MergeRequests::BuildService.new(project: target_project, current_user: current_user, params: merge_request_params).execute end def merge_request_params diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb index 46c4c102091..ebeba0ee5b8 100644 --- a/app/services/merge_requests/create_pipeline_service.rb +++ b/app/services/merge_requests/create_pipeline_service.rb @@ -63,4 +63,4 @@ module MergeRequests end end -MergeRequests::CreatePipelineService.prepend_if_ee('EE::MergeRequests::CreatePipelineService') +MergeRequests::CreatePipelineService.prepend_mod_with('MergeRequests::CreatePipelineService') diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 8186472ec65..c1292d924b2 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -53,4 +53,4 @@ module MergeRequests end end -MergeRequests::CreateService.include_if_ee('EE::MergeRequests::CreateService') +MergeRequests::CreateService.include_mod_with('MergeRequests::CreateService') diff --git a/app/services/merge_requests/get_urls_service.rb b/app/services/merge_requests/get_urls_service.rb index de3f2acdf63..7996fcb5273 100644 --- a/app/services/merge_requests/get_urls_service.rb +++ b/app/services/merge_requests/get_urls_service.rb @@ -1,13 +1,7 @@ # frozen_string_literal: true module MergeRequests - class GetUrlsService < BaseService - attr_reader :project - - def initialize(project) - @project = project - end - + class GetUrlsService < BaseProjectService def execute(changes) return [] unless project&.printing_merge_request_link_enabled diff --git a/app/services/merge_requests/handle_assignees_change_service.rb b/app/services/merge_requests/handle_assignees_change_service.rb index 77ff0791eb4..9ac386110f7 100644 --- a/app/services/merge_requests/handle_assignees_change_service.rb +++ b/app/services/merge_requests/handle_assignees_change_service.rb @@ -3,17 +3,13 @@ module MergeRequests class HandleAssigneesChangeService < MergeRequests::BaseService def async_execute(merge_request, old_assignees, options = {}) - if Feature.enabled?(:async_handle_merge_request_assignees_change, merge_request.target_project, default_enabled: :yaml) - MergeRequests::HandleAssigneesChangeWorker - .perform_async( - merge_request.id, - current_user.id, - old_assignees.map(&:id), - options - ) - else - execute(merge_request, old_assignees, options) - end + MergeRequests::HandleAssigneesChangeWorker + .perform_async( + merge_request.id, + current_user.id, + old_assignees.map(&:id), + options + ) end def execute(merge_request, old_assignees, options = {}) @@ -40,4 +36,4 @@ module MergeRequests end end -MergeRequests::HandleAssigneesChangeService.prepend_if_ee('EE::MergeRequests::HandleAssigneesChangeService') +MergeRequests::HandleAssigneesChangeService.prepend_mod_with('MergeRequests::HandleAssigneesChangeService') diff --git a/app/services/merge_requests/link_lfs_objects_service.rb b/app/services/merge_requests/link_lfs_objects_service.rb index 191da594095..4981d3efcae 100644 --- a/app/services/merge_requests/link_lfs_objects_service.rb +++ b/app/services/merge_requests/link_lfs_objects_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module MergeRequests - class LinkLfsObjectsService < ::BaseService + class LinkLfsObjectsService < ::BaseProjectService def execute(merge_request, oldrev: merge_request.diff_base_sha, newrev: merge_request.diff_head_sha) return if merge_request.source_project == project return if no_changes?(oldrev, newrev) diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index fe09c92aab9..3b9d3bccacf 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -61,7 +61,7 @@ module MergeRequests def squash_sha! params[:merge_request] = merge_request - squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute + squash_result = ::MergeRequests::SquashService.new(project: project, current_user: current_user, params: params).execute case squash_result[:status] when :success @@ -73,4 +73,4 @@ module MergeRequests end end -MergeRequests::MergeBaseService.prepend_if_ee('EE::MergeRequests::MergeBaseService') +MergeRequests::MergeBaseService.prepend_mod_with('MergeRequests::MergeBaseService') diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 27f474b0fe7..5e7eee4f1c3 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -8,16 +8,22 @@ module MergeRequests # Executed when you do merge via GitLab UI # class MergeService < MergeRequests::MergeBaseService + include Gitlab::Utils::StrongMemoize + GENERIC_ERROR_MESSAGE = 'An error occurred while merging' + LEASE_TIMEOUT = 15.minutes.to_i delegate :merge_jid, :state, to: :@merge_request def execute(merge_request, options = {}) if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService) - FfMergeService.new(project, current_user, params).execute(merge_request) + FfMergeService.new(project: project, current_user: current_user, params: params).execute(merge_request) return end + return if merge_request.merged? + return unless exclusive_lease(merge_request.id).try_obtain + @merge_request = merge_request @options = options @@ -34,6 +40,8 @@ module MergeRequests log_info("Merge process finished on JID #{merge_jid} with state #{state}") rescue MergeError => e handle_merge_error(log_message: e.message, save_message_on_model: true) + ensure + exclusive_lease(merge_request.id).cancel end private @@ -96,14 +104,14 @@ module MergeRequests rescue Gitlab::Git::PreReceiveError => e raise MergeError, "Something went wrong during merge pre-receive hook. #{e.message}".strip - rescue => e + rescue StandardError => e handle_merge_error(log_message: e.message) raise_error(GENERIC_ERROR_MESSAGE) end def after_merge log_info("Post merge started on JID #{merge_jid} with state #{state}") - MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) + MergeRequests::PostMergeService.new(project: project, current_user: current_user).execute(merge_request) log_info("Post merge finished on JID #{merge_jid} with state #{state}") if delete_source_branch? @@ -146,5 +154,13 @@ module MergeRequests # loaded from the database they're strings params.with_indifferent_access[:sha] == merge_request.diff_head_sha end + + def exclusive_lease(merge_request_id) + strong_memoize(:"exclusive_lease_#{merge_request_id}") do + lease_key = ['merge_requests_merge_service', merge_request_id].join(':') + + Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT) + end + end end end diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 9fecab85cc1..3e294aeaa07 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -157,7 +157,7 @@ module MergeRequests def merge_to_ref params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) } - result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request) + result = MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author, params: params).execute(merge_request) result[:status] == :success end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index 4d7d632ee14..ea3071b3c2d 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -12,20 +12,28 @@ module MergeRequests MAX_RETARGET_MERGE_REQUESTS = 4 def execute(merge_request) + return if merge_request.merged? + + # Mark the merge request as merged, everything that happens afterwards is + # executed once merge_request.mark_as_merged - close_issues(merge_request) - todo_service.merge_merge_request(merge_request, current_user) + create_event(merge_request) - create_note(merge_request) + todo_service.merge_merge_request(merge_request, current_user) + merge_request_activity_counter.track_merge_mr_action(user: current_user) + + create_note(merge_request) + close_issues(merge_request) notification_service.merge_mr(merge_request, current_user) - execute_hooks(merge_request, 'merge') invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers) merge_request.update_project_counter_caches delete_non_latest_diffs(merge_request) cancel_review_app_jobs!(merge_request) cleanup_environments(merge_request) cleanup_refs(merge_request) + + execute_hooks(merge_request, 'merge') end private @@ -36,7 +44,7 @@ module MergeRequests closed_issues = merge_request.visible_closing_issues_for(current_user) closed_issues.each do |issue| - Issues::CloseService.new(project, current_user).execute(issue, commit: merge_request) + Issues::CloseService.new(project: project, current_user: current_user).execute(issue, commit: merge_request) end end @@ -59,4 +67,4 @@ module MergeRequests end end -MergeRequests::PostMergeService.prepend_if_ee('EE::MergeRequests::PostMergeService') +MergeRequests::PostMergeService.prepend_mod_with('MergeRequests::PostMergeService') diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index 05ec87c7d60..cc1e08e1606 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -1,16 +1,16 @@ # frozen_string_literal: true module MergeRequests - class PushOptionsHandlerService + class PushOptionsHandlerService < ::BaseProjectService LIMIT = 10 - attr_reader :current_user, :errors, :changes, - :project, :push_options, :target_project + attr_reader :errors, :changes, + :push_options, :target_project + + def initialize(project:, current_user:, params: {}, changes:, push_options:) + super(project: project, current_user: current_user, params: params) - def initialize(project, current_user, changes, push_options) - @project = project @target_project = @project.default_merge_request_target - @current_user = current_user @changes = Gitlab::ChangesList.new(changes) @push_options = push_options @errors = [] @@ -95,16 +95,16 @@ module MergeRequests # Use BuildService to assign the standard attributes of a merge request merge_request = ::MergeRequests::BuildService.new( - project, - current_user, - create_params(branch) + project: project, + current_user: current_user, + params: create_params(branch) ).execute unless merge_request.errors.present? merge_request = ::MergeRequests::CreateService.new( - project, - current_user, - merge_request.attributes.merge(assignees: merge_request.assignees, + project: project, + current_user: current_user, + params: merge_request.attributes.merge(assignees: merge_request.assignees, label_ids: merge_request.label_ids) ).execute end @@ -114,9 +114,9 @@ module MergeRequests def update!(merge_request) merge_request = ::MergeRequests::UpdateService.new( - target_project, - current_user, - update_params(merge_request) + project: target_project, + current_user: current_user, + params: update_params(merge_request) ).execute(merge_request) collect_errors_from_merge_request(merge_request) unless merge_request.valid? diff --git a/app/services/merge_requests/rebase_service.rb b/app/services/merge_requests/rebase_service.rb index 87808a21a15..ae8398e2335 100644 --- a/app/services/merge_requests/rebase_service.rb +++ b/app/services/merge_requests/rebase_service.rb @@ -27,7 +27,7 @@ module MergeRequests repository.rebase(current_user, merge_request, skip_ci: @skip_ci) true - rescue => e + rescue StandardError => e log_error(exception: e, message: REBASE_ERROR, save_message_on_model: true) false diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index e04c5168cef..d5e2595a9c6 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -62,7 +62,7 @@ module MergeRequests # the latest diff state as the last _valid_ one. merge_requests_for_source_branch.reject(&:source_branch_exists?).each do |mr| MergeRequests::CloseService - .new(mr.target_project, @current_user) + .new(project: mr.target_project, current_user: @current_user) .execute(mr) end end @@ -96,7 +96,7 @@ module MergeRequests merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha) MergeRequests::PostMergeService - .new(merge_request.target_project, @current_user) + .new(project: merge_request.target_project, current_user: @current_user) .execute(merge_request) end end @@ -109,7 +109,7 @@ module MergeRequests merge_requests_for_forks.find_each do |mr| LinkLfsObjectsService - .new(mr.target_project) + .new(project: mr.target_project) .execute(mr, oldrev: @push.oldrev, newrev: @push.newrev) end end @@ -162,12 +162,7 @@ module MergeRequests end def refresh_pipelines_on_merge_requests(merge_request) - if Feature.enabled?(:code_review_async_pipeline_creation, project, default_enabled: :yaml) - create_pipeline_for(merge_request, current_user, async: true) - else - create_pipeline_for(merge_request, current_user, async: false) - UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id) - end + create_pipeline_for(merge_request, current_user, async: true) end def abort_auto_merges(merge_request) @@ -218,7 +213,7 @@ module MergeRequests # If the a commit no longer exists in this repo, gitlab_git throws # a Rugged::OdbError. This is fixed in https://gitlab.com/gitlab-org/gitlab_git/merge_requests/52 @commits = @project.repository.commits_between(common_ref, @push.newrev) if common_ref - rescue + rescue StandardError end elsif @push.branch_removed? # No commits for a deleted branch. @@ -309,4 +304,4 @@ module MergeRequests end end -MergeRequests::RefreshService.prepend_if_ee('EE::MergeRequests::RefreshService') +MergeRequests::RefreshService.prepend_mod_with('MergeRequests::RefreshService') diff --git a/app/services/merge_requests/remove_approval_service.rb b/app/services/merge_requests/remove_approval_service.rb index f2bf5de61c1..872e7e0c89c 100644 --- a/app/services/merge_requests/remove_approval_service.rb +++ b/app/services/merge_requests/remove_approval_service.rb @@ -41,4 +41,4 @@ module MergeRequests end end -MergeRequests::RemoveApprovalService.prepend_if_ee('EE::MergeRequests::RemoveApprovalService') +MergeRequests::RemoveApprovalService.prepend_mod_with('MergeRequests::RemoveApprovalService') diff --git a/app/services/merge_requests/resolve_todos_service.rb b/app/services/merge_requests/resolve_todos_service.rb index 0010b596eee..2d322a7de30 100644 --- a/app/services/merge_requests/resolve_todos_service.rb +++ b/app/services/merge_requests/resolve_todos_service.rb @@ -10,11 +10,7 @@ module MergeRequests end def async_execute - if Feature.enabled?(:resolve_merge_request_todos_async, merge_request.target_project, default_enabled: :yaml) - MergeRequests::ResolveTodosWorker.perform_async(merge_request.id, user.id) - else - execute - end + MergeRequests::ResolveTodosWorker.perform_async(merge_request.id, user.id) end def execute diff --git a/app/services/merge_requests/retarget_chain_service.rb b/app/services/merge_requests/retarget_chain_service.rb index e8101e447d2..dab6e198979 100644 --- a/app/services/merge_requests/retarget_chain_service.rb +++ b/app/services/merge_requests/retarget_chain_service.rb @@ -24,9 +24,11 @@ module MergeRequests next unless can?(current_user, :update_merge_request, other_merge_request.source_project) ::MergeRequests::UpdateService - .new(other_merge_request.source_project, current_user, - target_branch: merge_request.target_branch, - target_branch_was_deleted: true) + .new(project: other_merge_request.source_project, current_user: current_user, + params: { + target_branch: merge_request.target_branch, + target_branch_was_deleted: true + }) .execute(other_merge_request) end end diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb index faa2e921581..31c49b3ae70 100644 --- a/app/services/merge_requests/squash_service.rb +++ b/app/services/merge_requests/squash_service.rb @@ -29,7 +29,7 @@ module MergeRequests squash_sha = repository.squash(current_user, merge_request, message || merge_request.default_squash_commit_message) success(squash_sha: squash_sha) - rescue => e + rescue StandardError => e log_error(exception: e, message: 'Failed to squash merge request') false @@ -37,7 +37,7 @@ module MergeRequests def squash_in_progress? merge_request.squash_in_progress? - rescue => e + rescue StandardError => e log_error(exception: e, message: 'Failed to check squash in progress') raise SquashInProgressError, e.message diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb index b339a644e8c..f99db35fd49 100644 --- a/app/services/merge_requests/update_assignees_service.rb +++ b/app/services/merge_requests/update_assignees_service.rb @@ -20,7 +20,7 @@ module MergeRequests # Defer the more expensive operations (handle_assignee_changes) to the background MergeRequests::HandleAssigneesChangeService - .new(project, current_user) + .new(project: project, current_user: current_user) .async_execute(merge_request, old_assignees, execute_hooks: true) merge_request @@ -45,7 +45,7 @@ module MergeRequests end def assignee_ids - params.fetch(:assignee_ids).first(1) + params.fetch(:assignee_ids).reject { _1 == 0 }.first(1) end def params @@ -61,4 +61,4 @@ module MergeRequests end end -MergeRequests::UpdateAssigneesService.prepend_if_ee('EE::MergeRequests::UpdateAssigneesService') +MergeRequests::UpdateAssigneesService.prepend_mod_with('MergeRequests::UpdateAssigneesService') diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 8995c5f2411..b613d88aee4 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -4,7 +4,7 @@ module MergeRequests class UpdateService < MergeRequests::BaseService extend ::Gitlab::Utils::Override - def initialize(project, user = nil, params = {}) + def initialize(project:, current_user: nil, params: {}) super @target_branch_was_deleted = @params.delete(:target_branch_was_deleted) @@ -222,7 +222,7 @@ module MergeRequests def handle_assignees_change(merge_request, old_assignees) MergeRequests::HandleAssigneesChangeService - .new(project, current_user) + .new(project: project, current_user: current_user) .async_execute(merge_request, old_assignees) end @@ -295,6 +295,8 @@ module MergeRequests case attribute when :assignee_ids assignees_service.execute(merge_request) + when :spend_time + add_time_spent_service.execute(merge_request) else nil end @@ -302,9 +304,13 @@ module MergeRequests def assignees_service @assignees_service ||= ::MergeRequests::UpdateAssigneesService - .new(project, current_user, params) + .new(project: project, current_user: current_user, params: params) + end + + def add_time_spent_service + @add_time_spent_service ||= ::MergeRequests::AddSpentTimeService.new(project: project, current_user: current_user, params: params) end end end -MergeRequests::UpdateService.prepend_if_ee('EE::MergeRequests::UpdateService') +MergeRequests::UpdateService.prepend_mod_with('MergeRequests::UpdateService') diff --git a/app/services/metrics/dashboard/grafana_metric_embed_service.rb b/app/services/metrics/dashboard/grafana_metric_embed_service.rb index 6069d236e82..e94c8d92c3a 100644 --- a/app/services/metrics/dashboard/grafana_metric_embed_service.rb +++ b/app/services/metrics/dashboard/grafana_metric_embed_service.rb @@ -80,7 +80,7 @@ module Metrics def fetch_dashboard uid = GrafanaUidParser.new(grafana_url, project).parse - raise DashboardProcessingError.new(_('Dashboard uid not found')) unless uid + raise DashboardProcessingError, _('Dashboard uid not found') unless uid response = client.get_dashboard(uid: uid) @@ -89,7 +89,7 @@ module Metrics def fetch_datasource(dashboard) name = DatasourceNameParser.new(grafana_url, dashboard).parse - raise DashboardProcessingError.new(_('Datasource name not found')) unless name + raise DashboardProcessingError, _('Datasource name not found') unless name response = client.get_datasource(name: name) @@ -115,7 +115,7 @@ module Metrics def parse_json(json) Gitlab::Json.parse(json, symbolize_names: true) rescue JSON::ParserError - raise DashboardProcessingError.new(_('Grafana response contains invalid json')) + raise DashboardProcessingError, _('Grafana response contains invalid json') end end diff --git a/app/services/metrics/dashboard/transient_embed_service.rb b/app/services/metrics/dashboard/transient_embed_service.rb index 0a9c4bc7b86..29ea9909a36 100644 --- a/app/services/metrics/dashboard/transient_embed_service.rb +++ b/app/services/metrics/dashboard/transient_embed_service.rb @@ -39,7 +39,7 @@ module Metrics end def invalid_embed_json!(message) - raise DashboardProcessingError.new(_("Parsing error for param :embed_json. %{message}") % { message: message }) + raise DashboardProcessingError, _("Parsing error for param :embed_json. %{message}") % { message: message } end end end diff --git a/app/services/metrics/dashboard/update_dashboard_service.rb b/app/services/metrics/dashboard/update_dashboard_service.rb index d990e96ecb5..0574cb15e96 100644 --- a/app/services/metrics/dashboard/update_dashboard_service.rb +++ b/app/services/metrics/dashboard/update_dashboard_service.rb @@ -58,7 +58,7 @@ module Metrics target_branch: project.default_branch, title: params[:commit_message] } - merge_request = ::MergeRequests::CreateService.new(project, current_user, merge_request_params).execute + merge_request = ::MergeRequests::CreateService.new(project: project, current_user: current_user, params: merge_request_params).execute if merge_request.persisted? success(result.merge(merge_request: Gitlab::UrlBuilder.build(merge_request))) diff --git a/app/services/milestones/destroy_service.rb b/app/services/milestones/destroy_service.rb index 87c7a282081..2563f2f5390 100644 --- a/app/services/milestones/destroy_service.rb +++ b/app/services/milestones/destroy_service.rb @@ -7,11 +7,11 @@ module Milestones update_params = { milestone: nil, skip_milestone_email: true } milestone.issues.each do |issue| - Issues::UpdateService.new(parent, current_user, update_params).execute(issue) + Issues::UpdateService.new(project: parent, current_user: current_user, params: update_params).execute(issue) end milestone.merge_requests.each do |merge_request| - MergeRequests::UpdateService.new(parent, current_user, update_params).execute(merge_request) + MergeRequests::UpdateService.new(project: parent, current_user: current_user, params: update_params).execute(merge_request) end log_destroy_event_for(milestone) diff --git a/app/services/milestones/promote_service.rb b/app/services/milestones/promote_service.rb index 2431318cbb2..4417f17f33e 100644 --- a/app/services/milestones/promote_service.rb +++ b/app/services/milestones/promote_service.rb @@ -90,4 +90,4 @@ module Milestones end end -Milestones::PromoteService.prepend_if_ee('EE::Milestones::PromoteService') +Milestones::PromoteService.prepend_mod_with('Milestones::PromoteService') diff --git a/app/services/milestones/update_service.rb b/app/services/milestones/update_service.rb index 782c6bc3e35..b9a12a35d31 100644 --- a/app/services/milestones/update_service.rb +++ b/app/services/milestones/update_service.rb @@ -21,4 +21,4 @@ module Milestones end end -Milestones::UpdateService.prepend_if_ee('EE::Milestones::UpdateService') +Milestones::UpdateService.prepend_mod_with('Milestones::UpdateService') diff --git a/app/services/namespace_settings/update_service.rb b/app/services/namespace_settings/update_service.rb index c6c04b63690..de54eb87cc0 100644 --- a/app/services/namespace_settings/update_service.rb +++ b/app/services/namespace_settings/update_service.rb @@ -35,4 +35,4 @@ module NamespaceSettings end end -NamespaceSettings::UpdateService.prepend_if_ee('EE::NamespaceSettings::UpdateService') +NamespaceSettings::UpdateService.prepend_mod_with('NamespaceSettings::UpdateService') diff --git a/app/services/namespaces/in_product_marketing_emails_service.rb b/app/services/namespaces/in_product_marketing_emails_service.rb index eb81253bc08..61d5ed3bdf4 100644 --- a/app/services/namespaces/in_product_marketing_emails_service.rb +++ b/app/services/namespaces/in_product_marketing_emails_service.rb @@ -66,7 +66,6 @@ module Namespaces Experiment.add_group(:in_product_marketing_emails, variant: variant, group: group) end - # rubocop: disable CodeReuse/ActiveRecord def groups_for_track onboarding_progress_scope = OnboardingProgress .completed_actions_with_latest_in_range(completed_actions, range) @@ -75,9 +74,18 @@ module Namespaces # Filtering out sub-groups is a temporary fix to prevent calling # `.root_ancestor` on groups that are not root groups. # See https://gitlab.com/groups/gitlab-org/-/epics/5594 for more information. - Group.where(parent_id: nil).joins(:onboarding_progress).merge(onboarding_progress_scope) + Group + .top_most + .with_onboarding_progress + .merge(onboarding_progress_scope) + .merge(subscription_scope) + end + + def subscription_scope + {} end + # rubocop: disable CodeReuse/ActiveRecord def users_for_group(group) group.users .where(email_opted_in: true) @@ -136,3 +144,5 @@ module Namespaces end end end + +Namespaces::InProductMarketingEmailsService.prepend_mod diff --git a/app/services/namespaces/package_settings/update_service.rb b/app/services/namespaces/package_settings/update_service.rb index 0964963647a..cbadbe5c907 100644 --- a/app/services/namespaces/package_settings/update_service.rb +++ b/app/services/namespaces/package_settings/update_service.rb @@ -5,7 +5,10 @@ module Namespaces class UpdateService < BaseContainerService include Gitlab::Utils::StrongMemoize - ALLOWED_ATTRIBUTES = %i[maven_duplicates_allowed maven_duplicate_exception_regex].freeze + ALLOWED_ATTRIBUTES = %i[maven_duplicates_allowed + maven_duplicate_exception_regex + generic_duplicates_allowed + generic_duplicate_exception_regex].freeze def execute return ServiceResponse.error(message: 'Access Denied', http_status: 403) unless allowed? diff --git a/app/services/namespaces/statistics_refresher_service.rb b/app/services/namespaces/statistics_refresher_service.rb index c07b302839b..805060cdee9 100644 --- a/app/services/namespaces/statistics_refresher_service.rb +++ b/app/services/namespaces/statistics_refresher_service.rb @@ -9,7 +9,7 @@ module Namespaces root_storage_statistics.recalculate! rescue ActiveRecord::ActiveRecordError => e - raise RefresherError.new(e.message) + raise RefresherError, e.message end private diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index e63099a0820..542fafb901b 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -133,4 +133,4 @@ module Notes end end -Notes::CreateService.prepend_if_ee('EE::Notes::CreateService') +Notes::CreateService.prepend_mod_with('Notes::CreateService') diff --git a/app/services/notes/destroy_service.rb b/app/services/notes/destroy_service.rb index 85f54a39add..c25b1ab0379 100644 --- a/app/services/notes/destroy_service.rb +++ b/app/services/notes/destroy_service.rb @@ -24,4 +24,4 @@ module Notes end end -Notes::DestroyService.prepend_if_ee('EE::Notes::DestroyService') +Notes::DestroyService.prepend_mod_with('Notes::DestroyService') diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb index 4f3b2000e9a..b7ccdbc1cff 100644 --- a/app/services/notes/post_process_service.rb +++ b/app/services/notes/post_process_service.rb @@ -44,4 +44,4 @@ module Notes end end -Notes::PostProcessService.prepend_if_ee('EE::Notes::PostProcessService') +Notes::PostProcessService.prepend_mod_with('Notes::PostProcessService') diff --git a/app/services/notes/quick_actions_service.rb b/app/services/notes/quick_actions_service.rb index 36d9f1d7867..900ace24ab4 100644 --- a/app/services/notes/quick_actions_service.rb +++ b/app/services/notes/quick_actions_service.rb @@ -24,12 +24,12 @@ module Notes UPDATE_SERVICES end - def self.noteable_update_service(note) + def self.noteable_update_service_class(note) update_services[note.noteable_type] end def self.supported?(note) - !!noteable_update_service(note) + !!noteable_update_service_class(note) end def supported?(note) @@ -55,9 +55,23 @@ module Notes update_params[:spend_time][:note_id] = note.id end - self.class.noteable_update_service(note).new(note.resource_parent, current_user, update_params).execute(note.noteable) + noteable_update_service_class = self.class.noteable_update_service_class(note) + + # TODO: This conditional is necessary because we have not fully converted all possible + # noteable_update_service_class classes to use named arguments. See more details + # on the partial conversion at https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59182 + # Follow-on issue to address this is here: + # https://gitlab.com/gitlab-org/gitlab/-/issues/328734 + service = + if noteable_update_service_class.respond_to?(:constructor_container_arg) + noteable_update_service_class.new(**noteable_update_service_class.constructor_container_arg(note.resource_parent), current_user: current_user, params: update_params) + else + noteable_update_service_class.new(note.resource_parent, current_user, update_params) + end + + service.execute(note.noteable) end end end -Notes::QuickActionsService.prepend_if_ee('EE::Notes::QuickActionsService') +Notes::QuickActionsService.prepend_mod_with('Notes::QuickActionsService') diff --git a/app/services/notes/resolve_service.rb b/app/services/notes/resolve_service.rb index cf24795f050..75ce9e27c5b 100644 --- a/app/services/notes/resolve_service.rb +++ b/app/services/notes/resolve_service.rb @@ -5,7 +5,7 @@ module Notes def execute(note) note.resolve!(current_user) - ::MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable) + ::MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(note.noteable) end end end diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 76f9b6369b3..1cbb5916107 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -107,4 +107,4 @@ module Notes end end -Notes::UpdateService.prepend_if_ee('EE::Notes::UpdateService') +Notes::UpdateService.prepend_mod_with('Notes::UpdateService') diff --git a/app/services/notification_recipients/builder/base.rb b/app/services/notification_recipients/builder/base.rb index b41b969ad7c..e8f783136cc 100644 --- a/app/services/notification_recipients/builder/base.rb +++ b/app/services/notification_recipients/builder/base.rb @@ -100,7 +100,7 @@ module NotificationRecipients # Get project/group users with CUSTOM notification level # rubocop: disable CodeReuse/ActiveRecord def add_custom_notifications - return new_add_custom_notifications if Feature.enabled?(:notification_setting_recipient_refactor, project) + return new_add_custom_notifications if Feature.enabled?(:notification_setting_recipient_refactor, project, default_enabled: :yaml) user_ids = [] @@ -172,6 +172,8 @@ module NotificationRecipients # Get project users with WATCH notification level # rubocop: disable CodeReuse/ActiveRecord def project_watchers + return new_project_watchers if Feature.enabled?(:notification_setting_recipient_refactor, project, default_enabled: :yaml) + project_members_ids = user_ids_notifiable_on(project) user_ids_with_project_global = user_ids_notifiable_on(project, :global) @@ -184,16 +186,38 @@ module NotificationRecipients user_scope.where(id: user_ids_with_project_setting.concat(user_ids_with_group_setting).uniq) end + + def new_project_watchers + notification_by_sources = related_notification_settings_sources(:watch) + + return if notification_by_sources.blank? + + user_ids = NotificationSetting.from_union(notification_by_sources).select(:user_id) + + user_scope.where(id: user_ids) + end # rubocop: enable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord def group_watchers + return new_group_watchers if Feature.enabled?(:notification_setting_recipient_refactor, project, default_enabled: :yaml) + user_ids_with_group_global = user_ids_notifiable_on(group, :global) user_ids = user_ids_with_global_level_watch(user_ids_with_group_global) user_ids_with_group_setting = select_group_members_ids(group, [], user_ids_with_group_global, user_ids) user_scope.where(id: user_ids_with_group_setting) end + + def new_group_watchers + return [] unless group + + user_ids = group + .notification_settings + .where(source_or_global_setting_by_level_query(:watch)).select(:user_id) + + user_scope.where(id: user_ids) + end # rubocop: enable CodeReuse/ActiveRecord def add_subscribed_users diff --git a/app/services/notification_recipients/builder/default.rb b/app/services/notification_recipients/builder/default.rb index 19527ba84e6..58b0cd510c9 100644 --- a/app/services/notification_recipients/builder/default.rb +++ b/app/services/notification_recipients/builder/default.rb @@ -74,4 +74,4 @@ module NotificationRecipients end end -NotificationRecipients::Builder::Default.prepend_if_ee('EE::NotificationRecipients::Builder::Default') +NotificationRecipients::Builder::Default.prepend_mod_with('NotificationRecipients::Builder::Default') diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 6f1f3309ad9..9dfcfe748da 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -384,6 +384,7 @@ class NotificationService def send_service_desk_notification(note) return unless note.noteable_type == 'Issue' + return if note.confidential issue = note.noteable recipients = issue.email_participants_emails @@ -875,4 +876,4 @@ class NotificationService end end -NotificationService.prepend_if_ee('EE::NotificationService') +NotificationService.prepend_mod_with('NotificationService') diff --git a/app/services/packages/debian/extract_changes_metadata_service.rb b/app/services/packages/debian/extract_changes_metadata_service.rb index eb5baa7e53f..43a4db5bdfc 100644 --- a/app/services/packages/debian/extract_changes_metadata_service.rb +++ b/app/services/packages/debian/extract_changes_metadata_service.rb @@ -20,7 +20,7 @@ module Packages files: files } rescue ActiveModel::ValidationError => e - raise ExtractionError.new(e.message) + raise ExtractionError, e.message end private @@ -41,10 +41,10 @@ module Packages def files strong_memoize(:files) do - raise ExtractionError.new("is not a changes file") unless file_type == :changes - raise ExtractionError.new("Files field is missing") if fields['Files'].blank? - raise ExtractionError.new("Checksums-Sha1 field is missing") if fields['Checksums-Sha1'].blank? - raise ExtractionError.new("Checksums-Sha256 field is missing") if fields['Checksums-Sha256'].blank? + raise ExtractionError, "is not a changes file" unless file_type == :changes + raise ExtractionError, "Files field is missing" if fields['Files'].blank? + raise ExtractionError, "Checksums-Sha1 field is missing" if fields['Checksums-Sha1'].blank? + raise ExtractionError, "Checksums-Sha256 field is missing" if fields['Checksums-Sha256'].blank? init_entries_from_files entries_from_checksums_sha1 @@ -73,8 +73,8 @@ module Packages each_lines_for('Checksums-Sha1') do |line| sha1sum, size, filename = line.split entry = @entries[filename] - raise ExtractionError.new("#{filename} is listed in Checksums-Sha1 but not in Files") unless entry - raise ExtractionError.new("Size for #{filename} in Files and Checksums-Sha1 differ") unless entry.size == size.to_i + raise ExtractionError, "#{filename} is listed in Checksums-Sha1 but not in Files" unless entry + raise ExtractionError, "Size for #{filename} in Files and Checksums-Sha1 differ" unless entry.size == size.to_i entry.sha1sum = sha1sum end @@ -84,8 +84,8 @@ module Packages each_lines_for('Checksums-Sha256') do |line| sha256sum, size, filename = line.split entry = @entries[filename] - raise ExtractionError.new("#{filename} is listed in Checksums-Sha256 but not in Files") unless entry - raise ExtractionError.new("Size for #{filename} in Files and Checksums-Sha256 differ") unless entry.size == size.to_i + raise ExtractionError, "#{filename} is listed in Checksums-Sha256 but not in Files" unless entry + raise ExtractionError, "Size for #{filename} in Files and Checksums-Sha256 differ" unless entry.size == size.to_i entry.sha256sum = sha256sum end @@ -104,7 +104,7 @@ module Packages entry.package_file = ::Packages::PackageFileFinder.new(@package_file.package, filename).execute! entry.validate! rescue ActiveRecord::RecordNotFound - raise ExtractionError.new("#{filename} is listed in Files but was not uploaded") + raise ExtractionError, "#{filename} is listed in Files but was not uploaded" end end end diff --git a/app/services/packages/debian/extract_metadata_service.rb b/app/services/packages/debian/extract_metadata_service.rb index 015f472c7c9..f94587919b9 100644 --- a/app/services/packages/debian/extract_metadata_service.rb +++ b/app/services/packages/debian/extract_metadata_service.rb @@ -12,7 +12,7 @@ module Packages end def execute - raise ExtractionError.new('invalid package file') unless valid_package_file? + raise ExtractionError, 'invalid package file' unless valid_package_file? extract_metadata end diff --git a/app/services/packages/debian/generate_distribution_key_service.rb b/app/services/packages/debian/generate_distribution_key_service.rb new file mode 100644 index 00000000000..28c97c7681e --- /dev/null +++ b/app/services/packages/debian/generate_distribution_key_service.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +module Packages + module Debian + class GenerateDistributionKeyService + include Gitlab::Utils::StrongMemoize + + def initialize(current_user:, params: {}) + @current_user = current_user + @params = params + end + + def execute + raise ArgumentError, 'Please provide a user' unless current_user.is_a?(User) + + generate_key + end + + private + + attr_reader :current_user, :params + + def passphrase + strong_memoize(:passphrase) do + params[:passphrase] || ::User.random_password + end + end + + def pinentry_script_content + escaped_passphrase = Shellwords.escape(passphrase) + + <<~EOF + #!/bin/sh + + echo OK Pleased to meet you + + while read -r cmd; do + case "$cmd" in + GETPIN) echo D #{escaped_passphrase}; echo OK;; + *) echo OK;; + esac + done + EOF + end + + def using_pinentry + Gitlab::Gpg.using_tmp_keychain do + home_dir = Gitlab::Gpg.current_home_dir + + File.write("#{home_dir}/pinentry.sh", pinentry_script_content, mode: 'w', perm: 0755) + + File.write("#{home_dir}/gpg-agent.conf", "pinentry-program #{home_dir}/pinentry.sh\n", mode: 'w') + + GPGME::Ctx.new(armor: true, offline: true) do |ctx| + yield ctx + end + end + end + + def generate_key_params + # https://www.gnupg.org/documentation/manuals/gnupg/Unattended-GPG-key-generation.html + '<GnupgKeyParms format="internal">' + "\n" + + { + 'Key-Type': params[:key_type] || 'RSA', + 'Key-Length': params[:key_length] || 4096, + 'Key-Usage': params[:key_usage] || 'sign', + 'Name-Real': params[:name_real] || 'GitLab Debian repository', + 'Name-Email': params[:name_email] || Gitlab.config.gitlab.email_reply_to, + 'Name-Comment': params[:name_comment] || 'GitLab Debian repository automatic signing key', + 'Expire-Date': params[:expire_date] || 0, + 'Passphrase': passphrase + }.map { |k, v| "#{k}: #{v}\n" }.join + + '</GnupgKeyParms>' + end + + def generate_key + using_pinentry do |ctx| + # Generate key + ctx.generate_key generate_key_params + + key = ctx.keys.first # rubocop:disable Gitlab/KeysFirstAndValuesFirst + fingerprint = key.fingerprint + + # Export private key + data = GPGME::Data.new + ctx.export_keys fingerprint, data, GPGME::EXPORT_MODE_SECRET + data.seek 0 + private_key = data.read + + # Export public key + data = GPGME::Data.new + ctx.export_keys fingerprint, data + data.seek 0 + public_key = data.read + + { + private_key: private_key, + public_key: public_key, + passphrase: passphrase, + fingerprint: fingerprint + } + end + end + end + end +end diff --git a/app/services/packages/debian/generate_distribution_service.rb b/app/services/packages/debian/generate_distribution_service.rb new file mode 100644 index 00000000000..67348af1a49 --- /dev/null +++ b/app/services/packages/debian/generate_distribution_service.rb @@ -0,0 +1,207 @@ +# frozen_string_literal: true + +module Packages + module Debian + class GenerateDistributionService + include Gitlab::Utils::StrongMemoize + include ExclusiveLeaseGuard + + # used by ExclusiveLeaseGuard + DEFAULT_LEASE_TIMEOUT = 1.hour.to_i.freeze + + # From https://salsa.debian.org/ftp-team/dak/-/blob/991aaa27a7f7aa773bb9c0cf2d516e383d9cffa0/setup/core-init.d/080_metadatakeys#L9 + BINARIES_METADATA = %w( + Package + Source + Binary + Version + Essential + Installed-Size + Maintainer + Uploaders + Original-Maintainer + Build-Depends + Build-Depends-Indep + Build-Conflicts + Build-Conflicts-Indep + Architecture + Standards-Version + Format + Files + Dm-Upload-Allowed + Vcs-Browse + Vcs-Hg + Vcs-Darcs + Vcs-Svn + Vcs-Git + Vcs-Browser + Vcs-Arch + Vcs-Bzr + Vcs-Mtn + Vcs-Cvs + Checksums-Sha256 + Checksums-Sha1 + Replaces + Provides + Depends + Pre-Depends + Recommends + Suggests + Enhances + Conflicts + Breaks + Description + Origin + Bugs + Multi-Arch + Homepage + Tag + Package-Type + Installer-Menu-Item + ).freeze + + def initialize(distribution) + @distribution = distribution + @last_generated_at = nil + @md5sum = [] + @sha256 = [] + end + + def execute + try_obtain_lease do + @distribution.transaction do + @last_generated_at = @distribution.component_files.maximum(:created_at) + generate_component_files + generate_release + destroy_old_component_files + end + end + end + + private + + def generate_component_files + @distribution.components.ordered_by_name.each do |component| + @distribution.architectures.ordered_by_name.each do |architecture| + generate_component_file(component, :packages, architecture, :deb) + end + end + end + + def generate_component_file(component, component_file_type, architecture, package_file_type) + paragraphs = @distribution.package_files + .preload_debian_file_metadata + .with_debian_component_name(component.name) + .with_debian_architecture_name(architecture.name) + .with_debian_file_type(package_file_type) + .find_each + .map(&method(:package_stanza_from_fields)) + create_component_file(component, component_file_type, architecture, package_file_type, paragraphs.join("\n")) + end + + def package_stanza_from_fields(package_file) + [ + BINARIES_METADATA.map do |metadata_key| + rfc822_field(metadata_key, package_file.debian_fields[metadata_key]) + end, + rfc822_field('Section', package_file.debian_fields['Section'] || 'misc'), + rfc822_field('Priority', package_file.debian_fields['Priority'] || 'extra'), + rfc822_field('Filename', package_filename(package_file)), + rfc822_field('Size', package_file.size), + rfc822_field('MD5sum', package_file.file_md5), + rfc822_field('SHA256', package_file.file_sha256) + ].flatten.compact.join('') + end + + def package_filename(package_file) + letter = package_file.package.name.start_with?('lib') ? package_file.package.name[0..3] : package_file.package.name[0] + "#{pool_prefix(package_file)}/#{letter}/#{package_file.package.name}/#{package_file.file_name}" + end + + def pool_prefix(package_file) + case @distribution + when ::Packages::Debian::GroupDistribution + "pool/#{@distribution.codename}/#{package_file.package.project_id}" + else + "pool/#{@distribution.codename}/#{@distribution.container_id}" + end + end + + def create_component_file(component, component_file_type, architecture, package_file_type, content) + component_file = component.files.create!( + file_type: component_file_type, + architecture: architecture, + compression_type: nil, + file: CarrierWaveStringFile.new(content), + file_md5: Digest::MD5.hexdigest(content), + file_sha256: Digest::SHA256.hexdigest(content) + ) + @md5sum.append(" #{component_file.file_md5} #{component_file.size.to_s.rjust(8)} #{component_file.relative_path}") + @sha256.append(" #{component_file.file_sha256} #{component_file.size.to_s.rjust(8)} #{component_file.relative_path}") + end + + def generate_release + @distribution.file = CarrierWaveStringFile.new(release_header + release_sums) + @distribution.updated_at = release_date + @distribution.save! + end + + def release_header + strong_memoize(:release_header) do + [ + %w[origin label suite version codename].map do |attribute| + rfc822_field(attribute.capitalize, @distribution.attributes[attribute]) + end, + rfc822_field('Date', release_date.to_formatted_s(:rfc822)), + valid_until_field, + rfc822_field('NotAutomatic', !@distribution.automatic, !@distribution.automatic), + rfc822_field('ButAutomaticUpgrades', @distribution.automatic_upgrades, !@distribution.automatic && @distribution.automatic_upgrades), + rfc822_field('Architectures', @distribution.architectures.map { |architecture| architecture.name }.sort.join(' ')), + rfc822_field('Components', @distribution.components.map { |component| component.name }.sort.join(' ')), + rfc822_field('Description', @distribution.description) + ].flatten.compact.join('') + end + end + + def release_date + strong_memoize(:release_date) do + Time.now.utc + end + end + + def release_sums + ["MD5Sum:", @md5sum, "SHA256:", @sha256].flatten.compact.join("\n") + "\n" + end + + def rfc822_field(name, value, condition = true) + return unless condition + return if value.blank? + + "#{name}: #{value.to_s.gsub("\n\n", "\n.\n").gsub("\n", "\n ")}\n" + end + + def valid_until_field + return unless @distribution.valid_time_duration_seconds + + rfc822_field('Valid-Until', release_date.since(@distribution.valid_time_duration_seconds).to_formatted_s(:rfc822)) + end + + def destroy_old_component_files + # Only keep the last generation and one hour before + return if @last_generated_at.nil? + + @distribution.component_files.created_before(@last_generated_at - 1.hour).destroy_all # rubocop:disable Cop/DestroyAll + end + + # used by ExclusiveLeaseGuard + def lease_key + "packages:debian:generate_distribution_service:distribution:#{@distribution.id}" + end + + # used by ExclusiveLeaseGuard + def lease_timeout + DEFAULT_LEASE_TIMEOUT + end + end + end +end diff --git a/app/services/packages/generic/create_package_file_service.rb b/app/services/packages/generic/create_package_file_service.rb index 1451a022a39..42a191fb415 100644 --- a/app/services/packages/generic/create_package_file_service.rb +++ b/app/services/packages/generic/create_package_file_service.rb @@ -23,6 +23,10 @@ module Packages .new(project, current_user, package_params) .execute + unless Namespace::PackageSetting.duplicates_allowed?(package) + raise ::Packages::DuplicatePackageError if target_file_is_duplicate?(package) + end + package.update_column(:status, params[:status]) if params[:status] && params[:status] != package.status package.build_infos.safe_find_or_create_by!(pipeline: params[:build].pipeline) if params[:build].present? @@ -40,6 +44,10 @@ module Packages ::Packages::CreatePackageFileService.new(package, file_params).execute end + + def target_file_is_duplicate?(package) + package.package_files.with_file_name(params[:file_name]).exists? + end end end end diff --git a/app/services/packages/maven/find_or_create_package_service.rb b/app/services/packages/maven/find_or_create_package_service.rb index a6cffa3038c..c7ffd468864 100644 --- a/app/services/packages/maven/find_or_create_package_service.rb +++ b/app/services/packages/maven/find_or_create_package_service.rb @@ -6,7 +6,7 @@ module Packages def execute package = - ::Packages::Maven::PackageFinder.new(params[:path], current_user, project: project) + ::Packages::Maven::PackageFinder.new(current_user, project, path: params[:path]) .execute unless Namespace::PackageSetting.duplicates_allowed?(package) diff --git a/app/services/packages/nuget/metadata_extraction_service.rb b/app/services/packages/nuget/metadata_extraction_service.rb index 59125669f7d..dd5f1bcc936 100644 --- a/app/services/packages/nuget/metadata_extraction_service.rb +++ b/app/services/packages/nuget/metadata_extraction_service.rb @@ -26,7 +26,7 @@ module Packages end def execute - raise ExtractionError.new('invalid package file') unless valid_package_file? + raise ExtractionError, 'invalid package file' unless valid_package_file? extract_metadata(nuspec_file) end @@ -94,8 +94,8 @@ module Packages Zip::File.open(file_path) do |zip_file| entry = zip_file.glob('*.nuspec').first - raise ExtractionError.new('nuspec file not found') unless entry - raise ExtractionError.new('nuspec file too big') if entry.size > MAX_FILE_SIZE + raise ExtractionError, 'nuspec file not found' unless entry + raise ExtractionError, 'nuspec file too big' if entry.size > MAX_FILE_SIZE entry.get_input_stream.read end diff --git a/app/services/packages/nuget/search_service.rb b/app/services/packages/nuget/search_service.rb index 1eead1e62b3..fea424b3aa8 100644 --- a/app/services/packages/nuget/search_service.rb +++ b/app/services/packages/nuget/search_service.rb @@ -103,6 +103,7 @@ module Packages def nuget_packages Packages::Package.nuget + .displayable .has_version .without_nuget_temporary_name end diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb index 1bcab00bd92..8210072eab3 100644 --- a/app/services/packages/nuget/update_package_from_metadata_service.rb +++ b/app/services/packages/nuget/update_package_from_metadata_service.rb @@ -16,7 +16,7 @@ module Packages end def execute - raise InvalidMetadataError.new('package name and/or package version not found in metadata') unless valid_metadata? + raise InvalidMetadataError, 'package name and/or package version not found in metadata' unless valid_metadata? try_obtain_lease do @package_file.transaction do @@ -33,7 +33,7 @@ module Packages end end rescue ActiveRecord::RecordInvalid => e - raise InvalidMetadataError.new(e.message) + raise InvalidMetadataError, e.message end private @@ -45,7 +45,7 @@ module Packages ::Packages::UpdateTagsService .new(package, package_tags) .execute - rescue => e + rescue StandardError => e raise InvalidMetadataError, e.message end diff --git a/app/services/packages/pypi/create_package_service.rb b/app/services/packages/pypi/create_package_service.rb index cb8d9559dc9..b988c191734 100644 --- a/app/services/packages/pypi/create_package_service.rb +++ b/app/services/packages/pypi/create_package_service.rb @@ -13,7 +13,7 @@ module Packages ) unless meta.valid? - raise ActiveRecord::RecordInvalid.new(meta) + raise ActiveRecord::RecordInvalid, meta end Packages::Pypi::Metadatum.upsert(meta.attributes) diff --git a/app/services/packages/rubygems/process_gem_service.rb b/app/services/packages/rubygems/process_gem_service.rb index 59bf2a1ec28..109c87a0444 100644 --- a/app/services/packages/rubygems/process_gem_service.rb +++ b/app/services/packages/rubygems/process_gem_service.rb @@ -16,6 +16,7 @@ module Packages end def execute + raise ExtractionError, 'Gem was not processed - package_file is not set' unless package_file return success if process_gem error('Gem was not processed') @@ -26,8 +27,6 @@ module Packages attr_reader :package_file def process_gem - return false unless package_file - try_obtain_lease do package.transaction do rename_package_and_set_version @@ -106,8 +105,8 @@ module Packages Packages::PackageFile.find(package_file.id).file.use_file do |file_path| Gem::Package.new(File.open(file_path)) end - rescue - raise ExtractionError.new('Unable to read gem file') + rescue StandardError + raise ExtractionError, 'Unable to read gem file' end # used by ExclusiveLeaseGuard diff --git a/app/services/packages/terraform_module/create_package_service.rb b/app/services/packages/terraform_module/create_package_service.rb new file mode 100644 index 00000000000..fc376c70b00 --- /dev/null +++ b/app/services/packages/terraform_module/create_package_service.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Packages + module TerraformModule + class CreatePackageService < ::Packages::CreatePackageService + include Gitlab::Utils::StrongMemoize + + def execute + return error('Version is empty.', 400) if params[:module_version].blank? + return error('Package already exists.', 403) if current_package_exists_elsewhere? + return error('Package version already exists.', 403) if current_package_version_exists? + return error('File is too large.', 400) if file_size_exceeded? + + ActiveRecord::Base.transaction { create_terraform_module_package! } + end + + private + + def create_terraform_module_package! + package = create_package!(:terraform_module, name: name, version: params[:module_version]) + + ::Packages::CreatePackageFileService.new(package, file_params).execute + + package + end + + def current_package_exists_elsewhere? + ::Packages::Package + .for_projects(project.root_namespace.all_projects.id_not_in(project.id)) + .with_package_type(:terraform_module) + .with_name(name) + .exists? + end + + def current_package_version_exists? + project.packages + .with_package_type(:terraform_module) + .with_name(name) + .with_version(params[:module_version]) + .exists? + end + + def name + strong_memoize(:name) do + "#{params[:module_name]}/#{params[:module_system]}" + end + end + + def file_name + strong_memoize(:file_name) do + "#{params[:module_name]}-#{params[:module_system]}-#{params[:module_version]}.tgz" + end + end + + def file_params + { + file: params[:file], + size: params[:file].size, + file_sha256: params[:file].sha256, + file_name: file_name, + build: params[:build] + } + end + + def file_size_exceeded? + project.actual_limits.exceeded?(:generic_packages_max_file_size, params[:file].size) + end + end + end +end diff --git a/app/services/pages/migrate_from_legacy_storage_service.rb b/app/services/pages/migrate_from_legacy_storage_service.rb index b6aa08bba01..d102f93e863 100644 --- a/app/services/pages/migrate_from_legacy_storage_service.rb +++ b/app/services/pages/migrate_from_legacy_storage_service.rb @@ -59,7 +59,7 @@ module Pages end @logger.info(message: "Pages legacy storage migration: batch processed", migrated: @migrated, errored: @errored) - rescue => e + rescue StandardError => e # This method should never raise exception otherwise all threads might be killed # and this will result in queue starving (and deadlock) Gitlab::ErrorTracking.track_exception(e) @@ -81,7 +81,7 @@ module Pages @logger.error(message: "Pages legacy storage migration: project failed to be migrated: #{result[:message]}", project_id: project.id, pages_path: project.pages_path, duration: time.round(2)) @counters_lock.synchronize { @errored += 1 } end - rescue => e + rescue StandardError => e @counters_lock.synchronize { @errored += 1 } @logger.error(message: "Pages legacy storage migration: project failed to be migrated: #{result[:message]}", project_id: project&.id, pages_path: project&.pages_path) Gitlab::ErrorTracking.track_exception(e, project_id: project&.id) diff --git a/app/services/pages/zip_directory_service.rb b/app/services/pages/zip_directory_service.rb index 6cb79452e1b..895614a84a0 100644 --- a/app/services/pages/zip_directory_service.rb +++ b/app/services/pages/zip_directory_service.rb @@ -31,7 +31,7 @@ module Pages end success(archive_path: output_file, entries_count: entries_count) - rescue => e + rescue StandardError => e FileUtils.rm_f(output_file) if output_file raise e end diff --git a/app/services/pages_domains/obtain_lets_encrypt_certificate_service.rb b/app/services/pages_domains/obtain_lets_encrypt_certificate_service.rb index e14241158a6..ca5df4ce017 100644 --- a/app/services/pages_domains/obtain_lets_encrypt_certificate_service.rb +++ b/app/services/pages_domains/obtain_lets_encrypt_certificate_service.rb @@ -66,7 +66,7 @@ module PagesDomains project_id: pages_domain.project_id, pages_domain: pages_domain.domain ) - rescue => e + rescue StandardError => e # getting authorizations is an additional network request which can raise errors Gitlab::ErrorTracking.track_exception(e) end diff --git a/app/services/personal_access_tokens/create_service.rb b/app/services/personal_access_tokens/create_service.rb index 93a0135669f..7555ba26768 100644 --- a/app/services/personal_access_tokens/create_service.rb +++ b/app/services/personal_access_tokens/create_service.rb @@ -45,4 +45,4 @@ module PersonalAccessTokens end end -PersonalAccessTokens::CreateService.prepend_if_ee('EE::PersonalAccessTokens::CreateService') +PersonalAccessTokens::CreateService.prepend_mod_with('PersonalAccessTokens::CreateService') diff --git a/app/services/personal_access_tokens/revoke_service.rb b/app/services/personal_access_tokens/revoke_service.rb index 34d542acab1..0275d03bcc9 100644 --- a/app/services/personal_access_tokens/revoke_service.rb +++ b/app/services/personal_access_tokens/revoke_service.rb @@ -41,4 +41,4 @@ module PersonalAccessTokens end end -PersonalAccessTokens::RevokeService.prepend_if_ee('EE::PersonalAccessTokens::RevokeService') +PersonalAccessTokens::RevokeService.prepend_mod_with('PersonalAccessTokens::RevokeService') diff --git a/app/services/pod_logs/elasticsearch_service.rb b/app/services/pod_logs/elasticsearch_service.rb index 58d1bfbf835..28ccace62e5 100644 --- a/app/services/pod_logs/elasticsearch_service.rb +++ b/app/services/pod_logs/elasticsearch_service.rb @@ -24,7 +24,7 @@ module PodLogs end def get_raw_pods(result) - client = cluster&.application_elastic_stack&.elasticsearch_client + client = cluster&.elasticsearch_client return error(_('Unable to connect to Elasticsearch')) unless client result[:raw_pods] = ::Gitlab::Elasticsearch::Logs::Pods.new(client).pods(namespace) @@ -66,11 +66,9 @@ module PodLogs end def pod_logs(result) - client = cluster&.application_elastic_stack&.elasticsearch_client + client = cluster&.elasticsearch_client return error(_('Unable to connect to Elasticsearch')) unless client - chart_above_v2 = cluster.application_elastic_stack.chart_above_v2? - response = ::Gitlab::Elasticsearch::Logs::Lines.new(client).pod_logs( namespace, pod_name: result[:pod_name], @@ -79,7 +77,7 @@ module PodLogs start_time: result[:start_time], end_time: result[:end_time], cursor: result[:cursor], - chart_above_v2: chart_above_v2 + chart_above_v2: cluster.elastic_stack_adapter.chart_above_v2? ) result.merge!(response) diff --git a/app/services/post_receive_service.rb b/app/services/post_receive_service.rb index 3dc8fd8929a..faacabbb16c 100644 --- a/app/services/post_receive_service.rb +++ b/app/services/post_receive_service.rb @@ -56,7 +56,7 @@ class PostReceiveService end service = ::MergeRequests::PushOptionsHandlerService.new( - project, user, changes, push_options + project: project, current_user: user, changes: changes, push_options: push_options ).execute if service.errors.present? @@ -72,7 +72,7 @@ class PostReceiveService def merge_request_urls return [] unless repository&.repo_type&.project? - ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) + ::MergeRequests::GetUrlsService.new(project: project).execute(params[:changes]) end private @@ -98,4 +98,4 @@ class PostReceiveService end end -PostReceiveService.prepend_if_ee('EE::PostReceiveService') +PostReceiveService.prepend_mod_with('PostReceiveService') diff --git a/app/services/preview_markdown_service.rb b/app/services/preview_markdown_service.rb index afe2651b11a..af9c338b59e 100644 --- a/app/services/preview_markdown_service.rb +++ b/app/services/preview_markdown_service.rb @@ -68,4 +68,4 @@ class PreviewMarkdownService < BaseService end end -PreviewMarkdownService.prepend_if_ee('EE::PreviewMarkdownService') +PreviewMarkdownService.prepend_mod_with('PreviewMarkdownService') diff --git a/app/services/projects/after_rename_service.rb b/app/services/projects/after_rename_service.rb index a2cdb87e631..6d389035922 100644 --- a/app/services/projects/after_rename_service.rb +++ b/app/services/projects/after_rename_service.rb @@ -49,10 +49,8 @@ module Projects def first_ensure_no_registry_tags_are_present return unless project.has_container_registry_tags? - raise RenameFailedError.new( - "Project #{full_path_before} cannot be renamed because images are " \ + raise RenameFailedError, "Project #{full_path_before} cannot be renamed because images are " \ "present in its container registry" - ) end def expire_caches_before_rename @@ -144,9 +142,9 @@ module Projects Gitlab::AppLogger.error(error) - raise RenameFailedError.new(error) + raise RenameFailedError, error end end end -Projects::AfterRenameService.prepend_if_ee('EE::Projects::AfterRenameService') +Projects::AfterRenameService.prepend_mod_with('Projects::AfterRenameService') diff --git a/app/services/projects/autocomplete_service.rb b/app/services/projects/autocomplete_service.rb index 68086f636b7..55f16aa3e3d 100644 --- a/app/services/projects/autocomplete_service.rb +++ b/app/services/projects/autocomplete_service.rb @@ -39,4 +39,4 @@ module Projects end end -Projects::AutocompleteService.prepend_if_ee('EE::Projects::AutocompleteService') +Projects::AutocompleteService.prepend_mod_with('Projects::AutocompleteService') diff --git a/app/services/projects/cleanup_service.rb b/app/services/projects/cleanup_service.rb index 7bcaee75813..5eafa5f9b29 100644 --- a/app/services/projects/cleanup_service.rb +++ b/app/services/projects/cleanup_service.rb @@ -108,4 +108,4 @@ module Projects end end -Projects::CleanupService.prepend_if_ee('EE::Projects::CleanupService') +Projects::CleanupService.prepend_mod_with('Projects::CleanupService') diff --git a/app/services/projects/create_from_template_service.rb b/app/services/projects/create_from_template_service.rb index 3c66ff709c9..48dda09da71 100644 --- a/app/services/projects/create_from_template_service.rb +++ b/app/services/projects/create_from_template_service.rb @@ -58,4 +58,4 @@ module Projects end end -Projects::CreateFromTemplateService.prepend_if_ee('EE::Projects::CreateFromTemplateService') +Projects::CreateFromTemplateService.prepend_mod_with('Projects::CreateFromTemplateService') diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 5fb0bda912e..97ea7d87545 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -40,7 +40,7 @@ module Projects if namespace_id # Find matching namespace and check if it allowed # for current user if namespace_id passed. - unless allowed_namespace?(current_user, namespace_id) + unless current_user.can?(:create_projects, project_namespace) @project.namespace_id = nil deny_namespace return @project @@ -72,7 +72,7 @@ module Projects rescue ActiveRecord::RecordInvalid => e message = "Unable to save #{e.inspect}: #{e.record.errors.full_messages.join(", ")}" fail(error: message) - rescue => e + rescue StandardError => e @project.errors.add(:base, e.message) if @project fail(error: e.message) end @@ -83,13 +83,6 @@ module Projects @project.errors.add(:namespace, "is not valid") end - # rubocop: disable CodeReuse/ActiveRecord - def allowed_namespace?(user, namespace_id) - namespace = Namespace.find_by(id: namespace_id) - current_user.can?(:create_projects, namespace) - end - # rubocop: enable CodeReuse/ActiveRecord - def after_create_actions log_info("#{@project.owner.name} created a new project \"#{@project.full_name}\"") @@ -156,7 +149,7 @@ module Projects def create_readme commit_attrs = { - branch_name: @project.default_branch || 'master', + branch_name: @project.default_branch_or_main, commit_message: 'Initial commit', file_path: 'README.md', file_content: "# #{@project.name}\n\n#{@project.description}" @@ -174,7 +167,7 @@ module Projects @project.create_or_update_import_data(data: @import_data[:data], credentials: @import_data[:credentials]) if @import_data if @project.save - Service.create_from_active_default_integrations(@project, :project_id, with_templates: true) + Integration.create_from_active_default_integrations(@project, :project_id, with_templates: true) @project.create_labels unless @project.gitlab_project_import? @@ -271,7 +264,7 @@ module Projects end end -Projects::CreateService.prepend_if_ee('EE::Projects::CreateService') +Projects::CreateService.prepend_mod_with('Projects::CreateService') # Measurable should be at the bottom of the ancestor chain, so it will measure execution of EE::Projects::CreateService as well Projects::CreateService.prepend(Measurable) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 4ba48f74273..0682f3013d4 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -41,7 +41,7 @@ module Projects current_user.invalidate_personal_projects_count true - rescue => error + rescue StandardError => error attempt_rollback(project, error.message) false rescue Exception => error # rubocop:disable Lint/RescueException @@ -116,6 +116,7 @@ module Projects log_destroy_event trash_relation_repositories! trash_project_repositories! + destroy_web_hooks! if Feature.enabled?(:destroy_webhooks_before_the_project, project, default_enabled: :yaml) # Rails attempts to load all related records into memory before # destroying: https://github.com/rails/rails/issues/22510 @@ -131,6 +132,23 @@ module Projects log_info("Attempting to destroy #{project.full_path} (#{project.id})") end + # The project can have multiple webhooks with hundreds of thousands of web_hook_logs. + # By default, they are removed with "DELETE CASCADE" option defined via foreign_key. + # But such queries can exceed the statement_timeout limit and fail to delete the project. + # (see https://gitlab.com/gitlab-org/gitlab/-/issues/26259) + # + # To prevent that we use WebHooks::DestroyService. It deletes logs in batches and + # produces smaller and faster queries to the database. + def destroy_web_hooks! + project.hooks.find_each do |web_hook| + result = ::WebHooks::DestroyService.new(current_user).sync_destroy(web_hook) + + unless result[:status] == :success + raise_error(s_('DeleteProject|Failed to remove webhooks. Please try again or contact administrator.')) + end + end + end + def remove_registry_tags return true unless Gitlab.config.registry.enabled return false unless remove_legacy_registry_tags @@ -156,7 +174,7 @@ module Projects end def raise_error(message) - raise DestroyError.new(message) + raise DestroyError, message end def flush_caches(project) @@ -165,4 +183,4 @@ module Projects end end -Projects::DestroyService.prepend_if_ee('EE::Projects::DestroyService') +Projects::DestroyService.prepend_mod_with('Projects::DestroyService') diff --git a/app/services/projects/disable_deploy_key_service.rb b/app/services/projects/disable_deploy_key_service.rb index 9fb2e3398b2..e0f309875de 100644 --- a/app/services/projects/disable_deploy_key_service.rb +++ b/app/services/projects/disable_deploy_key_service.rb @@ -12,4 +12,4 @@ module Projects end end -Projects::DisableDeployKeyService.prepend_if_ee('EE::Projects::DisableDeployKeyService') +Projects::DisableDeployKeyService.prepend_mod_with('Projects::DisableDeployKeyService') diff --git a/app/services/projects/enable_deploy_key_service.rb b/app/services/projects/enable_deploy_key_service.rb index 0a24137bd61..581a6cc0ade 100644 --- a/app/services/projects/enable_deploy_key_service.rb +++ b/app/services/projects/enable_deploy_key_service.rb @@ -27,4 +27,4 @@ module Projects end end -Projects::EnableDeployKeyService.prepend_if_ee('EE::Projects::EnableDeployKeyService') +Projects::EnableDeployKeyService.prepend_mod_with('Projects::EnableDeployKeyService') diff --git a/app/services/projects/gitlab_projects_import_service.rb b/app/services/projects/gitlab_projects_import_service.rb index 38f0e2f7c1a..63a41d172ea 100644 --- a/app/services/projects/gitlab_projects_import_service.rb +++ b/app/services/projects/gitlab_projects_import_service.rb @@ -77,4 +77,4 @@ module Projects end end -Projects::GitlabProjectsImportService.prepend_if_ee('EE::Projects::GitlabProjectsImportService') +Projects::GitlabProjectsImportService.prepend_mod_with('Projects::GitlabProjectsImportService') diff --git a/app/services/projects/group_links/create_service.rb b/app/services/projects/group_links/create_service.rb index 3262839e246..d8fa2f36fcc 100644 --- a/app/services/projects/group_links/create_service.rb +++ b/app/services/projects/group_links/create_service.rb @@ -44,4 +44,4 @@ module Projects end end -Projects::GroupLinks::CreateService.prepend_if_ee('EE::Projects::GroupLinks::CreateService') +Projects::GroupLinks::CreateService.prepend_mod_with('Projects::GroupLinks::CreateService') diff --git a/app/services/projects/group_links/destroy_service.rb b/app/services/projects/group_links/destroy_service.rb index 229191e41f6..bfe704cd780 100644 --- a/app/services/projects/group_links/destroy_service.rb +++ b/app/services/projects/group_links/destroy_service.rb @@ -20,4 +20,4 @@ module Projects end end -Projects::GroupLinks::DestroyService.prepend_if_ee('EE::Projects::GroupLinks::DestroyService') +Projects::GroupLinks::DestroyService.prepend_mod_with('Projects::GroupLinks::DestroyService') diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 3d9d03c4a95..023f8494d99 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -64,4 +64,4 @@ module Projects end end -Projects::HashedStorage::MigrateAttachmentsService.prepend_if_ee('EE::Projects::HashedStorage::MigrateAttachmentsService') +Projects::HashedStorage::MigrateAttachmentsService.prepend_mod_with('Projects::HashedStorage::MigrateAttachmentsService') diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index adc7e38e4d5..c7989e04607 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -52,4 +52,4 @@ module Projects end end -Projects::HashedStorage::MigrateRepositoryService.prepend_if_ee('EE::Projects::HashedStorage::MigrateRepositoryService') +Projects::HashedStorage::MigrateRepositoryService.prepend_mod_with('Projects::HashedStorage::MigrateRepositoryService') diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb deleted file mode 100644 index b5589d556aa..00000000000 --- a/app/services/projects/housekeeping_service.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -# This is a compatibility class to avoid calling a non-existent -# class from sidekiq during deployment. -# -# We're deploying the rename of this class in 13.9. Nevertheless, -# we cannot remove this class entirely because there can be jobs -# referencing it. -# -# We can get rid of this class in 13.10 -# https://gitlab.com/gitlab-org/gitlab/-/issues/297580 -# -module Projects - class HousekeepingService < ::Repositories::HousekeepingService - end -end diff --git a/app/services/projects/import_export/export_service.rb b/app/services/projects/import_export/export_service.rb index c2a8db7b657..64c0f1ff4ac 100644 --- a/app/services/projects/import_export/export_service.rb +++ b/app/services/projects/import_export/export_service.rb @@ -112,7 +112,7 @@ module Projects def notify_error! notify_error - raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence) + raise Gitlab::ImportExport::Error, shared.errors.to_sentence end def notify_success diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index b4abb5b6df7..b5288aad6f0 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -29,7 +29,7 @@ module Projects Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path, importer: project.import_type) error(s_("ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}") % { project_safe_import_url: project.safe_import_url, project_full_path: project.full_path, message: e.message }) - rescue => e + rescue StandardError => e message = Projects::ImportErrorFilter.filter_message(e.message) Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path, importer: project.import_type) @@ -149,7 +149,7 @@ module Projects end end -Projects::ImportService.prepend_if_ee('EE::Projects::ImportService') +Projects::ImportService.prepend_mod_with('Projects::ImportService') # Measurable should be at the bottom of the ancestor chain, so it will measure execution of EE::Projects::ImportService as well Projects::ImportService.prepend(Measurable) diff --git a/app/services/projects/lfs_pointers/lfs_import_service.rb b/app/services/projects/lfs_pointers/lfs_import_service.rb index 2afcce7099b..3fc82f2c410 100644 --- a/app/services/projects/lfs_pointers/lfs_import_service.rb +++ b/app/services/projects/lfs_pointers/lfs_import_service.rb @@ -16,7 +16,7 @@ module Projects end success - rescue => e + rescue StandardError => e error(e.message) end end diff --git a/app/services/projects/operations/update_service.rb b/app/services/projects/operations/update_service.rb index 7dfe7fffa1b..c0734171ee5 100644 --- a/app/services/projects/operations/update_service.rb +++ b/app/services/projects/operations/update_service.rb @@ -135,4 +135,4 @@ module Projects end end -Projects::Operations::UpdateService.prepend_if_ee('::EE::Projects::Operations::UpdateService') +Projects::Operations::UpdateService.prepend_mod_with('Projects::Operations::UpdateService') diff --git a/app/services/projects/prometheus/alerts/notify_service.rb b/app/services/projects/prometheus/alerts/notify_service.rb index 93165a58470..db640a54745 100644 --- a/app/services/projects/prometheus/alerts/notify_service.rb +++ b/app/services/projects/prometheus/alerts/notify_service.rb @@ -63,7 +63,7 @@ module Projects def valid_alert_manager_token?(token, integration) valid_for_manual?(token) || valid_for_alerts_endpoint?(token, integration) || - valid_for_managed?(token) + valid_for_cluster?(token) end def valid_for_manual?(token) @@ -83,18 +83,20 @@ module Projects compare_token(token, integration.token) end - def valid_for_managed?(token) - prometheus_application = available_prometheus_application(project) - return false unless prometheus_application + def valid_for_cluster?(token) + cluster_integration = find_cluster_integration(project) + return false unless cluster_integration + + cluster_integration_token = cluster_integration.alert_manager_token if token - compare_token(token, prometheus_application.alert_manager_token) + compare_token(token, cluster_integration_token) else - prometheus_application.alert_manager_token.nil? + cluster_integration_token.nil? end end - def available_prometheus_application(project) + def find_cluster_integration(project) alert_id = gitlab_alert_id return unless alert_id @@ -105,7 +107,7 @@ module Projects return unless cluster&.enabled? return unless cluster.application_prometheus_available? - cluster.application_prometheus + cluster.application_prometheus || cluster.integration_prometheus end def find_alert(project, metric) diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 8a5e0706126..d9e49dfae61 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -47,16 +47,16 @@ module Projects @old_namespace = project.namespace if Project.where(namespace_id: @new_namespace.try(:id)).where('path = ? or name = ?', project.path, project.name).exists? - raise TransferError.new(s_("TransferProject|Project with same name or path in target namespace already exists")) + raise TransferError, s_("TransferProject|Project with same name or path in target namespace already exists") end if project.has_container_registry_tags? # We currently don't support renaming repository if it contains tags in container registry - raise TransferError.new(s_('TransferProject|Project cannot be transferred, because tags are present in its container registry')) + raise TransferError, s_('TransferProject|Project cannot be transferred, because tags are present in its container registry') end if project.has_packages?(:npm) && !new_namespace_has_same_root?(project) - raise TransferError.new(s_("TransferProject|Root namespace can't be updated if project has NPM packages")) + raise TransferError, s_("TransferProject|Root namespace can't be updated if project has NPM packages") end proceed_to_transfer @@ -170,7 +170,7 @@ module Projects # Move main repository unless move_repo_folder(@old_path, @new_path) - raise TransferError.new(s_("TransferProject|Cannot move project")) + raise TransferError, s_("TransferProject|Cannot move project") end # Disk path is changed; we need to ensure we reload it @@ -223,10 +223,10 @@ module Projects end def update_integrations - project.services.inherit.delete_all - Service.create_from_active_default_integrations(project, :project_id) + project.integrations.inherit.delete_all + Integration.create_from_active_default_integrations(project, :project_id) end end end -Projects::TransferService.prepend_if_ee('EE::Projects::TransferService') +Projects::TransferService.prepend_mod_with('Projects::TransferService') diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index 91632e50ba8..9eccc16a8b2 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -17,7 +17,7 @@ module Projects .from_and_to_forks(@project) merge_requests.find_each do |mr| - ::MergeRequests::CloseService.new(@project, @current_user).execute(mr) + ::MergeRequests::CloseService.new(project: @project, current_user: @current_user).execute(mr) log_info(message: "UnlinkForkService: Closed merge request", merge_request_id: mr.id) end diff --git a/app/services/projects/update_pages_configuration_service.rb b/app/services/projects/update_pages_configuration_service.rb index b63903c6c61..4272e1dc8b6 100644 --- a/app/services/projects/update_pages_configuration_service.rb +++ b/app/services/projects/update_pages_configuration_service.rb @@ -102,7 +102,7 @@ module Projects File.open(file, 'r') do |f| f.read end - rescue + rescue StandardError nil end end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 6fa42b293c5..8ea35131339 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -48,7 +48,7 @@ module Projects end rescue InvalidStateError => e error(e.message) - rescue => e + rescue StandardError => e error(e.message) raise e end @@ -145,7 +145,7 @@ module Projects FileUtils.mkdir_p(pages_path) begin FileUtils.move(public_path, previous_public_path) - rescue + rescue StandardError end FileUtils.move(archive_public_path, public_path) ensure @@ -267,4 +267,4 @@ module Projects end end -Projects::UpdatePagesService.prepend_if_ee('EE::Projects::UpdatePagesService') +Projects::UpdatePagesService.prepend_mod_with('Projects::UpdatePagesService') diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb index 8832a1bc027..9f4f6133d92 100644 --- a/app/services/projects/update_remote_mirror_service.rb +++ b/app/services/projects/update_remote_mirror_service.rb @@ -24,7 +24,7 @@ module Projects hard_retry_or_fail(remote_mirror, e.message, tries) error(e.message) - rescue => e + rescue StandardError => e remote_mirror.hard_fail!(e.message) raise e end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 8384bfa813f..541b333aae3 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -49,11 +49,11 @@ module Projects def validate! unless valid_visibility_level_change?(project, params[:visibility_level]) - raise ValidationError.new(s_('UpdateProject|New visibility level not allowed!')) + raise ValidationError, s_('UpdateProject|New visibility level not allowed!') end if renaming_project_with_container_registry_tags? - raise ValidationError.new(s_('UpdateProject|Cannot rename project because it contains container registry tags!')) + raise ValidationError, s_('UpdateProject|Cannot rename project because it contains container registry tags!') end validate_default_branch_change @@ -67,7 +67,7 @@ module Projects if project.change_head(params[:default_branch]) after_default_branch_change(previous_default_branch) else - raise ValidationError.new(s_("UpdateProject|Could not set the default branch")) + raise ValidationError, s_("UpdateProject|Could not set the default branch") end end @@ -170,4 +170,4 @@ module Projects end end -Projects::UpdateService.prepend_if_ee('EE::Projects::UpdateService') +Projects::UpdateService.prepend_mod_with('Projects::UpdateService') diff --git a/app/services/projects/update_statistics_service.rb b/app/services/projects/update_statistics_service.rb index a0793cff2df..71f5a8e633d 100644 --- a/app/services/projects/update_statistics_service.rb +++ b/app/services/projects/update_statistics_service.rb @@ -2,18 +2,49 @@ module Projects class UpdateStatisticsService < BaseService + include ::Gitlab::Utils::StrongMemoize + + STAT_TO_CACHED_METHOD = { + repository_size: :size, + commit_count: :commit_count + }.freeze + def execute return unless project Gitlab::AppLogger.info("Updating statistics for project #{project.id}") - project.statistics.refresh!(only: statistics.map(&:to_sym)) + expire_repository_caches + expire_wiki_caches + project.statistics.refresh!(only: statistics) end private + def expire_repository_caches + if statistics.empty? + project.repository.expire_statistics_caches + elsif method_caches_to_expire.present? + project.repository.expire_method_caches(method_caches_to_expire) + end + end + + def expire_wiki_caches + return unless project.wiki_enabled? && statistics.include?(:wiki_size) + + project.wiki.repository.expire_method_caches([:size]) + end + + def method_caches_to_expire + strong_memoize(:method_caches_to_expire) do + statistics.map { |stat| STAT_TO_CACHED_METHOD[stat] }.compact + end + end + def statistics - params[:statistics] + strong_memoize(:statistics) do + params[:statistics]&.map(&:to_sym) + end end end end diff --git a/app/services/prometheus/create_default_alerts_service.rb b/app/services/prometheus/create_default_alerts_service.rb index 4ae2743cc28..e59b0a8e8e3 100644 --- a/app/services/prometheus/create_default_alerts_service.rb +++ b/app/services/prometheus/create_default_alerts_service.rb @@ -84,7 +84,7 @@ module Prometheus def environment strong_memoize(:environment) do - EnvironmentsFinder.new(project, nil, name: 'production').execute.first || + Environments::EnvironmentsFinder.new(project, nil, name: 'production').execute.first || project.environments.first end end diff --git a/app/services/protected_branches/access_level_params.rb b/app/services/protected_branches/access_level_params.rb index e34bc23b4dc..6f7a289d9b4 100644 --- a/app/services/protected_branches/access_level_params.rb +++ b/app/services/protected_branches/access_level_params.rb @@ -34,4 +34,4 @@ module ProtectedBranches end end -ProtectedBranches::AccessLevelParams.prepend_if_ee('EE::ProtectedBranches::AccessLevelParams') +ProtectedBranches::AccessLevelParams.prepend_mod_with('ProtectedBranches::AccessLevelParams') diff --git a/app/services/protected_branches/api_service.rb b/app/services/protected_branches/api_service.rb index bf1a966472b..3e5122a1523 100644 --- a/app/services/protected_branches/api_service.rb +++ b/app/services/protected_branches/api_service.rb @@ -21,4 +21,4 @@ module ProtectedBranches end end -ProtectedBranches::ApiService.prepend_if_ee('EE::ProtectedBranches::ApiService') +ProtectedBranches::ApiService.prepend_mod_with('ProtectedBranches::ApiService') diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index 3c86d7d087d..37083a4a9e4 100644 --- a/app/services/protected_branches/create_service.rb +++ b/app/services/protected_branches/create_service.rb @@ -26,4 +26,4 @@ module ProtectedBranches end end -ProtectedBranches::CreateService.prepend_if_ee('EE::ProtectedBranches::CreateService') +ProtectedBranches::CreateService.prepend_mod_with('ProtectedBranches::CreateService') diff --git a/app/services/protected_branches/destroy_service.rb b/app/services/protected_branches/destroy_service.rb index acd15b0214f..dc177f0ac09 100644 --- a/app/services/protected_branches/destroy_service.rb +++ b/app/services/protected_branches/destroy_service.rb @@ -10,4 +10,4 @@ module ProtectedBranches end end -ProtectedBranches::DestroyService.prepend_if_ee('EE::ProtectedBranches::DestroyService') +ProtectedBranches::DestroyService.prepend_mod_with('ProtectedBranches::DestroyService') diff --git a/app/services/protected_branches/legacy_api_update_service.rb b/app/services/protected_branches/legacy_api_update_service.rb index 0cad23f20f7..8ff6c4bd734 100644 --- a/app/services/protected_branches/legacy_api_update_service.rb +++ b/app/services/protected_branches/legacy_api_update_service.rb @@ -49,4 +49,4 @@ module ProtectedBranches end end -ProtectedBranches::LegacyApiUpdateService.prepend_if_ee('EE::ProtectedBranches::LegacyApiUpdateService') +ProtectedBranches::LegacyApiUpdateService.prepend_mod_with('ProtectedBranches::LegacyApiUpdateService') diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb index 226aefb64d0..1815d92421e 100644 --- a/app/services/protected_branches/update_service.rb +++ b/app/services/protected_branches/update_service.rb @@ -11,4 +11,4 @@ module ProtectedBranches end end -ProtectedBranches::UpdateService.prepend_if_ee('EE::ProtectedBranches::UpdateService') +ProtectedBranches::UpdateService.prepend_mod_with('ProtectedBranches::UpdateService') diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index ea90d8e3dd8..ab489ba49ca 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -190,4 +190,4 @@ module QuickActions end end -QuickActions::InterpretService.prepend_if_ee('EE::QuickActions::InterpretService') +QuickActions::InterpretService.prepend_mod_with('QuickActions::InterpretService') diff --git a/app/services/quick_actions/target_service.rb b/app/services/quick_actions/target_service.rb index a465632ccfb..6eda3c89e6c 100644 --- a/app/services/quick_actions/target_service.rb +++ b/app/services/quick_actions/target_service.rb @@ -37,4 +37,4 @@ module QuickActions end end -QuickActions::TargetService.prepend_if_ee('EE::QuickActions::TargetService') +QuickActions::TargetService.prepend_mod_with('QuickActions::TargetService') diff --git a/app/services/releases/base_service.rb b/app/services/releases/base_service.rb index de7c97b3518..9dd0c9a007a 100644 --- a/app/services/releases/base_service.rb +++ b/app/services/releases/base_service.rb @@ -86,4 +86,4 @@ module Releases end end -Releases::BaseService.prepend_if_ee('EE::Releases::BaseService') +Releases::BaseService.prepend_mod_with('Releases::BaseService') diff --git a/app/services/releases/create_evidence_service.rb b/app/services/releases/create_evidence_service.rb index 78b6d77c2cb..64cce45e400 100644 --- a/app/services/releases/create_evidence_service.rb +++ b/app/services/releases/create_evidence_service.rb @@ -28,4 +28,4 @@ module Releases end end -Releases::CreateEvidenceService.prepend_if_ee('EE::Releases::CreateEvidenceService') +Releases::CreateEvidenceService.prepend_mod_with('Releases::CreateEvidenceService') diff --git a/app/services/releases/create_service.rb b/app/services/releases/create_service.rb index 11fdbaf3169..1096e207e02 100644 --- a/app/services/releases/create_service.rb +++ b/app/services/releases/create_service.rb @@ -57,7 +57,7 @@ module Releases create_evidence!(release, evidence_pipeline) success(tag: tag, release: release) - rescue => e + rescue StandardError => e error(e.message, 400) end diff --git a/app/services/repositories/changelog_service.rb b/app/services/repositories/changelog_service.rb index 0122bfb154d..bac3fdf36da 100644 --- a/app/services/repositories/changelog_service.rb +++ b/app/services/repositories/changelog_service.rb @@ -39,7 +39,7 @@ module Repositories project, user, version:, - branch: project.default_branch_or_master, + branch: project.default_branch_or_main, from: nil, to: branch, date: DateTime.now, diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index 620dfff91e2..84f4478f20f 100644 --- a/app/services/resource_access_tokens/create_service.rb +++ b/app/services/resource_access_tokens/create_service.rb @@ -120,4 +120,4 @@ module ResourceAccessTokens end end -ResourceAccessTokens::CreateService.prepend_if_ee('EE::ResourceAccessTokens::CreateService') +ResourceAccessTokens::CreateService.prepend_mod_with('ResourceAccessTokens::CreateService') diff --git a/app/services/resource_access_tokens/revoke_service.rb b/app/services/resource_access_tokens/revoke_service.rb index 0924ca3bac4..9543ea4b68d 100644 --- a/app/services/resource_access_tokens/revoke_service.rb +++ b/app/services/resource_access_tokens/revoke_service.rb @@ -67,4 +67,4 @@ module ResourceAccessTokens end end -ResourceAccessTokens::RevokeService.prepend_if_ee('EE::ResourceAccessTokens::RevokeService') +ResourceAccessTokens::RevokeService.prepend_mod_with('ResourceAccessTokens::RevokeService') diff --git a/app/services/resource_events/change_labels_service.rb b/app/services/resource_events/change_labels_service.rb index 89eb90e9360..3797d41a5df 100644 --- a/app/services/resource_events/change_labels_service.rb +++ b/app/services/resource_events/change_labels_service.rb @@ -44,4 +44,4 @@ module ResourceEvents end end -ResourceEvents::ChangeLabelsService.prepend_if_ee('EE::ResourceEvents::ChangeLabelsService') +ResourceEvents::ChangeLabelsService.prepend_mod_with('ResourceEvents::ChangeLabelsService') diff --git a/app/services/resource_events/merge_into_notes_service.rb b/app/services/resource_events/merge_into_notes_service.rb index 122bcb8550f..ea465c1e75e 100644 --- a/app/services/resource_events/merge_into_notes_service.rb +++ b/app/services/resource_events/merge_into_notes_service.rb @@ -37,4 +37,4 @@ module ResourceEvents end end -ResourceEvents::MergeIntoNotesService.prepend_if_ee('EE::ResourceEvents::MergeIntoNotesService') +ResourceEvents::MergeIntoNotesService.prepend_mod_with('ResourceEvents::MergeIntoNotesService') diff --git a/app/services/search/global_service.rb b/app/services/search/global_service.rb index 055034d87a1..661aafc70cd 100644 --- a/app/services/search/global_service.rb +++ b/app/services/search/global_service.rb @@ -38,4 +38,4 @@ module Search end end -Search::GlobalService.prepend_if_ee('EE::Search::GlobalService') +Search::GlobalService.prepend_mod_with('Search::GlobalService') diff --git a/app/services/search/group_service.rb b/app/services/search/group_service.rb index 4b2d8499582..daed0df83f3 100644 --- a/app/services/search/group_service.rb +++ b/app/services/search/group_service.rb @@ -31,4 +31,4 @@ module Search end end -Search::GroupService.prepend_if_ee('EE::Search::GroupService') +Search::GroupService.prepend_mod_with('Search::GroupService') diff --git a/app/services/search/project_service.rb b/app/services/search/project_service.rb index 4227dfe2fac..3181c0098cc 100644 --- a/app/services/search/project_service.rb +++ b/app/services/search/project_service.rb @@ -37,4 +37,4 @@ module Search end end -Search::ProjectService.prepend_if_ee('EE::Search::ProjectService') +Search::ProjectService.prepend_mod_with('Search::ProjectService') diff --git a/app/services/search/snippet_service.rb b/app/services/search/snippet_service.rb index 30401b28571..b629fd305d7 100644 --- a/app/services/search/snippet_service.rb +++ b/app/services/search/snippet_service.rb @@ -12,4 +12,4 @@ module Search end end -Search::SnippetService.prepend_if_ee('::EE::Search::SnippetService') +Search::SnippetService.prepend_mod_with('Search::SnippetService') diff --git a/app/services/search_service.rb b/app/services/search_service.rb index 84d7e33c3d0..389cf17e115 100644 --- a/app/services/search_service.rb +++ b/app/services/search_service.rb @@ -140,4 +140,4 @@ class SearchService attr_reader :current_user, :params end -SearchService.prepend_if_ee('EE::SearchService') +SearchService.prepend_mod_with('SearchService') diff --git a/app/services/security/ci_configuration/base_create_service.rb b/app/services/security/ci_configuration/base_create_service.rb new file mode 100644 index 00000000000..adb45244adb --- /dev/null +++ b/app/services/security/ci_configuration/base_create_service.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module Security + module CiConfiguration + class BaseCreateService + attr_reader :branch_name, :current_user, :project + + def initialize(project, current_user) + @project = project + @current_user = current_user + @branch_name = project.repository.next_branch(next_branch) + end + + def execute + project.repository.add_branch(current_user, branch_name, project.default_branch) + + attributes_for_commit = attributes + + result = ::Files::MultiService.new(project, current_user, attributes_for_commit).execute + + return ServiceResponse.error(message: result[:message]) unless result[:status] == :success + + track_event(attributes_for_commit) + ServiceResponse.success(payload: { branch: branch_name, success_path: successful_change_path }) + rescue Gitlab::Git::PreReceiveError => e + ServiceResponse.error(message: e.message) + rescue StandardError + project.repository.rm_branch(current_user, branch_name) if project.repository.branch_exists?(branch_name) + raise + end + + private + + def attributes + { + commit_message: message, + branch_name: branch_name, + start_branch: branch_name, + actions: [action] + } + end + + def existing_gitlab_ci_content + @gitlab_ci_yml ||= project.repository.gitlab_ci_yml_for(project.repository.root_ref_sha) + YAML.safe_load(@gitlab_ci_yml) if @gitlab_ci_yml + end + + def successful_change_path + merge_request_params = { source_branch: branch_name, description: description } + Gitlab::Routing.url_helpers.project_new_merge_request_url(project, merge_request: merge_request_params) + end + + def track_event(attributes_for_commit) + action = attributes_for_commit[:actions].first + + Gitlab::Tracking.event( + self.class.to_s, action[:action], label: action[:default_values_overwritten].to_s + ) + end + end + end +end diff --git a/app/services/security/ci_configuration/sast_create_service.rb b/app/services/security/ci_configuration/sast_create_service.rb index 8fc3b8d078c..f495cac18f8 100644 --- a/app/services/security/ci_configuration/sast_create_service.rb +++ b/app/services/security/ci_configuration/sast_create_service.rb @@ -2,64 +2,30 @@ module Security module CiConfiguration - class SastCreateService < ::BaseService + class SastCreateService < ::Security::CiConfiguration::BaseCreateService + attr_reader :params + def initialize(project, current_user, params) - @project = project - @current_user = current_user + super(project, current_user) @params = params - @branch_name = @project.repository.next_branch('set-sast-config') - end - - def execute - attributes_for_commit = attributes - result = ::Files::MultiService.new(@project, @current_user, attributes_for_commit).execute - - if result[:status] == :success - result[:success_path] = successful_change_path - track_event(attributes_for_commit) - else - result[:errors] = result[:message] - end - - result - - rescue Gitlab::Git::PreReceiveError => e - { status: :error, errors: e.message } end private - def attributes - actions = Security::CiConfiguration::SastBuildActions.new(@project.auto_devops_enabled?, @params, existing_gitlab_ci_content).generate - - @project.repository.add_branch(@current_user, @branch_name, @project.default_branch) - message = _('Set .gitlab-ci.yml to enable or configure SAST') - - { - commit_message: message, - branch_name: @branch_name, - start_branch: @branch_name, - actions: actions - } + def action + Security::CiConfiguration::SastBuildAction.new(project.auto_devops_enabled?, params, existing_gitlab_ci_content).generate end - def existing_gitlab_ci_content - gitlab_ci_yml = @project.repository.gitlab_ci_yml_for(@project.repository.root_ref_sha) - YAML.safe_load(gitlab_ci_yml) if gitlab_ci_yml + def next_branch + 'set-sast-config' end - def successful_change_path - description = _('Set .gitlab-ci.yml to enable or configure SAST security scanning using the GitLab managed template. You can [add variable overrides](https://docs.gitlab.com/ee/user/application_security/sast/#customizing-the-sast-settings) to customize SAST settings.') - merge_request_params = { source_branch: @branch_name, description: description } - Gitlab::Routing.url_helpers.project_new_merge_request_url(@project, merge_request: merge_request_params) + def message + _('Configure SAST in `.gitlab-ci.yml`, creating this file if it does not already exist') end - def track_event(attributes_for_commit) - action = attributes_for_commit[:actions].first - - Gitlab::Tracking.event( - self.class.to_s, action[:action], label: action[:default_values_overwritten].to_s - ) + def description + _('Configure SAST in `.gitlab-ci.yml` using the GitLab managed template. You can [add variable overrides](https://docs.gitlab.com/ee/user/application_security/sast/#customizing-the-sast-settings) to customize SAST settings.') end end end diff --git a/app/services/security/ci_configuration/sast_parser_service.rb b/app/services/security/ci_configuration/sast_parser_service.rb index a8fe5764d19..5220525d552 100644 --- a/app/services/security/ci_configuration/sast_parser_service.rb +++ b/app/services/security/ci_configuration/sast_parser_service.rb @@ -74,7 +74,7 @@ module Security def sast_excluded_analyzers strong_memoize(:sast_excluded_analyzers) do - all_analyzers = Security::CiConfiguration::SastBuildActions::SAST_DEFAULT_ANALYZERS.split(', ') rescue [] + all_analyzers = Security::CiConfiguration::SastBuildAction::SAST_DEFAULT_ANALYZERS.split(', ') rescue [] enabled_analyzers = sast_default_analyzers.split(',').map(&:strip) rescue [] excluded_analyzers = gitlab_ci_yml_attributes["SAST_EXCLUDED_ANALYZERS"] || sast_template_attributes["SAST_EXCLUDED_ANALYZERS"] diff --git a/app/services/security/ci_configuration/secret_detection_create_service.rb b/app/services/security/ci_configuration/secret_detection_create_service.rb new file mode 100644 index 00000000000..ff3458d36fc --- /dev/null +++ b/app/services/security/ci_configuration/secret_detection_create_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Security + module CiConfiguration + class SecretDetectionCreateService < ::Security::CiConfiguration::BaseCreateService + private + + def action + Security::CiConfiguration::SecretDetectionBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content).generate + end + + def next_branch + 'set-secret-detection-config' + end + + def message + _('Configure Secret Detection in `.gitlab-ci.yml`, creating this file if it does not already exist') + end + + def description + _('Configure Secret Detection in `.gitlab-ci.yml` using the GitLab managed template. You can [add variable overrides](https://docs.gitlab.com/ee/user/application_security/secret_detection/#customizing-settings) to customize Secret Detection settings.') + end + end + end +end diff --git a/app/services/service_response.rb b/app/services/service_response.rb index 74c0be22d46..6bc394d2ae2 100644 --- a/app/services/service_response.rb +++ b/app/services/service_response.rb @@ -18,6 +18,14 @@ class ServiceResponse self.http_status = http_status end + def [](key) + to_h[key] + end + + def to_h + (payload || {}).merge(status: status, message: message, http_status: http_status) + end + def success? status == :success end diff --git a/app/services/snippets/bulk_destroy_service.rb b/app/services/snippets/bulk_destroy_service.rb index a612d8f8dfc..430e8330b59 100644 --- a/app/services/snippets/bulk_destroy_service.rb +++ b/app/services/snippets/bulk_destroy_service.rb @@ -27,7 +27,7 @@ module Snippets rescue DeleteRepositoryError attempt_rollback_repositories service_response_error('Failed to delete snippet repositories.', 400) - rescue + rescue StandardError # In case the delete operation fails attempt_rollback_repositories service_response_error('Failed to remove snippets.', 400) diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index c95b459cd2a..aadf9b865b8 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -69,7 +69,7 @@ module Snippets end snippet_saved - rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ... + rescue StandardError => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ... Gitlab::ErrorTracking.log_exception(e, service: 'Snippets::CreateService') # If the commit action failed we need to remove the repository if exists diff --git a/app/services/snippets/destroy_service.rb b/app/services/snippets/destroy_service.rb index f1f80dbaf86..96157434462 100644 --- a/app/services/snippets/destroy_service.rb +++ b/app/services/snippets/destroy_service.rb @@ -30,7 +30,7 @@ module Snippets ServiceResponse.success(message: 'Snippet was deleted.') rescue DestroyError service_response_error('Failed to remove snippet repository.', 400) - rescue + rescue StandardError attempt_rollback_repository service_response_error('Failed to remove snippet.', 400) end @@ -59,4 +59,4 @@ module Snippets end end -Snippets::DestroyService.prepend_if_ee('EE::Snippets::DestroyService') +Snippets::DestroyService.prepend_mod_with('Snippets::DestroyService') diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb index aedb6a4819d..4088a08272d 100644 --- a/app/services/snippets/update_service.rb +++ b/app/services/snippets/update_service.rb @@ -78,7 +78,7 @@ module Snippets create_commit(snippet) true - rescue => e + rescue StandardError => e # Restore old attributes but re-assign changes so they're not lost unless snippet.previous_changes.empty? snippet.previous_changes.each { |attr, value| snippet[attr] = value[0] } diff --git a/app/services/spam/akismet_service.rb b/app/services/spam/akismet_service.rb index e11a1dbdd96..4e56972ccd5 100644 --- a/app/services/spam/akismet_service.rb +++ b/app/services/spam/akismet_service.rb @@ -20,13 +20,13 @@ module Spam created_at: DateTime.current, author: owner_name, author_email: owner_email, - referrer: options[:referrer] + referer: options[:referer] } begin is_spam, is_blatant = akismet_client.check(options[:ip_address], options[:user_agent], params) is_spam || is_blatant - rescue => e + rescue StandardError => e Gitlab::AppLogger.error("Unable to connect to Akismet: #{e}, skipping check") false end @@ -66,7 +66,7 @@ module Spam begin akismet_client.public_send(type, options[:ip_address], options[:user_agent], params) # rubocop:disable GitlabSecurity/PublicSend true - rescue => e + rescue StandardError => e Gitlab::AppLogger.error("Unable to connect to Akismet: #{e}, skipping!") false end diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index 2220198583c..3ae5111b994 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -53,7 +53,7 @@ module Spam if request options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s options[:user_agent] = request.env['HTTP_USER_AGENT'] - options[:referrer] = request.env['HTTP_REFERRER'] + options[:referer] = request.env['HTTP_REFERER'] else # TODO: This code is never used, because we do not perform a verification if there is not a # request. Why? Should it be deleted? Or should we check even if there is no request? @@ -123,8 +123,16 @@ module Spam # https://gitlab.com/gitlab-org/gitlab/-/issues/214739 target.spam! unless target.allow_possible_spam? create_spam_log(api) + when BLOCK_USER + # TODO: improve BLOCK_USER handling, non-existent until now + # https://gitlab.com/gitlab-org/gitlab/-/issues/329666 + target.spam! unless target.allow_possible_spam? + create_spam_log(api) when ALLOW target.clear_spam_flags! + when NOOP + # spamcheck is not explicitly rendering a verdict & therefore can't make a decision + target.clear_spam_flags! end end end diff --git a/app/services/spam/spam_constants.rb b/app/services/spam/spam_constants.rb index 2a16cfae78b..b654fbbbcc8 100644 --- a/app/services/spam/spam_constants.rb +++ b/app/services/spam/spam_constants.rb @@ -6,6 +6,7 @@ module Spam DISALLOW = "disallow" ALLOW = "allow" BLOCK_USER = "block" + NOOP = "noop" SUPPORTED_VERDICTS = { BLOCK_USER => { @@ -19,6 +20,9 @@ module Spam }, ALLOW => { priority: 4 + }, + NOOP => { + priority: 5 } }.freeze end diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index 7de3bad607a..7155017b73f 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -10,25 +10,56 @@ module Spam @request = request @user = user @options = options - @verdict_params = assemble_verdict_params(context) + @context = context end def execute - external_spam_check_result = external_verdict + spamcheck_result = nil + spamcheck_attribs = {} + spamcheck_error = false + + external_spam_check_round_trip_time = Benchmark.realtime do + spamcheck_result, spamcheck_attribs, spamcheck_error = spamcheck_verdict + end + + label = spamcheck_error ? 'ERROR' : spamcheck_result.to_s.upcase + + histogram.observe( { result: label }, external_spam_check_round_trip_time ) + + # assign result to a var for logging it before reassigning to nil when monitorMode is true + original_spamcheck_result = spamcheck_result + + spamcheck_result = nil if spamcheck_attribs&.fetch("monitorMode", "false") == "true" + akismet_result = akismet_verdict # filter out anything we don't recognise, including nils. - valid_results = [external_spam_check_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) } + valid_results = [spamcheck_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) } + # Treat nils - such as service unavailable - as ALLOW return ALLOW unless valid_results.any? # Favour the most restrictive result. - valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] } + final_verdict = valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] } + + logger.info(class: self.class.name, + akismet_verdict: akismet_verdict, + spam_check_verdict: original_spamcheck_result, + extra_attributes: spamcheck_attribs, + spam_check_rtt: external_spam_check_round_trip_time.real, + final_verdict: final_verdict, + username: user.username, + user_id: user.id, + target_type: target.class.to_s, + project_id: target.project_id + ) + + final_verdict end private - attr_reader :user, :target, :request, :options, :verdict_params + attr_reader :user, :target, :request, :options, :context def akismet_verdict if akismet.spam? @@ -38,54 +69,41 @@ module Spam end end - def external_verdict + def spamcheck_verdict return unless Gitlab::CurrentSettings.spam_check_endpoint_enabled - return if endpoint_url.blank? begin - result = Gitlab::HTTP.post(endpoint_url, body: verdict_params.to_json, headers: { 'Content-Type' => 'application/json' }) - return unless result - - json_result = Gitlab::Json.parse(result).with_indifferent_access - # @TODO metrics/logging - # Expecting: - # error: (string or nil) - # verdict: (string or nil) - # @TODO log if json_result[:error] - - json_result[:verdict] - rescue *Gitlab::HTTP::HTTP_ERRORS => e - # @TODO: log error via try_post https://gitlab.com/gitlab-org/gitlab/-/issues/219223 + result, attribs, _error = spamcheck_client.issue_spam?(spam_issue: target, user: user, context: context) + return [nil, attribs] unless result + + # @TODO log if error is not nil https://gitlab.com/gitlab-org/gitlab/-/issues/329545 + + return [result, attribs] if result == NOOP || attribs["monitorMode"] == "true" + + # Duplicate logic with Akismet logic in #akismet_verdict + if Gitlab::Recaptcha.enabled? && result != ALLOW + [CONDITIONAL_ALLOW, attribs] + else + [result, attribs] + end + rescue StandardError => e Gitlab::ErrorTracking.log_exception(e) - nil - rescue - # @TODO log - ALLOW + + # Default to ALLOW if any errors occur + [ALLOW, attribs, true] end end - def assemble_verdict_params(context) - return {} unless endpoint_url.present? - - project = target.try(:project) - - context.merge({ - target: { - title: target.spam_title, - description: target.spam_description, - type: target.class.to_s - }, - user: { - created_at: user.created_at, - email: user.email, - username: user.username - }, - user_in_project: user.authorized_project?(project) - }) + def spamcheck_client + @spamcheck_client ||= Gitlab::Spamcheck::Client.new + end + + def logger + @logger ||= Gitlab::AppJsonLogger.build end - def endpoint_url - @endpoint_url ||= Gitlab::CurrentSettings.current_application_settings.spam_check_endpoint_url + def histogram + @histogram ||= Gitlab::Metrics.histogram(:gitlab_spamcheck_request_duration_seconds, 'Request duration to the anti-spam service') end end end diff --git a/app/services/static_site_editor/config_service.rb b/app/services/static_site_editor/config_service.rb index 7b3115468a5..c8e7165e076 100644 --- a/app/services/static_site_editor/config_service.rb +++ b/app/services/static_site_editor/config_service.rb @@ -25,7 +25,7 @@ module StaticSiteEditor ServiceResponse.success(payload: data) rescue ValidationError => e ServiceResponse.error(message: e.message) - rescue => e + rescue StandardError => e Gitlab::ErrorTracking.track_and_raise_exception(e) end @@ -67,7 +67,7 @@ module StaticSiteEditor def check_for_duplicate_keys!(generated_data, file_data) duplicate_keys = generated_data.keys & file_data.keys - raise ValidationError.new("Duplicate key(s) '#{duplicate_keys}' found.") if duplicate_keys.present? + raise ValidationError, "Duplicate key(s) '#{duplicate_keys}' found." if duplicate_keys.present? end def merged_data(generated_data, file_data) diff --git a/app/services/submit_usage_ping_service.rb b/app/services/submit_usage_ping_service.rb index d628b1ea7c7..4942dd0e913 100644 --- a/app/services/submit_usage_ping_service.rb +++ b/app/services/submit_usage_ping_service.rb @@ -22,7 +22,7 @@ class SubmitUsagePingService usage_data = Gitlab::UsageData.data(force_refresh: true) - raise SubmissionError.new('Usage data is blank') if usage_data.blank? + raise SubmissionError, 'Usage data is blank' if usage_data.blank? raw_usage_data = save_raw_usage_data(usage_data) @@ -33,12 +33,12 @@ class SubmitUsagePingService headers: { 'Content-type' => 'application/json' } ) - raise SubmissionError.new("Unsuccessful response code: #{response.code}") unless response.success? + raise SubmissionError, "Unsuccessful response code: #{response.code}" unless response.success? version_usage_data_id = response.dig('conv_index', 'usage_data_id') || response.dig('dev_ops_score', 'usage_data_id') unless version_usage_data_id.is_a?(Integer) && version_usage_data_id > 0 - raise SubmissionError.new("Invalid usage_data_id in response: #{version_usage_data_id}") + raise SubmissionError, "Invalid usage_data_id in response: #{version_usage_data_id}" end raw_usage_data.update_version_metadata!(usage_data_id: version_usage_data_id) @@ -73,3 +73,5 @@ class SubmitUsagePingService end end end + +SubmitUsagePingService.prepend_mod diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb index f9783f4271f..6836700a67d 100644 --- a/app/services/suggestions/apply_service.rb +++ b/app/services/suggestions/apply_service.rb @@ -36,12 +36,22 @@ module Suggestions .track_apply_suggestion_action(user: current_user) end + def author + authors = suggestion_set.authors + + return unless authors.one? + + Gitlab::Git::User.from_gitlab(authors.first) + end + def multi_service params = { commit_message: commit_message, branch_name: suggestion_set.branch, start_branch: suggestion_set.branch, - actions: suggestion_set.actions + actions: suggestion_set.actions, + author_name: author&.name, + author_email: author&.email } ::Files::MultiService.new(suggestion_set.project, current_user, params) diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index 53e810035c5..2a2053cb912 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class SystemHooksService - BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember, Group, ProjectMember, User].freeze - def execute_hooks_for(model, event) data = build_event_data(model, event) @@ -12,7 +10,7 @@ class SystemHooksService end def execute_hooks(data, hooks_scope = :all) - SystemHook.hooks_for(hooks_scope).find_each do |hook| + SystemHook.executable.hooks_for(hooks_scope).find_each do |hook| hook.async_execute(data, 'system_hooks') end @@ -22,59 +20,6 @@ class SystemHooksService private def build_event_data(model, event) - # return entire event data from its builder class, if available. - return builder_driven_event_data(model, event) if builder_driven_event_data_available?(model) - - data = { - event_name: build_event_name(model, event), - created_at: model.created_at&.xmlschema, - updated_at: model.updated_at&.xmlschema - } - - case model - when Key - data.merge!( - key: model.key, - id: model.id - ) - - if model.user - data[:username] = model.user.username - end - when Project - data.merge!(project_data(model)) - - if event == :rename || event == :transfer - data[:old_path_with_namespace] = model.old_path_with_namespace - end - end - - data - end - - def build_event_name(model, event) - "#{model.class.name.downcase}_#{event}" - end - - def project_data(model) - owner = model.owner - - { - name: model.name, - path: model.path, - path_with_namespace: model.full_path, - project_id: model.id, - owner_name: owner.name, - owner_email: owner.respond_to?(:email) ? owner.email : "", - project_visibility: model.visibility.downcase - } - end - - def builder_driven_event_data_available?(model) - model.class.in?(BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES) - end - - def builder_driven_event_data(model, event) builder_class = case model when GroupMember Gitlab::HookData::GroupMemberBuilder @@ -84,6 +29,10 @@ class SystemHooksService Gitlab::HookData::ProjectMemberBuilder when User Gitlab::HookData::UserBuilder + when Project + Gitlab::HookData::ProjectBuilder + when Key + Gitlab::HookData::KeyBuilder end builder_class.new(model).build(event) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 4377bd8554b..56a6244eebf 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -338,4 +338,4 @@ module SystemNoteService end end -SystemNoteService.prepend_if_ee('EE::SystemNoteService') +SystemNoteService.prepend_mod_with('SystemNoteService') diff --git a/app/services/system_notes/base_service.rb b/app/services/system_notes/base_service.rb index 7341a25b133..ee7784c127b 100644 --- a/app/services/system_notes/base_service.rb +++ b/app/services/system_notes/base_service.rb @@ -13,10 +13,10 @@ module SystemNotes protected def create_note(note_summary) - note = Note.create(note_summary.note.merge(system: true)) - note.system_note_metadata = SystemNoteMetadata.new(note_summary.metadata) if note_summary.metadata? + note_params = note_summary.note.merge(system: true) + note_params[:system_note_metadata] = SystemNoteMetadata.new(note_summary.metadata) if note_summary.metadata? - note + Note.create(note_params) end def content_tag(*args) diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb index 60dd56e772a..ae4f65e785c 100644 --- a/app/services/system_notes/issuables_service.rb +++ b/app/services/system_notes/issuables_service.rb @@ -178,8 +178,7 @@ module SystemNotes if noteable.is_a?(ExternalIssue) noteable.project.external_issue_tracker.create_cross_reference_note(noteable, mentioner, author) else - issue_activity_counter.track_issue_cross_referenced_action(author: author) if noteable.is_a?(Issue) - + track_cross_reference_action create_note(NoteSummary.new(noteable, noteable.project, author, body, action: 'cross_reference')) end end @@ -414,7 +413,11 @@ module SystemNotes def issue_activity_counter Gitlab::UsageDataCounters::IssueActivityUniqueCounter end + + def track_cross_reference_action + issue_activity_counter.track_issue_cross_referenced_action(author: author) if noteable.is_a?(Issue) + end end end -SystemNotes::IssuablesService.prepend_if_ee('::EE::SystemNotes::IssuablesService') +SystemNotes::IssuablesService.prepend_mod_with('SystemNotes::IssuablesService') diff --git a/app/services/system_notes/time_tracking_service.rb b/app/services/system_notes/time_tracking_service.rb index 650e40680b1..a804a06fe4c 100644 --- a/app/services/system_notes/time_tracking_service.rb +++ b/app/services/system_notes/time_tracking_service.rb @@ -62,12 +62,12 @@ module SystemNotes if time_spent == :reset body = "removed time spent" else - spent_at = noteable.spent_at + spent_at = noteable.spent_at&.to_date parsed_time = Gitlab::TimeTrackingFormatter.output(time_spent.abs) action = time_spent > 0 ? 'added' : 'subtracted' text_parts = ["#{action} #{parsed_time} of time spent"] - text_parts << "at #{spent_at}" if spent_at + text_parts << "at #{spent_at}" if spent_at && spent_at != DateTime.current.to_date body = text_parts.join(' ') end diff --git a/app/services/terraform/remote_state_handler.rb b/app/services/terraform/remote_state_handler.rb index db47bc024ba..e9a13cee764 100644 --- a/app/services/terraform/remote_state_handler.rb +++ b/app/services/terraform/remote_state_handler.rb @@ -94,7 +94,7 @@ module Terraform end def find_state!(find_params) - find_state(find_params) || raise(ActiveRecord::RecordNotFound.new("Couldn't find state")) + find_state(find_params) || raise(ActiveRecord::RecordNotFound, "Couldn't find state") end end end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index e473a6dc594..fc6543a8efc 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -43,11 +43,11 @@ class TodoService # updates the todo counts for those users. # def destroy_target(target) - todo_users = UsersWithPendingTodosFinder.new(target).execute.to_a + todo_user_ids = target.todos.distinct_user_ids yield target - Users::UpdateTodoCountCacheService.new(todo_users).execute if todo_users.present? + Users::UpdateTodoCountCacheService.new(todo_user_ids).execute if todo_user_ids.present? end # When we reassign an assignable object (issuable, alert) we should: @@ -224,7 +224,7 @@ class TodoService return if users.empty? - users_with_pending_todos = pending_todos(users, attributes).pluck_user_id + users_with_pending_todos = pending_todos(users, attributes).distinct_user_ids users.reject! { |user| users_with_pending_todos.include?(user.id) && Feature.disabled?(:multiple_todos, user) } todos = users.map do |user| @@ -234,7 +234,7 @@ class TodoService Todo.create(attributes.merge(user_id: user.id)) end - Users::UpdateTodoCountCacheService.new(users).execute + Users::UpdateTodoCountCacheService.new(users.map(&:id)).execute todos end @@ -371,4 +371,4 @@ class TodoService end end -TodoService.prepend_if_ee('EE::TodoService') +TodoService.prepend_mod_with('TodoService') diff --git a/app/services/todos/destroy/confidential_issue_service.rb b/app/services/todos/destroy/confidential_issue_service.rb index 6cdd8c16894..fadc76b1181 100644 --- a/app/services/todos/destroy/confidential_issue_service.rb +++ b/app/services/todos/destroy/confidential_issue_service.rb @@ -37,7 +37,7 @@ module Todos def todos Todo.joins_issue_and_assignees .where(target: issues) - .where('issues.confidential = ?', true) + .where(issues: { confidential: true }) .where('todos.user_id != issues.author_id') .where('todos.user_id != issue_assignees.user_id') end diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index 6d4fc3865ac..dfe14225ade 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -9,7 +9,7 @@ module Todos def initialize(user_id, entity_id, entity_type) unless %w(Group Project).include?(entity_type) - raise ArgumentError.new("#{entity_type} is not an entity user can leave") + raise ArgumentError, "#{entity_type} is not an entity user can leave" end @user = UserFinder.new(user_id).find_by_id @@ -143,4 +143,4 @@ module Todos end end -Todos::Destroy::EntityLeaveService.prepend_if_ee('EE::Todos::Destroy::EntityLeaveService') +Todos::Destroy::EntityLeaveService.prepend_mod_with('Todos::Destroy::EntityLeaveService') diff --git a/app/services/user_project_access_changed_service.rb b/app/services/user_project_access_changed_service.rb index 11727f05f35..80490bd4c9a 100644 --- a/app/services/user_project_access_changed_service.rb +++ b/app/services/user_project_access_changed_service.rb @@ -26,4 +26,4 @@ class UserProjectAccessChangedService end end -UserProjectAccessChangedService.prepend_if_ee('EE::UserProjectAccessChangedService') +UserProjectAccessChangedService.prepend_mod_with('UserProjectAccessChangedService') diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb index 64844a3f002..c89a286cc8b 100644 --- a/app/services/users/activity_service.rb +++ b/app/services/users/activity_service.rb @@ -38,4 +38,4 @@ module Users end end -Users::ActivityService.prepend_ee_mod +Users::ActivityService.prepend_mod diff --git a/app/services/users/approve_service.rb b/app/services/users/approve_service.rb index fea7fc55d90..15486ddcd43 100644 --- a/app/services/users/approve_service.rb +++ b/app/services/users/approve_service.rb @@ -47,4 +47,4 @@ module Users end end -Users::ApproveService.prepend_if_ee('EE::Users::ApproveService') +Users::ApproveService.prepend_mod_with('Users::ApproveService') diff --git a/app/services/users/ban_service.rb b/app/services/users/ban_service.rb new file mode 100644 index 00000000000..247ed14966b --- /dev/null +++ b/app/services/users/ban_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Users + class BanService < BaseService + def initialize(current_user) + @current_user = current_user + end + + def execute(user) + if user.ban + log_event(user) + success + else + messages = user.errors.full_messages + error(messages.uniq.join('. ')) + end + end + + private + + def log_event(user) + Gitlab::AppLogger.info(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + end + end +end diff --git a/app/services/users/block_service.rb b/app/services/users/block_service.rb index 8513664ee85..37921c477b4 100644 --- a/app/services/users/block_service.rb +++ b/app/services/users/block_service.rb @@ -26,4 +26,4 @@ module Users end end -Users::BlockService.prepend_if_ee('EE::Users::BlockService') +Users::BlockService.prepend_mod_with('Users::BlockService') diff --git a/app/services/users/build_service.rb b/app/services/users/build_service.rb index b3b172f9df2..649cf281ab0 100644 --- a/app/services/users/build_service.rb +++ b/app/services/users/build_service.rb @@ -14,9 +14,11 @@ module Users end def execute(skip_authorization: false) + @skip_authorization = skip_authorization + raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_create_user? - user_params = build_user_params(skip_authorization: skip_authorization) + user_params = build_user_params user = User.new(user_params) if current_user&.admin? @@ -37,6 +39,8 @@ module Users private + attr_reader :skip_authorization + def identity_attributes [:extern_uid, :provider] end @@ -102,7 +106,7 @@ module Users ] end - def build_user_params(skip_authorization:) + def build_user_params if current_user&.admin? user_params = params.slice(*admin_create_params) @@ -111,10 +115,10 @@ module Users end else allowed_signup_params = signup_params - allowed_signup_params << :skip_confirmation if skip_authorization + allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation? user_params = params.slice(*allowed_signup_params) - if user_params[:skip_confirmation].nil? + if assign_skip_confirmation_from_settings?(user_params) user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting end @@ -136,6 +140,14 @@ module Users user_params end + def allow_caller_to_request_skip_confirmation? + skip_authorization + end + + def assign_skip_confirmation_from_settings?(user_params) + user_params[:skip_confirmation].nil? + end + def skip_user_confirmation_email_from_setting !Gitlab::CurrentSettings.send_user_confirmation_email end @@ -150,4 +162,4 @@ module Users end end -Users::BuildService.prepend_if_ee('EE::Users::BuildService') +Users::BuildService.prepend_mod_with('Users::BuildService') diff --git a/app/services/users/create_service.rb b/app/services/users/create_service.rb index ec8b3cea664..757ebd783ee 100644 --- a/app/services/users/create_service.rb +++ b/app/services/users/create_service.rb @@ -26,4 +26,4 @@ module Users end end -Users::CreateService.prepend_if_ee('EE::Users::CreateService') +Users::CreateService.prepend_mod_with('Users::CreateService') diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 613d2e4ad82..4ec875098fa 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -31,7 +31,7 @@ module Users end if !delete_solo_owned_groups && user.solo_owned_groups.present? - user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user' + user.errors.add(:base, 'You must transfer ownership or delete groups before you can remove user') return user end @@ -73,4 +73,4 @@ module Users end end -Users::DestroyService.prepend_if_ee('EE::Users::DestroyService') +Users::DestroyService.prepend_mod_with('Users::DestroyService') diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb index 1b46edd4d7d..a471f55e644 100644 --- a/app/services/users/migrate_to_ghost_user_service.rb +++ b/app/services/users/migrate_to_ghost_user_service.rb @@ -93,4 +93,4 @@ module Users end end -Users::MigrateToGhostUserService.prepend_if_ee('EE::Users::MigrateToGhostUserService') +Users::MigrateToGhostUserService.prepend_mod_with('Users::MigrateToGhostUserService') diff --git a/app/services/users/registrations_build_service.rb b/app/services/users/registrations_build_service.rb new file mode 100644 index 00000000000..9d7bf0a7e18 --- /dev/null +++ b/app/services/users/registrations_build_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Users + class RegistrationsBuildService < BuildService + extend ::Gitlab::Utils::Override + + private + + override :allow_caller_to_request_skip_confirmation? + def allow_caller_to_request_skip_confirmation? + true + end + + override :assign_skip_confirmation_from_settings? + def assign_skip_confirmation_from_settings?(user_params) + user_params[:skip_confirmation].blank? + end + end +end diff --git a/app/services/users/reject_service.rb b/app/services/users/reject_service.rb index 0e3eb3e5dde..833c30d9427 100644 --- a/app/services/users/reject_service.rb +++ b/app/services/users/reject_service.rb @@ -39,4 +39,4 @@ module Users end end -Users::RejectService.prepend_if_ee('EE::Users::RejectService') +Users::RejectService.prepend_mod_with('Users::RejectService') diff --git a/app/services/users/update_assigned_open_issue_count_service.rb b/app/services/users/update_assigned_open_issue_count_service.rb new file mode 100644 index 00000000000..2ed05853b2f --- /dev/null +++ b/app/services/users/update_assigned_open_issue_count_service.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Users + # Service class for calculating and caching the number of assigned open issues for a user. + class UpdateAssignedOpenIssueCountService + attr_accessor :target_user + + def initialize(target_user:) + @target_user = target_user + + raise ArgumentError, "Please provide a target user" unless target_user.is_a?(User) + end + + def execute + value = calculate_count + Rails.cache.write(cache_key, value, expires_in: User::COUNT_CACHE_VALIDITY_PERIOD) + + ServiceResponse.success(payload: { count: value }) + rescue StandardError => e + ServiceResponse.error(message: e.message) + end + + private + + def cache_key + ['users', target_user.id, 'assigned_open_issues_count'] + end + + def calculate_count + IssuesFinder.new(target_user, assignee_id: target_user.id, state: 'opened', non_archived: true).execute.count + end + end +end diff --git a/app/services/users/update_canonical_email_service.rb b/app/services/users/update_canonical_email_service.rb index e75452f60fd..c4b7a98f60b 100644 --- a/app/services/users/update_canonical_email_service.rb +++ b/app/services/users/update_canonical_email_service.rb @@ -7,7 +7,7 @@ module Users INCLUDED_DOMAINS_PATTERN = [/gmail.com/].freeze def initialize(user:) - raise ArgumentError.new("Please provide a user") unless user.is_a?(User) + raise ArgumentError, "Please provide a user" unless user.is_a?(User) @user = user end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index b69720eefd6..ff08c806319 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -34,7 +34,7 @@ module Users def execute!(*args, &block) result = execute(*args, &block) - raise ActiveRecord::RecordInvalid.new(@user) unless result[:status] == :success + raise ActiveRecord::RecordInvalid, @user unless result[:status] == :success true end @@ -96,4 +96,4 @@ module Users end end -Users::UpdateService.prepend_if_ee('EE::Users::UpdateService') +Users::UpdateService.prepend_mod_with('Users::UpdateService') diff --git a/app/services/users/update_todo_count_cache_service.rb b/app/services/users/update_todo_count_cache_service.rb index 03ab66bd64a..3407b22e355 100644 --- a/app/services/users/update_todo_count_cache_service.rb +++ b/app/services/users/update_todo_count_cache_service.rb @@ -4,31 +4,34 @@ module Users class UpdateTodoCountCacheService < BaseService QUERY_BATCH_SIZE = 10 - attr_reader :users + attr_reader :user_ids - # users - An array of User objects - def initialize(users) - @users = users + # user_ids - An array of User IDs + def initialize(user_ids) + @user_ids = user_ids end def execute - users.each_slice(QUERY_BATCH_SIZE) do |users_batch| - todo_counts = Todo.for_user(users_batch).count_grouped_by_user_id_and_state + user_ids.each_slice(QUERY_BATCH_SIZE) do |user_ids_batch| + todo_counts = Todo.for_user(user_ids_batch).count_grouped_by_user_id_and_state - users_batch.each do |user| - update_count_cache(user, todo_counts, :done) - update_count_cache(user, todo_counts, :pending) + user_ids_batch.each do |user_id| + update_count_cache(user_id, todo_counts, :done) + update_count_cache(user_id, todo_counts, :pending) end end end private - def update_count_cache(user, todo_counts, state) - count = todo_counts.fetch([user.id, state.to_s], 0) - expiration_time = user.count_cache_validity_period + def update_count_cache(user_id, todo_counts, state) + count = todo_counts.fetch([user_id, state.to_s], 0) - Rails.cache.write(['users', user.id, "todos_#{state}_count"], count, expires_in: expiration_time) + Rails.cache.write( + ['users', user_id, "todos_#{state}_count"], + count, + expires_in: User::COUNT_CACHE_VALIDITY_PERIOD + ) end end end diff --git a/app/services/users/upsert_credit_card_validation_service.rb b/app/services/users/upsert_credit_card_validation_service.rb new file mode 100644 index 00000000000..70a96b3ec6b --- /dev/null +++ b/app/services/users/upsert_credit_card_validation_service.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Users + class UpsertCreditCardValidationService < BaseService + def initialize(params) + @params = params.to_h.with_indifferent_access + end + + def execute + ::Users::CreditCardValidation.upsert(@params) + + ServiceResponse.success(message: 'CreditCardValidation was set') + rescue ActiveRecord::InvalidForeignKey, ActiveRecord::NotNullViolation => e + ServiceResponse.error(message: "Could not set CreditCardValidation: #{e.message}") + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, params: @params, class: self.class.to_s) + ServiceResponse.error(message: "Could not set CreditCardValidation: #{e.message}") + end + end +end diff --git a/app/services/verify_pages_domain_service.rb b/app/services/verify_pages_domain_service.rb index a9e219547d7..eab1e91dc89 100644 --- a/app/services/verify_pages_domain_service.rb +++ b/app/services/verify_pages_domain_service.rb @@ -90,7 +90,7 @@ class VerifyPagesDomainService < BaseService records.any? do |record| record == domain.keyed_verification_code || record == domain.verification_code end - rescue => err + rescue StandardError => err log_error("Failed to check TXT records on #{domain_name} for #{domain.domain}: #{err}") false end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 5a51b42f9f9..654d9356739 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -6,6 +6,18 @@ class WebHookService attr_reader :body, :headers, :code + def success? + false + end + + def redirection? + false + end + + def internal_server_error? + true + end + def initialize @headers = Gitlab::HTTP::Response::Headers.new({}) @body = '' @@ -15,6 +27,7 @@ class WebHookService REQUEST_BODY_SIZE_LIMIT = 25.megabytes GITLAB_EVENT_HEADER = 'X-Gitlab-Event' + MAX_FAILURES = 100 attr_accessor :hook, :data, :hook_name, :request_options @@ -33,6 +46,8 @@ class WebHookService end def execute + return { status: :error, message: 'Hook disabled' } unless hook.executable? + start_time = Gitlab::Metrics::System.monotonic_time response = if parsed_url.userinfo.blank? @@ -76,7 +91,11 @@ class WebHookService end def async_execute - WebHookWorker.perform_async(hook.id, data, hook_name) + if rate_limited?(hook) + log_rate_limit(hook) + else + WebHookWorker.perform_async(hook.id, data, hook_name) + end end private @@ -104,6 +123,8 @@ class WebHookService end def log_execution(trigger:, url:, request_data:, response:, execution_duration:, error_message: nil) + handle_failure(response, hook) + WebHookLog.create( web_hook: hook, trigger: trigger, @@ -118,6 +139,17 @@ class WebHookService ) end + def handle_failure(response, hook) + if response.success? || response.redirection? + hook.enable! + elsif response.internal_server_error? + next_backoff = hook.next_backoff + hook.update!(disabled_until: next_backoff.from_now, backoff_count: hook.backoff_count + 1) + else + hook.update!(recent_failures: hook.recent_failures + 1) if hook.recent_failures < MAX_FAILURES + end + end + def build_headers(hook_name) @headers ||= begin { @@ -142,4 +174,34 @@ class WebHookService response.body.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') end + + def rate_limited?(hook) + return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) + return false if rate_limit.nil? + + Gitlab::ApplicationRateLimiter.throttled?( + :web_hook_calls, + scope: [hook], + threshold: rate_limit + ) + end + + def rate_limit + @rate_limit ||= hook.rate_limit + end + + def log_rate_limit(hook) + payload = { + message: 'Webhook rate limit exceeded', + hook_id: hook.id, + hook_type: hook.type, + hook_name: hook_name + } + + Gitlab::AuthLogger.error(payload) + + # Also log into application log for now, so we can use this information + # to determine suitable limits for gitlab.com + Gitlab::AppLogger.error(payload) + end end diff --git a/app/services/wiki_pages/base_service.rb b/app/services/wiki_pages/base_service.rb index fd234630633..4ec884469eb 100644 --- a/app/services/wiki_pages/base_service.rb +++ b/app/services/wiki_pages/base_service.rb @@ -61,4 +61,4 @@ module WikiPages end end -WikiPages::BaseService.prepend_if_ee('EE::WikiPages::BaseService') +WikiPages::BaseService.prepend_mod_with('WikiPages::BaseService') diff --git a/app/services/wiki_pages/update_service.rb b/app/services/wiki_pages/update_service.rb index f2fc6b37c34..88275f8c417 100644 --- a/app/services/wiki_pages/update_service.rb +++ b/app/services/wiki_pages/update_service.rb @@ -2,6 +2,8 @@ module WikiPages class UpdateService < WikiPages::BaseService + UpdateError = Class.new(StandardError) + def execute(page) # this class is not thread safe! @old_slug = page.slug @@ -10,11 +12,15 @@ module WikiPages execute_hooks(page) ServiceResponse.success(payload: { page: page }) else - ServiceResponse.error( - message: _('Could not update wiki page'), - payload: { page: page } - ) + raise UpdateError, s_('Could not update wiki page') end + rescue UpdateError, WikiPage::PageChangedError, WikiPage::PageRenameError => e + page.update_attributes(@params) # rubocop:disable Rails/ActiveRecordAliases + + ServiceResponse.error( + message: e.message, + payload: { page: page } + ) end def usage_counter_action |