Add Rails/AddColumnIndex cop to prevent passing an unused but confu…

…sing `index:` key to `add_column` migrations.
rubocop · May 25, 2021 · d869bf9 · d869bf9
1 parent cf1ba4d
commit d869bf9
Show file tree

Hide file tree

Showing 7 changed files with 204 additions and 0 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### New features
 
 * [#486](https://github.com/rubocop/rubocop-rails/issues/486): Add new `Rails/ExpandedDateRange` cop. ([@koic][])
+* [#497](https://github.com/rubocop/rubocop-rails/issues/497): Add new `Rails/AddColumnIndex` cop. ([@dvandersluis][])
 
 ### Bug fixes
 

diff --git a/config/default.yml b/config/default.yml
@@ -83,6 +83,16 @@ Rails/ActiveSupportAliases:
   Enabled: true
   VersionAdded: '0.48'
 
+Rails/AddColumnIndex:
+  Description: >-
+    Rails migrations don't make use of a given `index` key, but also
+    doesn't given an error when it's used, so it makes it seem like an
+    index might be used.
+  Enabled: pending
+  VersionAdded: '2.11'
+  Include:
+    - db/migrate/*.rb
+
 Rails/AfterCommitOverride:
   Description: >-
                   This cop enforces that there is only one call to `after_commit`

diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc
@@ -21,6 +21,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide].
 * xref:cops_rails.adoc#railsactiverecordcallbacksorder[Rails/ActiveRecordCallbacksOrder]
 * xref:cops_rails.adoc#railsactiverecordoverride[Rails/ActiveRecordOverride]
 * xref:cops_rails.adoc#railsactivesupportaliases[Rails/ActiveSupportAliases]
+* xref:cops_rails.adoc#railsaddcolumnindex[Rails/AddColumnIndex]
 * xref:cops_rails.adoc#railsaftercommitoverride[Rails/AfterCommitOverride]
 * xref:cops_rails.adoc#railsapplicationcontroller[Rails/ApplicationController]
 * xref:cops_rails.adoc#railsapplicationjob[Rails/ApplicationJob]

diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc
@@ -214,6 +214,45 @@ are not used.
 [1, 2, 'a'].prepend('b')
 ----
 
+== Rails/AddColumnIndex
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.11
+| -
+|===
+
+This cop checks for migrations using `add_column` that have an `index`
+key. `add_column` does not accept `index`, but also does not raise an
+error for extra keys, so it is possible to mistakenly add the key without
+realizing it will not actually add an index.
+
+=== Examples
+
+[source,ruby]
+----
+# bad (will not add an index)
+add_column :table, :column, :integer, index: true
+
+# good
+add_column :table, :column, :integer
+add_index :table, :column
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| Include
+| `db/migrate/*.rb`
+| Array
+|===
+
 == Rails/AfterCommitOverride
 
 |===

diff --git a/lib/rubocop/cop/rails/add_column_index.rb b/lib/rubocop/cop/rails/add_column_index.rb
@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks for migrations using `add_column` that have an `index`
+      # key. `add_column` does not accept `index`, but also does not raise an
+      # error for extra keys, so it is possible to mistakenly add the key without
+      # realizing it will not actually add an index.
+      #
+      # @example
+      #   # bad (will not add an index)
+      #   add_column :table, :column, :integer, index: true
+      #
+      #   # good
+      #   add_column :table, :column, :integer
+      #   add_index :table, :column
+      #
+      class AddColumnIndex < Base
+        extend AutoCorrector
+        include RangeHelp
+
+        MSG = '`add_column` does not accept an `index` key, use `add_index` instead.'
+        RESTRICT_ON_SEND = %i[add_column].freeze
+
+        # @!method add_column_with_index(node)
+        def_node_matcher :add_column_with_index, <<~PATTERN
+          (
+            send nil? :add_column $_table $_column
+              <(hash <$(pair {(sym :index) (str "index")} $_) ...>) ...>
+          )
+        PATTERN
+
+        def on_send(node)
+          table, column, pair, value = add_column_with_index(node)
+          return unless pair
+
+          add_offense(pair) do |corrector|
+            corrector.remove(index_range(pair))
+
+            add_index = "add_index #{table.source}, #{column.source}"
+            add_index_opts = ''
+
+            if value.hash_type?
+              hash = value.loc.expression.adjust(begin_pos: 1, end_pos: -1).source.strip
+              add_index_opts = ", #{hash}"
+            end
+
+            corrector.insert_after(node, "\n#{add_index}#{add_index_opts}")
+          end
+        end
+
+        private
+
+        def index_range(pair_node)
+          range_with_surrounding_comma(
+            range_with_surrounding_space(range: pair_node.loc.expression, side: :left),
+            :left
+          )
+        end
+      end
+    end
+  end
+end
diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb
@@ -10,6 +10,7 @@
 require_relative 'rails/active_record_callbacks_order'
 require_relative 'rails/active_record_override'
 require_relative 'rails/active_support_aliases'
+require_relative 'rails/add_column_index'
 require_relative 'rails/after_commit_override'
 require_relative 'rails/application_controller'
 require_relative 'rails/application_job'

diff --git a/spec/rubocop/cop/rails/add_column_index_spec.rb b/spec/rubocop/cop/rails/add_column_index_spec.rb
@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::AddColumnIndex, :config do
+  it 'registers an offense and corrects when an `add_column` call has `index: true`' do
+    expect_offense(<<~RUBY)
+      add_column :table, :column, :integer, default: 0, index: true
+                                                        ^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      add_column :table, :column, :integer, default: 0
+      add_index :table, :column
+    RUBY
+  end
+
+  it 'registers an offense and corrects when an `add_column` call has `index:` with a hash' do
+    expect_offense(<<~RUBY)
+      add_column :table, :column, :integer, default: 0, index: { unique: true, name: 'my_unique_index' }
+                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      add_column :table, :column, :integer, default: 0
+      add_index :table, :column, unique: true, name: 'my_unique_index'
+    RUBY
+  end
+
+  it 'registers an offense and corrects with there is another hash key after `index`' do
+    expect_offense(<<~RUBY)
+      add_column :table, :column, :integer, index: true, default: 0
+                                            ^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      add_column :table, :column, :integer, default: 0
+      add_index :table, :column
+    RUBY
+  end
+
+  it 'registers an offense and corrects with string keys' do
+    expect_offense(<<~RUBY)
+      add_column :table, :column, :integer, 'index' => true, default: 0
+                                            ^^^^^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      add_column :table, :column, :integer, default: 0
+      add_index :table, :column
+    RUBY
+  end
+
+  it 'registers an offense and corrects when on multiple lines' do
+    expect_offense(<<~RUBY)
+      add_column :table, :column, :integer,
+                 index: true,
+                 ^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+                 default: 0
+    RUBY
+
+    expect_correction(<<~RUBY)
+      add_column :table, :column, :integer,
+                 default: 0
+      add_index :table, :column
+    RUBY
+  end
+
+  it 'can correct multiple `add_column` calls' do
+    expect_offense(<<~RUBY)
+      add_column :table, :column, :integer, default: 0, index: true
+                                                        ^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+      add_column :table, :column2, :integer, default: 0, index: true
+                                                         ^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      add_column :table, :column, :integer, default: 0
+      add_index :table, :column
+      add_column :table, :column2, :integer, default: 0
+      add_index :table, :column2
+    RUBY
+  end
+
+  it 'does not register an offense without an `index` key to `add_column`' do
+    expect_no_offenses(<<~RUBY)
+      add_column :table, :column, :integer, default: 0
+    RUBY
+  end
+end