Skip to content

Add migration helpers for fetching and copying check constraints

Yannis Roussos requested to merge 247489-update-create-column-from-to-also-copy-constraints into master

What does this MR do?

This MR adds migration helpers for copying check constraints

  • Adds check_constraints_for(table, column, schema: nil) private method to Gitlab::Database::MigrationHelpers.

    Returns all the check constraints defined for a column.

  • Adds the copy_check_constraints(table, old, new, schema: nil) migration helper.

    It copies all constraints defined in column old to column new.

  • Updates the create_column_from migration helper to also copy all existing check constraints to the new column.

  • Updates the specs for all helpers that use create_column_from

  • Adds new specs for copy_check_constraints

Related Issue: #247489 (closed)

What does this MR does not do?

It only focusses on check constraints and adding support for them to create_column_from

It does not address the existing issue with rolling back a create_column_from that converts NOT NULL constraints to column IS NOT NULL check constraints.

That will be discussed and dealt with in #259699 (closed)

Proof of concept

Given the following test migration:

class TestCopyCheckConstraints < ActiveRecord::Migration[6.0]
  include Gitlab::Database::MigrationHelpers

  DOWNTIME = false
  disable_ddl_transaction!

  def up
    create_table :test_copy_check_constraints, if_not_exists: true, id: false do |t|
      t.text :aaaa
      t.text :aaaa2
      t.text :bbbb
      t.text :bbbb2
      t.text :bbbb3
      t.bigint :count, null: false
      t.bigint :count2
    end

    add_not_null_constraint :test_copy_check_constraints, :bbbb
    add_text_limit :test_copy_check_constraints, :bbbb, 255

    add_not_null_constraint :test_copy_check_constraints, :bbbb3
    add_text_limit :test_copy_check_constraints, :bbbb3, 255

    add_check_constraint :test_copy_check_constraints, 'count > 0', 'check_count_positive'

    # Nothing should happen
    copy_check_constraints :test_copy_check_constraints, :aaaa, :aaaa2

    # The two constraints should be copied
    copy_check_constraints :test_copy_check_constraints, :bbbb, :bbbb2

    # No additional constraints should be added to bbbb3 as the ones
    # copied from :bbb should try to use the same names
    copy_check_constraints :test_copy_check_constraints, :bbbb, :bbbb3

    # The custom check constraint should be copied and use an auto generated name
    # `null:false` should not be copied as a check constraint
    copy_check_constraints :test_copy_check_constraints, :count, :count2

    # The custom check constraint should be copied and use an auto generated name
    # `null:false` should be converted to a check constraint
    create_column_from :test_copy_check_constraints, :count, :new_count
  end

  def down
    drop_table :test_copy_check_constraints
  end
end

When the migration runs, the correct constraints are added to the table (diff annotated for better reading experience)

$ bundle exec rake db:migrate
 ... ... ...

$ git diff db/structure.sql
... ...
+CREATE TABLE test_copy_check_constraints (
+    aaaa text,
+    aaaa2 text,
+    bbbb text,
+    bbbb2 text,
+    bbbb3 text,
+    count bigint NOT NULL,
+    count2 bigint,
+    new_count bigint,

# Manually added constraints for bbbb
+    CONSTRAINT check_d7d49d475d CHECK ((bbbb IS NOT NULL))
+    CONSTRAINT check_48560e521e CHECK ((char_length(bbbb) <= 255)),

# All constraints were properly copied from bbbb to bbbb2
+    CONSTRAINT check_9dcd761e37 CHECK ((bbbb2 IS NOT NULL)),
+    CONSTRAINT check_17c59b658a CHECK ((char_length(bbbb2) <= 255)),

# No duplicate constraints added for bbbb3 - only the manually added ones are there
+    CONSTRAINT check_46e55d045a CHECK ((bbbb3 IS NOT NULL)),
+    CONSTRAINT check_395c951858 CHECK ((char_length(bbbb3) <= 255)),

# Manually added constraint for count
+    CONSTRAINT check_count_positive CHECK ((count > 0)),

# Custom check constraint was copied to count2
+    CONSTRAINT check_1fc2397501 CHECK ((count2 > 0)),

# new_count was created from count with only difference the switch of the NOT NULL
+    CONSTRAINT check_48b708485f CHECK ((new_count IS NOT NULL)),
+    CONSTRAINT check_ce41781033 CHECK ((new_count > 0)),

+);

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Yannis Roussos

Merge request reports