Merge pull request rails#50783 from Shopify/append-pk-to-returning-columns-if-returning-is-empty

Always request `primary_key` in `RETURNING` if no other columns requested

Author: eileencodes

activerecord/lib/active_record/model_schema.rb

@@ -431,8 +431,12 @@ def columns
       end
 
       def _returning_columns_for_insert # :nodoc:
-        @_returning_columns_for_insert ||= columns.filter_map do |c|
-          c.name if connection.return_value_after_insert?(c)
+        @_returning_columns_for_insert ||= begin
+          auto_populated_columns = columns.filter_map do |c|
+            c.name if connection.return_value_after_insert?(c)
+          end
+
+          auto_populated_columns.empty? ? Array(primary_key) : auto_populated_columns
         end
       end
 

activerecord/test/cases/persistence_test.rb

@@ -25,6 +25,7 @@
 require "models/chat_message"
 require "models/default"
 require "models/post_with_prefetched_pk"
+require "models/pk_autopopulated_by_a_trigger_record"
 
 class PersistenceTest < ActiveRecord::TestCase
   fixtures :topics, :companies, :developers, :accounts, :minimalistics, :authors, :author_addresses,
@@ -1574,6 +1575,13 @@ def test_it_is_possible_to_update_parts_of_the_query_constraints_config
 
     assert_equal("blue", ClothingItem.find_by(id: clothing_item.id).color)
   end
+
+  def test_model_with_no_auto_populated_fields_still_returns_primary_key_after_insert
+    record = PkAutopopulatedByATriggerRecord.create
+
+    assert_not_nil record.id
+    assert record.id > 0
+  end if current_adapter?(:PostgreSQLAdapter)
 end
 
 class QueryConstraintsTest < ActiveRecord::TestCase

activerecord/test/cases/view_test.rb

@@ -193,6 +193,12 @@ def test_insert_record
         assert_equal "Rails in Action", new_book.name
       end
 
+      def test_insert_record_populates_primary_key
+        book = PrintedBook.create! name: "Rails in Action", status: 0, format: "paperback"
+        assert_not_nil book.id
+        assert book.id > 0
+      end if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter) && supports_insert_returning?
+
       def test_update_record_to_fail_view_conditions
         book = PrintedBook.first
         book.format = "ebook"

activerecord/test/models/pk_autopopulated_by_a_trigger_record.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class PkAutopopulatedByATriggerRecord < ActiveRecord::Base
+  self.primary_key = :id
+end

activerecord/test/schema/schema.rb

@@ -1483,6 +1483,32 @@
   end
 end
 
+if ActiveRecord::TestCase.current_adapter?(:PostgreSQLAdapter)
+  ActiveRecord::Base.connection.create_table :pk_autopopulated_by_a_trigger_records, force: true, id: false do |t|
+    t.integer :id, null: false
+  end
+
+  ActiveRecord::Base.connection.execute(
+    <<-SQL
+      CREATE OR REPLACE FUNCTION populate_column()
+      RETURNS TRIGGER AS $$
+      DECLARE
+        max_value INTEGER;
+      BEGIN
+          SELECT MAX(id) INTO max_value FROM pk_autopopulated_by_a_trigger_records;
+          NEW.id = COALESCE(max_value, 0) + 1;
+          RETURN NEW;
+      END;
+      $$ LANGUAGE plpgsql;
+
+      CREATE TRIGGER before_insert_trigger
+      BEFORE INSERT ON pk_autopopulated_by_a_trigger_records
+      FOR EACH ROW
+      EXECUTE FUNCTION populate_column();
+    SQL
+  )
+end
+
 Course.connection.create_table :courses, force: true do |t|
   t.column :name, :string, null: false
   t.column :college_id, :integer, index: true