Skip to content

Enforce text columns with a limit over strings on migrations

Yannis Roussos requested to merge 30453-add-rubocop-to-enforce-text-columns-with-limit into master

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 using text always.

    Adds a new Migration/String cop to disallow string in column definitions and removes the existing Migration/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 a text column is added and checks if a limit is also added to that column in the same migration (i.e. a call to the add_text_limit migration helper, which was introduced with !29181 (merged)).

    The Migration/Text cop checks for text 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 the change_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

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 🤖 GitLab Bot 🤖

Merge request reports