Skip to content

Fix the AddLimitToTextColumns cop when table name is a constant

🐦 Context

From !140848 (comment 1751399594)

When we create a migration manipulating text columns, we have a rubocop cop looking to enforce using a limit (more information in https://docs.gitlab.com/ee/development/database/strings_and_the_text_data_type.html). Basically, we want to make sure that all text columns have a size constraint.

The cop will look for those t.text statements for example and starting parsing the rest of the file looking for the limit statement.

The problem is that the cop expects the table name to be a symbol or better said a node with a value.

Guess what happens with this dummy migration:

class MyMigration < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!

  TABLE_NAME = :foobar

  def up
    create_table TABLE_NAME do |t|
      t.text :name
    end
  end
end

Well, the cop will parse the create_table statement and find the node TABLE_NAME. Then, it will call #value on it but 💥 That node is a constant = it has no #value method.

This NoMethodError will bubble up and the $ rubocop execution will stop with an exit code of 1.

🤔 What does this MR do and why?

  • Make the Migration/AddLimitToTextColumns cop accept constants as table names.
  • Update the related spec.

🏁 MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

🏷 Limitations

I still think that the cop has a limitation: if we mix the usage of a constant and the table name directly, it will not properly parse the migration.

Example:

class MyMigration < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!

  TABLE_NAME = :foobar

  def up
    create_table TABLE_NAME do |t|
      t.text :name
    end

    add_text_limit :foobar, :name, 512
  end
end

The above migration will still trigger the cop warning because we don't really handle the mapping TABLE_NAME <-> :foobar.

I think that's reasonable enough. Using different approaches (constant vs direct table name) is not a good idea anyway.

🖥 Screenshots or screen recordings

This is not a user facing change

How to set up and validate locally

One way to test this locally, is to create a dummy migration in db/migrate/dummy_migration.rb:

class MyMigration < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!

  TABLE_NAME = :foobar

  def up
    create_table TABLE_NAME do |t|
      t.text :name
    end
  end
end

1️⃣ With master

$ bundle exec rubocop --only Migration/AddLimitToTextColumns db/migrate/dummy_migration.rb
Inspecting 1 file
An error occurred while Migration/AddLimitToTextColumns cop was inspecting /Users/david/projects/gitlab-development-kit/gitlab/db/migrate/dummy_migration.rb:6:2.
To see the complete backtrace run rubocop -d.
.

1 file inspected, no offenses detected

1 error occurred:
An error occurred while Migration/AddLimitToTextColumns cop was inspecting /Users/david/projects/gitlab-development-kit/gitlab/db/migrate/dummy_migration.rb:6:2.
Errors are usually caused by RuboCop bugs.
Please, report your problems to RuboCop's issue tracker.
https://github.com/rubocop/rubocop/issues

Mention the following information in the issue report:
1.57.2 (using Parser 3.3.0.2, rubocop-ast 1.29.0, running on ruby 3.2.3) [arm64-darwin23]

💥

2️⃣ With this branch

$ bundle exec rubocop --only Migration/AddLimitToTextColumns db/migrate/dummy_migration.rb
Inspecting 1 file
C

Offenses:

db/migrate/dummy_migration.rb:8:9: C: Migration/AddLimitToTextColumns: 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 or by using .text... limit: 255 inside create_table
      t.text :name
        ^^^^

1 file inspected, 1 offense detected

All good 🎉

Refs

Closes #235456 (closed)

Edited by Peter Leitzen

Merge request reports