Merge pull request #197 from pocke/Rails/UniqueValidationWithoutIndex

Add Rails/UniqueValidationWithoutIndex cop

.rubocop.yml

@@ -82,6 +82,7 @@ Metrics/BlockLength:
     - 'Rakefile'
     - '**/*.rake'
     - 'spec/**/*.rb'
+    - '*.gemspec'
 
 Naming/FileName:
   Exclude:

CHANGELOG.md

@@ -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][])

config/default.yml

@@ -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

lib/rubocop-rails.rb

@@ -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!
 

lib/rubocop/cop/mixin/active_record_helper.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # A mixin to extend cops for Active Record features
+    module ActiveRecordHelper
+      extend NodePattern::Macros
+
+      def_node_search :find_set_table_name, <<~PATTERN
+        (send self :table_name= {str sym})
+      PATTERN
+
+      def_node_search :find_belongs_to, <<~PATTERN
+        (send nil? :belongs_to {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)
+        [class_node, *namespaces]
+          .reverse
+          .map { |klass| klass.identifier.children[1] }.join('_')
+          .tableize
+      end
+
+      # Resolve relation into column name.
+      # It just returns column_name if the column exists.
+      # Or it tries to resolve column_name as a relation.
+      # It returns `nil` if it can't resolve.
+      #
+      # @param name [String]
+      # @param class_node [RuboCop::AST::Node]
+      # @param table [RuboCop::Rails::SchemaLoader::Table]
+      # @return [String, nil]
+      def resolve_relation_into_column(name:, class_node:, table:)
+        return name if table.with_column?(name: name)
+
+        find_belongs_to(class_node) do |belongs_to|
+          next unless belongs_to.first_argument.value.to_s == name
+
+          fk = foreign_key_of(belongs_to) || "#{name}_id"
+          return fk if table.with_column?(name: fk)
+        end
+        nil
+      end
+
+      def foreign_key_of(belongs_to)
+        options = belongs_to.last_argument
+        return unless options.hash_type?
+
+        options.each_pair.find do |pair|
+          next unless pair.key.sym_type? && pair.key.value == :foreign_key
+          next unless pair.value.sym_type? || pair.value.str_type?
+
+          break pair.value.value.to_s
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails/unique_validation_without_index.rb

@@ -0,0 +1,133 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # When you define a uniqueness validation in Active Record model,
+      # you also should add a unique index for the column. There are two reasons
+      # First, duplicated records may occur even if Active Record'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.
+      #
+      # Note that the cop does nothing if db/schema.rb does not exist.
+      #
+      # @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 true 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! do |name|
+            klass = class_node(node)
+            resolve_relation_into_column(
+              name: name.to_s,
+              class_node: klass,
+              table: schema.table_by(name: table_name(klass))
+            )
+          end
+          ret.include?(nil) ? nil : ret.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

lib/rubocop/cop/rails_cops.rb

@@ -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'
@@ -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'

lib/rubocop/rails/schema_loader.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Rails
+    # It loads db/schema.rb and return Schema object.
+    # Cops refers database schema information with this module.
+    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)
+        return @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