diff --git a/changelog/new_merge_pull_request_1378_from.md b/changelog/new_merge_pull_request_1378_from.md new file mode 100644 index 0000000000..108d37bf8f --- /dev/null +++ b/changelog/new_merge_pull_request_1378_from.md @@ -0,0 +1 @@ +* [#1378](https://github.com/rubocop/rubocop-rails/pull/1378): Add new `Rails/IndexNames` cop. ([@corsonknowles][]) diff --git a/config/default.yml b/config/default.yml index 0e7d183460..1e7a530c6a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -624,6 +624,16 @@ Rails/IndexBy: VersionAdded: '2.5' VersionChanged: '2.8' +Rails/IndexNames: + Description: 'Checks for and removes custom index names. Since 7.1, Rails can ensure unique index names without exceeding the length limit.' + Enabled: pending + SafeAutoCorrect: false + StartAfterMigrationVersion: null + VersionAdded: <> + VersionChanged: <> + Include: + - db/**/*.rb + Rails/IndexWith: Description: 'Prefer `index_with` over `each_with_object`, `to_h`, or `map`.' Enabled: true diff --git a/lib/rubocop/cop/mixin/migrations_helper.rb b/lib/rubocop/cop/mixin/migrations_helper.rb index 76dedda41f..4b5add9271 100644 --- a/lib/rubocop/cop/mixin/migrations_helper.rb +++ b/lib/rubocop/cop/mixin/migrations_helper.rb @@ -12,7 +12,7 @@ module MigrationsHelper (send (const (const {nil? cbase} :ActiveRecord) :Migration) :[] - (float _)) + (float $_)) _) PATTERN @@ -21,6 +21,14 @@ def in_migration?(node) migration_class?(class_node) end end + + def in_supported_migration?(node, minimum_version) + node.each_ancestor(:class).any? do |class_node| + migration_class?(class_node) do |version| + version >= minimum_version + end + end + end end end end diff --git a/lib/rubocop/cop/rails/index_names.rb b/lib/rubocop/cop/rails/index_names.rb new file mode 100644 index 0000000000..0a40b66cb1 --- /dev/null +++ b/lib/rubocop/cop/rails/index_names.rb @@ -0,0 +1,148 @@ +# typed: false +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Checks for custom index names in migrations. + # + # @safety + # Migration files must not be altered after they are run, and it is important to + # run identical migrations in each environment. Autocorrect should only be used + # while composing a migration, before executing it. + # See: https://guides.rubyonrails.org/active_record_migrations.html#changing-existing-migrations + # + # @example + # # bad + # class ExampleMigration < ActiveRecord::Migration[7.2] + # def change + # change_table :users do |t| + # t.index [:email], name: 'index_custom_name' + # end + # end + # end + # + # # bad + # class ExampleMigration < ActiveRecord::Migration[7.2] + # def change + # add_index :table, :column, name: 'index_custom_name' + # end + # end + # + # # good + # class ExampleMigration < ActiveRecord::Migration[7.2] + # def change + # create_table :users do |t| + # t.index [:email] + # end + # end + # end + # + # # ok (custom name may differentiate this index from an index on the same columns with default order) + # class ExampleMigration < ActiveRecord::Migration[7.2] + # def change + # create_table :robots do |t| + # t.index [:priority, :created_at], + # name: 'index_robots_by_recency', + # order: { priority: "ASC NULLS LAST", created_at: :asc } + # end + # end + # end + # + # # ok (custom name may be necessary to meet DB character limit for index names in older Rails versions) + # class ExampleMigration < ActiveRecord::Migration[7.0] + # def change + # create_table :users do |t| + # t.index [:email], name: 'index_custom_name' + # end + # end + # end + # + class IndexNames < Base + include MigrationsHelper + include RangeHelp + extend AutoCorrector + extend TargetRailsVersion + + minimum_target_rails_version 7.1 + + MSG = 'Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically.' + RESTRICT_ON_SEND = %i[index add_index].freeze + # Keys that do not justify a duplicative index or a custom name, as they represent stricter column conditions.: + # unique, length, nulls_not_distinct + # Refer to: https://github.com/rails/rails/blob/b943622bdc746370ac860bfd3240cc0b8ca59d90/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L1477 + VALID_REASONS_FOR_CUSTOM_NAME = Set.new(%i[where type using comment algorithm include opclass order]).freeze + + def on_send(node) + return unless node.last_argument&.hash_type? + return unless in_supported_migration?(node, 7.1) + return unless (name_pair = find_name_pair(node)) + + return if node.last_argument.pairs.any? { |pair| VALID_REASONS_FOR_CUSTOM_NAME.include?(pair.key.value) } + + add_offense(node) do |corrector| + remove_name_argument(corrector, name_pair, node) + end + end + + private + + def find_name_pair(node) + node.last_argument.pairs.find { |pair| pair.key.value == :name || pair.key.value == 'name' } + end + + def remove_name_argument(corrector, name_pair, node) + range = name_argument_range(name_pair, node) + corrector.remove(range) + remove_extra_comma_and_space(corrector, range, node) + end + + def name_argument_range(name_pair, node) + hash_node = name_pair.parent + if hash_node.pairs.size == 1 + # If name: is the only argument, remove the entire hash + range_between(node.arguments[-2].source_range.end_pos, node.source_range.end_pos) + else + # Remove the name: argument and any preceding comma and space + start_pos = previous_comma_pos(name_pair) || name_pair.source_range.begin_pos + range_between(start_pos, name_pair.source_range.end_pos) + end + end + + def previous_comma_pos(pair) + source = pair.parent.source + pair_start = pair.source_range.begin_pos - pair.parent.source_range.begin_pos + previous_content = source[0...pair_start] + comma_index = previous_content.rindex(',') + comma_index ? pair.parent.source_range.begin_pos + comma_index : nil + end + + def remove_extra_comma_and_space(corrector, removed_range, node) + remaining_source = remaining_source(node, removed_range) + next_relevant_content = remaining_source.lstrip + range_to_remove = if next_relevant_content.start_with?(',') + space_after_comma(removed_range, next_relevant_content) + else + leading_space(remaining_source, removed_range) + end + + corrector.remove(range_to_remove) + end + + def remaining_source(node, removed_range) + node.source[removed_range.end_pos - node.source_range.begin_pos..] + end + + def space_after_comma(removed_range, next_relevant_content) + space_after_comma = next_relevant_content[1..].match(/\A\s*/)[0] + range_between(removed_range.end_pos, removed_range.end_pos + 1 + space_after_comma.length) + end + + def leading_space(remaining_source, removed_range) + leading_space = remaining_source.match(/\A\s*/)[0] + range_between(removed_range.end_pos, removed_range.end_pos + leading_space.length) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index b7eddc4fe4..a6543e0e3d 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -69,6 +69,7 @@ require_relative 'rails/ignored_columns_assignment' require_relative 'rails/ignored_skip_action_filter_option' require_relative 'rails/index_by' +require_relative 'rails/index_names' require_relative 'rails/index_with' require_relative 'rails/inquiry' require_relative 'rails/inverse_of' diff --git a/spec/rubocop/cop/rails/index_names_spec.rb b/spec/rubocop/cop/rails/index_names_spec.rb new file mode 100644 index 0000000000..bb92503091 --- /dev/null +++ b/spec/rubocop/cop/rails/index_names_spec.rb @@ -0,0 +1,291 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::IndexNames, :config do + context 'Rails 7.0', :rails70 do + context 'when t.index has a custom name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.0] + def change + change_table :users do |t| + t.index [:email], name: 'index_custom_name' + end + end + end + RUBY + end + end + + context 'when add_index has a name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.0] + def change + add_index :table, :column, name: 'index_custom_name' + end + end + RUBY + end + end + end + + context 'Rails 7.2', :rails72 do + context 'when using an earlier migration version' do + context 'when t.index has a custom name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.0] + def change + change_table :users do |t| + t.index [:email], name: 'index_custom_name' + end + end + end + RUBY + end + end + + context 'when add_index has a name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.0] + def change + add_index :table, :column, name: 'index_custom_name' + end + end + RUBY + end + end + end + + context 'when t.index has a custom name argument' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + change_table :users do |t| + t.index [:email], name: 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + end + end + + context 'when t.index has no name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email] + end + end + end + RUBY + end + end + + context 'when add_index has no name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + add_index :table, :column + end + end + RUBY + end + end + + context 'when add_index is outside of a migration' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + add_index :table, :column, name: 'index_custom_name' + end + RUBY + end + end + + context 'when t.index is outside of a migration' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + create_table :users do |t| + t.index [:email], name: 'index_custom_name' + end + end + RUBY + end + end + + context 'when unrelated index method has a name argument' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + index :email, name: 'index_custom_name' + RUBY + end + end + + context 'when t.index has a custom name old style hash argument' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], 'name' => 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + end + end + + context 'when t.index has multiple arguments and custom name' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], unique: true, name: 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + end + end + + context 'when t.index has a reason for a custom name and possibly distinct index on the same keys' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], include: :tags, name: 'index_custom_name' + end + end + end + RUBY + end + end + + context 'when correcting an offense' do + it 'removes the custom name argument without removing following arguments' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], name: 'index_custom_name', unique: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + + expect_correction(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], unique: true + end + end + end + RUBY + end + + it 'removes only the name argument when there are other arguments and starts at name:' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], unique: true, name: 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + + expect_correction(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], unique: true + end + end + end + RUBY + end + + it 'removes the name argument when it is the only keyword argument' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], name: 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + + expect_correction(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email] + end + end + end + RUBY + end + + it 'removes only the name => argument for classic hash style' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email], "name" => 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + end + RUBY + + expect_correction(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + create_table :users do |t| + t.index [:email] + end + end + end + RUBY + end + end + + context 'when add_index has a name argument' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + add_index :table, :column, name: 'index_custom_name' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid specifying a custom name for common indexes. Let Rails handle the index name automatically. + end + end + RUBY + + expect_correction(<<~RUBY) + class ExampleMigration < ActiveRecord::Migration[7.2] + def change + add_index :table, :column + end + end + RUBY + end + end + end +end