Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #221] Make Rails/UniqueValidationWithoutIndex aware of add_index #222

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
5 changes: 4 additions & 1 deletion lib/rubocop/cop/rails/unique_validation_without_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
48 changes: 40 additions & 8 deletions lib/rubocop/rails/schema_loader/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -121,21 +142,19 @@ 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)
end

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

Expand All @@ -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
41 changes: 41 additions & 0 deletions spec/rubocop/cop/rails/unique_validation_without_index_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
22 changes: 22 additions & 0 deletions spec/rubocop/rails/schema_loader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down