Testing and fixing composite keys stuff

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Unreleased
 
+# 1.2.0 - 2024-08-31
+
+* Add support for composite primary keys in Rails 7.2
+
 # 1.1.5 - 2024-05-18
 
 * Add compatibility for Rails 7.2
@@ -22,29 +26,29 @@
 
 # 1.1.0 - 2020-02-24
 
-* Added methods which return the SQL used by this gem: `assoc_exists_sql`, `assoc_not_exists_sql`, `compare_assoc_count_sql`, `only_assoc_count_sql`  
+* Added methods which return the SQL used by this gem: `assoc_exists_sql`, `assoc_not_exists_sql`, `compare_assoc_count_sql`, `only_assoc_count_sql`
   [Documentation for them](https://maxlap.github.io/activerecord_where_assoc/ActiveRecordWhereAssoc/SqlReturningMethods.html)
 
 # 1.0.1
 
-* Fix broken urls in error messages 
+* Fix broken urls in error messages
 
 # 1.0.0
 
 * Now supports polymorphic belongs_to
 
 # 0.1.3
 
-* Use `SELECT 1` instead of `SELECT 0`...  
+* Use `SELECT 1` instead of `SELECT 0`...
   ... it just seems more natural that way.
 * Bugfixes
 
 # 0.1.2
 
-* It is now possible to pass a `Range` as first argument to `#where_assoc_count`.  
+* It is now possible to pass a `Range` as first argument to `#where_assoc_count`.
   Ex: Users that have between 10 and 20 posts
   `User.where_assoc_count(10..20, :==, :posts)`
-  The operator in that case must be either :== or :!=.  
-  This will use `BETWEEN` and `NOT BETWEEN`.  
+  The operator in that case must be either :== or :!=.
+  This will use `BETWEEN` and `NOT BETWEEN`.
   Ranges that exclude the last value, i.e. `5...10`, are also supported, resulting in `BETWEEN 5 and 9`.
   Ranges with infinities are also supported.

gemfiles/rails_7_1.gemfile

@@ -2,7 +2,7 @@
 
 source "https://rubygems.org"
 
-gem "activerecord", "~> 7.1.x"
+gem "activerecord", "~> 7.1.0"
 gem "sqlite3", "~> 1.4.0"
 gem "pg", "~> 1.1"
 gem "mysql2", "~> 0.5" if ENV["CI"] || ENV["ALL_DB"] || ENV["DB"] == "mysql"

lib/active_record_where_assoc/core_logic.rb

@@ -364,6 +364,9 @@ def self.process_association_step_limits(current_scope, reflection, relation_kla
       if reflection.klass.table_name.include?(".") || option_value(options, :never_alias_limit)
         # We use unscoped to avoid duplicating the conditions in the query, which is noise. (unless it
         # could helps the query planner of the DB, if someone can show it to be worth it, then this can be changed.)
+        if reflection.klass.primary_key.is_a?(Array)
+          raise NeverAliasLimitDoesntWorkWithCompositePrimaryKeysError, "Sorry, it just doesn't work..."
+        end
 
         reflection.klass.unscoped.where(reflection.klass.primary_key.to_sym => current_scope)
       else
@@ -396,14 +399,12 @@ def self.build_alias_scope_for_recursive_association(reflection, poly_belongs_to
       alias_scope = foreign_klass.base_class.unscoped
       alias_scope = alias_scope.from("#{table.name} #{ALIAS_TABLE.name}")
 
-      primary_key_constraints = 
-        Array.wrap(primary_key).map do |a_primary_key|
+      primary_key_constraints =
+        Array(primary_key).map do |a_primary_key|
           table[a_primary_key].eq(ALIAS_TABLE[a_primary_key])
-        end 
+        end
 
-      primary_key_constraints.any? ? 
-        alias_scope.where(primary_key_constraints.inject(&:and)) : 
-        alias_scope
+      alias_scope.where(primary_key_constraints.inject(&:and))
     end
 
     def self.wrapper_and_join_constraints(record_class, reflection, options = {})
@@ -435,12 +436,12 @@ def self.wrapper_and_join_constraints(record_class, reflection, options = {})
       constraint_foreign_keys = Array.wrap(foreign_key)
       constraint_key_map = constraint_keys.zip(constraint_foreign_keys)
 
-      primary_foreign_key_constraints = 
+      primary_foreign_key_constraints =
         constraint_key_map.map do |primary_and_foreign_keys|
-          a_primary_key, a_foreign_key = primary_and_foreign_keys 
+          a_primary_key, a_foreign_key = primary_and_foreign_keys
 
           table[a_primary_key].eq(foreign_table[a_foreign_key])
-        end 
+        end
 
       constraints = primary_foreign_key_constraints.inject(&:and)
 

lib/active_record_where_assoc/exceptions.rb

@@ -6,4 +6,7 @@ class MySQLDoesntSupportSubLimitError < StandardError
 
   class PolymorphicBelongsToWithoutClasses < StandardError
   end
+
+  class NeverAliasLimitDoesntWorkWithCompositePrimaryKeysError < StandardError
+  end
 end

test/support/base_test_model.rb

@@ -86,10 +86,10 @@ def self.testable_has_and_belongs_to_many(association_name, given_scope = nil, *
     testable_association(:has_and_belongs_to_many, association_name, given_scope, **options)
   end
 
-  def self.create_default!(*source_associations)
+  def self.create_default!(*source_associations, **attributes)
     condition_value = TestHelpers.condition_value_result_for(*source_associations) || 1
     condition_value *= need_test_condition_value_for(:default_scope)
-    create!(test_condition_column => condition_value)
+    create!(test_condition_column => condition_value, **attributes)
   end
 
   def create_has_one!(association_name, attrs = {})
@@ -99,17 +99,26 @@ def create_has_one!(association_name, attrs = {})
 
     target_model = reflection.klass
 
-    old_matched_ids = target_model.where(reflection.foreign_key => self.id).unscope(:offset, :limit).pluck(:id).to_a
+    old_matched_ids = target_model.where(reflection.foreign_key => [self.id]).unscope(:offset, :limit).pluck(*target_model.primary_key).to_a
     record = send("create_#{association_name}!", attrs)
-    target_model.where(id: old_matched_ids).unscope(:offset, :limit).update_all(reflection.foreign_key => self.id)
+
+    if old_matched_ids.present?
+      if reflection.foreign_key.is_a?(Array)
+        to_update = reflection.foreign_key.zip(self.id).to_h
+      else
+        to_update = {reflection.foreign_key => self.id}
+      end
+      target_model.where(target_model.primary_key => old_matched_ids).unscope(:offset, :limit).update_all(to_update)
+    end
+
     record
   end
 
   # does a #create! and automatically fills the column with a value that matches the merge of the condition on
   # the matching association of each passed source_associations
   def create_assoc!(association_name, *source_associations)
     options = source_associations.extract_options!
-    options = options.reverse_merge(allow_no_source: false, adhoc_value: nil, skip_default: false, use_bad_type: false)
+    options = options.reverse_merge(allow_no_source: false, adhoc_value: nil, skip_default: false, use_bad_type: false, attributes: {})
 
     raise "Must be a direct association, not #{association_name.inspect}" unless association_name =~ /^[mobz]p?l?\d+$/
     raise "Need at least one source model or a nil instead" if !options[:allow_no_source] && source_associations.empty?
@@ -123,19 +132,18 @@ def create_assoc!(association_name, *source_associations)
 
     target_model = options[:target_model] || reflection.klass
 
-    if options[:skip_attributes]
-      attributes = {}
-    else
+    if !options[:skip_attributes]
       condition_value = target_model.test_condition_value_for(:default_scope) unless options[:skip_default]
 
       if source_associations.present?
         condition_value ||= 1
         condition_value *= TestHelpers.condition_value_result_for(*source_associations)
       end
 
-      attributes = { target_model.test_condition_column => condition_value,
-                     target_model.adhoc_column_name => options[:adhoc_value],
-      }
+      attributes = options[:attributes].merge(
+        target_model.test_condition_column => condition_value,
+        target_model.adhoc_column_name => options[:adhoc_value],
+      )
     end
 
     case association_macro

test/support/custom_asserts.rb

@@ -54,15 +54,15 @@ def assert_exists_with_matching_from(start_from, association_name, *args, &block
       msgs << "Expected query from where_assoc_exists to include the SQL from assoc_exists_sql"
     end
     if !exists_relation.exists?
-      msgs << "Expected a match but got none for S0.where_assoc_exists(#{association_name.inspect}, ...)"
+      msgs << "Expected a match but got none for where_assoc_exists(#{association_name.inspect}, ...)"
     end
 
     not_exists_relation = start_from.where_assoc_not_exists(association_name, *args, &block)
     if !not_exists_relation.to_sql.include?(start_from.assoc_not_exists_sql(association_name, *args, &block))
       msgs << "Expected query from where_assoc_not_exists to include the SQL from assoc_not_exists_sql"
     end
     if not_exists_relation.exists?
-      msgs << "Expected no matches but got one for S0.where_assoc_not_exists(#{association_name.inspect}, ...)"
+      msgs << "Expected no matches but got one for where_assoc_not_exists(#{association_name.inspect}, ...)"
     end
     assert msgs.empty?, msgs.map { |s| "  #{s}" }.join("\n")
   end
@@ -74,15 +74,15 @@ def assert_exists_without_matching_from(start_from, association_name, *args, &bl
       msgs << "Expected query from where_assoc_exists to include the SQL from assoc_exists_sql"
     end
     if exists_relation.exists?
-      msgs << "Expected no matches but got one for S0.where_assoc_exists(#{association_name.inspect}, ...)"
+      msgs << "Expected no matches but got one for where_assoc_exists(#{association_name.inspect}, ...)"
     end
 
     not_exists_relation = start_from.where_assoc_not_exists(association_name, *args, &block)
     if !not_exists_relation.to_sql.include?(start_from.assoc_not_exists_sql(association_name, *args, &block))
       msgs << "Expected query from where_assoc_not_exists to include the SQL from assoc_not_exists_sql"
     end
     if !not_exists_relation.exists?
-      msgs << "Expected a match but got none for S0.where_assoc_not_exists(#{association_name.inspect}, ...)"
+      msgs << "Expected a match but got none for where_assoc_not_exists(#{association_name.inspect}, ...)"
     end
     assert msgs.empty?, msgs.map { |s| "  #{s}" }.join("\n")
   end
@@ -163,10 +163,13 @@ def manual_testing_results_from(start_from, matching_nb, operator, association_n
 
     record_sets = record_sets.map do |records|
       next records if records.blank?
-      scope = records.first.class.base_class.unscoped.where(id: records.map(&:id))
+
+      record_klass = records.first.class.base_class
+
+      scope = record_klass.unscoped.where(record_klass.primary_key => records.map(&:id))
       scope = scope.where(conditions)
       scope = ActiveRecordWhereAssoc::CoreLogic.apply_proc_scope(scope, block) if block
-      scope.pluck(:id)
+      scope.pluck(*record_klass.primary_key)
     end
 
     correct_result = record_sets.any? do |records|

test/support/models.rb

@@ -286,4 +286,31 @@ class UnabstractModel < AbstractModel
 end
 
 class NeverAbstractedModel < BaseTestModel
+end
+
+if ActiveRecord.gem_version >= Gem::Version.new("7.2")
+  class Ck0 < BaseTestModel
+    self.primary_key = [:an_id0, :a_str0]
+    setup_test_default_scope
+
+    testable_has_many :m1, class_name: "Ck1", foreign_key: [:an_id0, :a_str0]
+    testable_has_one :o1, -> { order("ck1s.an_id1 DESC, ck1s.a_str1 DESC") }, class_name: "Ck1", foreign_key: [:an_id0, :a_str0]
+    # I don't think has_and_belongs_to_many supports composite keys?
+    #testable_has_and_belongs_to_many :z1, class_name: "Ck1"
+
+    testable_has_many :m2m1, class_name: "Ck2", through: :m1, source: :m2, foreign_key: [:an_id0, :a_str0]
+  end
+
+  class Ck1 < BaseTestModel
+    self.primary_key = [:an_id1, :a_str1]
+    setup_test_default_scope
+
+    testable_belongs_to :b0, class_name: "Ck0", foreign_key: [:an_id0, :a_str0]
+    testable_has_many :m2, class_name: "Ck2", foreign_key: [:an_id1, :a_str1]
+  end
+
+  class Ck2 < BaseTestModel
+    self.primary_key = [:an_id2, :a_str2]
+    setup_test_default_scope
+  end
 end

test/support/schema.rb

@@ -162,4 +162,37 @@
     t.integer :never_abstracted_models_column, limit: 8
     t.integer :never_abstracted_models_adhoc_column, limit: 8
   end
+
+  create_table :ck0s, primary_key: [:an_id0, :a_str0] do |t|
+    t.integer :an_id0
+    t.string :a_str0, limit: 20 # Needed for MySQL's primary keys
+
+    t.integer :ck0s_column
+    t.integer :ck0s_adhoc_column
+  end
+
+  create_table :ck1s, primary_key: [:an_id1, :a_str1] do |t|
+    t.integer :an_id1
+    t.string :a_str1, limit: 20 # Needed for MySQL's primary keys
+
+    t.integer :an_id0
+    t.string :a_str0, limit: 20 # Needed for MySQL's primary keys
+
+    t.integer :ck1s_column
+    t.integer :ck1s_adhoc_column
+  end
+
+  create_join_table "ck0s", "ck1s"
+
+
+  create_table :ck2s, primary_key: [:an_id2, :a_str2] do |t|
+    t.integer :an_id2
+    t.string :a_str2, limit: 20 # Needed for MySQL's primary keys
+
+    t.integer :an_id1
+    t.string :a_str1, limit: 20 # Needed for MySQL's primary keys
+
+    t.integer :ck2s_column
+    t.integer :ck2s_adhoc_column
+  end
 end

test/tests/wa_composite_keys_test.rb

@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require_relative "../test_helper"
+
+describe "wa" do
+  next if ActiveRecord.gem_version < Gem::Version.new("7.2")
+
+  it "belongs_to with composite_key works" do
+    ck1 = Ck1.create_default!(an_id1: 0, a_str1: "hi")
+    ck1.create_assoc!(:b0, :Ck1_b0, attributes: {an_id0: 1, a_str0: "bar"})
+    ck0_spam = ck1.create_assoc!(:b0, :Ck1_b0, attributes: {an_id0: 1, a_str0: "spam"})
+    ck1.save! # Save the updates ids
+
+    assert_wa_from(Ck1, 1, :b0)
+
+    ck1_1 = Ck1.create_default!(an_id1: 1, a_str1: "foo")
+    ck1_2 = Ck1.create_default!(an_id1: 12, a_str1: "foo")
+
+    assert_equal [ck1], Ck1.where_assoc_count(1, :==, :b0).to_a.sort_by(&:an_id1)
+    assert_equal [ck1_1, ck1_2], Ck1.where_assoc_count(0, :==, :b0).to_a.sort_by(&:an_id1)
+  end
+
+  it "has_many with composite_key works" do
+    ck0 = Ck0.create_default!(an_id0: 1, a_str0: "foo")
+    ck0.create_assoc!(:m1, :Ck0_m1, attributes: {an_id1: 1, a_str1: "bar"})
+    ck0.create_assoc!(:m1, :Ck0_m1, attributes: {an_id1: 1, a_str1: "spam"})
+    ck0.create_assoc!(:m1, :Ck0_m1, attributes: {an_id1: 2, a_str1: "bar"})
+
+    Ck1.create_default!(an_id1: 42, a_str1: "foo")
+
+    assert_wa_from(Ck0, 3, :m1)
+  end
+
+  it "has_one with composite_key works" do
+    skip if Test::SelectedDBHelper == Test::MySQL
+
+    ck0 = Ck0.create_default!(an_id0: 1, a_str0: "foo")
+    ck0.create_assoc!(:o1, :Ck0_o1, attributes: {an_id1: 1, a_str1: "bar"})
+    ck0.create_assoc!(:o1, :Ck0_o1, attributes: {an_id1: 1, a_str1: "spam"})
+    ck0.create_assoc!(:o1, :Ck0_o1, attributes: {an_id1: 2, a_str1: "bar"})
+
+    assert_wa_from(Ck0, 1, :o1)
+  end
+
+  # I don't think has_and_belongs_to_many supports composite keys?
+  # it "has_and_belongs_to_many with composite_key works" do
+  #   ck0 = Ck0.create_default!(an_id0: 1, a_str0: "foo")
+  #   ck0.create_assoc!(:z1, :Ck0_z1, attributes: {an_id1: 1, a_str1: "bar"})
+  #   ck0.create_assoc!(:z1, :Ck0_z1, attributes: {an_id1: 1, a_str1: "spam"})
+  #   ck0.create_assoc!(:z1, :Ck0_z1, attributes: {an_id1: 2, a_str1: "bar"})
+
+  #   assert_wa_from(Ck0, 3, :z1)
+  # end
+
+
+  it "has_many through has_many with composite_key works" do
+    ck0 = Ck0.create_default!(an_id0: 1, a_str0: "foo")
+    ck0.create_assoc!(:m1, :Ck0_m1, attributes: {an_id1: 1, a_str1: "bar"})
+    ck1_2 = ck0.create_assoc!(:m1, :Ck0_m1, attributes: {an_id1: 1, a_str1: "spam"})
+    ck0.create_assoc!(:m1, :Ck0_m1, attributes: {an_id1: 2, a_str1: "bar"})
+
+    assert_wa_from(Ck0, 0, :m2m1)
+
+    ck1_2.create_assoc!(:m2, :Ck1_m2, :Ck0_m2m1, attributes: {an_id2: 130, a_str2: "hello"})
+    assert_wa_from(Ck0, 1, :m2m1)
+  end
+
+  it "raise on composite_key with never_alias_limit" do
+    skip if Test::SelectedDBHelper == Test::MySQL
+
+    sql = Ck0.where_assoc_exists(:o1) { from("hello") }.to_sql
+    assert !sql.include?("an_int0")
+
+    assert_raises(ActiveRecordWhereAssoc::NeverAliasLimitDoesntWorkWithCompositePrimaryKeysError) {
+      Ck0.where_assoc_exists(:o1, nil, never_alias_limit: true) { from("hello") }.to_sql
+    }
+  end
+end