Merge branch '345196-update-timestamps-with-timezone-helper' into 'master'

Update `add_timestamps_with_timezone` helper

See merge request gitlab-org/gitlab!74121

lib/gitlab/database/migration_helpers.rb

@@ -10,8 +10,6 @@ module Gitlab
 
       # https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
       MAX_IDENTIFIER_NAME_LENGTH = 63
-
-      PERMITTED_TIMESTAMP_COLUMNS = %i[created_at updated_at deleted_at].to_set.freeze
       DEFAULT_TIMESTAMP_COLUMNS = %i[created_at updated_at].freeze
 
       # Adds `created_at` and `updated_at` columns with timezone information.
@@ -28,33 +26,23 @@ module Gitlab
       #  :default - The default value for the column.
       #  :null - When set to `true` the column will allow NULL values.
       #        The default is to not allow NULL values.
-      #  :columns - the column names to create. Must be one
-      #             of `Gitlab::Database::MigrationHelpers::PERMITTED_TIMESTAMP_COLUMNS`.
+      #  :columns - the column names to create. Must end with `_at`.
       #             Default value: `DEFAULT_TIMESTAMP_COLUMNS`
       #
       # All options are optional.
       def add_timestamps_with_timezone(table_name, options = {})
-        options[:null] = false if options[:null].nil?
         columns = options.fetch(:columns, DEFAULT_TIMESTAMP_COLUMNS)
-        default_value = options[:default]
-
-        validate_not_in_transaction!(:add_timestamps_with_timezone, 'with default value') if default_value
 
         columns.each do |column_name|
           validate_timestamp_column_name!(column_name)
 
-          # If default value is presented, use `add_column_with_default` method instead.
-          if default_value
-            add_column_with_default(
+          add_column(
             table_name,
             column_name,
             :datetime_with_timezone,
-              default: default_value,
-              allow_null: options[:null]
+            default: options[:default],
+            null: options[:null] || false
           )
-          else
-            add_column(table_name, column_name, :datetime_with_timezone, **options)
-          end
         end
       end
 
@@ -1818,11 +1806,11 @@ into similar problems in the future (e.g. when new tables are created).
       end
 
       def validate_timestamp_column_name!(column_name)
-        return if PERMITTED_TIMESTAMP_COLUMNS.member?(column_name)
+        return if column_name.to_s.end_with?('_at')
 
         raise <<~MESSAGE
           Illegal timestamp column name! Got #{column_name}.
-          Must be one of: #{PERMITTED_TIMESTAMP_COLUMNS.to_a}
+          Must end with `_at`}
         MESSAGE
       end
 

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -31,16 +31,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
   end
 
   describe '#add_timestamps_with_timezone' do
-    let(:in_transaction) { false }
-
-    before do
-      allow(model).to receive(:transaction_open?).and_return(in_transaction)
-      allow(model).to receive(:disable_statement_timeout)
-    end
-
     it 'adds "created_at" and "updated_at" fields with the "datetime_with_timezone" data type' do
       Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name|
-        expect(model).to receive(:add_column).with(:foo, column_name, :datetime_with_timezone, { null: false })
+        expect(model).to receive(:add_column)
+          .with(:foo, column_name, :datetime_with_timezone, { default: nil, null: false })
       end
 
       model.add_timestamps_with_timezone(:foo)
@@ -48,7 +42,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
 
     it 'can disable the NOT NULL constraint' do
       Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name|
-        expect(model).to receive(:add_column).with(:foo, column_name, :datetime_with_timezone, { null: true })
+        expect(model).to receive(:add_column)
+          .with(:foo, column_name, :datetime_with_timezone, { default: nil, null: true })
       end
 
       model.add_timestamps_with_timezone(:foo, null: true)
@@ -64,9 +59,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
     it 'can add choice of acceptable columns' do
       expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, anything)
       expect(model).to receive(:add_column).with(:foo, :deleted_at, :datetime_with_timezone, anything)
+      expect(model).to receive(:add_column).with(:foo, :processed_at, :datetime_with_timezone, anything)
       expect(model).not_to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, anything)
 
-      model.add_timestamps_with_timezone(:foo, columns: [:created_at, :deleted_at])
+      model.add_timestamps_with_timezone(:foo, columns: [:created_at, :deleted_at, :processed_at])
     end
 
     it 'cannot add unacceptable column names' do
@@ -74,29 +70,6 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
         model.add_timestamps_with_timezone(:foo, columns: [:bar])
       end.to raise_error %r/Illegal timestamp column name/
     end
-
-    context 'in a transaction' do
-      let(:in_transaction) { true }
-
-      before do
-        allow(model).to receive(:add_column).with(any_args).and_call_original
-        allow(model).to receive(:add_column)
-          .with(:foo, anything, :datetime_with_timezone, anything)
-          .and_return(nil)
-      end
-
-      it 'cannot add a default value' do
-        expect do
-          model.add_timestamps_with_timezone(:foo, default: :i_cause_an_error)
-        end.to raise_error %r/add_timestamps_with_timezone/
-      end
-
-      it 'can add columns without defaults' do
-        expect do
-          model.add_timestamps_with_timezone(:foo)
-        end.not_to raise_error
-      end
-    end
   end
 
   describe '#create_table_with_constraints' do