From 9c9eac639dbab8061814a601d94d89a07b805b46 Mon Sep 17 00:00:00 2001 From: Masataka Pocke Kuwabara Date: Sat, 8 Feb 2020 03:16:48 +0900 Subject: [PATCH] Add `Rails/UniqueValidationWithoutIndex` cop --- .rubocop.yml | 1 + CHANGELOG.md | 4 + config/default.yml | 7 + lib/rubocop-rails.rb | 3 + lib/rubocop/cop/mixin/active_record_helper.rb | 62 +++ .../rails/unique_validation_without_index.rb | 133 +++++++ lib/rubocop/cop/rails_cops.rb | 2 + lib/rubocop/rails/schema_loader.rb | 61 +++ lib/rubocop/rails/schema_loader/schema.rb | 153 +++++++ manual/cops.md | 1 + manual/cops_rails.md | 36 ++ rubocop-rails.gemspec | 1 + .../unique_validation_without_index_spec.rb | 372 ++++++++++++++++++ spec/rubocop/rails/schema_loader_spec.rb | 100 +++++ 14 files changed, 936 insertions(+) create mode 100644 lib/rubocop/cop/mixin/active_record_helper.rb create mode 100644 lib/rubocop/cop/rails/unique_validation_without_index.rb create mode 100644 lib/rubocop/rails/schema_loader.rb create mode 100644 lib/rubocop/rails/schema_loader/schema.rb create mode 100644 spec/rubocop/cop/rails/unique_validation_without_index_spec.rb create mode 100644 spec/rubocop/rails/schema_loader_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index f549d93326..628f765390 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -82,6 +82,7 @@ Metrics/BlockLength: - 'Rakefile' - '**/*.rake' - 'spec/**/*.rb' + - '*.gemspec' Naming/FileName: Exclude: diff --git a/CHANGELOG.md b/CHANGELOG.md index 772bdc0217..0678ea6bc6 100644 --- a/CHANGELOG.md +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 065b8425c3..7dead435a4 100644 --- a/config/default.yml +++ b/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 diff --git a/lib/rubocop-rails.rb b/lib/rubocop-rails.rb index 9ce301f1bd..fb215b534c 100644 --- a/lib/rubocop-rails.rb +++ b/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! diff --git a/lib/rubocop/cop/mixin/active_record_helper.rb b/lib/rubocop/cop/mixin/active_record_helper.rb new file mode 100644 index 0000000000..90b86dd815 --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/rails/unique_validation_without_index.rb b/lib/rubocop/cop/rails/unique_validation_without_index.rb new file mode 100644 index 0000000000..ea9a6bf27a --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 1c7966a8b3..45e465d94c 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/lib/rubocop/rails/schema_loader.rb b/lib/rubocop/rails/schema_loader.rb new file mode 100644 index 0000000000..7f59f19867 --- /dev/null +++ b/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 diff --git a/lib/rubocop/rails/schema_loader/schema.rb b/lib/rubocop/rails/schema_loader/schema.rb new file mode 100644 index 0000000000..cbb1882926 --- /dev/null +++ b/lib/rubocop/rails/schema_loader/schema.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +module RuboCop + module Rails + module SchemaLoader + # Represent db/schema.rb + class Schema + attr_reader :tables + + def initialize(ast) + @tables = [] + build!(ast) + end + + def table_by(name:) + tables.find do |table| + table.name == name + end + end + + private + + def build!(ast) + raise "Unexpected type: #{ast.type}" unless ast.block_type? + + each_table(ast) do |table_def| + @tables << Table.new(table_def) + end + end + + def each_table(ast) + case ast.body.type + when :begin + ast.body.children.each do |node| + next unless node.block_type? && node.method?(:create_table) + + yield(node) + end + else + yield ast.body + end + end + end + + # Reprecent a table + class Table + attr_reader :name, :columns, :indices + + def initialize(node) + @name = node.send_node.first_argument.value + @columns = build_columns(node) + @indices = build_indices(node) + end + + def with_column?(name:) + @columns.any? { |c| c.name == name } + end + + private + + def build_columns(node) + each_content(node).map do |child| + next unless child.send_type? + next if child.method?(:index) + + Column.new(child) + end.compact + end + + def build_indices(node) + each_content(node).map do |child| + next unless child.send_type? + next unless child.method?(:index) + + Index.new(child) + end.compact + end + + def each_content(node) + return enum_for(__method__, node) unless block_given? + + case node.body.type + when :begin + node.body.children.each do |child| + yield(child) + end + else + yield(node.body) + end + end + end + + # Reprecent a column + class Column + attr_reader :name, :type, :not_null + + def initialize(node) + @name = node.first_argument.value + @type = node.method_name + @not_null = nil + + analyze_keywords!(node) + end + + private + + def analyze_keywords!(node) + pairs = node.arguments.last + return unless pairs.hash_type? + + pairs.each_pair do |k, v| + if k.value == :null + @not_null = v.true_type? ? false : true + end + end + end + end + + # Reprecent an index + class Index + attr_reader :name, :columns, :unique + + def initialize(node) + node.first_argument + @columns = build_columns(node) + @unique = nil + + analyze_keywords!(node) + end + + private + + def build_columns(node) + node.first_argument.values.map(&:value) + end + + def analyze_keywords!(node) + pairs = node.arguments.last + return unless pairs.hash_type? + + pairs.each_pair do |k, v| + case k.value + when :name + @name = v.value + when :unique + @unique = true + end + end + end + end + end + end +end diff --git a/manual/cops.md b/manual/cops.md index 00cf65ca00..2c56f775a5 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -56,6 +56,7 @@ * [Rails/SkipsModelValidations](cops_rails.md#railsskipsmodelvalidations) * [Rails/TimeZone](cops_rails.md#railstimezone) * [Rails/UniqBeforePluck](cops_rails.md#railsuniqbeforepluck) +* [Rails/UniqueValidationWithoutIndex](cops_rails.md#railsuniquevalidationwithoutindex) * [Rails/UnknownEnv](cops_rails.md#railsunknownenv) * [Rails/Validation](cops_rails.md#railsvalidation) diff --git a/manual/cops_rails.md b/manual/cops_rails.md index 4c30d2f926..dfa7a62796 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -2498,6 +2498,42 @@ Name | Default value | Configurable values EnforcedStyle | `conservative` | `conservative`, `aggressive` AutoCorrect | `false` | Boolean +## Rails/UniqueValidationWithoutIndex + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | No | 2.5 | - + +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. + +### Examples + +```ruby +# 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 } +``` + +### Configurable attributes + +Name | Default value | Configurable values +--- | --- | --- +Include | `app/models/**/*.rb` | Array + ## Rails/UnknownEnv Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/rubocop-rails.gemspec b/rubocop-rails.gemspec index 4aef122620..0dcbc1a00f 100644 --- a/rubocop-rails.gemspec +++ b/rubocop-rails.gemspec @@ -31,6 +31,7 @@ Gem::Specification.new do |s| 'bug_tracker_uri' => 'https://github.com/rubocop-hq/rubocop-rails/issues' } + s.add_runtime_dependency 'activesupport' # Rack::Utils::SYMBOL_TO_STATUS_CODE, which is used by HttpStatus cop, was # introduced in rack 1.1 s.add_runtime_dependency 'rack', '>= 1.1' diff --git a/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb b/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb new file mode 100644 index 0000000000..54e7cc18b9 --- /dev/null +++ b/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb @@ -0,0 +1,372 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::UniqueValidationWithoutIndex, :config do + subject(:cop) { described_class.new(config) } + + context 'without db/schema.rb' do + it 'does nothing' do + expect_no_offenses(<<~RUBY) + class User < ApplicationRecord + validates :account, uniqueness: true + end + RUBY + end + end + + context 'with db/schema.rb' do + let(:schema_path) do + f = Tempfile.create('rubocop-rails-UniqueValidationWithoutIndex-test-') + f.close + Pathname(f.path) + end + + before do + RuboCop::Rails::SchemaLoader.reset! + schema_path.write(schema) + allow(RuboCop::Rails::SchemaLoader).to receive(:db_schema_path) + .and_return(schema_path) + end + + after do + RuboCop::Rails::SchemaLoader.reset! + schema_path.unlink + end + + 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 + # t.index ["account"], name: "index_users_on_account" + end + 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 'when the table has an index but it is not unique' 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 + t.index ["account"], name: "index_users_on_account" + end + 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 'when the table has an index but it is not unique' 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 + t.index ["account"], name: "index_users_on_account" + end + 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 + t.index ["account"], name: "index_users_on_account", unique: true + end + end + RUBY + + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class User + validates :account, uniqueness: true + end + RUBY + end + end + + context 'when the validation is for two columns' do + context 'without proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "written_articles", force: :cascade do |t| + t.bitint "user_id", null: false + t.bitint "article_id", null: false + t.index ["user_id"], name: "idx_uid", unique: true + t.index ["article_id"], name: "idx_aid", unique: true + end + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + class WrittenArticles + validates :user_id, uniqueness: { scope: :article_id } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + RUBY + end + end + + context 'with proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "written_articles", force: :cascade do |t| + t.bitint "user_id", null: false + t.bitint "article_id", null: false + t.index ["user_id", "article_id"], name: "idx_uid_aid", unique: true + end + end + RUBY + + it 'registers an offense' do + expect_no_offenses(<<~RUBY) + class WrittenArticles + validates :user_id, uniqueness: { scope: :article_id } + end + RUBY + end + end + end + + context 'when the validation is for three columns' do + context 'without proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "written_articles", force: :cascade do |t| + t.bitint "a_id", null: false + t.bitint "b_id", null: false + t.bitint "c_id", null: false + t.index ["a_id"], name: "idx_aid", unique: true + t.index ["b_id"], name: "idx_bid", unique: true + t.index ["c_id"], name: "idx_cid", unique: true + end + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + class WrittenArticles + validates :a_id, uniqueness: { scope: [:b_id, :c_id] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + RUBY + end + end + + context 'with proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "written_articles", force: :cascade do |t| + t.bitint "a_id", null: false + t.bitint "b_id", null: false + t.bitint "c_id", null: false + t.index ["a_id", "b_id", "c_id"], name: "idx_ids", unique: true + end + end + RUBY + + it 'registers an offense' do + expect_no_offenses(<<~RUBY) + class WrittenArticles + validates :a_id, uniqueness: { scope: [:b_id, :c_id] } + end + RUBY + end + end + end + + context 'when the validation is for a relation with _id column' do + context 'without proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "articles", force: :cascade do |t| + t.bitint "user_id", null: false + t.index ["user_id"], name: "idx_user_id" + end + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + class Article + belongs_to :user + validates :user, uniqueness: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + RUBY + end + end + + context 'with proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "articles", force: :cascade do |t| + t.bitint "user_id", null: false + t.index ["user_id"], name: "idx_user_id", unique: true + end + end + RUBY + + it 'does not register offense' do + expect_no_offenses(<<~RUBY) + class Article + belongs_to :user + validates :user, uniqueness: true + end + RUBY + end + end + + context 'without column definition' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "articles", force: :cascade do |t| + t.bitint "member_id", null: false + t.index ["user_id"], name: "idx_user_id", unique: true + end + end + RUBY + + it 'ignores it' do + expect_no_offenses(<<~RUBY) + class Article + belongs_to :user + validates :user, uniqueness: true + end + RUBY + end + end + end + + context 'when the validation is for a relation with foreign_key: option' do + context 'without proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "articles", force: :cascade do |t| + t.bitint "user_id", null: false + t.index ["user_id"], name: "idx_user_id" + end + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + class Article + belongs_to :member, foreign_key: :user_id + validates :member, uniqueness: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + RUBY + end + end + + context 'with proper index' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "articles", force: :cascade do |t| + t.bitint "user_id", null: false + t.index ["user_id"], name: "idx_user_id", unique: true + end + end + RUBY + + it 'does not register offense' do + expect_no_offenses(<<~RUBY) + class Article + belongs_to :member, foreign_key: :user_id + validates :member, uniqueness: true + end + RUBY + end + end + + context 'without column definition' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "articles", force: :cascade do |t| + t.bitint "foo_id", null: false + t.index ["user_id"], name: "idx_user_id", unique: true + end + end + RUBY + + it 'ignores it' do + expect_no_offenses(<<~RUBY) + class Article + belongs_to :member, foreign_key: :user_id + validates :member, uniqueness: true + end + RUBY + end + end + end + + context 'with ActiveRecord::Base.table_name=' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "members", force: :cascade do |t| + t.string "account", null: false + end + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + class User + self.table_name = 'members' + validates :account, uniqueness: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + RUBY + end + end + + context 'with nested class' do + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "admin_users", force: :cascade do |t| + t.string "account", null: false + end + end + RUBY + + it 'registers an offense' do + expect_offense(<<~RUBY) + module Admin + class User + validates :account, uniqueness: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Uniqueness validation should be with a unique index. + end + end + RUBY + end + end + end +end diff --git a/spec/rubocop/rails/schema_loader_spec.rb b/spec/rubocop/rails/schema_loader_spec.rb new file mode 100644 index 0000000000..81248508ff --- /dev/null +++ b/spec/rubocop/rails/schema_loader_spec.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Rails::SchemaLoader do + describe '.load' do + require 'parser/ruby27' + let(:target_ruby_version) { 2.7 } + + around do |example| + described_class.reset! + example.run + described_class.reset! + end + + context 'without schema.rb' do + it do + expect(described_class.load(target_ruby_version)).to be nil + end + end + + context 'with schema.rb' do + subject(:loaded_schema) do + described_class.load(target_ruby_version) + end + + let(:rails_root) { Pathname(Dir.mktmpdir) } + let(:schema) { <<~RUBY } + ActiveRecord::Schema.define(version: 2020_02_02_075409) do + create_table "users", force: :cascade do |t| + t.string "account", null: false + t.index ["account"], name: "index_users_on_account", unique: true + end + + create_table "articles", force: :cascade do |t| + t.string "title", null: false + t.bigint "user_id" + end + end + RUBY + + before do + rails_root.join('app/models').mkpath + rails_root.join('db').mkpath + rails_root.join('db/schema.rb').write(schema) + end + + after do + rails_root.rmtree + end + + shared_examples 'returns a schema' do + it 'returns a schema' do + klass = RuboCop::Rails::SchemaLoader::Schema + expect(loaded_schema.is_a?(klass)).to be(true) + expect(loaded_schema.tables.size).to eq 2 + end + + it 'has a column in users table' do + table = loaded_schema.table_by(name: 'users') + expect(table.name).to eq 'users' + expect(table.columns.size).to eq 1 + expect(table.columns.first.name).to eq 'account' + expect(table.columns.first.not_null).to be true + end + + it 'has an index in users table' do + table = loaded_schema.table_by(name: 'users') + expect(table.indices.size).to eq 1 + expect(table.indices.first.name).to eq 'index_users_on_account' + expect(table.indices.first.columns).to eq ['account'] + expect(table.indices.first.unique).to be true + end + + it 'has articles table' do + table = loaded_schema.table_by(name: 'articles') + expect(table.name).to eq 'articles' + + expect(table.columns.size).to eq 2 + expect(table.columns.last.type).to eq :bigint + end + end + + context 'when the current directory is Rails.root' do + before do + allow(Pathname).to receive(:pwd).and_return(rails_root) + end + + it_behaves_like 'returns a schema' + end + + context 'when the current directory is a sub-directory of Rails.root' do + before do + allow(Pathname).to receive(:pwd) + .and_return(rails_root.join('app/models')) + end + + it_behaves_like 'returns a schema' + end + end + end +end