Commit 3eb7f79f authored by Andreas Brandl's avatar Andreas Brandl

Simplify limits to text columns on new tables

Changes the rubocop to complain about usage for new
migrations.
parent f9360924
...@@ -11,8 +11,8 @@ class CreateVulnerabilityFindingLinks < ActiveRecord::Migration[6.0] ...@@ -11,8 +11,8 @@ class CreateVulnerabilityFindingLinks < ActiveRecord::Migration[6.0]
create_table :vulnerability_finding_links, if_not_exists: true do |t| create_table :vulnerability_finding_links, if_not_exists: true do |t|
t.timestamps_with_timezone null: false 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.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 :name, limit: 255 # rubocop:disable Migration/AddLimitToTextColumns
t.text :url, limit: 2048, null: false t.text :url, limit: 2048, null: false # rubocop:disable Migration/AddLimitToTextColumns
end end
add_text_limit :vulnerability_finding_links, :name, 255 add_text_limit :vulnerability_finding_links, :name, 255
......
...@@ -12,7 +12,7 @@ When adding new columns that will be used to store strings or other textual info ...@@ -12,7 +12,7 @@ When adding new columns that will be used to store strings or other textual info
1. We always use the `text` data type instead of the `string` data type. 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 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` the `#text ... limit: 100` helper (see below) when creating a table, or by using the `add_text_limit`
when altering an existing table. when altering an existing table.
The standard Rails `text` column type can not be defined with a limit, but we extend `create_table` to The standard Rails `text` column type can not be defined with a limit, but we extend `create_table` to
...@@ -43,8 +43,8 @@ Don't use text columns for `attr_encrypted` attributes. Use a ...@@ -43,8 +43,8 @@ Don't use text columns for `attr_encrypted` attributes. Use a
## Create a new table with text columns ## 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 When adding a new table, the limits for all text columns should be added in the same migration as
the table creation. We add a `#text_limit` method to Rails' `create_table`, which allows adding limits the table creation. We add a `limit:` attribute to Rails' `#text` method, which allows adding a limit
for text columns. for this column.
For example, consider a migration that creates a table with two text columns, For example, consider a migration that creates a table with two text columns,
`db/migrate/20200401000001_create_db_guides.rb`: `db/migrate/20200401000001_create_db_guides.rb`:
...@@ -54,11 +54,8 @@ class CreateDbGuides < Gitlab::Database::Migration[1.0] ...@@ -54,11 +54,8 @@ class CreateDbGuides < Gitlab::Database::Migration[1.0]
def change def change
create_table :db_guides do |t| create_table :db_guides do |t|
t.bigint :stars, default: 0, null: false t.bigint :stars, default: 0, null: false
t.text :title t.text :title, limit: 128
t.text :notes t.text :notes, limit: 1024
t.text_limit :title, 128
t.text_limit :notes, 1024
end end
end end
end end
......
...@@ -30,16 +30,21 @@ module Gitlab ...@@ -30,16 +30,21 @@ module Gitlab
helper_context = self helper_context = self
super do |t| super do |t|
t.define_singleton_method(:text_limit) do |column_name, limit, name: nil| t.define_singleton_method(:text) do |column_name, **kwargs|
# rubocop:disable GitlabSecurity/PublicSend limit = kwargs.delete(:limit)
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) super(column_name, **kwargs)
definition = "char_length(#{column_name}) <= #{limit}"
t.check_constraint(definition, name: name) 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 end
t.instance_eval(&block) unless block.nil? t.instance_eval(&block) unless block.nil?
......
...@@ -13,8 +13,13 @@ module RuboCop ...@@ -13,8 +13,13 @@ module RuboCop
class AddLimitToTextColumns < RuboCop::Cop::Cop class AddLimitToTextColumns < RuboCop::Cop::Cop
include MigrationHelpers 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). ' \ 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_node_matcher :reverting?, <<~PATTERN
(def :down ...) (def :down ...)
...@@ -37,15 +42,29 @@ module RuboCop ...@@ -37,15 +42,29 @@ module RuboCop
node.each_descendant(:send) do |send_node| node.each_descendant(:send) do |send_node|
next unless text_operation?(send_node) next unless text_operation?(send_node)
# We require a limit for the same table and attribute name if text_operation_with_limit?(send_node)
if text_limit_missing?(node, *table_and_attribute_name(send_node)) add_offense(send_node, location: :selector, message: TEXT_LIMIT_ATTRIBUTE_NOT_ALLOWED) if version(node) < TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE
add_offense(send_node, location: :selector) 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 end
end end
private 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) def text_operation?(node)
# Don't complain about text arrays # Don't complain about text arrays
return false if array_column?(node) return false if array_column?(node)
......
...@@ -235,78 +235,17 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do ...@@ -235,78 +235,17 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do
] ]
end end
context 'when no check constraints are defined' do context 'using a limit: attribute on .text' do
it 'creates the table as expected' do it 'creates the table as expected' do
migration.create_table table_name do |t| migration.create_table table_name do |t|
t.timestamps_with_timezone t.timestamps_with_timezone
t.integer :some_id, null: false t.integer :some_id, null: false
t.boolean :active, null: false, default: true t.boolean :active, null: false, default: true
t.text :name t.text :name, limit: 100
end end
expect_table_columns_to_match(column_attributes, table_name) expect_table_columns_to_match(column_attributes, table_name)
end expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 100')
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 end
end end
......
...@@ -11,6 +11,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do ...@@ -11,6 +11,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
before do before do
allow(cop).to receive(:in_migration?).and_return(true) 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 end
context 'when text columns are defined without a limit' do context 'when text columns are defined without a limit' do
...@@ -63,11 +64,8 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do ...@@ -63,11 +64,8 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
create_table :test_text_limits_create do |t| create_table :test_text_limits_create do |t|
t.integer :test_id, null: false t.integer :test_id, null: false
t.text :title t.text :title, limit: 100
t.text :description t.text :description, limit: 255
t.text_limit :title, 100
t.text_limit :description, 255
end end
add_column :test_text_limits, :email, :text add_column :test_text_limits, :email, :text
...@@ -82,6 +80,30 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do ...@@ -82,6 +80,30 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
end end
RUBY RUBY
end 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 end
context 'when text array columns are defined without a limit' do context 'when text array columns are defined without a limit' do
......
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