Skip to content

Commit

Permalink
[Fix #215] Fix a false positive for Rails/UniqueValidationWithoutIndex
Browse files Browse the repository at this point in the history
Fixes #215.

This PR fixes a false positive for `Rails/UniqueValidationWithoutIndex`
when using Expression Indexes for PostgreSQL.

Since it is difficult to predict a kind of string described in Expression Indexes,
this implementation judges based on whether or not an Expression Indexes include
a column name.
There is a possibility of false negative, but in most cases I expect it works.

## References

- https://www.postgresql.org/docs/12/indexes-expressional.html
- rails/rails@edc2b77
  • Loading branch information
koic committed Mar 26, 2020
1 parent e952888 commit ac766e3
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Bug fixes

* [#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][])

## 2.5.0 (2020-03-24)

Expand Down
15 changes: 12 additions & 3 deletions lib/rubocop/cop/rails/unique_validation_without_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class UniqueValidationWithoutIndex < Cop
def on_send(node)
return unless node.method?(:validates)
return unless uniqueness_part(node)
return if condition_part?(node)
return unless schema
return if with_index?(node)

Expand All @@ -47,13 +48,21 @@ def with_index?(node)
table = schema.table_by(name: table_name(klass))
return true unless table # Skip analysis if it can't find the table

return true if condition_part?(node)

names = column_names(node)
return true unless names

table.indices.any? do |index|
index.unique && index.columns.to_set == names
index.unique &&
(index.columns.to_set == names ||
include_column_names_in_expression_index?(index, names))
end
end

def include_column_names_in_expression_index?(index, column_names)
return false unless (expression_index = index.expression)

column_names.all? do |column_name|
expression_index.include?(column_name)
end
end

Expand Down
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 @@ -406,5 +406,46 @@ class User
RUBY
end
end

context 'with expression indexes' do
context 'when column name is included in expression index' do
let(:schema) { <<~RUBY }
ActiveRecord::Schema.define(version: 2020_02_02_075409) do
create_table 'emails', force: :cascade do |t|
t.string 'address', null: false
t.index 'lower(address)', name: 'index_emails_on_lower_address', unique: true
end
end
RUBY

it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
class Email < ApplicationRecord
validates :address, presence: true, uniqueness: { case_sensitive: false }, email: true
end
RUBY
end
end

context 'when column name is not included in expression index' do
let(:schema) { <<~RUBY }
ActiveRecord::Schema.define(version: 2020_02_02_075409) do
create_table 'emails', force: :cascade do |t|
t.string 'address', null: false
t.index 'lower(unexpected_column_name)', name: 'index_emails_on_lower_address', unique: true
end
end
RUBY

it 'registers an offense' do
expect_offense(<<~RUBY)
class Email < ApplicationRecord
validates :address, presence: true, uniqueness: { case_sensitive: false }, email: true
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index.
end
RUBY
end
end
end
end
end

0 comments on commit ac766e3

Please sign in to comment.