Merge branch 'ignore-column-remove-never' into 'master'

Introduce ignore_column remove_never: true

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138666



Merged-by: Felipe Artur <fcardozo@gitlab.com>
Approved-by: Leaminn Ma <lma@gitlab.com>
Approved-by: Felipe Artur <fcardozo@gitlab.com>
Reviewed-by: Leaminn Ma <lma@gitlab.com>
Co-authored-by: Dylan Griffith <dyl.griffith@gmail.com>
Co-authored-by: Leaminn Ma <lma@gitlab.com>

.rubocop_todo/cop/ignored_columns.yml

 ---
 Cop/IgnoredColumns:
   Exclude:
-    - 'app/models/loose_foreign_keys/deleted_record.rb'
     - 'ee/lib/ee/gitlab/background_migration/create_vulnerability_links.rb'
     - 'ee/lib/ee/gitlab/background_migration/migrate_vulnerabilities_feedback_to_vulnerabilities_state_transition.rb'
     - 'spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb'

app/models/batched_git_ref_updates/deletion.rb

@@ -15,7 +15,7 @@ class Deletion < ApplicationRecord
     # This column must be ignored otherwise Rails will cache the default value and `bulk_insert!` will start saving
     # incorrect partition_id.
-    ignore_column :partition_id, remove_with: '3000.0', remove_after: '3000-01-01'
+    ignore_column :partition_id, remove_never: true
     belongs_to :project, inverse_of: :to_be_deleted_git_refs

app/models/concerns/ignorable_columns.rb

@@ -3,13 +3,11 @@
 module IgnorableColumns
   extend ActiveSupport::Concern
-  ColumnIgnore = Struct.new(:remove_after, :remove_with) do
+  ColumnIgnore = Struct.new(:remove_after, :remove_with, :remove_never) do
     def safe_to_remove?
-      Date.today > remove_after
-    end
-    def to_s
-      "(#{remove_after}, #{remove_with})"
+      return false if remove_never
+      Date.today > remove_after
     end
   end
@@ -17,14 +15,17 @@ def to_s
     # Ignore database columns in a model
     #
     # Indicate the earliest date and release we can stop ignoring the column with +remove_after+ (a date string) and +remove_with+ (a release)
-    def ignore_columns(*columns, remove_after:, remove_with:)
-      raise ArgumentError, 'Please indicate when we can stop ignoring columns with remove_after (date string YYYY-MM-DD), example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless Gitlab::Regex.utc_date_regex.match?(remove_after)
-      raise ArgumentError, 'Please indicate in which release we can stop ignoring columns with remove_with, example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_with
+    def ignore_columns(*columns, remove_after: nil, remove_with: nil, remove_never: false)
+      unless remove_never
+        raise ArgumentError, 'Please indicate when we can stop ignoring columns with remove_after (date string YYYY-MM-DD), example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_after && Gitlab::Regex.utc_date_regex.match?(remove_after)
+        raise ArgumentError, 'Please indicate in which release we can stop ignoring columns with remove_with, example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_with
+      end
       self.ignored_columns += columns.flatten # rubocop:disable Cop/IgnoredColumns
       columns.flatten.each do |column|
-        self.ignored_columns_details[column.to_sym] = ColumnIgnore.new(Date.parse(remove_after), remove_with)
+        remove_after_date = remove_after ? Date.parse(remove_after) : nil
+        self.ignored_columns_details[column.to_sym] = ColumnIgnore.new(remove_after_date, remove_with, remove_never)
       end
     end

app/models/loose_foreign_keys/deleted_record.rb

@@ -6,9 +6,13 @@ class LooseForeignKeys::DeletedRecord < Gitlab::Database::SharedModel
   PARTITION_DURATION = 1.day
   include PartitionedTable
+  include IgnorableColumns
   self.primary_key = :id
-  self.ignored_columns = %i[partition]
+
+  # This column must be ignored otherwise Rails will cache the default value and `bulk_insert!` will start saving
+  # incorrect partition.
+  ignore_column :partition, remove_never: true
   partitioned_by :partition, strategy: :sliding_list,
     next_partition_if: -> (active_partition) do

spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb

@@ -53,11 +53,11 @@
         expect(subject.execute).to eq(
           [
             ['Testing::A', {
-              'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0'),
-              'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1')
+              'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0', false),
+              'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1', false)
             }],
             ['Testing::B', {
-              'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0')
+              'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0', false)
             }]
           ])
       end

spec/models/concerns/ignorable_columns_spec.rb

@@ -27,6 +27,12 @@
     expect { subject.ignore_columns(:name, remove_after: nil, remove_with: 12.6) }.to raise_error(ArgumentError, /Please indicate/)
   end
+  it 'allows setting remove_never: true and not setting other remove options' do
+    expect do
+      subject.ignore_columns(%i[name created_at], remove_never: true)
+    end.to change { subject.ignored_columns }.from([]).to(%w[name created_at])
+  end
+
   it 'requires remove_after attribute to be set' do
     expect { subject.ignore_columns(:name, remove_after: "not a date", remove_with: 12.6) }.to raise_error(ArgumentError, /Please indicate/)
   end
@@ -73,9 +79,11 @@
   end
   describe IgnorableColumns::ColumnIgnore do
-    subject { described_class.new(remove_after, remove_with) }
+    subject { described_class.new(remove_after, remove_with, remove_never) }
+    let(:remove_after) { nil }
     let(:remove_with) { double }
+    let(:remove_never) { false }
     describe '#safe_to_remove?' do
       context 'after remove_after date has passed' do
@@ -93,6 +101,14 @@
           expect(subject.safe_to_remove?).to be_falsey
         end
       end
+
+      context 'with remove_never: true' do
+        let(:remove_never) { true }
+
+        it 'is false' do
+          expect(subject.safe_to_remove?).to be_falsey
+        end
+      end
     end
   end
 end