Enforce text columns with a limit over strings on migrations
What does this MR do?
This MR follows the completion of !29181 (merged), which added migration helpers for managing check constraints and limits for text columns.
It enforces rules on how text columns are used in migrations:
-
We now disallow using of
string
columns (i.e.varchar(N)
) in favor of usingtext
always.Adds a new
Migration/String
cop to disallowstring
in column definitions and removes the existingMigration/AddLimitToString
cop as it is no longer required. -
We also enforce placing a limit on all newly added
text
columns.Adds a new
Migration/Text
cop that triggers whenever atext
column is added and checks if a limit is also added to that column in the same migration (i.e. a call to theadd_text_limit
migration helper, which was introduced with !29181 (merged)).The
Migration/Text
cop checks fortext
columns introduced in any possible way: When helpers for creating or altering tables are used (create_table
,create_table_if_not_exists
,change_table
), helpers that directly add a column to a table (add_column
,add_column_with_default
) or thechange_column_type_concurrently
, which changes a column to another type (this is an edge case most often encountered when a migration converts an id to a text after realizing that an integer was not the proper choice - we even have one such case in our migrations)
Both new cops work only on up
and change
methods. We skip enforcing this on down
methods to allow migrations to revert back and be consistent with the current schema, which may be have columns defined as strings or as texts without limits.
Related issue: #30453 (closed)
Demo: Strings are no longer allowed
db/migrate/20200413075308_test_text_limits.rb
class TestTextLimits < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
create_table :test_strings, id: false do |t|
t.integer :test_id, null: false
t.string :name
end
add_column :test_strings, :email, :string
add_column_with_default :test_strings, :role, :string, default: 'default'
change_column_type_concurrently :test_strings, :test_id, :string
end
def down
# This should be allowed to keep consistency with existing schema
add_column :test_strings, :strings_allowed_on_down, :string
drop_table :test_strings
end
end
$ rubocop db/migrate/20200413075308_test_text_limits.rb
Inspecting 1 file
C
Offenses:
db/migrate/20200413075308_test_text_limits.rb:12:9: C: Migration/String: Do not use the string data type, use text instead
t.string :name
^^^^^^
db/migrate/20200413075308_test_text_limits.rb:15:5: C: Migration/String: Do not use the string data type, use text instead
add_column :test_strings, :email, :string
^^^^^^^^^^
db/migrate/20200413075308_test_text_limits.rb:17:5: C: Migration/String: Do not use the string data type, use text instead
add_column_with_default :test_strings, :role, :string, default: 'default'
^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20200413075308_test_text_limits.rb:19:5: C: Migration/String: Do not use the string data type, use text instead
change_column_type_concurrently :test_strings, :test_id, :string
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Notice how there is no offence for strings_allowed_on_down
as it is on the down
method.
Demo: Texts without limits are no longer allowed
db/migrate/20200413084027_test_text_limits.rb
class TestTextLimits < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
create_table :test_text_limits, id: false do |t|
t.integer :test_id, null: false
t.text :name
end
add_column :test_text_limits, :email, :text
add_column_with_default :test_text_limits, :role, :text, default: 'default'
change_column_type_concurrently :test_text_limits, :test_id, :text
# Those are limits in attributes with the same name but on different tables
# than the ones the attributes were added
# and should not stop the offence from being generated!
add_text_limit :another_table, :name, 255
add_text_limit :another_table, :email, 255
# Uncommenting the following makes rubocop generate no offence
# add_text_limit :test_text_limits, :name, 255
# add_text_limit :test_text_limits, :email, 255
# add_text_limit :test_text_limits, :role, 255
# add_text_limit :test_text_limits, :test_id, 255
end
def down
drop_table :test_text_limits
# Texts without limit on down generate no offence
create_table :no_offence_on_down, id: false do |t|
t.integer :test_id, null: false
t.text :name
end
add_column :no_offence_on_down, :email, :text
add_column_with_default :no_offence_on_down, :role, :text, default: 'default'
drop_table :no_offence_on_down
end
end
$ rubocop db/migrate/20200413084027_test_text_limits.rb
Inspecting 1 file
C
Offenses:
db/migrate/20200413084027_test_text_limits.rb:12:9: C: Migration/Text: Always add a limit using add_text_limit when adding a text column
t.text :name
^^^^
db/migrate/20200413084027_test_text_limits.rb:15:5: C: Migration/Text: Always add a limit using add_text_limit when adding a text column
add_column :test_text_limits, :email, :text
^^^^^^^^^^
db/migrate/20200413084027_test_text_limits.rb:17:5: C: Migration/Text: Always add a limit using add_text_limit when adding a text column
add_column_with_default :test_text_limits, :role, :text, default: 'default'
^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20200413084027_test_text_limits.rb:19:5: C: Migration/Text: Always add a limit using add_text_limit when adding a text column
change_column_type_concurrently :test_text_limits, :test_id, :text
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1 file inspected, 4 offenses detected
Screenshots
Does this MR meet the acceptance criteria?
Conformity
- [-] Changelog entry
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides - [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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