Merge branch 'ab/create-table' into 'master'

create_table with text limits

See merge request gitlab-org/gitlab!69304

db/migrate/20201029144444_create_vulnerability_finding_links.rb

@@ -11,8 +11,8 @@ class CreateVulnerabilityFindingLinks < ActiveRecord::Migration[6.0]
     create_table :vulnerability_finding_links, if_not_exists: true do |t|
       t.timestamps_with_timezone null: false
       t.references :vulnerability_occurrence, index: { name: 'finding_links_on_vulnerability_occurrence_id' }, null: false, foreign_key: { on_delete: :cascade }
-      t.text :name, limit: 255
-      t.text :url, limit: 2048, null: false
+      t.text :name
+      t.text :url, null: false
     end
 
     add_text_limit :vulnerability_finding_links, :name, 255

doc/development/database/strings_and_the_text_data_type.md

@@ -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: 100` 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,34 +43,24 @@ 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 `limit:` attribute to Rails' `#text` method, which allows adding a limit
+for this column.
 
 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
-
-      t.text_limit :title, 128
-      t.text_limit :notes, 1024
+      t.text :title, limit: 128
+      t.text :notes, limit: 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

lib/gitlab/database/migration_helpers.rb

@@ -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.

lib/gitlab/database/migration_helpers/v2.rb

@@ -6,6 +6,65 @@ module Gitlab
       module V2
         include Gitlab::Database::MigrationHelpers
 
+        # Superseded by `create_table` override below
+        def create_table_with_constraints(*_)
+          raise <<~EOM
+            #create_table_with_constraints is not supported anymore - use #create_table instead, for example:
+
+              create_table :db_guides do |t|
+                t.bigint :stars, default: 0, null: false
+                t.text :title, limit: 128
+                t.text :notes, limit: 1024
+
+                t.check_constraint 'stars > 1000', name: 'so_many_stars'
+              end
+
+            See https://docs.gitlab.com/ee/development/database/strings_and_the_text_data_type.html
+          EOM
+        end
+
+        # 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 :db_guides do |t|
+        #      t.bigint :stars, default: 0, null: false
+        #      t.text :title, limit: 128
+        #      t.text :notes, limit: 1024
+        #
+        #      t.check_constraint 'stars > 1000', name: 'so_many_stars'
+        #    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) do |column_name, **kwargs|
+              limit = kwargs.delete(:limit)
+
+              super(column_name, **kwargs)
+
+              if limit
+                # rubocop:disable GitlabSecurity/PublicSend
+                name = helper_context.send(:text_limit_name, table_name, column_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
+            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+.

rubocop/cop/migration/add_limit_to_text_columns.rb

@@ -13,8 +13,13 @@ module RuboCop
       class AddLimitToTextColumns < RuboCop::Cop::Cop
         include MigrationHelpers
 
+        TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE = 2021_09_10_00_00_00
+
         MSG = 'Text columns should always have a limit set (255 is suggested). ' \
-          'You can add a limit to a `text` column by using `add_text_limit`'
+          'You can add a limit to a `text` column by using `add_text_limit` or by using `.text... limit: 255` inside `create_table`'
+
+        TEXT_LIMIT_ATTRIBUTE_NOT_ALLOWED = 'Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. ' \
+        'You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table`'
 
         def_node_matcher :reverting?, <<~PATTERN
           (def :down ...)
@@ -37,15 +42,29 @@ module RuboCop
           node.each_descendant(:send) do |send_node|
             next unless text_operation?(send_node)
 
-            # We require a limit for the same table and attribute name
-            if text_limit_missing?(node, *table_and_attribute_name(send_node))
-              add_offense(send_node, location: :selector)
+            if text_operation_with_limit?(send_node)
+              add_offense(send_node, location: :selector, message: TEXT_LIMIT_ATTRIBUTE_NOT_ALLOWED) if version(node) < TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE
+            else
+              # We require a limit for the same table and attribute name
+              if text_limit_missing?(node, *table_and_attribute_name(send_node))
+                add_offense(send_node, location: :selector)
+              end
             end
           end
         end
 
         private
 
+        def text_operation_with_limit?(node)
+          migration_method = node.children[1]
+
+          return unless migration_method == :text
+
+          if attributes = node.children[3]
+            attributes.pairs.find { |pair| pair.key.value == :limit }.present?
+          end
+        end
+
         def text_operation?(node)
           # Don't complain about text arrays
           return false if array_column?(node)

spec/lib/gitlab/database/migration_helpers/v2_spec.rb

@@ -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,34 @@ 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 'using a limit: attribute on .text' 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, limit: 100
+        end
+
+        expect_table_columns_to_match(column_attributes, table_name)
+        expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 100')
+      end
+    end
+  end
+
   describe '#with_lock_retries' do
     let(:model) do
       ActiveRecord::Migration.new.extend(described_class)

spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
 
     before do
       allow(cop).to receive(:in_migration?).and_return(true)
+      allow(cop).to receive(:version).and_return(described_class::TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE + 5)
     end
 
     context 'when text columns are defined without a limit' do
@@ -26,7 +27,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,13 +62,10 @@ 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
-
-                t.text_limit :title, 100
-                t.text_limit :description, 255
+                t.text :title, limit: 100
+                t.text :description, limit: 255
               end
 
               add_column :test_text_limits, :email, :text
@@ -82,6 +80,30 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
           end
         RUBY
       end
+
+      context 'for migrations before 2021_09_10_00_00_00' do
+        it 'when limit: attribute is used (which is not supported yet for this version): registers an offense' do
+          allow(cop).to receive(:version).and_return(described_class::TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE - 5)
+
+          expect_offense(<<~RUBY)
+            class TestTextLimits < ActiveRecord::Migration[6.0]
+              def up
+                create_table :test_text_limit_attribute do |t|
+                  t.integer :test_id, null: false
+                  t.text :name, limit: 100
+                    ^^^^ Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table`
+                end
+
+                create_table_with_constraints :test_text_limit_attribute do |t|
+                  t.integer :test_id, null: false
+                  t.text :name, limit: 100
+                    ^^^^ Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table`
+                end
+              end
+            end
+          RUBY
+        end
+      end
     end
 
     context 'when text array columns are defined without a limit' do