Commit f9360924 authored by Andreas Brandl's avatar Andreas Brandl

Extend Rails' create_table method for text limits

This extends Rails' method create_table and adds a `#text_limit` method
for convenience.

Additionally, we remove the with_lock_retries wrapping here to avoid
creating subtransactions. For now, this has to be dealt with explicitly
at the migration level.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/337792
parent a50a945f
......@@ -11,11 +11,13 @@ info: To determine the technical writer assigned to the Stage/Group associated w
When adding new columns that will be used to store strings or other textual information:
1. We always use the `text` data type instead of the `string` data type.
1. `text` columns should always have a limit set, either by using the `create_table_with_constraints` helper
when creating a table, or by using the `add_text_limit` when altering an existing table.
1. `text` columns should always have a limit set, either by using the `create_table` with
the `#text_limit` helper (see below) when creating a table, or by using the `add_text_limit`
when altering an existing table.
The `text` data type can not be defined with a limit, so `create_table_with_constraints` and `add_text_limit` enforce
that by adding a [check constraint](https://www.postgresql.org/docs/11/ddl-constraints.html) on the column.
The standard Rails `text` column type can not be defined with a limit, but we extend `create_table` to
add a `limit: 255` option. Outside of `create_table`, `add_text_limit` can be used to add a [check constraint](https://www.postgresql.org/docs/11/ddl-constraints.html)
to an already existing column.
## Background information
......@@ -41,15 +43,16 @@ Don't use text columns for `attr_encrypted` attributes. Use a
## Create a new table with text columns
When adding a new table, the limits for all text columns should be added in the same migration as
the table creation.
the table creation. We add a `#text_limit` method to Rails' `create_table`, which allows adding limits
for text columns.
For example, consider a migration that creates a table with two text columns,
`db/migrate/20200401000001_create_db_guides.rb`:
```ruby
class CreateDbGuides < Gitlab::Database::Migration[1.0]
def up
create_table_with_constraints :db_guides do |t|
def change
create_table :db_guides do |t|
t.bigint :stars, default: 0, null: false
t.text :title
t.text :notes
......@@ -58,17 +61,9 @@ class CreateDbGuides < Gitlab::Database::Migration[1.0]
t.text_limit :notes, 1024
end
end
def down
# No need to drop the constraints, drop_table takes care of everything
drop_table :db_guides
end
end
```
Note that the `create_table_with_constraints` helper uses the `with_lock_retries` helper
internally, so we don't need to manually wrap the method call in the migration.
## Add a text column to an existing table
Adding a column to an existing table requires an exclusive lock for that table. Even though that lock
......
......@@ -73,6 +73,7 @@ module Gitlab
end
end
# @deprecated Use `create_table` in V2 instead
#
# Creates a new table, optionally allowing the caller to add check constraints to the table.
# Aside from that addition, this method should behave identically to Rails' `create_table` method.
......
......@@ -6,6 +6,46 @@ module Gitlab
module V2
include Gitlab::Database::MigrationHelpers
# Superseded by `create_table` override below
undef_method :create_table_with_constraints
# Creates a new table, optionally allowing the caller to add text limit constraints to the table.
# This method only extends Rails' `create_table` method
#
# Example:
#
# create_table :some_table do |t|
# t.integer :thing, null: false
# t.text :other_thing
#
# t.check_constraint 'thing IS NOT NULL', name: 'thing_is_not_null'
# t.text_limit :other_thing, 255
# end
#
# See Rails' `create_table` for more info on the available arguments.
#
# When adding foreign keys to other tables, consider wrapping the call into a with_lock_retries block
# to avoid traffic stalls.
def create_table(table_name, *args, **kwargs, &block)
helper_context = self
super do |t|
t.define_singleton_method(:text_limit) do |column_name, limit, name: nil|
# rubocop:disable GitlabSecurity/PublicSend
name = helper_context.send(:text_limit_name, table_name, column_name, name: name)
helper_context.send(:validate_check_constraint_name!, name)
# rubocop:enable GitlabSecurity/PublicSend
column_name = helper_context.quote_column_name(column_name)
definition = "char_length(#{column_name}) <= #{limit}"
t.check_constraint(definition, name: name)
end
t.instance_eval(&block) unless block.nil?
end
end
# Executes the block with a retry mechanism that alters the +lock_timeout+ and +sleep_time+ between attempts.
# The timings can be controlled via the +timing_configuration+ parameter.
# If the lock was not acquired within the retry period, a last attempt is made without using +lock_timeout+.
......
......@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe Gitlab::Database::MigrationHelpers::V2 do
include Database::TriggerHelpers
include Database::TableSchemaHelpers
let(:migration) do
ActiveRecord::Migration.new.extend(described_class)
......@@ -221,6 +222,95 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do
end
end
describe '#create_table' do
let(:table_name) { :test_table }
let(:column_attributes) do
[
{ name: 'id', sql_type: 'bigint', null: false, default: nil },
{ name: 'created_at', sql_type: 'timestamp with time zone', null: false, default: nil },
{ name: 'updated_at', sql_type: 'timestamp with time zone', null: false, default: nil },
{ name: 'some_id', sql_type: 'integer', null: false, default: nil },
{ name: 'active', sql_type: 'boolean', null: false, default: 'true' },
{ name: 'name', sql_type: 'text', null: true, default: nil }
]
end
context 'when no check constraints are defined' do
it 'creates the table as expected' do
migration.create_table table_name do |t|
t.timestamps_with_timezone
t.integer :some_id, null: false
t.boolean :active, null: false, default: true
t.text :name
end
expect_table_columns_to_match(column_attributes, table_name)
end
end
context 'when a text_limit defined' do
context 'when the text_limit is explicity named' do
it 'creates the table as expected' do
migration.create_table table_name do |t|
t.timestamps_with_timezone
t.integer :some_id, null: false
t.boolean :active, null: false, default: true
t.text :name
t.text_limit :name, 255, name: 'check_name_length'
t.check_constraint 'some_id > 0', name: 'some_id_is_positive'
end
expect_table_columns_to_match(column_attributes, table_name)
expect_check_constraint(table_name, 'check_name_length', 'char_length(name) <= 255')
expect_check_constraint(table_name, 'some_id_is_positive', 'some_id > 0')
end
end
context 'when the text_limit is not named' do
it 'creates the table as expected, naming the text limit' do
migration.create_table table_name do |t|
t.timestamps_with_timezone
t.integer :some_id, null: false
t.boolean :active, null: false, default: true
t.text :name
t.text_limit :name, 255
t.check_constraint 'some_id > 0', name: 'some_id_is_positive'
end
expect_table_columns_to_match(column_attributes, table_name)
expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 255')
expect_check_constraint(table_name, 'some_id_is_positive', 'some_id > 0')
end
end
context 'when text_limit is given invalid name' do
let(:expected_max_length) { described_class::MAX_IDENTIFIER_NAME_LENGTH }
let(:expected_error_message) { "The maximum allowed constraint name is #{expected_max_length} characters" }
context 'when the explicit text limit name is not valid' do
it 'raises an error' do
too_long_length = expected_max_length + 1
expect do
migration.create_table table_name do |t|
t.timestamps_with_timezone
t.integer :some_id, null: false
t.boolean :active, null: false, default: true
t.text :name
t.text_limit :name, 255, name: ('a' * too_long_length)
end
end.to raise_error(expected_error_message)
end
end
end
end
end
describe '#with_lock_retries' do
let(:model) do
ActiveRecord::Migration.new.extend(described_class)
......
......@@ -26,7 +26,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
^^^^ #{msg}
end
create_table_with_constraints :test_text_limits_create do |t|
create_table :test_text_limits_create do |t|
t.integer :test_id, null: false
t.text :title
t.text :description
......@@ -61,7 +61,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
t.text :name
end
create_table_with_constraints :test_text_limits_create do |t|
create_table :test_text_limits_create do |t|
t.integer :test_id, null: false
t.text :title
t.text :description
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment