From 5a3247dd8c77e8ae98c4d1586726e382b143406c Mon Sep 17 00:00:00 2001
From: Daniel Colson <danieljamescolson@gmail.com>
Date: Fri, 12 Jul 2019 13:32:50 -0400
Subject: [PATCH] Avoid stubbing id for records without primary key

Fixes #1305

Before Rails 6 when the build_stubbed strategy assigned an id on a
record without a primary key column, the `id=` method would no-op, and
the record would end up looking like a new_record (i.e. `new_record?`
would return true because the id was nil).

This problem surfaced because of a [change in Rails 6][rails], which
caused `id=` to raise a potentially confusing
`ActiveModel::MissingAttributeError: can't write unknown attribute ''`
for records without a primary key column.

Since build_stubbed stubbed was calling `id=` for all instances,
regardless of whether they had primary keys, it was raising the above
error.

To avoid this error, we check whether the instance has a primary_key
defined before setting the id.

We also changed `persisted?` and `new_record?` to be less dependent on
the id. That way those methods will work as expected for records without
primary keys.

[rails]: https://github.com/rails/rails/commit/b6828fc91531ae0cc0a0f216705dd19112596301

Co-authored-by: Jesse Bailey <jbailey117@gmail.com>
---
 lib/factory_bot/strategy/stub.rb       | 13 ++++++--
 spec/acceptance/stub_spec.rb           | 41 ++++++++++++++++++++++++++
 spec/support/macros/define_constant.rb | 10 +++++--
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/lib/factory_bot/strategy/stub.rb b/lib/factory_bot/strategy/stub.rb
index d18b28f..bc5f309 100644
--- a/lib/factory_bot/strategy/stub.rb
+++ b/lib/factory_bot/strategy/stub.rb
@@ -44,15 +44,17 @@ module FactoryBot
       end
 
       def stub_database_interaction_on_result(result_instance)
-        result_instance.id ||= next_id
+        if has_settable_id?(result_instance)
+          result_instance.id ||= next_id
+        end
 
         result_instance.instance_eval do
           def persisted?
-            !new_record?
+            true
           end
 
           def new_record?
-            id.nil?
+            false
           end
 
           def destroyed?
@@ -68,6 +70,11 @@ module FactoryBot
         end
       end
 
+      def has_settable_id?(result_instance)
+        !result_instance.class.respond_to?(:primary_key) ||
+          result_instance.class.primary_key
+      end
+
       def clear_changes_information(result_instance)
         if result_instance.respond_to?(:clear_changes_information)
           result_instance.clear_changes_information
diff --git a/spec/acceptance/stub_spec.rb b/spec/acceptance/stub_spec.rb
index dac6bfd..0065860 100644
--- a/spec/acceptance/stub_spec.rb
+++ b/spec/acceptance/stub_spec.rb
@@ -58,3 +58,44 @@ describe "a stubbed instance overriding strategy" do
     expect(subject.user).not_to be_new_record
   end
 end
+
+describe "a stubbed instance with no primary key" do
+  it "builds a stubbed instance" do
+    using_model_without_pk do
+      FactoryBot.define do
+        factory :model_without_pk
+      end
+
+      model = FactoryBot.build_stubbed(:model_without_pk)
+      expect(model).to be_truthy
+    end
+  end
+
+  it "behaves like a persisted record" do
+    using_model_without_pk do
+      FactoryBot.define do
+        factory :model_without_pk
+      end
+
+      model = FactoryBot.build_stubbed(:model_without_pk)
+      expect(model).to be_persisted
+      expect(model).not_to be_new_record
+    end
+  end
+
+  def using_model_without_pk
+    define_class("ModelWithoutPk", ActiveRecord::Base)
+
+    connection = ActiveRecord::Base.connection
+    begin
+      clear_generated_table("model_without_pks")
+      connection.create_table("model_without_pks", id: false) do |t|
+        t.column :updated_at, :datetime
+      end
+
+      yield
+    ensure
+      clear_generated_table("model_without_pks")
+    end
+  end
+end
diff --git a/spec/support/macros/define_constant.rb b/spec/support/macros/define_constant.rb
index 07155e0..90f84c8 100644
--- a/spec/support/macros/define_constant.rb
+++ b/spec/support/macros/define_constant.rb
@@ -33,13 +33,17 @@ module DefineConstantMacros
 
   def clear_generated_tables
     created_tables.each do |table_name|
-      ActiveRecord::Base.
-        connection.
-        execute("DROP TABLE IF EXISTS #{table_name}")
+      clear_generated_table(table_name)
     end
     created_tables.clear
   end
 
+  def clear_generated_table(table_name)
+    ActiveRecord::Base.
+      connection.
+      execute("DROP TABLE IF EXISTS #{table_name}")
+  end
+
   private
 
   def created_tables