Merge pull request #48564 from gmcgibbon/use_id_pk_cpk_book

Use id in composite primary key of cpk book and fix related bugs

activerecord/lib/active_record/associations/collection_association.rb

@@ -61,22 +61,22 @@ def ids_writer(ids)
         primary_key = reflection.association_primary_key
         pk_type = klass.type_for_attribute(primary_key)
         ids = Array(ids).compact_blank
-        ids.map! { |i| pk_type.cast(i) }
+        ids.map! { |id| pk_type.cast(id) }
 
         records = if klass.composite_primary_key?
           query_records = ids.map { |values_set| klass.where(primary_key.zip(values_set).to_h) }.inject(&:or)
 
-          query_records.index_by do |r|
-            primary_key.map { |pk| r.public_send(pk) }
+          query_records.index_by do |record|
+            primary_key.map { |primary_key| record._read_attribute(primary_key) }
           end
         else
-          klass.where(primary_key => ids).index_by do |r|
-            r.public_send(primary_key)
+          klass.where(primary_key => ids).index_by do |record|
+            record._read_attribute(primary_key)
           end
         end.values_at(*ids).compact
 
         if records.size != ids.size
-          found_ids = records.map { |record| record.public_send(primary_key) }
+          found_ids = records.map { |record| record._read_attribute(primary_key) }
           not_found_ids = ids - found_ids
           klass.all.raise_record_not_found_exception!(ids, records.size, ids.size, primary_key, not_found_ids)
         else

activerecord/lib/active_record/attribute_methods/primary_key.rb

@@ -41,9 +41,9 @@ def id=(value)
       # Queries the primary key column's value.
       def id?
         if self.class.composite_primary_key?
-          @primary_key.all? { |col| query_attribute(col) }
+          @primary_key.all? { |col| _query_attribute(col) }
         else
-          query_attribute(@primary_key)
+          _query_attribute(@primary_key)
         end
       end
 

activerecord/lib/active_record/attribute_methods/query.rb

@@ -13,27 +13,38 @@ module Query
       def query_attribute(attr_name)
         value = self.public_send(attr_name)
 
-        case value
-        when true        then true
-        when false, nil  then false
-        else
-          if !type_for_attribute(attr_name) { false }
-            if Numeric === value || !value.match?(/[^0-9]/)
-              !value.to_i.zero?
+        query_cast_attribute(attr_name, value)
+      end
+
+      def _query_attribute(attr_name) # :nodoc:
+        value = self._read_attribute(attr_name.to_s)
+
+        query_cast_attribute(attr_name, value)
+      end
+
+      alias :attribute? :query_attribute
+      private :attribute?
+
+      private
+        def query_cast_attribute(attr_name, value)
+          case value
+          when true        then true
+          when false, nil  then false
+          else
+            if !type_for_attribute(attr_name) { false }
+              if Numeric === value || !value.match?(/[^0-9]/)
+                !value.to_i.zero?
+              else
+                return false if ActiveModel::Type::Boolean::FALSE_VALUES.include?(value)
+                !value.blank?
+              end
+            elsif value.respond_to?(:zero?)
+              !value.zero?
             else
-              return false if ActiveModel::Type::Boolean::FALSE_VALUES.include?(value)
               !value.blank?
             end
-          elsif value.respond_to?(:zero?)
-            !value.zero?
-          else
-            !value.blank?
           end
         end
-      end
-
-      alias :attribute? :query_attribute
-      private :attribute?
     end
   end
 end

activerecord/lib/active_record/relation/calculations.rb

@@ -328,7 +328,13 @@ def ids
       primary_key_array = Array(primary_key)
 
       if loaded?
-        result = records.pluck(*primary_key_array)
+        result = records.map do |record|
+          if primary_key_array.one?
+            record._read_attribute(primary_key_array.first)
+          else
+            primary_key_array.map { |column| record._read_attribute(column) }
+          end
+        end
         return @async ? Promise::Complete.new(result) : result
       end
 

activerecord/test/cases/associations/belongs_to_associations_test.rb

@@ -367,7 +367,7 @@ def test_building_the_belonging_object_for_composite_primary_key
 
   def test_belongs_to_with_inverse_association_for_composite_primary_key
     author = Cpk::Author.new(name: "John")
-    book = author.books.build(number: 1, title: "The Rails Way")
+    book = author.books.build(id: [nil, 1], title: "The Rails Way")
     order = Cpk::Order.new(book: book, status: "paid")
     author.save!
 

activerecord/test/cases/associations/eager_test.rb

@@ -1728,7 +1728,7 @@ def test_preloading_has_many_through_with_custom_scope
 
   test "preloading has_one with cpk" do
     order = Cpk::Order.create!(shop_id: 2)
-    book = Cpk::Book.create!(order: order, author_id: 1, number: 3)
+    book = Cpk::Book.create!(order: order, id: [1, 3])
     assert_equal book, Cpk::Order.eager_load(:book).find_by(id: order.id).book
   end
 

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -1633,7 +1633,7 @@ def test_tags_has_manu_posts_through_association_with_composite_query_constraint
 
   def test_loading_cpk_association_with_unpersisted_owner
     order = Cpk::Order.create!(shop_id: 1)
-    book = Cpk::BookWithOrderAgreements.new(number: 2, author_id: 1, order: order)
+    book = Cpk::BookWithOrderAgreements.new(id: [1, 2], order: order)
     order_agreement = Cpk::OrderAgreement.create!(order: order)
 
     assert_equal([order_agreement], book.order_agreements.to_a)

activerecord/test/cases/associations/has_one_associations_test.rb

@@ -146,8 +146,8 @@ def test_nullification_on_destroyed_association
   end
 
   def test_nullification_on_cpk_association
-    book = Cpk::Book.create!(author_id: 1, number: 2)
-    other_book = Cpk::Book.create!(author_id: 3, number: 4)
+    book = Cpk::Book.create!(id: [1, 2])
+    other_book = Cpk::Book.create!(id: [3, 4])
     order = Cpk::OrderWithNullifiedBook.create!(book: book)
 
     order.book = other_book
@@ -156,17 +156,6 @@ def test_nullification_on_cpk_association
     assert_nil book.shop_id
   end
 
-  def test_nullification_on_cpk_association_with_pk_column
-    chapter = Cpk::Chapter.create!(author_id: 1, number: 2)
-    other_chapter = Cpk::Chapter.create!(author_id: 1, number: 4)
-    book = Cpk::NullifiedBook.create!(chapter: chapter, number: 1, author_id: 1)
-
-    book.chapter = other_chapter
-
-    assert_nil chapter.book_number
-    assert_not_nil chapter.author_id
-  end
-
   def test_natural_assignment_to_nil_after_destroy
     firm = companies(:rails_core)
     old_account_id = firm.account.id

activerecord/test/cases/associations/has_one_through_associations_test.rb

@@ -449,7 +449,7 @@ def test_has_one_through_do_not_cache_association_reader_if_the_though_method_ha
 
   def test_loading_cpk_association_with_unpersisted_owner
     order = Cpk::Order.create!(shop_id: 1)
-    book = Cpk::BookWithOrderAgreements.new(number: 2, author_id: 1, order: order)
+    book = Cpk::BookWithOrderAgreements.new(id: [1, 2], order: order)
     order_agreement = Cpk::OrderAgreement.create!(order: order)
 
     assert_equal(order_agreement, book.order_agreement)

activerecord/test/cases/associations/inverse_associations_test.rb

@@ -605,7 +605,7 @@ def test_find_on_child_instances_with_ids_should_set_inverse_instances
 
   def test_inverse_should_be_set_on_composite_primary_key_child
     author = Cpk::Author.new(name: "John")
-    book = author.books.build(number: 1, title: "The Rails Way")
+    book = author.books.build(id: [nil, 1], title: "The Rails Way")
     Cpk::Order.new(book: book, status: "paid")
     author.save!
 

activerecord/test/cases/autosave_association_test.rb

@@ -489,7 +489,7 @@ def test_validation_does_not_validate_non_dirty_association_target
 
   test "composite primary key autosave" do
     assert_nothing_raised do
-      Cpk::Order.create!(id: [1, 2], book: Cpk::Book.new(title: "Book", author_id: 3, number: 4))
+      Cpk::Order.create!(id: [1, 2], book: Cpk::Book.new(title: "Book", id: [3, 4]))
     end
   end
 end
@@ -783,23 +783,23 @@ def test_assign_ids_with_belongs_to_cpk_model
   end
 
   def test_assign_ids_with_cpk_for_two_models
-    books = [cpk_books(:cpk_great_author_first_book).id, cpk_books(:cpk_great_author_second_book).id]
+    book_ids = [cpk_books(:cpk_great_author_first_book).id, cpk_books(:cpk_great_author_second_book).id]
     order = cpk_orders(:cpk_groceries_order_1)
 
     assert_empty order.books
 
-    order.book_ids = books
+    order.book_ids = book_ids
     order.save
     order.reload
 
-    assert_equal books, order.book_ids
+    assert_equal book_ids, order.book_ids
     assert_equal 2, order.books.length
     assert_includes order.books, cpk_books(:cpk_great_author_first_book)
     assert_includes order.books, cpk_books(:cpk_great_author_second_book)
   end
 
   def test_has_one_cpk_has_one_autosave_with_id
-    book = Cpk::Book.create!(author_id: 1, number: 3, shop_id: 2)
+    book = Cpk::Book.create!(id: [1, 3], shop_id: 2)
     order = Cpk::OrderWithPrimaryKeyAssociatedBook.create!(book: book, shop_id: 2)
 
     assert_equal(book.order.id, order.id)
@@ -1069,7 +1069,7 @@ def test_should_destroy_a_parent_association_as_part_of_the_save_transaction_if_
 
   # belongs_to for CPK
   def test_autosave_cpk_association_should_destroy_parent_association_when_marked_for_destruction
-    book = Cpk::Book.new(title: "Book", author_id: 1, number: 2)
+    book = Cpk::Book.new(title: "Book", id: [1, 2])
     Cpk::Order.create!(id: [3, 4], book: book)
 
     book.order.mark_for_destruction

activerecord/test/cases/base_test.rb

@@ -1011,8 +1011,7 @@ def test_dup_for_a_composite_primary_key_model
     new_book = book.dup
 
     assert_equal "The first book", new_book.title
-    assert_nil new_book.author_id
-    assert_nil new_book.number
+    assert_equal([nil, nil], new_book.id)
   end
 
   DeveloperSalary = Struct.new(:amount)

activerecord/test/cases/batches_test.rb

@@ -803,11 +803,11 @@ def test_find_in_batches_should_return_a_sized_enumerator
 
   test ".find_each with multiple column ordering and using composite primary key" do
     Cpk::Book.insert_all!([
-      { author_id: 1, number: 1 },
-      { author_id: 2, number: 1 },
-      { author_id: 2, number: 2 }
+      { author_id: 1, id: 1 },
+      { author_id: 2, id: 1 },
+      { author_id: 2, id: 2 }
     ])
-    books = Cpk::Book.order(author_id: :asc, number: :desc).to_a
+    books = Cpk::Book.order(author_id: :asc, id: :desc).to_a
 
     index = 0
     Cpk::Book.find_each(batch_size: 1, order: [:asc, :desc]) do |book|
@@ -819,35 +819,35 @@ def test_find_in_batches_should_return_a_sized_enumerator
 
   test ".in_batches should start from the start option when using composite primary key with multiple column ordering" do
     Cpk::Book.insert_all!([
-      { author_id: 1, number: 1 },
-      { author_id: 1, number: 2 },
-      { author_id: 1, number: 3 }
+      { author_id: 1, id: 1 },
+      { author_id: 1, id: 2 },
+      { author_id: 1, id: 3 }
     ])
-    second_book = Cpk::Book.order(author_id: :asc, number: :desc).second
+    second_book = Cpk::Book.order(author_id: :asc, id: :desc).second
     relation = Cpk::Book.in_batches(of: 1, start: second_book.id, order: [:asc, :desc]).first
     assert_equal second_book, relation.first
   end
 
   test ".in_batches should end at the finish option when using composite primary key with multiple column ordering" do
     Cpk::Book.insert_all!([
-      { author_id: 1, number: 1 },
-      { author_id: 1, number: 2 },
-      { author_id: 1, number: 3 }
+      { author_id: 1, id: 1 },
+      { author_id: 1, id: 2 },
+      { author_id: 1, id: 3 }
     ])
-    second_book = Cpk::Book.order(author_id: :asc, number: :desc).second
+    second_book = Cpk::Book.order(author_id: :asc, id: :desc).second
     relation = Cpk::Book.in_batches(of: 1, finish: second_book.id, order: [:asc, :desc]).to_a.last
     assert_equal second_book, relation.first
   end
 
   test ".in_batches with scope and multiple column ordering and using composite primary key" do
     Cpk::Book.insert_all!([
-      { author_id: 1, number: 1 },
-      { author_id: 1, number: 2 },
-      { author_id: 1, number: 3 }
+      { author_id: 1, id: 1 },
+      { author_id: 1, id: 2 },
+      { author_id: 1, id: 3 }
     ])
-    book1, book2 = Cpk::Book.order(author_id: :asc, number: :desc).first(2)
-    author_id, number = book1.id
-    relation = Cpk::Book.where("author_id >= ? AND number < ?", author_id, number).in_batches(of: 1, order: [:asc, :desc]).first
+    book1, book2 = Cpk::Book.order(author_id: :asc, id: :desc).first(2)
+    author_id, id = book1.id
+    relation = Cpk::Book.where("author_id >= ? AND id < ?", author_id, id).in_batches(of: 1, order: [:asc, :desc]).first
     assert_equal book2, relation.first
   end
 end

activerecord/test/cases/calculations_test.rb

@@ -394,7 +394,7 @@ def test_distinct_count_with_group_by_and_order_and_limit
 
   def test_count_for_a_composite_primary_key_model
     book = cpk_books(:cpk_great_author_first_book)
-    assert_equal(1, Cpk::Book.where(author_id: book.author_id, number: book.number).count)
+    assert_equal(1, Cpk::Book.where(author_id: book.author_id, id: book.id).count)
   end
 
   def test_group_by_count_for_a_composite_primary_key_model
@@ -957,13 +957,13 @@ def test_ids_for_a_composite_primary_key
   end
 
   def test_pluck_for_a_composite_primary_key
-    assert_equal Cpk::Book.all.pluck([:author_id, :number]).sort, Cpk::Book.all.ids.sort
+    assert_equal Cpk::Book.all.pluck([:author_id, :id]).sort, Cpk::Book.all.ids.sort
   end
 
   def test_ids_for_a_composite_primary_key_with_scope
     book = cpk_books(:cpk_great_author_first_book)
 
-    assert_equal [[book.author_id, book.number]], Cpk::Book.all.where(title: book.title).ids
+    assert_equal [book.id], Cpk::Book.all.where(title: book.title).ids
   end
 
   def test_ids_for_a_composite_primary_key_on_loaded_relation
@@ -972,7 +972,7 @@ def test_ids_for_a_composite_primary_key_on_loaded_relation
     relation.to_a
 
     assert_predicate relation, :loaded?
-    assert_equal [[book.author_id, book.number]], relation.ids
+    assert_equal [book.id], relation.ids
   end
 
   def test_ids_with_scope

activerecord/test/cases/core_test.rb

@@ -158,11 +158,11 @@ def test_find_by_cache_does_not_duplicate_entries
   def test_composite_pk_models_added_to_a_set
     library = Set.new
     # with primary key present
-    library << Cpk::Book.new(author_id: 1, number: 2)
+    library << Cpk::Book.new(id: [1, 2])
 
     # duplicate
-    library << Cpk::Book.new(author_id: 1, number: 3)
-    library << Cpk::Book.new(author_id: 1, number: 3)
+    library << Cpk::Book.new(id: [1, 3])
+    library << Cpk::Book.new(id: [1, 3])
 
     # without primary key being set
     library << Cpk::Book.new(title: "Book A")
@@ -172,19 +172,19 @@ def test_composite_pk_models_added_to_a_set
   end
 
   def test_composite_pk_models_equality
-    assert Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 2)
+    assert Cpk::Book.new(id: [1, 2]) == Cpk::Book.new(id: [1, 2])
 
-    assert_not Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 3)
+    assert_not Cpk::Book.new(id: [1, 2]) == Cpk::Book.new(id: [1, 3])
     assert_not Cpk::Book.new == Cpk::Book.new
     assert_not Cpk::Book.new(title: "Book A") == Cpk::Book.new(title: "Book B")
     assert_not Cpk::Book.new(author_id: 1) == Cpk::Book.new(author_id: 1)
     assert_not Cpk::Book.new(author_id: 1, title: "Same title") == Cpk::Book.new(author_id: 1, title: "Same title")
   end
 
   def test_composite_pk_models_hash
-    assert_equal Cpk::Book.new(author_id: 1, number: 2).hash, Cpk::Book.new(author_id: 1, number: 2).hash
+    assert_equal Cpk::Book.new(id: [1, 2]).hash, Cpk::Book.new(id: [1, 2]).hash
 
-    assert_not_equal Cpk::Book.new(author_id: 1, number: 2).hash, Cpk::Book.new(author_id: 1, number: 3).hash
+    assert_not_equal Cpk::Book.new(id: [1, 2]).hash, Cpk::Book.new(id: [1, 3]).hash
     assert_not_equal Cpk::Book.new.hash, Cpk::Book.new.hash
     assert_not_equal Cpk::Book.new(title: "Book A").hash, Cpk::Book.new(title: "Book B").hash
     assert_not_equal Cpk::Book.new(author_id: 1).hash, Cpk::Book.new(author_id: 1).hash