Support composite primary keys

fatkodima · fatkodima · commit b95eb0b15ae1 · 2023-08-22T20:39:15.000+03:00
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -104,9 +104,9 @@ jobs:
           - ruby-version: 3.0.5
             gemfile: gemfiles/Gemfile.activerecord-6.0.x
           - ruby-version: 3.1.3
-            gemfile: gemfiles/Gemfile.activerecord-6.1.x
+            gemfile: gemfiles/Gemfile.activerecord-6.0.x
           - ruby-version: 3.2.0
-            gemfile: gemfiles/Gemfile.activerecord-6.1.x
+            gemfile: gemfiles/Gemfile.activerecord-6.0.x
 
           - ruby-version: 2.5.9
             gemfile: gemfiles/Gemfile.activerecord-6.1.x
@@ -128,7 +128,16 @@ jobs:
           - ruby-version: 3.1.3
             gemfile: gemfiles/Gemfile.activerecord-7.0.x
           - ruby-version: 3.2.0
-            gemfile: gemfiles/Gemfile.activerecord-6.1.x
+            gemfile: gemfiles/Gemfile.activerecord-7.0.x
+
+          - ruby-version: 2.7.7
+            gemfile: gemfiles/Gemfile.activerecord-head
+          - ruby-version: 3.0.5
+            gemfile: gemfiles/Gemfile.activerecord-head
+          - ruby-version: 3.1.3
+            gemfile: gemfiles/Gemfile.activerecord-head
+          - ruby-version: 3.2.0
+            gemfile: gemfiles/Gemfile.activerecord-head
     env:
       BUNDLE_GEMFILE: ${{ github.workspace }}/${{ matrix.gemfile }}
     steps:
diff --git a/active_record_doctor.gemspec b/active_record_doctor.gemspec
@@ -4,8 +4,6 @@ $LOAD_PATH.push File.expand_path("lib", __dir__)
 
 require "active_record_doctor/version"
 
-ACTIVE_RECORD_SPEC = ">= 4.2.0"
-
 Gem::Specification.new do |s|
   s.name        = "active_record_doctor"
   s.version     = ActiveRecordDoctor::VERSION
@@ -20,12 +18,11 @@ Gem::Specification.new do |s|
 
   s.required_ruby_version = ">= 2.1.0"
 
-  s.add_dependency "activerecord", ACTIVE_RECORD_SPEC
+  s.add_dependency "activerecord", ">= 4.2.0"
 
   s.add_development_dependency "minitest-fork_executor", "~> 1.0.2"
   s.add_development_dependency "mysql2", "~> 0.5.3"
   s.add_development_dependency "pg", "~> 1.1.4"
-  s.add_development_dependency "railties", ACTIVE_RECORD_SPEC
   s.add_development_dependency "rake", "~> 12.3.3"
   s.add_development_dependency "transient_record", "= 1.0.0.rc1"
 
diff --git a/gemfiles/Gemfile.activerecord-4.2.x b/gemfiles/Gemfile.activerecord-4.2.x
@@ -2,4 +2,5 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 4.2.0"
+gem "railties", "~> 4.2.0"
 gem "pg", "<= 0.20"
diff --git a/gemfiles/Gemfile.activerecord-5.0.x b/gemfiles/Gemfile.activerecord-5.0.x
@@ -2,5 +2,6 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 5.0.0"
+gem "railties", "~> 5.0.0"
 gem "pg", "~> 1.0.0"
 gem "mysql2", "~> 0.5.3"
diff --git a/gemfiles/Gemfile.activerecord-5.1.x b/gemfiles/Gemfile.activerecord-5.1.x
@@ -2,5 +2,6 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 5.1.0"
+gem "railties", "~> 5.1.0"
 gem "pg", "~> 1.0.0"
 gem "mysql2", "~> 0.5.3"
diff --git a/gemfiles/Gemfile.activerecord-5.2.x b/gemfiles/Gemfile.activerecord-5.2.x
@@ -2,6 +2,7 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 5.2.0"
+gem "railties", "~> 5.2.0"
 
 # Older versions result in lots of warnings in Ruby 2.7.
 gem "pg", "~> 1.2.0"
diff --git a/gemfiles/Gemfile.activerecord-6.0.x b/gemfiles/Gemfile.activerecord-6.0.x
@@ -2,6 +2,7 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 6.0.0"
+gem "railties", "~> 6.0.0"
 
 # Older versions result in lots of warnings in Ruby 2.7.
 gem "pg", "~> 1.2.0"
diff --git a/gemfiles/Gemfile.activerecord-6.1.x b/gemfiles/Gemfile.activerecord-6.1.x
@@ -2,6 +2,7 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 6.1.0"
+gem "railties", "~> 6.1.0"
 
 # Older versions result in lots of warnings in Ruby 2.7.
 gem "pg", "~> 1.4.5"
diff --git a/gemfiles/Gemfile.activerecord-7.0.x b/gemfiles/Gemfile.activerecord-7.0.x
@@ -2,6 +2,7 @@ source "https://rubygems.org"
 gemspec path: File.join(File.dirname(__FILE__), "..")
 
 gem "activerecord", "~> 7.0.0"
+gem "railties", "~> 7.0.0"
 
 # Older versions result in lots of warnings in Ruby 2.7.
 gem "pg", "~> 1.4.5"
diff --git a/gemfiles/Gemfile.activerecord-head b/gemfiles/Gemfile.activerecord-head
@@ -0,0 +1,9 @@
+source "https://rubygems.org"
+gemspec path: File.join(File.dirname(__FILE__), "..")
+
+gem "activerecord", github: "rails/rails", branch: "main"
+gem "railties", github: "rails/rails", branch: "main"
+
+# Older versions result in lots of warnings in Ruby 2.7.
+gem "pg", "~> 1.4.5"
+gem "mysql2", "~> 0.5.3"
diff --git a/lib/active_record_doctor/detectors/base.rb b/lib/active_record_doctor/detectors/base.rb
@@ -118,10 +118,10 @@ def indexes(table_name)
       end
 
       def primary_key(table_name)
-        primary_key_name = connection.primary_key(table_name)
-        return nil if primary_key_name.nil?
+        primary_key_names = Array(connection.primary_key(table_name))
+        return nil if primary_key_names.empty?
 
-        column(table_name, primary_key_name)
+        primary_key_names.map { |column_name| column(table_name, column_name) }
       end
 
       def column(table_name, column_name)
diff --git a/lib/active_record_doctor/detectors/extraneous_indexes.rb b/lib/active_record_doctor/detectors/extraneous_indexes.rb
@@ -21,7 +21,7 @@ class ExtraneousIndexes < Base # :nodoc:
 
       def message(table:, extraneous_index:, replacement_indexes:)
         if replacement_indexes.nil?
-          "remove #{extraneous_index} from #{table} - coincides with the primary key on the table"
+          "remove #{extraneous_index} from #{table} - covered by the primary key index on the table"
         else
           # rubocop:disable Layout/LineLength
           "remove the index #{extraneous_index} from the table #{table} - queries should be able to use the following #{'index'.pluralize(replacement_indexes.count)} instead: #{replacement_indexes.join(' or ')}"
@@ -62,7 +62,8 @@ def indexed_primary_keys
           each_table(except: config(:ignore_tables)) do |table|
             each_index(table, except: config(:ignore_indexes), multicolumn_only: true) do |index|
               primary_key = connection.primary_key(table)
-              if index.columns == [primary_key] && index.where.nil?
+
+              if replaceable_columns?(index.columns, index.unique, primary_key, true) && index.where.nil?
                 problem!(table: table, extraneous_index: index.name, replacement_indexes: nil)
               end
             end
@@ -76,23 +77,27 @@ def replaceable_with?(index1, index2)
         return false if index1.where != index2.where
         return false if opclasses(index1) != opclasses(index2)
 
-        index1_columns = Array(index1.columns)
-        index2_columns = Array(index2.columns)
+        replaceable_columns?(index1.columns, index1.unique, index2.columns, index2.unique)
+      end
+
+      def opclasses(index)
+        index.respond_to?(:opclasses) ? index.opclasses : nil
+      end
 
-        case [index1.unique, index2.unique]
+      def replaceable_columns?(columns1, unique1, columns2, unique2)
+        columns1 = Array(columns1)
+        columns2 = Array(columns2)
+
+        case [unique1, unique2]
         when [true, true]
-          (index2_columns - index1_columns).empty?
+          (columns2 - columns1).empty?
         when [true, false]
           false
         else
-          prefix?(index1_columns, index2_columns)
+          prefix?(columns1, columns2)
         end
       end
 
-      def opclasses(index)
-        index.respond_to?(:opclasses) ? index.opclasses : nil
-      end
-
       def prefix?(lhs, rhs)
         lhs.count <= rhs.count && rhs[0...lhs.count] == lhs
       end
diff --git a/lib/active_record_doctor/detectors/mismatched_foreign_key_type.rb b/lib/active_record_doctor/detectors/mismatched_foreign_key_type.rb
@@ -31,6 +31,7 @@ def detect
 
             next if ignored?("#{table}.#{from_column.name}", config(:ignore_columns))
 
+            # TODO: Add support for composite primary keys when https://github.com/rails/rails/pull/47637 is merged.
             to_table = foreign_key.to_table
             to_column = column(to_table, foreign_key.primary_key)
 
diff --git a/lib/active_record_doctor/detectors/missing_presence_validation.rb b/lib/active_record_doctor/detectors/missing_presence_validation.rb
@@ -34,7 +34,7 @@ def detect
       end
 
       def validator_needed?(model, column)
-        ![model.primary_key, "created_at", "updated_at", "created_on", "updated_on"].include?(column.name) &&
+        ![*Array(model.primary_key), "created_at", "updated_at", "created_on", "updated_on"].include?(column.name) &&
           (!column.null || not_null_check_constraint_exists?(model.table_name, column))
       end
 
diff --git a/lib/active_record_doctor/detectors/short_primary_key_type.rb b/lib/active_record_doctor/detectors/short_primary_key_type.rb
@@ -21,8 +21,10 @@ def message(table:, column:)
 
       def detect
         each_table(except: config(:ignore_tables)) do |table|
-          column = primary_key(table)
-          next if column.nil?
+          columns = primary_key(table)
+          next if columns.nil? || columns.size > 1
+
+          column = columns.first
           next if !integer?(column) || bigint?(column)
 
           problem!(table: table, column: column.name)
diff --git a/test/active_record_doctor/detectors/extraneous_indexes_test.rb b/test/active_record_doctor/detectors/extraneous_indexes_test.rb
@@ -7,7 +7,7 @@ def test_index_on_primary_key_is_duplicate
     end
 
     assert_problems(<<OUTPUT)
-remove index_users_on_id from users - coincides with the primary key on the table
+remove index_users_on_id from users - covered by the primary key index on the table
 OUTPUT
   end
 
@@ -22,13 +22,35 @@ def test_partial_index_on_primary_key
     refute_problems
   end
 
+  def test_index_on_composite_primary_key_columns
+    skip("ActiveRecord < 7.1 doesn't support composite primary keys") if ActiveRecord::VERSION::STRING < "7.1"
+
+    create_table(:users, primary_key: [:company_id, :id]) do |t|
+      t.bigint :company_id
+      t.bigint :id
+      t.string :email
+      t.index [:company_id, :id], name: "index_1"
+      t.index [:company_id], name: "index_2"
+      t.index [:id], name: "index_3"
+      t.index [:company_id, :id, :email], unique: true, name: "index_4"
+    end
+
+    assert_problems(<<OUTPUT)
+remove index_1 from users - covered by the primary key index on the table
+remove index_2 from users - covered by the primary key index on the table
+remove index_4 from users - covered by the primary key index on the table
+remove the index index_1 from the table users - queries should be able to use the following index instead: index_4
+remove the index index_2 from the table users - queries should be able to use the following indices instead: index_1 or index_4
+OUTPUT
+  end
+
   def test_index_on_non_standard_primary_key
     create_table(:profiles, primary_key: :user_id) do |t|
       t.index :user_id
     end
 
     assert_problems(<<OUTPUT)
-remove index_profiles_on_user_id from profiles - coincides with the primary key on the table
+remove index_profiles_on_user_id from profiles - covered by the primary key index on the table
 OUTPUT
   end
 
@@ -243,7 +265,7 @@ def test_config_ignore_tables_string_ignores_exact_match
     CONFIG
 
     assert_problems(<<OUTPUT)
-remove index_users_on_id from users - coincides with the primary key on the table
+remove index_users_on_id from users - covered by the primary key index on the table
 OUTPUT
   end
 
diff --git a/test/active_record_doctor/detectors/missing_presence_validation_test.rb b/test/active_record_doctor/detectors/missing_presence_validation_test.rb
@@ -139,6 +139,17 @@ def test_timestamps_are_not_reported
     refute_problems
   end
 
+  def test_composite_primary_key_is_not_reported
+    skip("ActiveRecord < 7.1 doesn't support composite primary keys") if ActiveRecord::VERSION::STRING < "7.1"
+
+    create_table(:users, primary_key: [:company_id, :id]) do |t|
+      t.bigint :company_id
+      t.bigint :id
+    end.define_model
+
+    refute_problems
+  end
+
   def test_models_with_non_existent_tables_are_skipped
     define_model(:User)
 
diff --git a/test/active_record_doctor/detectors/short_primary_key_type_test.rb b/test/active_record_doctor/detectors/short_primary_key_type_test.rb
@@ -50,6 +50,17 @@ def test_no_primary_key_is_not_reported
     refute_problems
   end
 
+  def test_composite_primary_key_is_not_reported
+    skip("ActiveRecord < 7.1 doesn't support composite primary keys") if ActiveRecord::VERSION::STRING < "7.1"
+
+    create_table(:companies, primary_key: [:country_id, :id]) do |t|
+      t.integer :country_id
+      t.integer :id
+    end
+
+    refute_problems
+  end
+
   def test_config_ignore_tables
     create_table(:companies, id: :integer)
 
