Merge branch 'validate-check-constraint-name-length' into 'master'

Validate CHECK CONSTRAINT name length

See merge request gitlab-org/gitlab!33856

db/migrate/20200424135319_create_nuget_dependency_link_metadata.rb

@@ -7,7 +7,7 @@ class CreateNugetDependencyLinkMetadata < ActiveRecord::Migration[6.0]
 
   disable_ddl_transaction!
 
-  CONSTRAINT_NAME = 'packages_nuget_dependency_link_metadata_target_framework_constraint'
+  CONSTRAINT_NAME = 'packages_nuget_dependency_link_metadata_target_framework_constr'
 
   def up
     unless table_exists?(:packages_nuget_dependency_link_metadata)

db/migrate/20200519074709_update_resource_state_events_constraint_to_support_epic_id.rb

@@ -8,7 +8,7 @@ class UpdateResourceStateEventsConstraintToSupportEpicId < ActiveRecord::Migrati
   disable_ddl_transaction!
 
   OLD_CONSTRAINT = 'resource_state_events_must_belong_to_issue_or_merge_request'
-  CONSTRAINT_NAME = 'resource_state_events_must_belong_to_issue_or_merge_request_or_epic'
+  CONSTRAINT_NAME = 'resource_state_events_must_belong_to_issue_or_merge_request_or_'
 
   def up
     remove_check_constraint :resource_state_events, OLD_CONSTRAINT

db/migrate/20200603073101_change_constraint_name_on_resource_state_events.rb

@@ -4,7 +4,7 @@ class ChangeConstraintNameOnResourceStateEvents < ActiveRecord::Migration[6.0]
   DOWNTIME = false
 
   NEW_CONSTRAINT_NAME = 'state_events_must_belong_to_issue_or_merge_request_or_epic'
-  OLD_CONSTRAINT_NAME = 'resource_state_events_must_belong_to_issue_or_merge_request_or_epic'
+  OLD_CONSTRAINT_NAME = 'resource_state_events_must_belong_to_issue_or_merge_request_or_'
 
   def up
     execute "ALTER TABLE resource_state_events RENAME CONSTRAINT #{OLD_CONSTRAINT_NAME} TO #{NEW_CONSTRAINT_NAME};"

lib/gitlab/database/migration_helpers.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module Database
     module MigrationHelpers
+      # https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
+      MAX_IDENTIFIER_NAME_LENGTH = 63
       BACKGROUND_MIGRATION_BATCH_SIZE = 1000 # Number of rows to process per job
       BACKGROUND_MIGRATION_JOB_BUFFER_SIZE = 1000 # Number of jobs to bulk queue at a time
 
@@ -1209,6 +1211,8 @@ into similar problems in the future (e.g. when new tables are created).
       #
       # rubocop:disable Gitlab/RailsLogger
       def add_check_constraint(table, check, constraint_name, validate: true)
+        validate_check_constraint_name!(constraint_name)
+
         # Transactions would result in ALTER TABLE locks being held for the
         # duration of the transaction, defeating the purpose of this method.
         if transaction_open?
@@ -1244,6 +1248,8 @@ into similar problems in the future (e.g. when new tables are created).
       end
 
       def validate_check_constraint(table, constraint_name)
+        validate_check_constraint_name!(constraint_name)
+
         unless check_constraint_exists?(table, constraint_name)
           raise missing_schema_object_message(table, "check constraint", constraint_name)
         end
@@ -1256,6 +1262,8 @@ into similar problems in the future (e.g. when new tables are created).
       end
 
       def remove_check_constraint(table, constraint_name)
+        validate_check_constraint_name!(constraint_name)
+
         # DROP CONSTRAINT requires an EXCLUSIVE lock
         # Use with_lock_retries to make sure that this will not timeout
         with_lock_retries do
@@ -1330,6 +1338,12 @@ into similar problems in the future (e.g. when new tables are created).
 
       private
 
+      def validate_check_constraint_name!(constraint_name)
+        if constraint_name.to_s.length > MAX_IDENTIFIER_NAME_LENGTH
+          raise "The maximum allowed constraint name is #{MAX_IDENTIFIER_NAME_LENGTH} characters"
+        end
+      end
+
       def statement_timeout_disabled?
         # This is a string of the form "100ms" or "0" when disabled
         connection.select_value('SHOW statement_timeout') == "0"

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -2075,6 +2075,34 @@ describe Gitlab::Database::MigrationHelpers do
       allow(model).to receive(:check_constraint_exists?).and_return(false)
     end
 
+    context 'constraint name validation' do
+      it 'raises an error when too long' do
+        expect do
+          model.add_check_constraint(
+            :test_table,
+            'name IS NOT NULL',
+            'a' * (Gitlab::Database::MigrationHelpers::MAX_IDENTIFIER_NAME_LENGTH + 1)
+          )
+        end.to raise_error(RuntimeError)
+      end
+
+      it 'does not raise error when the length is acceptable' do
+        constraint_name = 'a' * Gitlab::Database::MigrationHelpers::MAX_IDENTIFIER_NAME_LENGTH
+
+        expect(model).to receive(:transaction_open?).and_return(false)
+        expect(model).to receive(:check_constraint_exists?).and_return(false)
+        expect(model).to receive(:with_lock_retries).and_call_original
+        expect(model).to receive(:execute).with(/ADD CONSTRAINT/)
+
+        model.add_check_constraint(
+          :test_table,
+          'name IS NOT NULL',
+          constraint_name,
+          validate: false
+        )
+      end
+    end
+
     context 'inside a transaction' do
       it 'raises an error' do
         expect(model).to receive(:transaction_open?).and_return(true)