Skip to content

Commit

Permalink
Add Rails/UniqueValidationWithoutIndex cop
Browse files Browse the repository at this point in the history
  • Loading branch information
pocke committed Feb 8, 2020
1 parent da64cc2 commit ac6cf91
Show file tree
Hide file tree
Showing 13 changed files with 757 additions and 0 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### New features

* [#197](https://github.com/rubocop-hq/rubocop-rails/pull/197): Add `Rails/UniqueValidationWithoutIndex` cop. ([@pocke][])

### Bug fixes

* [#180](https://github.com/rubocop-hq/rubocop-rails/issues/180): Fix a false positive for `HttpPositionalArguments` when using `get` method with `:to` option. ([@koic][])
Expand Down
7 changes: 7 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,13 @@ Rails/UniqBeforePluck:
- aggressive
AutoCorrect: false

Rails/UniqueValidationWithoutIndex:
Description: 'Uniqueness validation should be with a unique index.'
Enabled: true
VersionAdded: '2.5'
Include:
- app/models/**/*.rb

Rails/UnknownEnv:
Description: 'Use correct environment name.'
Enabled: true
Expand Down
3 changes: 3 additions & 0 deletions lib/rubocop-rails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

require 'rubocop'
require 'rack/utils'
require 'active_support/inflector'

require_relative 'rubocop/rails'
require_relative 'rubocop/rails/version'
require_relative 'rubocop/rails/inject'
require_relative 'rubocop/rails/schema_loader'
require_relative 'rubocop/rails/schema_loader/schema'

RuboCop::Rails::Inject.defaults!

Expand Down
25 changes: 25 additions & 0 deletions lib/rubocop/cop/mixin/active_record_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

module RuboCop
module Cop
# A mixin to extend cops for ActiveRecord features
module ActiveRecordHelper
extend NodePattern::Macros

def_node_search :find_set_table_name, <<-PATTERN
(send self :table_name= {str sym})
PATTERN

def table_name(class_node)
table_name = find_set_table_name(class_node).to_a.last&.first_argument
return table_name.value.to_s if table_name

namespaces = class_node.each_ancestor(:class, :module).to_a
[class_node, *namespaces]
.reverse
.map { |klass| klass.identifier.children[1] }.join('_')
.tableize
end
end
end
end
123 changes: 123 additions & 0 deletions lib/rubocop/cop/rails/unique_validation_without_index.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# When you define uniqueness validation in ActiveRecord model,
# you also should add a unique index for the column. It has two reasons.
# First, duplicated records may occur even if ActiveRecord's validation
# is defined.
# Second, it will cause slow queries. The validation executes a `SELECT`
# statement with the target column when inserting/updating a record.
# If the column does not have an index and the table is large,
# the query will be heavy.
#
# @example
# # bad - if the schema does not have a unique index
# validates :account, uniqueness: true
#
# # good - if the schema has a unique index
# validates :account, uniqueness: true
#
# # good - even if the schema does not have a unique index
# validates :account, length: { minimum: MIN_LENGTH }
#
class UniqueValidationWithoutIndex < Cop
include ActiveRecordHelper

MSG = 'Uniqueness validation should be with a unique index'

def on_send(node)
return unless node.method?(:validates)
return unless uniqueness_part(node)
return unless schema
return if with_index?(node)

add_offense(node)
end

private

def with_index?(node)
klass = class_node(node)
return true unless klass # Skip analysis

table = schema.table_by(name: table_name(klass))
return true unless table # Skip analysis if it can't find the table

names = column_names(node)
return unless names

table.indices.any? do |index|
index.unique && index.columns.to_set == names
end
end

def column_names(node)
arg = node.first_argument
return unless arg.str_type? || arg.sym_type?

ret = [arg.value]
names_from_scope = column_names_from_scope(node)
ret.concat(names_from_scope) if names_from_scope

ret.map(&:to_s).to_set
end

def column_names_from_scope(node)
uniq = uniqueness_part(node)
return unless uniq.hash_type?

scope = find_scope(uniq)
return unless scope

case scope.type
when :sym, :str
[scope.value]
when :array
array_node_to_array(scope)
end
end

def find_scope(pairs)
pairs.each_pair.find do |pair|
key = pair.key
next unless key.sym_type? && key.value == :scope

break pair.value
end
end

def class_node(node)
node.each_ancestor.find(&:class_type?)
end

def uniqueness_part(node)
pairs = node.arguments.last
return unless pairs.hash_type?

pairs.each_pair.find do |pair|
next unless pair.key.sym_type? && pair.key.value == :uniqueness

break pair.value
end
end

def array_node_to_array(node)
node.values.map do |elm|
case elm.type
when :str, :sym
elm.value
else
return nil
end
end
end

def schema
RuboCop::Rails::SchemaLoader.load(target_ruby_version)
end
end
end
end
end
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require_relative 'mixin/active_record_helper'
require_relative 'mixin/target_rails_version'

require_relative 'rails/action_filter'
Expand Down Expand Up @@ -57,5 +58,6 @@
require_relative 'rails/skips_model_validations'
require_relative 'rails/time_zone'
require_relative 'rails/uniq_before_pluck'
require_relative 'rails/unique_validation_without_index'
require_relative 'rails/unknown_env'
require_relative 'rails/validation'
60 changes: 60 additions & 0 deletions lib/rubocop/rails/schema_loader.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# frozen_string_literal: true

module RuboCop
module Rails
# Load db/schema.rb
module SchemaLoader
extend self

# It parses `db/schema.rb` and return it.
# It returns `nil` if it can't find `db/schema.rb`.
# So a cop that uses the loader should handle `nil` properly.
#
# @return [Schema, nil]
def load(target_ruby_version)
@schema if defined?(@schema)

@schema = load!(target_ruby_version)
end

def reset!
return unless instance_variable_defined?(:@schema)

remove_instance_variable(:@schema)
end

private

def load!(target_ruby_version)
path = db_schema_path
return unless path

ast = parse(path, target_ruby_version)
Schema.new(ast)
end

def db_schema_path
path = Pathname.pwd
until path.root?
schema_path = path.join('db/schema.rb')
return schema_path if schema_path.exist?

path = path.join('../').cleanpath
end

nil
end

def parse(path, target_ruby_version)
klass_name = :"Ruby#{target_ruby_version.to_s.sub('.', '')}"
klass = ::Parser.const_get(klass_name)
parser = klass.new(RuboCop::AST::Builder.new)

buffer = Parser::Source::Buffer.new(path, 1)
buffer.source = path.read

parser.parse(buffer)
end
end
end
end
Loading

0 comments on commit ac6cf91

Please sign in to comment.