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 #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)