Merge pull request rails#50983 from fatkodima/fix-counter-caches-for-cpk

Fix counter caches to work for models with composite primary keys

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Fix counter caches when the foreign key is composite.
+
+    If the model holding the counter cache had a composite primary key,
+    inserting a dependent record would fail with an `ArgumentError`
+    `Expected corresponding value for...`
+
+    *fatkodima*
+
 *   Fix loading of schema cache for multiple databases.
 
     Before this change, if you have multiple databases configured in your

activerecord/lib/active_record/counter_cache.rb

@@ -65,7 +65,7 @@ def reset_counters(id, *counters, touch: nil)
           updates.merge!(touch_updates)
         end
 
-        unscoped.where(primary_key => object.id).update_all(updates) if updates.any?
+        unscoped.where(primary_key => [object.id]).update_all(updates) if updates.any?
 
         true
       end
@@ -112,6 +112,7 @@ def reset_counters(id, *counters, touch: nil)
       #   #    `updated_at` = '2016-10-13T09:59:23-05:00'
       #   #  WHERE id IN (10, 15)
       def update_counters(id, counters)
+        id = [id] if composite_primary_key? && id.is_a?(Array) && !id[0].is_a?(Array)
         unscoped.where!(primary_key => id).update_counters(counters)
       end
 
@@ -199,7 +200,7 @@ def destroy_row
         if affected_rows > 0
           each_counter_cached_associations do |association|
             foreign_key = association.reflection.foreign_key.to_sym
-            unless destroyed_by_association && destroyed_by_association.foreign_key.to_sym == foreign_key
+            unless destroyed_by_association && _foreign_keys_equal?(destroyed_by_association.foreign_key, foreign_key)
               association.decrement_counters
             end
           end
@@ -213,5 +214,9 @@ def each_counter_cached_associations
           yield association(name.to_sym) if reflection.belongs_to? && reflection.counter_cache_column
         end
       end
+
+      def _foreign_keys_equal?(fkey1, fkey2)
+        fkey1 == fkey2 || Array(fkey1).map(&:to_sym) == Array(fkey2).map(&:to_sym)
+      end
   end
 end

activerecord/test/cases/counter_cache_test.rb

@@ -17,10 +17,12 @@
 require "models/subscriber"
 require "models/subscription"
 require "models/book"
+require "models/cpk"
 require "active_support/core_ext/enumerable"
 
 class CounterCacheTest < ActiveRecord::TestCase
-  fixtures :topics, :categories, :categorizations, :cars, :dogs, :dog_lovers, :people, :friendships, :subscribers, :subscriptions, :books
+  fixtures :topics, :categories, :categorizations, :cars, :dogs, :dog_lovers, :people, :friendships, :subscribers, :subscriptions, :books,
+    :cpk_orders, :cpk_books
 
   class ::SpecialTopic < ::Topic
     has_many :special_replies, foreign_key: "parent_id"
@@ -42,11 +44,25 @@ class ::SpecialReply < ::Reply
   end
 
   test "increment counter by specific amount" do
-    assert_difference "@topic.reload.replies_count", +2 do
+    assert_difference -> { @topic.reload.replies_count }, +2 do
       Topic.increment_counter(:replies_count, @topic.id, by: 2)
     end
   end
 
+  test "increment counter for cpk model" do
+    order = Cpk::Order.first
+    assert_difference -> { order.reload.books_count } do
+      Cpk::Order.increment_counter(:books_count, order.id)
+    end
+  end
+
+  test "increment counter for multiple cpk model records" do
+    order1, order2 = Cpk::Order.first(2)
+    assert_difference [-> { order1.reload.books_count }, -> { order2.reload.books_count }] do
+      Cpk::Order.increment_counter(:books_count, [order1.id, order2.id])
+    end
+  end
+
   test "decrement counter" do
     assert_difference "@topic.reload.replies_count", -1 do
       Topic.decrement_counter(:replies_count, @topic.id)
@@ -59,6 +75,13 @@ class ::SpecialReply < ::Reply
     end
   end
 
+  test "decrement counter for cpk model" do
+    order = Cpk::Order.first
+    assert_difference -> { order.reload.books_count }, -1 do
+      Cpk::Order.decrement_counter(:books_count, order.id)
+    end
+  end
+
   test "reset counters" do
     # throw the count off by 1
     Topic.increment_counter(:replies_count, @topic.id)
@@ -148,6 +171,17 @@ class ::SpecialReply < ::Reply
     end
   end
 
+  test "reset counters for cpk model" do
+    order = Cpk::Order.first
+    # throw the count off by 1
+    Cpk::Order.increment_counter(:books_count, order.id)
+
+    # check that it gets reset
+    assert_difference -> { order.reload.books_count }, -1 do
+      Cpk::Order.reset_counters(order.id, :books)
+    end
+  end
+
   test "update counter with initial null value" do
     category = categories(:general)
     assert_equal 2, category.categorizations.count
@@ -177,6 +211,13 @@ class ::SpecialReply < ::Reply
     end
   end
 
+  test "update counter for decrement for cpk model" do
+    order = Cpk::Order.first
+    assert_difference -> { order.reload.books_count }, -3 do
+      Cpk::Order.update_counters(order.id, books_count: -3)
+    end
+  end
+
   test "update other counters on parent destroy" do
     david, joanna = dog_lovers(:david, :joanna)
     _ = joanna # squelch a warning

activerecord/test/models/cpk/book.rb

@@ -5,7 +5,7 @@ class Book < ActiveRecord::Base
     attr_accessor :fail_destroy
 
     self.table_name = :cpk_books
-    belongs_to :order, autosave: true, query_constraints: [:shop_id, :order_id]
+    belongs_to :order, autosave: true, query_constraints: [:shop_id, :order_id], counter_cache: true
     belongs_to :author, class_name: "Cpk::Author"
 
     has_many :chapters, query_constraints: [:author_id, :book_id]

activerecord/test/schema/schema.rb

@@ -285,6 +285,7 @@
   create_table :cpk_orders, force: true do |t|
     t.integer :shop_id
     t.string :status
+    t.integer :books_count, default: 0
   end
 
   create_table :cpk_order_tags, primary_key: [:order_id, :tag_id], force: true do |t|