From 3fa9c7c41f8ba99b102eb505cd7276e3bd4a4280 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 28 Mar 2020 12:30:36 +0900 Subject: [PATCH] [Fix #221] Make `Rails/UniqueValidationWithoutIndex` aware of `add_index` Fixes #221. This PR makes `Rails/UniqueValidationWithoutIndex` aware of `add_index` in db/schema.rb. Rails 4.0 and Rails 4.1 support will drop shortly, but Rails 4.2 may be a bit ahead. There may be changes to these support after the bug fix release of RoboCop Rails 2.5 series. --- CHANGELOG.md | 1 + .../rails/unique_validation_without_index.rb | 5 +- lib/rubocop/rails/schema_loader/schema.rb | 48 +++++++++++++++---- .../unique_validation_without_index_spec.rb | 41 ++++++++++++++++ spec/rubocop/rails/schema_loader_spec.rb | 22 +++++++++ 5 files changed, 108 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad28ccef09..5632f2b7d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#213](https://github.com/rubocop-hq/rubocop-rails/pull/213): Fix a false positive for `Rails/UniqueValidationWithoutIndex` when using conditions. ([@sunny][]) * [#215](https://github.com/rubocop-hq/rubocop-rails/issues/215): Fix a false positive for `Rails/UniqueValidationWithoutIndex` when using Expression Indexes. ([@koic][]) * [#214](https://github.com/rubocop-hq/rubocop-rails/issues/214): Fix an error for `Rails/UniqueValidationWithoutIndex`when a table has no column definition. ([@koic][]) +* [#221](https://github.com/rubocop-hq/rubocop-rails/issues/221): Make `Rails/UniqueValidationWithoutIndex` aware of `add_index` in db/schema.rb. ([@koic][]) ## 2.5.0 (2020-03-24) diff --git a/lib/rubocop/cop/rails/unique_validation_without_index.rb b/lib/rubocop/cop/rails/unique_validation_without_index.rb index 2f1f79b5fd..8cbf4759c9 100644 --- a/lib/rubocop/cop/rails/unique_validation_without_index.rb +++ b/lib/rubocop/cop/rails/unique_validation_without_index.rb @@ -51,7 +51,10 @@ def with_index?(node) names = column_names(node) return true unless names - table.indices.any? do |index| + # Compatibility for Rails 4.2. + add_indicies = schema.add_indicies_by(table_name: table_name(klass)) + + (table.indices + add_indicies).any? do |index| index.unique && (index.columns.to_set == names || include_column_names_in_expression_index?(index, names)) diff --git a/lib/rubocop/rails/schema_loader/schema.rb b/lib/rubocop/rails/schema_loader/schema.rb index 7057e08734..cd07538a94 100644 --- a/lib/rubocop/rails/schema_loader/schema.rb +++ b/lib/rubocop/rails/schema_loader/schema.rb @@ -5,10 +5,12 @@ module Rails module SchemaLoader # Represent db/schema.rb class Schema - attr_reader :tables + attr_reader :tables, :add_indicies def initialize(ast) @tables = [] + @add_indicies = [] + build!(ast) end @@ -18,6 +20,12 @@ def table_by(name:) end end + def add_indicies_by(table_name:) + add_indicies.select do |add_index| + add_index.table_name == table_name + end + end + private def build!(ast) @@ -26,6 +34,11 @@ def build!(ast) each_table(ast) do |table_def| @tables << Table.new(table_def) end + + # Compatibility for Rails 4.2. + each_add_index(ast) do |add_index_def| + @add_indicies << AddIndex.new(add_index_def) + end end def each_table(ast) @@ -40,6 +53,14 @@ def each_table(ast) yield ast.body end end + + def each_add_index(ast) + ast.body.children.each do |node| + next if !node&.send_type? || !node.method?(:add_index) + + yield(node) + end + end end # Represent a table @@ -121,8 +142,7 @@ class Index attr_reader :name, :columns, :expression, :unique def initialize(node) - node.first_argument - @columns, @expression = build_columns_or_expr(node) + @columns, @expression = build_columns_or_expr(node.first_argument) @unique = nil analyze_keywords!(node) @@ -130,12 +150,11 @@ def initialize(node) private - def build_columns_or_expr(node) - arg = node.first_argument - if arg.array_type? - [arg.values.map(&:value), nil] + def build_columns_or_expr(columns) + if columns.array_type? + [columns.values.map(&:value), nil] else - [[], arg.value] + [[], columns.value] end end @@ -153,6 +172,19 @@ def analyze_keywords!(node) end end end + + # Represent an `add_index` + class AddIndex < Index + attr_reader :table_name + + def initialize(node) + @table_name = node.first_argument.value + @columns, @expression = build_columns_or_expr(node.arguments[1]) + @unique = nil + + analyze_keywords!(node) + end + end end end end diff --git a/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb b/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb index 8ddc74d1bc..c36d515019 100644 --- a/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb +++ b/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb @@ -464,5 +464,46 @@ class Email < ApplicationRecord end end end + + context 'when db/schema.rb has been dumped using `add_index` for index' do + context 'when the table does not have any indices' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "users", force: :cascade do |t| + t.string "account", null: false + end + add_index "users", "account", name: "index_users_on_account" + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + class User + validates :account, uniqueness: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + RUBY + end + end + + context 'with a unique index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "users", force: :cascade do |t| + t.string "account", null: false + end + add_index "users", ["account"], name: "index_users_on_account", unique: true + end + RUBY + + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class User + validates :account, uniqueness: true + end + RUBY + end + end + end end end diff --git a/spec/rubocop/rails/schema_loader_spec.rb b/spec/rubocop/rails/schema_loader_spec.rb index 29c0f55105..3dcb1e4383 100644 --- a/spec/rubocop/rails/schema_loader_spec.rb +++ b/spec/rubocop/rails/schema_loader_spec.rb @@ -85,6 +85,28 @@ expect(table.indices.first.name).to eq 'index_title_lower_unique' expect(table.indices.first.unique).to be true end + + context 'when an index in users table specified by `add_index`' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "users", force: :cascade do |t| + t.string "account", null: false + end + add_index "users", ["account"], name: "index_users_on_account", unique: true + add_index "users", ["email"], name: "index_users_on_email", unique: true + add_index "books", ["isbn"], name: "index_books_on_isbn", unique: true + end + RUBY + + it 'has an `add_index` for users table' do + add_indicies = loaded_schema.add_indicies_by(table_name: 'users') + expect(add_indicies.size).to eq 2 + expect(add_indicies.first.name).to eq 'index_users_on_account' + expect(add_indicies.first.table_name).to eq 'users' + expect(add_indicies.first.columns).to eq ['account'] + expect(add_indicies.first.unique).to be true + end + end end context 'when the current directory is Rails.root' do