diff options
65 files changed, 1586 insertions, 90 deletions
diff --git a/app/assets/javascripts/confirm_danger_modal.js b/app/assets/javascripts/confirm_danger_modal.js index 5b9e70e3c09..ad70d9be16f 100644 --- a/app/assets/javascripts/confirm_danger_modal.js +++ b/app/assets/javascripts/confirm_danger_modal.js @@ -3,14 +3,14 @@ import { Rails } from '~/lib/utils/rails_ujs'; import { rstrip } from './lib/utils/common_utils'; function openConfirmDangerModal($form, $modal, text) { - const $input = $('.js-confirm-danger-input', $modal); + const $input = $('.js-legacy-confirm-danger-input', $modal); $input.val(''); $('.js-confirm-text', $modal).text(text || ''); $modal.modal('show'); - const confirmTextMatch = $('.js-confirm-danger-match', $modal).text(); - const $submit = $('.js-confirm-danger-submit', $modal); + const confirmTextMatch = $('.js-legacy-confirm-danger-match', $modal).text(); + const $submit = $('.js-legacy-confirm-danger-submit', $modal); $submit.disable(); $input.focus(); @@ -25,7 +25,7 @@ function openConfirmDangerModal($form, $modal, text) { }); // eslint-disable-next-line @gitlab/no-global-event-off - $('.js-confirm-danger-submit', $modal) + $('.js-legacy-confirm-danger-submit', $modal) .off('click') .on('click', () => { if ($form.data('remote')) { @@ -47,7 +47,7 @@ function getModal($btn) { } export default function initConfirmDangerModal() { - $(document).on('click', '.js-confirm-danger', (e) => { + $(document).on('click', '.js-legacy-confirm-danger', (e) => { const $btn = $(e.target); const checkFieldName = $btn.data('checkFieldName'); const checkFieldCompareValue = $btn.data('checkCompareValue'); diff --git a/app/assets/javascripts/init_confirm_danger.js b/app/assets/javascripts/init_confirm_danger.js new file mode 100644 index 00000000000..3e7f60bc237 --- /dev/null +++ b/app/assets/javascripts/init_confirm_danger.js @@ -0,0 +1,23 @@ +import Vue from 'vue'; +import ConfirmDanger from './vue_shared/components/confirm_danger/confirm_danger.vue'; + +export default () => { + const el = document.querySelector('.js-confirm-danger'); + if (!el) return null; + + const { phrase, buttonText, confirmDangerMessage } = el.dataset; + + return new Vue({ + el, + render: (createElement) => + createElement(ConfirmDanger, { + props: { + phrase, + buttonText, + }, + provide: { + confirmDangerMessage, + }, + }), + }); +}; diff --git a/app/assets/javascripts/monitoring/components/charts/empty_chart.vue b/app/assets/javascripts/monitoring/components/charts/empty_chart.vue index 4b54cffe231..ae079da0b0b 100644 --- a/app/assets/javascripts/monitoring/components/charts/empty_chart.vue +++ b/app/assets/javascripts/monitoring/components/charts/empty_chart.vue @@ -1,8 +1,12 @@ <script> import chartEmptyStateIllustration from '@gitlab/svgs/dist/illustrations/chart-empty-state.svg'; +import { GlSafeHtmlDirective } from '@gitlab/ui'; import { chartHeight } from '../../constants'; export default { + directives: { + SafeHtml: GlSafeHtmlDirective, + }, data() { return { height: chartHeight, @@ -18,14 +22,15 @@ export default { created() { this.chartEmptyStateIllustration = chartEmptyStateIllustration; }, + safeHtmlConfig: { ADD_TAGS: ['use'] }, }; </script> <template> <div class="d-flex flex-column justify-content-center"> <div + v-safe-html:[$options.safeHtmlConfig]="chartEmptyStateIllustration" class="gl-mt-3 svg-w-100 d-flex align-items-center" :style="svgContainerStyle" - v-html="chartEmptyStateIllustration /* eslint-disable-line vue/no-v-html */" ></div> <h5 class="text-center gl-mt-3">{{ __('No data to display') }}</h5> </div> diff --git a/app/assets/javascripts/pages/projects/edit/index.js b/app/assets/javascripts/pages/projects/edit/index.js index 335d8d481fc..38351d30a8d 100644 --- a/app/assets/javascripts/pages/projects/edit/index.js +++ b/app/assets/javascripts/pages/projects/edit/index.js @@ -1,5 +1,5 @@ import { PROJECT_BADGE } from '~/badges/constants'; -import initConfirmDangerModal from '~/confirm_danger_modal'; +import initLegacyConfirmDangerModal from '~/confirm_danger_modal'; import dirtySubmitFactory from '~/dirty_submit/dirty_submit_factory'; import initFilePickers from '~/file_pickers'; import mountBadgeSettings from '~/pages/shared/mount_badge_settings'; @@ -13,7 +13,7 @@ import initProjectPermissionsSettings from '../shared/permissions'; import initProjectLoadingSpinner from '../shared/save_project_loader'; initFilePickers(); -initConfirmDangerModal(); +initLegacyConfirmDangerModal(); initSettingsPanels(); initProjectDeleteButton(); mountBadgeSettings(PROJECT_BADGE); diff --git a/app/assets/javascripts/vue_shared/components/confirm_danger/confirm_danger.vue b/app/assets/javascripts/vue_shared/components/confirm_danger/confirm_danger.vue new file mode 100644 index 00000000000..294450851c5 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/confirm_danger/confirm_danger.vue @@ -0,0 +1,49 @@ +<script> +import { GlButton, GlModalDirective } from '@gitlab/ui'; +import { CONFIRM_DANGER_MODAL_ID } from './constants'; +import ConfirmDangerModal from './confirm_danger_modal.vue'; + +export default { + name: 'ConfirmDanger', + components: { + GlButton, + ConfirmDangerModal, + }, + directives: { + GlModal: GlModalDirective, + }, + props: { + disabled: { + type: Boolean, + required: false, + default: false, + }, + phrase: { + type: String, + required: true, + }, + buttonText: { + type: String, + required: true, + }, + }, + modalId: CONFIRM_DANGER_MODAL_ID, +}; +</script> +<template> + <div> + <gl-button + v-gl-modal="$options.modalId" + class="gl-button" + variant="danger" + :disabled="disabled" + data-testid="confirm-danger-button" + >{{ buttonText }}</gl-button + > + <confirm-danger-modal + :modal-id="$options.modalId" + :phrase="phrase" + @confirm="$emit('confirm')" + /> + </div> +</template> diff --git a/app/assets/javascripts/vue_shared/components/confirm_danger/confirm_danger_modal.vue b/app/assets/javascripts/vue_shared/components/confirm_danger/confirm_danger_modal.vue new file mode 100644 index 00000000000..ebb1506895e --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/confirm_danger/confirm_danger_modal.vue @@ -0,0 +1,89 @@ +<script> +import { GlModal, GlFormGroup, GlFormInput, GlSprintf } from '@gitlab/ui'; +import { + CONFIRM_DANGER_MODAL_BUTTON, + CONFIRM_DANGER_MODAL_TITLE, + CONFIRM_DANGER_PHRASE_TEXT, + CONFIRM_DANGER_WARNING, +} from './constants'; + +export default { + name: 'ConfirmDangerModal', + components: { + GlModal, + GlFormGroup, + GlFormInput, + GlSprintf, + }, + inject: { + confirmDangerMessage: { + default: '', + }, + confirmButtonText: { + default: CONFIRM_DANGER_MODAL_BUTTON, + }, + }, + props: { + modalId: { + type: String, + required: true, + }, + phrase: { + type: String, + required: true, + }, + }, + data() { + return { confirmationPhrase: '' }; + }, + computed: { + isValid() { + return ( + this.confirmationPhrase.length && this.equalString(this.confirmationPhrase, this.phrase) + ); + }, + actionPrimary() { + return { + text: this.confirmButtonText, + attributes: [{ variant: 'danger', disabled: !this.isValid }], + }; + }, + }, + methods: { + equalString(a, b) { + return a.trim().toLowerCase() === b.trim().toLowerCase(); + }, + }, + i18n: { + CONFIRM_DANGER_MODAL_BUTTON, + CONFIRM_DANGER_MODAL_TITLE, + CONFIRM_DANGER_WARNING, + CONFIRM_DANGER_PHRASE_TEXT, + }, +}; +</script> +<template> + <gl-modal + ref="modal" + :modal-id="modalId" + :data-testid="modalId" + :title="$options.i18n.CONFIRM_DANGER_MODAL_TITLE" + :action-primary="actionPrimary" + @primary="$emit('confirm')" + > + <p v-if="confirmDangerMessage" class="text-danger" data-testid="confirm-danger-message"> + {{ confirmDangerMessage }} + </p> + <p data-testid="confirm-danger-warning">{{ $options.i18n.CONFIRM_DANGER_WARNING }}</p> + <p data-testid="confirm-danger-phrase"> + <gl-sprintf :message="$options.i18n.CONFIRM_DANGER_PHRASE_TEXT"> + <template #phrase_code> + <code>{{ phrase }}</code> + </template> + </gl-sprintf> + </p> + <gl-form-group class="form-control" :state="isValid"> + <gl-form-input v-model="confirmationPhrase" data-testid="confirm-danger-input" type="text" /> + </gl-form-group> + </gl-modal> +</template> diff --git a/app/assets/javascripts/vue_shared/components/confirm_danger/constants.js b/app/assets/javascripts/vue_shared/components/confirm_danger/constants.js new file mode 100644 index 00000000000..ddb381e87a3 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/confirm_danger/constants.js @@ -0,0 +1,11 @@ +import { __ } from '~/locale'; + +export const CONFIRM_DANGER_MODAL_ID = 'confirm-danger-modal'; +export const CONFIRM_DANGER_MODAL_TITLE = __('Confirmation required'); +export const CONFIRM_DANGER_MODAL_BUTTON = __('Confirm'); +export const CONFIRM_DANGER_WARNING = __( + 'This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention.', +); +export const CONFIRM_DANGER_PHRASE_TEXT = __( + 'Please type %{phrase_code} to proceed or close this modal to cancel.', +); diff --git a/app/models/concerns/loose_foreign_key.rb b/app/models/concerns/loose_foreign_key.rb index 8bbd6980d4d..102292672b3 100644 --- a/app/models/concerns/loose_foreign_key.rb +++ b/app/models/concerns/loose_foreign_key.rb @@ -7,20 +7,18 @@ module LooseForeignKey # Loose foreign keys allow delayed processing of associated database records # with similar guarantees than a database foreign key. # - # TODO: finalize this later once the async job is in place - # # Prerequisites: # # To start using the concern, you'll need to install a database trigger to the parent # table in a standard DB migration (not post-migration). # - # > add_loose_foreign_key_support(:projects, :gitlab_main) + # > track_record_deletions(:projects) # # Usage: # # > class Ci::Build < ApplicationRecord # > - # > loose_foreign_key :security_scans, :build_id, on_delete: :async_delete, gitlab_schema: :gitlab_main + # > loose_foreign_key :security_scans, :build_id, on_delete: :async_delete # > # > # associations can be still defined, the dependent options is no longer necessary: # > has_many :security_scans, class_name: 'Security::Scan' @@ -32,14 +30,6 @@ module LooseForeignKey # - :async_delete - deletes the children rows via an asynchronous process. # - :async_nullify - sets the foreign key column to null via an asynchronous process. # - # Options for gitlab_schema: - # - # - :gitlab_ci - # - :gitlab_main - # - # The value can be determined by calling `Model.gitlab_schema` where the Model represents - # the model for the child table. - # # How it works: # # When adding loose foreign key support to the table, a DELETE trigger is installed @@ -69,23 +59,17 @@ module LooseForeignKey end on_delete_options = %i[async_delete async_nullify] - gitlab_schema_options = %i(gitlab_main gitlab_ci) unless on_delete_options.include?(symbolized_options[:on_delete]&.to_sym) raise "Invalid on_delete option given: #{symbolized_options[:on_delete]}. Valid options: #{on_delete_options.join(', ')}" end - unless gitlab_schema_options.include?(symbolized_options[:gitlab_schema]&.to_sym) - raise "Invalid gitlab_schema option given: #{symbolized_options[:gitlab_schema]}. Valid options: #{gitlab_schema_options.join(', ')}" - end - definition = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( table_name.to_s, to_table.to_s, { column: column.to_s, - on_delete: symbolized_options[:on_delete].to_sym, - gitlab_schema: symbolized_options[:gitlab_schema].to_sym + on_delete: symbolized_options[:on_delete].to_sym } ) diff --git a/app/models/integrations/zentao.rb b/app/models/integrations/zentao.rb index 2b3e5b6001f..ab6242a649b 100644 --- a/app/models/integrations/zentao.rb +++ b/app/models/integrations/zentao.rb @@ -66,7 +66,8 @@ module Integrations type: 'password', name: 'api_token', title: s_('ZentaoIntegration|ZenTao API token'), - non_empty_password_title: s_('ZentaoIntegration|Enter API token'), + non_empty_password_title: s_('ZentaoIntegration|Enter new ZenTao API token'), + non_empty_password_help: s_('ProjectService|Leave blank to use your current token.'), required: true }, { diff --git a/app/models/loose_foreign_keys/deleted_record.rb b/app/models/loose_foreign_keys/deleted_record.rb index ca5a2800a03..01d6d068ca4 100644 --- a/app/models/loose_foreign_keys/deleted_record.rb +++ b/app/models/loose_foreign_keys/deleted_record.rb @@ -1,5 +1,32 @@ # frozen_string_literal: true class LooseForeignKeys::DeletedRecord < ApplicationRecord - extend SuppressCompositePrimaryKeyWarning + self.primary_key = :id + + scope :for_table, -> (table) { where(fully_qualified_table_name: table) } + scope :ordered_by_id, -> { order(:id, :primary_key_value) } + # This needs to be parameterized once we start adding partitions + scope :for_partition, -> { where(partition: 1) } + + enum status: { pending: 1, processed: 2 }, _prefix: :status + + def self.load_batch_for_table(table, batch_size) + for_table(table) + .for_partition + .status_pending + .ordered_by_id + .limit(batch_size) + .to_a + end + + def self.mark_records_processed_for_table_between(table, from_record, to_record) + from = from_record.id + to = to_record.id + + for_table(table) + .for_partition + .status_pending + .where(id: from..to) + .update_all(status: :processed) + end end diff --git a/app/models/loose_foreign_keys/modification_tracker.rb b/app/models/loose_foreign_keys/modification_tracker.rb new file mode 100644 index 00000000000..12f3156b98c --- /dev/null +++ b/app/models/loose_foreign_keys/modification_tracker.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module LooseForeignKeys + class ModificationTracker + MAX_DELETES = 100_000 + MAX_UPDATES = 50_000 + MAX_RUNTIME = 3.minutes + + delegate :monotonic_time, to: :'Gitlab::Metrics::System' + + def initialize + @delete_count_by_table = Hash.new { |h, k| h[k] = 0 } + @update_count_by_table = Hash.new { |h, k| h[k] = 0 } + @start_time = monotonic_time + end + + def add_deletions(table, count) + @delete_count_by_table[table] += count + end + + def add_updates(table, count) + @update_count_by_table[table] += count + end + + def over_limit? + @delete_count_by_table.values.sum >= MAX_DELETES || + @update_count_by_table.values.sum >= MAX_UPDATES || + monotonic_time - @start_time >= MAX_RUNTIME + end + + def stats + { + over_limit: over_limit?, + delete_count_by_table: @delete_count_by_table, + update_count_by_table: @update_count_by_table, + delete_count: @delete_count_by_table.values.sum, + update_count: @update_count_by_table.values.sum + } + end + end +end diff --git a/app/models/release.rb b/app/models/release.rb index eac6346cc60..0fda6940249 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -34,6 +34,7 @@ class Release < ApplicationRecord project: [:project_feature, :route, { namespace: :route }]) } scope :with_milestones, -> { joins(:milestone_releases) } + scope :with_group_milestones, -> { joins(:milestones).where.not(milestones: { group_id: nil }) } scope :recent, -> { sorted.limit(MAX_NUMBER_TO_DISPLAY) } scope :without_evidence, -> { left_joins(:evidences).where(::Releases::Evidence.arel_table[:id].eq(nil)) } scope :released_within_2hrs, -> { where(released_at: Time.zone.now - 1.hour..Time.zone.now + 1.hour) } diff --git a/app/services/loose_foreign_keys/batch_cleaner_service.rb b/app/services/loose_foreign_keys/batch_cleaner_service.rb new file mode 100644 index 00000000000..a623524b2b1 --- /dev/null +++ b/app/services/loose_foreign_keys/batch_cleaner_service.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module LooseForeignKeys + class BatchCleanerService + def initialize(parent_klass:, deleted_parent_records:, modification_tracker: LooseForeignKeys::ModificationTracker.new, models_by_table_name:) + @parent_klass = parent_klass + @deleted_parent_records = deleted_parent_records + @modification_tracker = modification_tracker + @models_by_table_name = models_by_table_name + end + + def execute + parent_klass.loose_foreign_key_definitions.each do |foreign_key_definition| + run_cleaner_service(foreign_key_definition, with_skip_locked: true) + break if modification_tracker.over_limit? + + run_cleaner_service(foreign_key_definition, with_skip_locked: false) + break if modification_tracker.over_limit? + end + + return if modification_tracker.over_limit? + + # At this point, all associations are cleaned up, we can update the status of the parent records + LooseForeignKeys::DeletedRecord + .mark_records_processed_for_table_between(deleted_parent_records.first.fully_qualified_table_name, deleted_parent_records.first, deleted_parent_records.last) + end + + private + + attr_reader :parent_klass, :deleted_parent_records, :modification_tracker, :models_by_table_name + + def record_result(cleaner, result) + if cleaner.async_delete? + modification_tracker.add_deletions(result[:table], result[:affected_rows]) + elsif cleaner.async_nullify? + modification_tracker.add_updates(result[:table], result[:affected_rows]) + end + end + + def run_cleaner_service(foreign_key_definition, with_skip_locked:) + cleaner = CleanerService.new( + model: models_by_table_name.fetch(foreign_key_definition.to_table), + foreign_key_definition: foreign_key_definition, + deleted_parent_records: deleted_parent_records, + with_skip_locked: with_skip_locked + ) + + loop do + result = cleaner.execute + record_result(cleaner, result) + + break if modification_tracker.over_limit? || result[:affected_rows] == 0 + end + end + end +end diff --git a/app/services/loose_foreign_keys/cleaner_service.rb b/app/services/loose_foreign_keys/cleaner_service.rb new file mode 100644 index 00000000000..8fe053e2edf --- /dev/null +++ b/app/services/loose_foreign_keys/cleaner_service.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module LooseForeignKeys + # rubocop: disable CodeReuse/ActiveRecord + class CleanerService + DELETE_LIMIT = 1000 + UPDATE_LIMIT = 500 + + delegate :connection, to: :model + + def initialize(model:, foreign_key_definition:, deleted_parent_records:, with_skip_locked: false) + @model = model + @foreign_key_definition = foreign_key_definition + @deleted_parent_records = deleted_parent_records + @with_skip_locked = with_skip_locked + end + + def execute + result = connection.execute(build_query) + + { affected_rows: result.cmd_tuples, table: foreign_key_definition.to_table } + end + + def async_delete? + foreign_key_definition.on_delete == :async_delete + end + + def async_nullify? + foreign_key_definition.on_delete == :async_nullify + end + + private + + attr_reader :model, :foreign_key_definition, :deleted_parent_records, :with_skip_locked + + def build_query + query = if async_delete? + delete_query + elsif async_nullify? + update_query + else + raise "Invalid on_delete argument: #{foreign_key_definition.on_delete}" + end + + unless query.include?(%{"#{foreign_key_definition.column}" IN (}) + raise("FATAL: foreign key condition is missing from the generated query: #{query}") + end + + query + end + + def arel_table + @arel_table ||= model.arel_table + end + + def primary_keys + @primary_keys ||= connection.primary_keys(model.table_name).map { |key| arel_table[key] } + end + + def quoted_table_name + @quoted_table_name ||= Arel.sql(connection.quote_table_name(model.table_name)) + end + + def delete_query + query = Arel::DeleteManager.new + query.from(quoted_table_name) + + add_in_query_with_limit(query, DELETE_LIMIT) + end + + def update_query + query = Arel::UpdateManager.new + query.table(quoted_table_name) + query.set([[arel_table[foreign_key_definition.column], nil]]) + + add_in_query_with_limit(query, UPDATE_LIMIT) + end + + # IN query with one or composite primary key + # WHERE (primary_key1, primary_key2) IN (subselect) + def add_in_query_with_limit(query, limit) + columns = Arel::Nodes::Grouping.new(primary_keys) + query.where(columns.in(in_query_with_limit(limit))).to_sql + end + + # Builds the following sub-query + # SELECT primary_keys FROM table WHERE foreign_key IN (1, 2, 3) LIMIT N + def in_query_with_limit(limit) + in_query = Arel::SelectManager.new + in_query.from(quoted_table_name) + in_query.where(arel_table[foreign_key_definition.column].in(deleted_parent_records.map(&:primary_key_value))) + in_query.projections = primary_keys + in_query.take(limit) + in_query.lock(Arel.sql('FOR UPDATE SKIP LOCKED')) if with_skip_locked + in_query + end + end + # rubocop: enable CodeReuse/ActiveRecord +end diff --git a/app/services/loose_foreign_keys/process_deleted_records_service.rb b/app/services/loose_foreign_keys/process_deleted_records_service.rb new file mode 100644 index 00000000000..735fc8a2415 --- /dev/null +++ b/app/services/loose_foreign_keys/process_deleted_records_service.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +module LooseForeignKeys + class ProcessDeletedRecordsService + BATCH_SIZE = 1000 + + def initialize(connection:) + @connection = connection + end + + def execute + modification_tracker = ModificationTracker.new + + tracked_tables.cycle do |table| + records = load_batch_for_table(table) + + if records.empty? + tracked_tables.delete(table) + next + end + + break if modification_tracker.over_limit? + + model = find_parent_model!(table) + + LooseForeignKeys::BatchCleanerService + .new(parent_klass: model, + deleted_parent_records: records, + modification_tracker: modification_tracker, + models_by_table_name: models_by_table_name) + .execute + + break if modification_tracker.over_limit? + end + + modification_tracker.stats + end + + private + + attr_reader :connection + + def load_batch_for_table(table) + fully_qualified_table_name = "#{current_schema}.#{table}" + LooseForeignKeys::DeletedRecord.load_batch_for_table(fully_qualified_table_name, BATCH_SIZE) + end + + def find_parent_model!(table) + models_by_table_name.fetch(table) + end + + def current_schema + @current_schema = connection.current_schema + end + + def tracked_tables + @tracked_tables ||= models_by_table_name + .select { |table_name, model| model.respond_to?(:loose_foreign_key_definitions) } + .keys + end + + def models_by_table_name + @models_by_table_name ||= begin + all_models + .select(&:base_class?) + .index_by(&:table_name) + end + end + + def all_models + ApplicationRecord.descendants + end + end +end diff --git a/app/views/groups/settings/_remove_button.html.haml b/app/views/groups/settings/_remove_button.html.haml index a04dba68b92..75413a2975e 100644 --- a/app/views/groups/settings/_remove_button.html.haml +++ b/app/views/groups/settings/_remove_button.html.haml @@ -4,4 +4,4 @@ .gl-alert-body = html_escape(_("This group can't be removed because it is linked to a subscription. To remove this group, %{linkStart}link the subscription%{linkEnd} with a different group.")) % { linkStart: "<a href=\"#{help_page_path('subscriptions/index', anchor: 'change-the-linked-namespace')}\">".html_safe, linkEnd: '</a>'.html_safe } -= button_to _('Remove group'), '#', class: ['btn gl-button btn-danger js-confirm-danger', ('disabled' if group.paid?)], data: { 'confirm-danger-message' => remove_group_message(group), 'testid' => 'remove-group-button' } += button_to _('Remove group'), '#', class: ['btn gl-button btn-danger js-legacy-confirm-danger', ('disabled' if group.paid?)], data: { 'confirm-danger-message' => remove_group_message(group), 'testid' => 'remove-group-button' } diff --git a/app/views/projects/_new_project_fields.html.haml b/app/views/projects/_new_project_fields.html.haml index 256c3ebad0a..5e20e21e314 100644 --- a/app/views/projects/_new_project_fields.html.haml +++ b/app/views/projects/_new_project_fields.html.haml @@ -72,7 +72,7 @@ - e.try do .form-group .form-check.gl-mb-3 - = check_box_tag 'project[initialize_with_sast]', '1', true, class: 'form-check-input', data: { track_experiment: e.name, track_label: track_label, track_action: 'activate_form_input', track_property: 'init_with_sast' } + = check_box_tag 'project[initialize_with_sast]', '1', true, class: 'form-check-input', data: { qa_selector: 'initialize_with_sast_checkbox', track_experiment: e.name, track_label: track_label, track_action: 'activate_form_input', track_property: 'init_with_sast' } = label_tag 'project[initialize_with_sast]', class: 'form-check-label' do = s_('ProjectsNew|Enable Static Application Security Testing (SAST)') .form-text.text-muted @@ -81,7 +81,7 @@ - e.try(:free_indicator) do .form-group .form-check.gl-mb-3 - = check_box_tag 'project[initialize_with_sast]', '1', true, class: 'form-check-input', data: { track_experiment: e.name, track_label: track_label, track_action: 'activate_form_input', track_property: 'init_with_sast' } + = check_box_tag 'project[initialize_with_sast]', '1', true, class: 'form-check-input', data: { qa_selector: 'initialize_with_sast_checkbox', track_experiment: e.name, track_label: track_label, track_action: 'activate_form_input', track_property: 'init_with_sast' } = label_tag 'project[initialize_with_sast]', class: 'form-check-label' do = s_('ProjectsNew|Enable Static Application Security Testing (SAST)') %span.badge.badge-info.badge-pill.gl-badge.sm= _('Free') diff --git a/app/views/projects/_remove_fork.html.haml b/app/views/projects/_remove_fork.html.haml index 8fa21966683..92eb29dc407 100644 --- a/app/views/projects/_remove_fork.html.haml +++ b/app/views/projects/_remove_fork.html.haml @@ -8,4 +8,4 @@ %p %strong= _('Once removed, the fork relationship cannot be restored. This project will no longer be able to receive or send merge requests to the source project or other forks.') = link_to _('Learn more.'), help_page_path('user/project/settings/index', anchor: 'removing-a-fork-relationship'), target: '_blank', rel: 'noopener noreferrer' - = button_to _('Remove fork relationship'), '#', class: "gl-button btn btn-danger js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_warning_message(@project) } + = button_to _('Remove fork relationship'), '#', class: "gl-button btn btn-danger js-legacy-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_warning_message(@project) } diff --git a/app/views/projects/_transfer.html.haml b/app/views/projects/_transfer.html.haml index ee717c2deca..e48008e1cc6 100644 --- a/app/views/projects/_transfer.html.haml +++ b/app/views/projects/_transfer.html.haml @@ -14,4 +14,4 @@ = label_tag :new_namespace_id, _('Select a new namespace'), class: 'gl-font-weight-bold' .form-group = select_tag :new_namespace_id, namespaces_options(nil), include_blank: true, class: 'select2' - = f.submit 'Transfer project', class: "gl-button btn btn-danger js-confirm-danger qa-transfer-button", data: { "confirm-danger-message" => transfer_project_message(@project) } + = f.submit 'Transfer project', class: "gl-button btn btn-danger js-legacy-confirm-danger qa-transfer-button", data: { "confirm-danger-message" => transfer_project_message(@project) } diff --git a/app/views/projects/_visibility_modal.html.haml b/app/views/projects/_visibility_modal.html.haml index f75216a71b6..db98b978f04 100644 --- a/app/views/projects/_visibility_modal.html.haml +++ b/app/views/projects/_visibility_modal.html.haml @@ -20,10 +20,10 @@ %li = _("Current forks will keep their visibility level.").html_safe %label{ for: "confirm_path_input" } - = _("To confirm, type %{phrase_code}").html_safe % { phrase_code: '<code class="js-confirm-danger-match">%{phrase_name}</code>'.html_safe % { phrase_name: @project.full_path } } + = _("To confirm, type %{phrase_code}").html_safe % { phrase_code: '<code class="js-legacy-confirm-danger-match">%{phrase_name}</code>'.html_safe % { phrase_name: @project.full_path } } .form-group - = text_field_tag 'confirm_path_input', '', class: 'form-control js-confirm-danger-input qa-confirm-input' + = text_field_tag 'confirm_path_input', '', class: 'form-control js-legacy-confirm-danger-input qa-confirm-input' .form-actions %button.btn.gl-button.btn-default.gl-mr-4{ type: "button", "data-dismiss": "modal" } = _('Cancel') - = submit_tag _('Reduce project visibility'), class: "btn gl-button btn-danger js-confirm-danger-submit qa-confirm-button", disabled: true + = submit_tag _('Reduce project visibility'), class: "btn gl-button btn-danger js-legacy-confirm-danger-submit qa-confirm-button", disabled: true diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 926a0610577..6421aef14cf 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -23,7 +23,7 @@ .js-project-permissions-form - if show_visibility_confirm_modal?(@project) = render "visibility_modal" - = f.submit _('Save changes'), class: "btn gl-button btn-confirm #{('js-confirm-danger' if show_visibility_confirm_modal?(@project))}", data: { qa_selector: 'visibility_features_permissions_save_button', check_field_name: ("project[visibility_level]" if show_visibility_confirm_modal?(@project)), check_compare_value: @project.visibility_level } + = f.submit _('Save changes'), class: "btn gl-button btn-confirm #{('js-legacy-confirm-danger' if show_visibility_confirm_modal?(@project))}", data: { qa_selector: 'visibility_features_permissions_save_button', check_field_name: ("project[visibility_level]" if show_visibility_confirm_modal?(@project)), check_compare_value: @project.visibility_level } %section.rspec-merge-request-settings.settings.merge-requests-feature.no-animate#js-merge-request-settings{ class: [('expanded' if expanded), ('hidden' if @project.project_feature.send(:merge_requests_access_level) == 0)], data: { qa_selector: 'merge_request_settings_content' } } .settings-header diff --git a/app/views/shared/_confirm_modal.html.haml b/app/views/shared/_confirm_modal.html.haml index 8b13bb948ee..4cb3f6d1739 100644 --- a/app/views/shared/_confirm_modal.html.haml +++ b/app/views/shared/_confirm_modal.html.haml @@ -12,10 +12,10 @@ %p %span.js-warning-text= _('This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention.') %br - - phrase_code = '<code class="js-confirm-danger-match">%{phrase_name}</code>'.html_safe % { phrase_name: phrase } + - phrase_code = '<code class="js-legacy-confirm-danger-match">%{phrase_name}</code>'.html_safe % { phrase_name: phrase } = _('Please type %{phrase_code} to proceed or close this modal to cancel.').html_safe % { phrase_code: phrase_code } .form-group - = text_field_tag 'confirm_name_input', '', class: 'form-control js-confirm-danger-input qa-confirm-input' + = text_field_tag 'confirm_name_input', '', class: 'form-control js-legacy-confirm-danger-input qa-confirm-input' .form-actions - = submit_tag _('Confirm'), class: "gl-button btn btn-danger js-confirm-danger-submit qa-confirm-button" + = submit_tag _('Confirm'), class: "gl-button btn btn-danger js-legacy-confirm-danger-submit qa-confirm-button" diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 2bb961bb8c6..2f33d154d99 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -363,6 +363,15 @@ :weight: 1 :idempotent: :tags: [] +- :name: cronjob:loose_foreign_keys_cleanup + :worker_name: LooseForeignKeys::CleanupWorker + :feature_category: :sharding + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:member_invitation_reminder_emails :worker_name: MemberInvitationReminderEmailsWorker :feature_category: :subgroups diff --git a/app/workers/loose_foreign_keys/cleanup_worker.rb b/app/workers/loose_foreign_keys/cleanup_worker.rb new file mode 100644 index 00000000000..4df2d26ed16 --- /dev/null +++ b/app/workers/loose_foreign_keys/cleanup_worker.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module LooseForeignKeys + class CleanupWorker + include ApplicationWorker + include Gitlab::ExclusiveLeaseHelpers + include CronjobQueue # rubocop: disable Scalability/CronWorkerContext + + feature_category :sharding + data_consistency :always + idempotent! + + def perform + return if Feature.disabled?(:loose_foreign_key_cleanup, default_enabled: :yaml) + + in_lock(self.class.name.underscore, ttl: 1.hour, retries: 0) do + # TODO: Iterate over the connections + # https://gitlab.com/gitlab-org/gitlab/-/issues/341513 + stats = ProcessDeletedRecordsService.new(connection: ApplicationRecord.connection).execute + log_extra_metadata_on_done(:stats, stats) + end + end + end +end diff --git a/config/feature_flags/development/loose_foreign_key_cleanup.yml b/config/feature_flags/development/loose_foreign_key_cleanup.yml new file mode 100644 index 00000000000..a5ed9192a19 --- /dev/null +++ b/config/feature_flags/development/loose_foreign_key_cleanup.yml @@ -0,0 +1,8 @@ +--- +name: loose_foreign_key_cleanup +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69165 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/343545 +milestone: '14.4' +type: development +group: group::sharding +default_enabled: false diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 0e4e6f5cc84..b871aa92c7f 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -714,6 +714,9 @@ Gitlab.ee do Settings.cron_jobs['app_sec_dast_profile_schedule_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['app_sec_dast_profile_schedule_worker']['cron'] ||= '7-59/15 * * * *' Settings.cron_jobs['app_sec_dast_profile_schedule_worker']['job_class'] = 'AppSec::Dast::ProfileScheduleWorker' + Settings.cron_jobs['loose_foreign_keys_cleanup_worker'] ||= Settingslogic.new({}) + Settings.cron_jobs['loose_foreign_keys_cleanup_worker']['cron'] ||= '*/5 * * * *' + Settings.cron_jobs['loose_foreign_keys_cleanup_worker']['job_class'] = 'LooseForeignKeys::CleanupWorker' end # diff --git a/config/metrics/counts_28d/20210216183203_product_analytics_test_metrics_union.yml b/config/metrics/counts_28d/20210216183203_product_analytics_test_metrics_union.yml index 924a19cae74..8ea92369e0b 100644 --- a/config/metrics/counts_28d/20210216183203_product_analytics_test_metrics_union.yml +++ b/config/metrics/counts_28d/20210216183203_product_analytics_test_metrics_union.yml @@ -11,7 +11,7 @@ status: removed milestone_removed: '13.11' milestone: '13.7' introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49886 -time_frame: 7d +time_frame: 28d data_source: redis_hll distribution: - ce diff --git a/config/metrics/counts_28d/20210216183648_github.yml b/config/metrics/counts_28d/20210216183648_github.yml index efa4cca301f..c33cc1809af 100644 --- a/config/metrics/counts_28d/20210216183648_github.yml +++ b/config/metrics/counts_28d/20210216183648_github.yml @@ -8,7 +8,7 @@ product_group: group::import product_category: importers value_type: number status: active -time_frame: all +time_frame: 28d data_source: database distribution: - ce diff --git a/config/metrics/counts_28d/20210216183650_bitbucket.yml b/config/metrics/counts_28d/20210216183650_bitbucket.yml index 5731b6b3cae..557a38cad1c 100644 --- a/config/metrics/counts_28d/20210216183650_bitbucket.yml +++ b/config/metrics/counts_28d/20210216183650_bitbucket.yml @@ -8,7 +8,7 @@ product_group: group::import product_category: importers value_type: number status: active -time_frame: all +time_frame: 28d data_source: database distribution: - ce diff --git a/config/metrics/counts_28d/20210216183652_bitbucket_server.yml b/config/metrics/counts_28d/20210216183652_bitbucket_server.yml index be4e45b8975..d9f255a0b2d 100644 --- a/config/metrics/counts_28d/20210216183652_bitbucket_server.yml +++ b/config/metrics/counts_28d/20210216183652_bitbucket_server.yml @@ -8,7 +8,7 @@ product_group: group::import product_category: importers value_type: number status: active -time_frame: all +time_frame: 28d data_source: database distribution: - ce diff --git a/config/metrics/counts_28d/20210216184559_ci_templates_total_unique_counts_monthly.yml b/config/metrics/counts_28d/20210216184559_ci_templates_total_unique_counts_monthly.yml index 66b8e714723..c80a7dada11 100644 --- a/config/metrics/counts_28d/20210216184559_ci_templates_total_unique_counts_monthly.yml +++ b/config/metrics/counts_28d/20210216184559_ci_templates_total_unique_counts_monthly.yml @@ -159,6 +159,7 @@ options: - p_ci_templates_implicit_security_api_fuzzing - p_ci_templates_implicit_security_dast - p_ci_templates_implicit_security_cluster_image_scanning + - p_ci_templates_kaniko distribution: - ce - ee diff --git a/config/metrics/counts_28d/20210929102434_p_ci_templates_implicit_jobs_build_monthly.yml b/config/metrics/counts_28d/20210929102434_p_ci_templates_implicit_jobs_build_monthly.yml index 4fa6d3d8843..a923fd255b7 100644 --- a/config/metrics/counts_28d/20210929102434_p_ci_templates_implicit_jobs_build_monthly.yml +++ b/config/metrics/counts_28d/20210929102434_p_ci_templates_implicit_jobs_build_monthly.yml @@ -9,7 +9,7 @@ value_type: number status: active milestone: "14.4" introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71157 -time_frame: 7d +time_frame: 28d data_source: redis_hll data_category: optional instrumentation_class: RedisHLLMetric diff --git a/config/metrics/counts_28d/20210929102736_p_ci_templates_implicit_jobs_deploy_latest_monthly.yml b/config/metrics/counts_28d/20210929102736_p_ci_templates_implicit_jobs_deploy_latest_monthly.yml index 9d19a6ffa72..28ed156f483 100644 --- a/config/metrics/counts_28d/20210929102736_p_ci_templates_implicit_jobs_deploy_latest_monthly.yml +++ b/config/metrics/counts_28d/20210929102736_p_ci_templates_implicit_jobs_deploy_latest_monthly.yml @@ -9,7 +9,7 @@ value_type: number status: active milestone: "14.4" introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71157 -time_frame: 7d +time_frame: 28d data_source: redis_hll data_category: optional instrumentation_class: RedisHLLMetric diff --git a/config/metrics/counts_28d/20210929103010_p_ci_templates_implicit_jobs_deploy_monthly.yml b/config/metrics/counts_28d/20210929103010_p_ci_templates_implicit_jobs_deploy_monthly.yml index 5b7b7924c4a..13f3bb050db 100644 --- a/config/metrics/counts_28d/20210929103010_p_ci_templates_implicit_jobs_deploy_monthly.yml +++ b/config/metrics/counts_28d/20210929103010_p_ci_templates_implicit_jobs_deploy_monthly.yml @@ -9,7 +9,7 @@ value_type: number status: active milestone: "14.4" introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71157 -time_frame: 7d +time_frame: 28d data_source: redis_hll data_category: optional instrumentation_class: RedisHLLMetric diff --git a/config/metrics/counts_28d/20211015154445_p_ci_templates_kaniko_monthly.yml b/config/metrics/counts_28d/20211015154445_p_ci_templates_kaniko_monthly.yml new file mode 100644 index 00000000000..1278c880072 --- /dev/null +++ b/config/metrics/counts_28d/20211015154445_p_ci_templates_kaniko_monthly.yml @@ -0,0 +1,25 @@ +--- +key_path: redis_hll_counters.ci_templates.p_ci_templates_kaniko_monthly +description: '' +product_section: ops +product_stage: verify +product_group: group::pipeline authoring +product_category: pipeline_authoring +value_type: number +status: active +milestone: '14.3' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72400 +time_frame: 28d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +distribution: + - ce + - ee +tier: + - free + - premium + - ultimate +options: + events: + - p_ci_templates_kaniko diff --git a/config/metrics/counts_7d/20210216181347_g_project_management_issue_cross_referenced_weekly.yml b/config/metrics/counts_7d/20210216181347_g_project_management_issue_cross_referenced_weekly.yml index 13094d1f7fd..d74794c08c7 100644 --- a/config/metrics/counts_7d/20210216181347_g_project_management_issue_cross_referenced_weekly.yml +++ b/config/metrics/counts_7d/20210216181347_g_project_management_issue_cross_referenced_weekly.yml @@ -8,7 +8,7 @@ product_group: group::project management product_category: issue_tracking value_type: number status: active -time_frame: 28d +time_frame: 7d data_source: redis_hll instrumentation_class: RedisHLLMetric options: diff --git a/config/metrics/counts_7d/20210216184557_ci_templates_total_unique_counts_weekly.yml b/config/metrics/counts_7d/20210216184557_ci_templates_total_unique_counts_weekly.yml index cf0e69c6da2..31813735ea4 100644 --- a/config/metrics/counts_7d/20210216184557_ci_templates_total_unique_counts_weekly.yml +++ b/config/metrics/counts_7d/20210216184557_ci_templates_total_unique_counts_weekly.yml @@ -159,6 +159,7 @@ options: - p_ci_templates_implicit_security_api_fuzzing - p_ci_templates_implicit_security_dast - p_ci_templates_implicit_security_cluster_image_scanning + - p_ci_templates_kaniko distribution: - ce - ee diff --git a/config/metrics/counts_7d/20210514013545_i_code_review_user_resolve_conflict_weekly.yml b/config/metrics/counts_7d/20210514013545_i_code_review_user_resolve_conflict_weekly.yml index f0b31b7e556..b25ed55bae1 100644 --- a/config/metrics/counts_7d/20210514013545_i_code_review_user_resolve_conflict_weekly.yml +++ b/config/metrics/counts_7d/20210514013545_i_code_review_user_resolve_conflict_weekly.yml @@ -10,7 +10,7 @@ product_category: code_review value_type: number status: active milestone: "13.12" -time_frame: 28d +time_frame: 7d data_source: redis_hll instrumentation_class: RedisHLLMetric options: diff --git a/config/metrics/counts_7d/20211015154445_p_ci_templates_kaniko_weekly.yml b/config/metrics/counts_7d/20211015154445_p_ci_templates_kaniko_weekly.yml new file mode 100644 index 00000000000..f101eefc933 --- /dev/null +++ b/config/metrics/counts_7d/20211015154445_p_ci_templates_kaniko_weekly.yml @@ -0,0 +1,25 @@ +--- +key_path: redis_hll_counters.ci_templates.p_ci_templates_kaniko_weekly +description: '' +product_section: ops +product_stage: release +product_group: group::release +product_category: continuous_delivery +value_type: number +status: active +milestone: '14.5' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72400 +time_frame: 7d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +distribution: + - ce + - ee +tier: + - free + - premium + - ultimate +options: + events: + - p_ci_templates_kaniko diff --git a/config/metrics/counts_all/20210216174902_g_analytics_merge_request.yml b/config/metrics/counts_all/20210216174902_g_analytics_merge_request.yml index c15990a9309..af9338f028a 100644 --- a/config/metrics/counts_all/20210216174902_g_analytics_merge_request.yml +++ b/config/metrics/counts_all/20210216174902_g_analytics_merge_request.yml @@ -8,7 +8,7 @@ product_group: group::optimize product_category: value_type: number status: removed -time_frame: 7d +time_frame: all data_source: redis_hll distribution: - ce diff --git a/config/metrics/counts_all/20210715094459_releases_with_milestones.yml b/config/metrics/counts_all/20210715094459_releases_with_milestones.yml index 5d853604580..0be5497e561 100644 --- a/config/metrics/counts_all/20210715094459_releases_with_milestones.yml +++ b/config/metrics/counts_all/20210715094459_releases_with_milestones.yml @@ -10,7 +10,7 @@ value_type: number status: active milestone: "14.4" introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71287' -time_frame: 28d +time_frame: all data_source: database instrumentation_class: 'CountUsersAssociatingMilestonesToReleasesMetric' data_category: Optional diff --git a/db/post_migrate/20211011104843_add_new_loose_fk_index.rb b/db/post_migrate/20211011104843_add_new_loose_fk_index.rb new file mode 100644 index 00000000000..710d0917d7f --- /dev/null +++ b/db/post_migrate/20211011104843_add_new_loose_fk_index.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddNewLooseFkIndex < Gitlab::Database::Migration[1.0] + include Gitlab::Database::PartitioningMigrationHelpers + + disable_ddl_transaction! + + INDEX_NAME = 'index_loose_foreign_keys_deleted_records_for_loading_records' + + def up + add_concurrent_partitioned_index :loose_foreign_keys_deleted_records, + %I[fully_qualified_table_name id primary_key_value partition], + where: 'status = 1', + name: INDEX_NAME + end + + def down + remove_concurrent_partitioned_index_by_name :loose_foreign_keys_deleted_records, INDEX_NAME + end +end diff --git a/db/schema_migrations/20211011104843 b/db/schema_migrations/20211011104843 new file mode 100644 index 00000000000..78789b94ece --- /dev/null +++ b/db/schema_migrations/20211011104843 @@ -0,0 +1 @@ +e2812344b16cd51c544235bae8a365713ab7e34652c2c05511a7fe9d84c05fb1
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index fbadeae0377..045f800f8f7 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23947,6 +23947,10 @@ ALTER TABLE ONLY zentao_tracker_data ALTER TABLE ONLY zoom_meetings ADD CONSTRAINT zoom_meetings_pkey PRIMARY KEY (id); +CREATE INDEX index_loose_foreign_keys_deleted_records_for_loading_records ON ONLY loose_foreign_keys_deleted_records USING btree (fully_qualified_table_name, id, primary_key_value, partition) WHERE (status = 1); + +CREATE INDEX index_8be8640437 ON gitlab_partitions_static.loose_foreign_keys_deleted_records_1 USING btree (fully_qualified_table_name, id, primary_key_value, partition) WHERE (status = 1); + CREATE INDEX index_product_analytics_events_experimental_project_and_time ON ONLY product_analytics_events_experimental USING btree (project_id, collector_tstamp); CREATE INDEX product_analytics_events_expe_project_id_collector_tstamp_idx10 ON gitlab_partitions_static.product_analytics_events_experimental_10 USING btree (project_id, collector_tstamp); @@ -27249,6 +27253,8 @@ ALTER INDEX analytics_cycle_analytics_merge_request_stage_events_pkey ATTACH PAR ALTER INDEX analytics_cycle_analytics_merge_request_stage_events_pkey ATTACH PARTITION gitlab_partitions_static.analytics_cycle_analytics_merge_request_stage_events_31_pkey; +ALTER INDEX index_loose_foreign_keys_deleted_records_for_loading_records ATTACH PARTITION gitlab_partitions_static.index_8be8640437; + ALTER INDEX loose_foreign_keys_deleted_records_pkey ATTACH PARTITION gitlab_partitions_static.loose_foreign_keys_deleted_records_1_pkey; ALTER INDEX index_product_analytics_events_experimental_project_and_time ATTACH PARTITION gitlab_partitions_static.product_analytics_events_expe_project_id_collector_tstamp_idx10; diff --git a/doc/development/index.md b/doc/development/index.md index 3780c986367..17d7eaf9779 100644 --- a/doc/development/index.md +++ b/doc/development/index.md @@ -181,6 +181,7 @@ the [reviewer values](https://about.gitlab.com/handbook/engineering/workflow/rev - [Avoid modules with instance variables](module_with_instance_variables.md), if possible - [Guidelines for reusing abstractions](reusing_abstractions.md) +- [Ruby 3 gotchas](ruby3_gotchas.md) ### Rails Framework related diff --git a/doc/development/ruby3_gotchas.md b/doc/development/ruby3_gotchas.md new file mode 100644 index 00000000000..7216e1c4ed6 --- /dev/null +++ b/doc/development/ruby3_gotchas.md @@ -0,0 +1,139 @@ +--- +stage: none +group: unassigned +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Ruby 3 gotchas + +This section documents several problems we found while working on [Ruby 3 support](https://gitlab.com/groups/gitlab-org/-/epics/5149) +and which led to subtle bugs or test failures that were difficult to understand. We encourage every GitLab contributor +who writes Ruby code on a regular basis to familiarize themselves with these issues. + +The complete list of changes to the Ruby 3 language and standard library is found [here](https://rubyreferences.github.io/rubychanges/3.0.html). + +## `Hash#each` consistently yields a 2-element array to lambdas + +Consider the following code snippet: + +```ruby +def foo(a, b) + p [a, b] +end + +def bar(a, b = 2) + p [a, b] +end + +foo_lambda = method(:foo).to_proc +bar_lambda = method(:bar).to_proc + +{ a: 1 }.each(&foo_lambda) +{ a: 1 }.each(&bar_lambda) +``` + +In Ruby 2.7, the output of this program suggests that yielding hash entries to lambdas behaves +differently depending on how many required arguments there are: + +```ruby +# Ruby 2.7 +{ a: 1 }.each(&foo_lambda) # prints [:a, 1] +{ a: 1 }.each(&bar_lambda) # prints [[:a, 1], 2] +``` + +Ruby 3 makes this behavior consistent and always attempts to yield hash entries as a single `[key, value]` array: + +```ruby +# Ruby 3.0 +{ a: 1 }.each(&foo_lambda) # `foo': wrong number of arguments (given 1, expected 2) (ArgumentError) +{ a: 1 }.each(&bar_lambda) # prints [[:a, 1], 2] +``` + +To write code that works under both 2.7 and 3.0, consider the following options: + +- Always pass the lambda body as a block: `{ a: 1 }.each { |a, b| p [a, b] }`. +- Deconstruct the lambda arguments: `{ a: 1 }.each(&->((a, b)) { p [a, b] })`. + +We recommend always passing the block explicitly, and prefer two required arguments as block parameters. + +More information can be found in [Ruby issue 12706](https://bugs.ruby-lang.org/issues/12706). + +## `Symbol#to_proc` returns signature metadata consistent with lambdas + +A common idiom in Ruby is to obtain procs via the `&:<symbol>` shorthand and +pass them to higher-order functions: + +```ruby +[1, 2, 3].each(&:to_s) +``` + +Ruby desugars `&:<symbol>` to `Symbol#to_proc`, which we can `call` with +the method _receiver_ as its first argument (here `Integer`), and all method _arguments_ +(here none) as its remaining arguments. + +This behaves the same in both Ruby 2.7 and Ruby 3; where Ruby 3 diverges is when capturing +this proc and inspecting its call signature. +This is often done when writing DSLs or using other forms of meta-programming: + +```ruby +p = :foo.to_proc # This usually happens via a conversion through `&:foo` + +# Ruby 2.7: prints [[:rest]] (-1) +# Ruby 3.0: prints [[:req], [:rest]] (-2) +puts "#{p.parameters} (#{p.arity})" +``` + +Ruby 2.7 reports zero required and one optional parameter for this proc, while Ruby 3 reports one required +and one optional parameter. As described above, Ruby 2.7 is incorrect: the first argument must +always be passed, as it is the receiver of the method the proc represents, and methods cannot be +called without a receiver. + +Ruby 3 corrects this, meaning code that tests proc arity or parameter lists might now break and +has to be updated. + +More information can be found in [Ruby issue 16260](https://bugs.ruby-lang.org/issues/16260). + +## `OpenStruct` does not evaluate fields lazily + +The `OpenStruct` implementation has undergone a partial rewrite in Ruby 3, resulting in +behavioral changes. In Ruby 2.7, `OpenStruct` defines methods lazily, when the method is first accessed. +In Ruby 3.0, it defines these methods eagerly in the initializer, which can break classes that inherit from `OpenStruct` +and override these methods. + +Don't inherit from `OpenStruct` for these reasons; ideally, don't use it at all. +`OpenStruct` is [considered problematic](https://ruby-doc.org/stdlib-3.0.2/libdoc/ostruct/rdoc/OpenStruct.html#class-OpenStruct-label-Caveats) for various reasons. When writing new code, prefer a `Struct` instead, which is simpler in implementation, although less flexible. + +## `Regexp` and `Range` instances are frozen + +It is not necessary anymore to explicitly `freeze` `Regexp` or `Range` instances, since Ruby 3 freezes +them automatically upon creation. + +This has a subtle side-effect: Tests that stub method calls on these types now fail with an error, since +RSpec cannot stub frozen objects: + +```ruby +# Ruby 2.7: works +# Ruby 3.0: error: "can't modify frozen object" +allow(subject.function_returning_range).to receive(:max).and_return(42) +``` + +Rewrite affected tests by not stubbing method calls on frozen objects. The example above can be rewritten as: + +```ruby +# Works with any Ruby version +allow(subject).to receive(:function_returning_range).and_return(1..42) +``` + +## Table tests fail with Ruby 3.0.2 + +Ruby 3.0.2 has a known bug that causes [table tests](testing_guide/best_practices.md#table-based--parameterized-tests) +to fail when table values consist of integer values. +The reasons are documented in [issue 337614](https://gitlab.com/gitlab-org/gitlab/-/issues/337614). +This problem has been fixed in Ruby and the fix is expected to be included in Ruby 3.0.3, which is not yet +available at this time. + +The problem only affects users who run an unpatched Ruby 3.0.2. This is likely the case when you +installed Ruby manually or via tools like `asdf`. Users of the `gitlab-development-kit (GDK)` +are also affected by this problem. + +Build images are not affected, since they include the patch set addressing this bug. diff --git a/lib/feature.rb b/lib/feature.rb index f8d34e9c386..d30594792c3 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -155,13 +155,13 @@ class Feature def flipper if Gitlab::SafeRequestStore.active? - Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance + Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance(memoize: true) else @flipper ||= build_flipper_instance end end - def build_flipper_instance + def build_flipper_instance(memoize: false) active_record_adapter = Flipper::Adapters::ActiveRecord.new( feature_class: FlipperFeature, gate_class: FlipperGate) @@ -182,7 +182,7 @@ class Feature expires_in: 1.minute) Flipper.new(flipper_adapter).tap do |flip| - flip.memoize = true + flip.memoize = memoize end end diff --git a/lib/gitlab/ci/templates/Kaniko.gitlab-ci.yml b/lib/gitlab/ci/templates/Kaniko.gitlab-ci.yml new file mode 100644 index 00000000000..f1b1c20b4e0 --- /dev/null +++ b/lib/gitlab/ci/templates/Kaniko.gitlab-ci.yml @@ -0,0 +1,47 @@ +# To contribute improvements to CI/CD templates, please follow the Development guide at: +# https://docs.gitlab.com/ee/development/cicd/templates.html +# This specific template is located at: +# https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Kaniko.gitlab-ci.yml + +# Build and publish a tag/branch to Gitlab Docker Registry using Kaniko and Gitlab Docker executor. +# Kaniko can build Docker images without using Docker-In-Docker and it's permission +# drawbacks. No additional configuration required. +kaniko-build: + variables: + # Additional options for Kaniko executor. + # For more details see https://github.com/GoogleContainerTools/kaniko/blob/master/README.md#additional-flags + KANIKO_ARGS: "" + stage: build + image: + # For latest releases see https://github.com/GoogleContainerTools/kaniko/releases + # Only debug/*-debug versions of the Kaniko image are known to work within Gitlab CI + name: gcr.io/kaniko-project/executor:debug + entrypoint: [""] + script: + # Compose docker tag name + # Git Branch/Tag to Docker Image Tag Mapping + # * Default Branch: main -> latest + # * Branch: feature/my-feature -> branch-feature-my-feature + # * Tag: v1.0.0/beta2 -> v1.0.0-beta2 + - | + if [ "$CI_COMMIT_REF_NAME" = $CI_DEFAULT_BRANCH ]; then + VERSION="latest" + elif [ -n "$CI_COMMIT_TAG" ];then + NOSLASH=$(echo "$CI_COMMIT_TAG" | tr -s / - ) + SANITIZED="${NOSLASH//[^a-zA-Z0-9\-\.]/}" + VERSION="$SANITIZED" + else \ + NOSLASH=$(echo "$CI_COMMIT_REF_NAME" | tr -s / - ) + SANITIZED="${NOSLASH//[^a-zA-Z0-9\-]/}" + VERSION="branch-$SANITIZED" + fi + - echo $VERSION + - mkdir -p /kaniko/.docker + # Write credentials to access Gitlab Container Registry within the runner/ci + - echo "{\"auths\":{\"$CI_REGISTRY\":{\"auth\":\"$(echo -n ${CI_REGISTRY_USER}:${CI_REGISTRY_PASSWORD} | base64 | tr -d '\n')\"}}}" > /kaniko/.docker/config.json + # Build and push the container. To disable push add --no-push + - /kaniko/executor --context $CI_PROJECT_DIR --dockerfile $CI_PROJECT_DIR/Dockerfile --destination $CI_REGISTRY_IMAGE:$VERSION $KANIKO_ARGS + # Run this job in a branch/tag where a Dockerfile exists + rules: + - exists: + - Dockerfile diff --git a/lib/gitlab/usage_data_counters/known_events/ci_templates.yml b/lib/gitlab/usage_data_counters/known_events/ci_templates.yml index 99bdd3ca9e9..60a709e9a1d 100644 --- a/lib/gitlab/usage_data_counters/known_events/ci_templates.yml +++ b/lib/gitlab/usage_data_counters/known_events/ci_templates.yml @@ -559,3 +559,7 @@ category: ci_templates redis_slot: ci_templates aggregation: weekly +- name: p_ci_templates_kaniko + category: ci_templates + redis_slot: ci_templates + aggregation: weekly diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bc98c157f5d..7fe22c674d3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -26669,6 +26669,9 @@ msgstr "" msgid "ProjectService|Enter new password." msgstr "" +msgid "ProjectService|Enter new token" +msgstr "" + msgid "ProjectService|Issue URL" msgstr "" @@ -26684,6 +26687,9 @@ msgstr "" msgid "ProjectService|Leave blank to use your current password." msgstr "" +msgid "ProjectService|Leave blank to use your current token." +msgstr "" + msgid "ProjectService|Mock service URL" msgstr "" @@ -39702,7 +39708,7 @@ msgstr "" msgid "ZentaoIntegration|Base URL of the ZenTao instance." msgstr "" -msgid "ZentaoIntegration|Enter API token" +msgid "ZentaoIntegration|Enter new ZenTao API token" msgstr "" msgid "ZentaoIntegration|If different from Web URL." diff --git a/qa/qa/page/project/new.rb b/qa/qa/page/project/new.rb index 3ecdabeeed2..5ff52527774 100644 --- a/qa/qa/page/project/new.rb +++ b/qa/qa/page/project/new.rb @@ -13,6 +13,7 @@ module QA view 'app/views/projects/_new_project_fields.html.haml' do element :initialize_with_readme_checkbox + element :initialize_with_sast_checkbox element :project_namespace_field, 'namespaces_options' # rubocop:disable QA/ElementWithPattern element :project_name, 'text_field :name' # rubocop:disable QA/ElementWithPattern element :project_path, 'text_field :path' # rubocop:disable QA/ElementWithPattern @@ -79,6 +80,13 @@ module QA choose visibility.capitalize end + # Disable experiment for SAST at project creation https://gitlab.com/gitlab-org/gitlab/-/issues/333196 + def disable_initialize_with_sast + return unless has_element?(:initialize_with_sast_checkbox) + + uncheck_element(:initialize_with_sast_checkbox) + end + def click_github_link click_link 'GitHub' end diff --git a/qa/qa/resource/project.rb b/qa/qa/resource/project.rb index 26c800d470a..43f2039fd78 100644 --- a/qa/qa/resource/project.rb +++ b/qa/qa/resource/project.rb @@ -93,6 +93,7 @@ module QA new_page.choose_name(@name) new_page.add_description(@description) new_page.set_visibility(@visibility) + new_page.disable_initialize_with_sast new_page.disable_initialize_with_readme unless @initialize_with_readme new_page.create_new_project end diff --git a/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb index af4e7126c29..34a7431e328 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb @@ -5,13 +5,13 @@ module QA describe 'Project', :requires_admin do shared_examples 'successful project creation' do it 'creates a new project' do - Page::Project::Show.perform do |project| - expect(project).to have_content(project_name) - expect(project).to have_content( + Page::Project::Show.perform do |project_page| + expect(project_page).to have_content(project_name) + expect(project_page).to have_content( /Project \S?#{project_name}\S+ was successfully created/ ) - expect(project).to have_content('create awesome project test') - expect(project).to have_content('The repository for this project is empty') + expect(project_page).to have_content('create awesome project test') + expect(project_page).to have_content('The repository for this project is empty') end end end diff --git a/spec/features/projects/user_changes_project_visibility_spec.rb b/spec/features/projects/user_changes_project_visibility_spec.rb index 39b8cddd005..345d16982fd 100644 --- a/spec/features/projects/user_changes_project_visibility_spec.rb +++ b/spec/features/projects/user_changes_project_visibility_spec.rb @@ -22,7 +22,7 @@ RSpec.describe 'User changes public project visibility', :js do click_button 'Save changes' end - find('.js-confirm-danger-input').send_keys(project.path_with_namespace) + find('.js-legacy-confirm-danger-input').send_keys(project.path_with_namespace) page.within '.modal' do click_button 'Reduce project visibility' diff --git a/spec/frontend/vue_shared/components/confirm_danger/confirm_danger_modal_spec.js b/spec/frontend/vue_shared/components/confirm_danger/confirm_danger_modal_spec.js new file mode 100644 index 00000000000..f75694bd504 --- /dev/null +++ b/spec/frontend/vue_shared/components/confirm_danger/confirm_danger_modal_spec.js @@ -0,0 +1,99 @@ +import { GlModal, GlSprintf } from '@gitlab/ui'; +import { + CONFIRM_DANGER_WARNING, + CONFIRM_DANGER_MODAL_BUTTON, + CONFIRM_DANGER_MODAL_ID, +} from '~/vue_shared/components/confirm_danger/constants'; +import ConfirmDangerModal from '~/vue_shared/components/confirm_danger/confirm_danger_modal.vue'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; + +describe('Confirm Danger Modal', () => { + const confirmDangerMessage = 'This is a dangerous activity'; + const confirmButtonText = 'Confirm button text'; + const phrase = 'You must construct additional pylons'; + const modalId = CONFIRM_DANGER_MODAL_ID; + + let wrapper; + + const findModal = () => wrapper.findComponent(GlModal); + const findConfirmationPhrase = () => wrapper.findByTestId('confirm-danger-phrase'); + const findConfirmationInput = () => wrapper.findByTestId('confirm-danger-input'); + const findDefaultWarning = () => wrapper.findByTestId('confirm-danger-warning'); + const findAdditionalMessage = () => wrapper.findByTestId('confirm-danger-message'); + const findPrimaryAction = () => findModal().props('actionPrimary'); + const findPrimaryActionAttributes = (attr) => findPrimaryAction().attributes[0][attr]; + + const createComponent = ({ provide = {} } = {}) => + shallowMountExtended(ConfirmDangerModal, { + propsData: { + modalId, + phrase, + }, + provide, + stubs: { GlSprintf }, + }); + + beforeEach(() => { + wrapper = createComponent({ provide: { confirmDangerMessage, confirmButtonText } }); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + it('renders the default warning message', () => { + expect(findDefaultWarning().text()).toBe(CONFIRM_DANGER_WARNING); + }); + + it('renders any additional messages', () => { + expect(findAdditionalMessage().text()).toBe(confirmDangerMessage); + }); + + it('renders the confirm button', () => { + expect(findPrimaryAction().text).toBe(confirmButtonText); + expect(findPrimaryActionAttributes('variant')).toBe('danger'); + }); + + it('renders the correct confirmation phrase', () => { + expect(findConfirmationPhrase().text()).toBe( + `Please type ${phrase} to proceed or close this modal to cancel.`, + ); + }); + + describe('without injected data', () => { + beforeEach(() => { + wrapper = createComponent(); + }); + + it('does not render any additional messages', () => { + expect(findAdditionalMessage().exists()).toBe(false); + }); + + it('renders the default confirm button', () => { + expect(findPrimaryAction().text).toBe(CONFIRM_DANGER_MODAL_BUTTON); + }); + }); + + describe('with a valid confirmation phrase', () => { + beforeEach(() => { + wrapper = createComponent(); + }); + + it('enables the confirm button', async () => { + expect(findPrimaryActionAttributes('disabled')).toBe(true); + + await findConfirmationInput().vm.$emit('input', phrase); + + expect(findPrimaryActionAttributes('disabled')).toBe(false); + }); + + it('emits a `confirm` event when the button is clicked', async () => { + expect(wrapper.emitted('confirm')).toBeUndefined(); + + await findConfirmationInput().vm.$emit('input', phrase); + await findModal().vm.$emit('primary'); + + expect(wrapper.emitted('confirm')).not.toBeUndefined(); + }); + }); +}); diff --git a/spec/frontend/vue_shared/components/confirm_danger/confirm_danger_spec.js b/spec/frontend/vue_shared/components/confirm_danger/confirm_danger_spec.js new file mode 100644 index 00000000000..220f897c035 --- /dev/null +++ b/spec/frontend/vue_shared/components/confirm_danger/confirm_danger_spec.js @@ -0,0 +1,61 @@ +import { GlButton } from '@gitlab/ui'; +import ConfirmDanger from '~/vue_shared/components/confirm_danger/confirm_danger.vue'; +import ConfirmDangerModal from '~/vue_shared/components/confirm_danger/confirm_danger_modal.vue'; +import { CONFIRM_DANGER_MODAL_ID } from '~/vue_shared/components/confirm_danger/constants'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; + +describe('Confirm Danger Modal', () => { + let wrapper; + + const phrase = 'En Taro Adun'; + const buttonText = 'Click me!'; + const modalId = CONFIRM_DANGER_MODAL_ID; + + const findBtn = () => wrapper.findComponent(GlButton); + const findModal = () => wrapper.findComponent(ConfirmDangerModal); + const findModalProps = () => findModal().props(); + + const createComponent = (props = {}) => + shallowMountExtended(ConfirmDanger, { + propsData: { + buttonText, + phrase, + ...props, + }, + }); + + beforeEach(() => { + wrapper = createComponent(); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + it('renders the button', () => { + expect(wrapper.html()).toContain(buttonText); + }); + + it('sets the modal properties', () => { + expect(findModalProps()).toMatchObject({ + modalId, + phrase, + }); + }); + + it('will disable the button if `disabled=true`', () => { + expect(findBtn().attributes('disabled')).toBeUndefined(); + + wrapper = createComponent({ disabled: true }); + + expect(findBtn().attributes('disabled')).toBe('true'); + }); + + it('will emit `confirm` when the modal confirms', () => { + expect(wrapper.emitted('confirm')).toBeUndefined(); + + findModal().vm.$emit('confirm'); + + expect(wrapper.emitted('confirm')).not.toBeUndefined(); + }); +}); diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 9d4820f9a4c..dce2eedf6c3 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -102,12 +102,14 @@ RSpec.describe Feature, stub_feature_flags: false do describe '.flipper' do context 'when request store is inactive' do - it 'memoizes the Flipper instance' do + it 'memoizes the Flipper instance but does not not enable Flipper memoization' do expect(Flipper).to receive(:new).once.and_call_original 2.times do - described_class.send(:flipper) + described_class.flipper end + + expect(described_class.flipper.adapter.memoizing?).to eq(false) end end @@ -115,9 +117,11 @@ RSpec.describe Feature, stub_feature_flags: false do it 'memoizes the Flipper instance' do expect(Flipper).to receive(:new).once.and_call_original - described_class.send(:flipper) + described_class.flipper described_class.instance_variable_set(:@flipper, nil) - described_class.send(:flipper) + described_class.flipper + + expect(described_class.flipper.adapter.memoizing?).to eq(true) end end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb index 16517b39a45..cf21c98dbd5 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb @@ -83,7 +83,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::External do end end - it 'respects the defined payload schema' do + it 'respects the defined payload schema', :saas do expect(::Gitlab::HTTP).to receive(:post) do |_url, params| expect(params[:body]).to match_schema('/external_validation') expect(params[:timeout]).to eq(described_class::DEFAULT_VALIDATION_REQUEST_TIMEOUT) diff --git a/spec/lib/gitlab/ci/templates/kaniko_gitlab_ci_yaml_spec.rb b/spec/lib/gitlab/ci/templates/kaniko_gitlab_ci_yaml_spec.rb new file mode 100644 index 00000000000..c7dbbea4622 --- /dev/null +++ b/spec/lib/gitlab/ci/templates/kaniko_gitlab_ci_yaml_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Kaniko.gitlab-ci.yml' do + subject(:template) { Gitlab::Template::GitlabCiYmlTemplate.find('Kaniko') } + + describe 'the created pipeline' do + let(:pipeline_branch) { 'master' } + let(:project) { create(:project, :custom_repo, files: { 'Dockerfile' => 'FROM alpine:latest' }) } + let(:user) { project.owner } + let(:service) { Ci::CreatePipelineService.new(project, user, ref: pipeline_branch ) } + let(:pipeline) { service.execute!(:push).payload } + let(:build_names) { pipeline.builds.pluck(:name) } + + before do + stub_ci_pipeline_yaml_file(template.content) + allow(Ci::BuildScheduleWorker).to receive(:perform).and_return(true) + end + + it 'creates "kaniko-build" job' do + expect(build_names).to include('kaniko-build') + end + end +end diff --git a/spec/models/concerns/loose_foreign_key_spec.rb b/spec/models/concerns/loose_foreign_key_spec.rb index ce5e33261a9..42da69eb75e 100644 --- a/spec/models/concerns/loose_foreign_key_spec.rb +++ b/spec/models/concerns/loose_foreign_key_spec.rb @@ -9,8 +9,8 @@ RSpec.describe LooseForeignKey do self.table_name = 'projects' - loose_foreign_key :issues, :project_id, on_delete: :async_delete, gitlab_schema: :gitlab_main - loose_foreign_key 'merge_requests', 'project_id', 'on_delete' => 'async_nullify', 'gitlab_schema' => :gitlab_main + loose_foreign_key :issues, :project_id, on_delete: :async_delete + loose_foreign_key 'merge_requests', 'project_id', 'on_delete' => 'async_nullify' end end @@ -28,7 +28,6 @@ RSpec.describe LooseForeignKey do expect(definition.to_table).to eq('merge_requests') expect(definition.column).to eq('project_id') expect(definition.on_delete).to eq(:async_nullify) - expect(definition.options[:gitlab_schema]).to eq(:gitlab_main) end context 'validation' do @@ -39,9 +38,9 @@ RSpec.describe LooseForeignKey do self.table_name = 'projects' - loose_foreign_key :issues, :project_id, on_delete: :async_delete, gitlab_schema: :gitlab_main - loose_foreign_key :merge_requests, :project_id, on_delete: :async_nullify, gitlab_schema: :gitlab_main - loose_foreign_key :merge_requests, :project_id, on_delete: :destroy, gitlab_schema: :gitlab_main + loose_foreign_key :issues, :project_id, on_delete: :async_delete + loose_foreign_key :merge_requests, :project_id, on_delete: :async_nullify + loose_foreign_key :merge_requests, :project_id, on_delete: :destroy end end @@ -50,28 +49,12 @@ RSpec.describe LooseForeignKey do end end - context 'gitlab_schema validation' do - let(:invalid_class) do - Class.new(ApplicationRecord) do - include LooseForeignKey - - self.table_name = 'projects' - - loose_foreign_key :merge_requests, :project_id, on_delete: :async_nullify, gitlab_schema: :unknown - end - end - - it 'raises error when invalid `gitlab_schema` option was given' do - expect { invalid_class }.to raise_error /Invalid gitlab_schema option given: unknown/ - end - end - context 'inheritance validation' do let(:inherited_project_class) do Class.new(Project) do include LooseForeignKey - loose_foreign_key :issues, :project_id, on_delete: :async_delete, gitlab_schema: :gitlab_main + loose_foreign_key :issues, :project_id, on_delete: :async_delete end end diff --git a/spec/models/loose_foreign_keys/deleted_record_spec.rb b/spec/models/loose_foreign_keys/deleted_record_spec.rb new file mode 100644 index 00000000000..8494cd91c6e --- /dev/null +++ b/spec/models/loose_foreign_keys/deleted_record_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::DeletedRecord, type: :model do + let_it_be(:table) { 'public.projects' } + + let_it_be(:deleted_record_1) { described_class.create!(partition: 1, fully_qualified_table_name: table, primary_key_value: 5) } + let_it_be(:deleted_record_2) { described_class.create!(partition: 1, fully_qualified_table_name: table, primary_key_value: 1) } + let_it_be(:deleted_record_3) { described_class.create!(partition: 1, fully_qualified_table_name: 'public.other_table', primary_key_value: 3) } + let_it_be(:deleted_record_4) { described_class.create!(partition: 1, fully_qualified_table_name: table, primary_key_value: 1) } # duplicate + + describe '.load_batch_for_table' do + it 'loads records and orders them by creation date' do + records = described_class.load_batch_for_table(table, 10) + + expect(records).to eq([deleted_record_1, deleted_record_2, deleted_record_4]) + end + + it 'supports configurable batch size' do + records = described_class.load_batch_for_table(table, 2) + + expect(records).to eq([deleted_record_1, deleted_record_2]) + end + end + + describe '.mark_records_processed_for_table_between' do + it 'marks processed exactly one record' do + described_class.mark_records_processed_for_table_between(table, deleted_record_2, deleted_record_2) + + expect(described_class.status_pending.count).to eq(3) + expect(described_class.status_processed.count).to eq(1) + + processed_record = described_class.status_processed.first + expect(processed_record).to eq(deleted_record_2) + end + + it 'deletes two records' do + described_class.mark_records_processed_for_table_between(table, deleted_record_2, deleted_record_4) + + expect(described_class.status_pending.count).to eq(2) + expect(described_class.status_processed.count).to eq(2) + end + + it 'deletes all records' do + described_class.mark_records_processed_for_table_between(table, deleted_record_1, deleted_record_4) + + expect(described_class.status_pending.count).to eq(1) + expect(described_class.status_processed.count).to eq(3) + end + end +end diff --git a/spec/models/loose_foreign_keys/modification_tracker_spec.rb b/spec/models/loose_foreign_keys/modification_tracker_spec.rb new file mode 100644 index 00000000000..13e5f920d05 --- /dev/null +++ b/spec/models/loose_foreign_keys/modification_tracker_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::ModificationTracker do + subject(:tracker) { described_class.new } + + describe '#over_limit?' do + it 'is true when deletion MAX_DELETES is exceeded' do + stub_const('LooseForeignKeys::ModificationTracker::MAX_DELETES', 5) + + tracker.add_deletions('issues', 10) + expect(tracker).to be_over_limit + end + + it 'is false when MAX_DELETES is not exceeded' do + tracker.add_deletions('issues', 3) + + expect(tracker).not_to be_over_limit + end + + it 'is true when deletion MAX_UPDATES is exceeded' do + stub_const('LooseForeignKeys::ModificationTracker::MAX_UPDATES', 5) + + tracker.add_updates('issues', 3) + tracker.add_updates('issues', 4) + + expect(tracker).to be_over_limit + end + + it 'is false when MAX_UPDATES is not exceeded' do + tracker.add_updates('projects', 3) + + expect(tracker).not_to be_over_limit + end + + it 'is true when max runtime is exceeded' do + monotonic_time_before = 1 # this will be the start time + monotonic_time_after = described_class::MAX_RUNTIME.to_i + 1 # this will be returned when over_limit? is called + + allow(Gitlab::Metrics::System).to receive(:monotonic_time).and_return(monotonic_time_before, monotonic_time_after) + + tracker + + expect(tracker).to be_over_limit + end + + it 'is false when max runtime is not exceeded' do + expect(tracker).not_to be_over_limit + end + end + + describe '#stats' do + it 'exposes stats' do + freeze_time do + tracker + tracker.add_deletions('issues', 5) + tracker.add_deletions('issues', 2) + tracker.add_deletions('projects', 2) + + tracker.add_updates('projects', 3) + + expect(tracker.stats).to eq({ + over_limit: false, + delete_count_by_table: { 'issues' => 7, 'projects' => 2 }, + update_count_by_table: { 'projects' => 3 }, + delete_count: 9, + update_count: 3 + }) + end + end + end +end diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb new file mode 100644 index 00000000000..dcef419cc9a --- /dev/null +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::BatchCleanerService do + include MigrationsHelpers + + def create_table_structure + migration = ActiveRecord::Migration.new.extend(Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers) + + migration.create_table :loose_fk_parent_table + + migration.create_table :loose_fk_child_table_1 do |t| + t.bigint :parent_id + end + + migration.create_table :loose_fk_child_table_2 do |t| + t.bigint :parent_id_with_different_column + end + + migration.track_record_deletions(:loose_fk_parent_table) + end + + let(:parent_model) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_parent_table' + + include LooseForeignKey + + loose_foreign_key :loose_fk_child_table_1, :parent_id, on_delete: :async_delete + loose_foreign_key :loose_fk_child_table_2, :parent_id_with_different_column, on_delete: :async_nullify + end + end + + let(:child_model_1) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_1' + end + end + + let(:child_model_2) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_2' + end + end + + let(:loose_fk_child_table_1) { table(:loose_fk_child_table_1) } + let(:loose_fk_child_table_2) { table(:loose_fk_child_table_2) } + let(:parent_record_1) { parent_model.create! } + let(:other_parent_record) { parent_model.create! } + + before(:all) do + create_table_structure + end + + before do + parent_record_1 + + loose_fk_child_table_1.create!(parent_id: parent_record_1.id) + loose_fk_child_table_1.create!(parent_id: parent_record_1.id) + + # these will not be deleted + loose_fk_child_table_1.create!(parent_id: other_parent_record.id) + loose_fk_child_table_1.create!(parent_id: other_parent_record.id) + + loose_fk_child_table_2.create!(parent_id_with_different_column: parent_record_1.id) + loose_fk_child_table_2.create!(parent_id_with_different_column: parent_record_1.id) + + # these will not be deleted + loose_fk_child_table_2.create!(parent_id_with_different_column: other_parent_record.id) + loose_fk_child_table_2.create!(parent_id_with_different_column: other_parent_record.id) + end + + after(:all) do + migration = ActiveRecord::Migration.new + migration.drop_table :loose_fk_parent_table + migration.drop_table :loose_fk_child_table_1 + migration.drop_table :loose_fk_child_table_2 + end + + context 'when parent records are deleted' do + before do + parent_record_1.delete + + expect(loose_fk_child_table_1.count).to eq(4) + expect(loose_fk_child_table_2.count).to eq(4) + + described_class.new(parent_klass: parent_model, + deleted_parent_records: LooseForeignKeys::DeletedRecord.status_pending.all, + models_by_table_name: { + 'loose_fk_child_table_1' => child_model_1, + 'loose_fk_child_table_2' => child_model_2 + }).execute + end + + it 'cleans up the child records' do + expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty + expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) + end + + it 'cleans up the parent DeletedRecord' do + expect(LooseForeignKeys::DeletedRecord.status_pending.count).to eq(0) + end + + it 'does not delete unrelated records' do + expect(loose_fk_child_table_1.where(parent_id: other_parent_record.id).count).to eq(2) + expect(loose_fk_child_table_2.where(parent_id_with_different_column: other_parent_record.id).count).to eq(2) + end + end +end diff --git a/spec/services/loose_foreign_keys/cleaner_service_spec.rb b/spec/services/loose_foreign_keys/cleaner_service_spec.rb new file mode 100644 index 00000000000..6f37ac49435 --- /dev/null +++ b/spec/services/loose_foreign_keys/cleaner_service_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::CleanerService do + let(:schema) { ApplicationRecord.connection.current_schema } + let(:deleted_records) do + [ + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.projects", primary_key_value: non_existing_record_id), + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.projects", primary_key_value: non_existing_record_id) + ] + end + + let(:loose_fk_definition) do + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + 'projects', + 'issues', + { + column: 'project_id', + on_delete: :async_nullify + } + ) + end + + subject(:cleaner_service) do + described_class.new( + model: Issue, + foreign_key_definition: loose_fk_definition, + deleted_parent_records: deleted_records + ) + end + + context 'when invalid foreign key definition is passed' do + context 'when invalid on_delete argument was given' do + before do + loose_fk_definition.options[:on_delete] = :invalid + end + + it 'raises KeyError' do + expect { cleaner_service.execute }.to raise_error(StandardError, /Invalid on_delete argument/) + end + end + end + + describe 'query generation' do + context 'when single primary key is used' do + let(:issue) { create(:issue) } + + let(:deleted_records) do + [ + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.projects", primary_key_value: issue.project_id) + ] + end + + it 'generates an IN query for nullifying the rows' do + expected_query = %{UPDATE "issues" SET "project_id" = NULL WHERE ("issues"."id") IN (SELECT "issues"."id" FROM "issues" WHERE "issues"."project_id" IN (#{issue.project_id}) LIMIT 500)} + expect(ApplicationRecord.connection).to receive(:execute).with(expected_query).and_call_original + + cleaner_service.execute + + issue.reload + expect(issue.project_id).to be_nil + end + + it 'generates an IN query for deleting the rows' do + loose_fk_definition.options[:on_delete] = :async_delete + + expected_query = %{DELETE FROM "issues" WHERE ("issues"."id") IN (SELECT "issues"."id" FROM "issues" WHERE "issues"."project_id" IN (#{issue.project_id}) LIMIT 1000)} + expect(ApplicationRecord.connection).to receive(:execute).with(expected_query).and_call_original + + cleaner_service.execute + + expect(Issue.exists?(id: issue.id)).to eq(false) + end + end + + context 'when composite primary key is used' do + let!(:user) { create(:user) } + let!(:project) { create(:project) } + + let(:loose_fk_definition) do + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + 'users', + 'project_authorizations', + { + column: 'user_id', + on_delete: :async_delete + } + ) + end + + let(:deleted_records) do + [ + LooseForeignKeys::DeletedRecord.new(fully_qualified_table_name: "#{schema}.users", primary_key_value: user.id) + ] + end + + subject(:cleaner_service) do + described_class.new( + model: ProjectAuthorization, + foreign_key_definition: loose_fk_definition, + deleted_parent_records: deleted_records + ) + end + + before do + project.add_developer(user) + end + + it 'generates an IN query for deleting the rows' do + expected_query = %{DELETE FROM "project_authorizations" WHERE ("project_authorizations"."user_id", "project_authorizations"."project_id", "project_authorizations"."access_level") IN (SELECT "project_authorizations"."user_id", "project_authorizations"."project_id", "project_authorizations"."access_level" FROM "project_authorizations" WHERE "project_authorizations"."user_id" IN (#{user.id}) LIMIT 1000)} + expect(ApplicationRecord.connection).to receive(:execute).with(expected_query).and_call_original + + cleaner_service.execute + + expect(ProjectAuthorization.exists?(user_id: user.id)).to eq(false) + end + + context 'when the query generation is incorrect (paranoid check)' do + it 'raises error if the foreign key condition is missing' do + expect_next_instance_of(LooseForeignKeys::CleanerService) do |instance| + expect(instance).to receive(:delete_query).and_return('wrong query') + end + + expect { cleaner_service.execute }.to raise_error /FATAL: foreign key condition is missing from the generated query/ + end + end + end + + context 'when with_skip_locked parameter is true' do + subject(:cleaner_service) do + described_class.new( + model: Issue, + foreign_key_definition: loose_fk_definition, + deleted_parent_records: deleted_records, + with_skip_locked: true + ) + end + + it 'generates a query with the SKIP LOCKED clause' do + expect(ApplicationRecord.connection).to receive(:execute).with(/FOR UPDATE SKIP LOCKED/).and_call_original + + cleaner_service.execute + end + end + end +end diff --git a/spec/workers/loose_foreign_keys/cleanup_worker_spec.rb b/spec/workers/loose_foreign_keys/cleanup_worker_spec.rb new file mode 100644 index 00000000000..574534a5d47 --- /dev/null +++ b/spec/workers/loose_foreign_keys/cleanup_worker_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::CleanupWorker do + include MigrationsHelpers + + def create_table_structure + migration = ActiveRecord::Migration.new.extend(Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers) + + migration.create_table :loose_fk_parent_table_1 + migration.create_table :loose_fk_parent_table_2 + + migration.create_table :loose_fk_child_table_1_1 do |t| + t.bigint :parent_id + end + + migration.create_table :loose_fk_child_table_1_2 do |t| + t.bigint :parent_id_with_different_column + end + + migration.create_table :loose_fk_child_table_2_1 do |t| + t.bigint :parent_id + end + + migration.track_record_deletions(:loose_fk_parent_table_1) + migration.track_record_deletions(:loose_fk_parent_table_2) + end + + let!(:parent_model_1) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_parent_table_1' + + include LooseForeignKey + + loose_foreign_key :loose_fk_child_table_1_1, :parent_id, on_delete: :async_delete + loose_foreign_key :loose_fk_child_table_1_2, :parent_id_with_different_column, on_delete: :async_nullify + end + end + + let!(:parent_model_2) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_parent_table_2' + + include LooseForeignKey + + loose_foreign_key :loose_fk_child_table_2_1, :parent_id, on_delete: :async_delete + end + end + + let!(:child_model_1) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_1_1' + end + end + + let!(:child_model_2) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_1_2' + end + end + + let!(:child_model_3) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_child_table_2_1' + end + end + + let(:loose_fk_parent_table_1) { table(:loose_fk_parent_table_1) } + let(:loose_fk_parent_table_2) { table(:loose_fk_parent_table_2) } + let(:loose_fk_child_table_1_1) { table(:loose_fk_child_table_1_1) } + let(:loose_fk_child_table_1_2) { table(:loose_fk_child_table_1_2) } + let(:loose_fk_child_table_2_1) { table(:loose_fk_child_table_2_1) } + + before(:all) do + create_table_structure + end + + after(:all) do + migration = ActiveRecord::Migration.new + + migration.drop_table :loose_fk_parent_table_1 + migration.drop_table :loose_fk_parent_table_2 + migration.drop_table :loose_fk_child_table_1_1 + migration.drop_table :loose_fk_child_table_1_2 + migration.drop_table :loose_fk_child_table_2_1 + end + + before do + parent_record_1 = loose_fk_parent_table_1.create! + loose_fk_child_table_1_1.create!(parent_id: parent_record_1.id) + loose_fk_child_table_1_2.create!(parent_id_with_different_column: parent_record_1.id) + + parent_record_2 = loose_fk_parent_table_1.create! + 2.times { loose_fk_child_table_1_1.create!(parent_id: parent_record_2.id) } + 3.times { loose_fk_child_table_1_2.create!(parent_id_with_different_column: parent_record_2.id) } + + parent_record_3 = loose_fk_parent_table_2.create! + 5.times { loose_fk_child_table_2_1.create!(parent_id: parent_record_3.id) } + + parent_model_1.delete_all + parent_model_2.delete_all + end + + it 'cleans up all rows' do + described_class.new.perform + + expect(loose_fk_child_table_1_1.count).to eq(0) + expect(loose_fk_child_table_1_2.where(parent_id_with_different_column: nil).count).to eq(4) + expect(loose_fk_child_table_2_1.count).to eq(0) + end + + context 'when deleting in batches' do + before do + stub_const('LooseForeignKeys::CleanupWorker::BATCH_SIZE', 2) + end + + it 'cleans up all rows' do + expect(LooseForeignKeys::BatchCleanerService).to receive(:new).exactly(:twice).and_call_original + + described_class.new.perform + + expect(loose_fk_child_table_1_1.count).to eq(0) + expect(loose_fk_child_table_1_2.where(parent_id_with_different_column: nil).count).to eq(4) + expect(loose_fk_child_table_2_1.count).to eq(0) + end + end + + context 'when the deleted rows count limit have been reached' do + def count_deletable_rows + loose_fk_child_table_1_1.count + loose_fk_child_table_2_1.count + end + + before do + stub_const('LooseForeignKeys::ModificationTracker::MAX_DELETES', 2) + stub_const('LooseForeignKeys::CleanerService::DELETE_LIMIT', 1) + end + + it 'cleans up 2 rows' do + expect { described_class.new.perform }.to change { count_deletable_rows }.by(-2) + end + end + + context 'when the loose_foreign_key_cleanup feature flag is off' do + before do + stub_feature_flags(loose_foreign_key_cleanup: false) + end + + it 'does nothing' do + expect { described_class.new.perform }.not_to change { LooseForeignKeys::DeletedRecord.status_processed.count } + end + end +end |