Reject sequence definitions for Active Record primary keys (#419)

Alternative to [thoughtbot/factory_bot#1586][]
Depends on [thoughtbot/factory_bot#1587][]

First, introduce the `FactoryBotRails::FactoryValidator` class to serve
as a generic `factory_bot.compile_factory` event observer.

Throughout the lifecycle of the `FactoryBotRails::Railtie`, afford
various initializers with opportunities to add purpose-built validators
to the instance's internal set.

When `factory_bot.compile_factory` events are published, iterate through
the list of validators and forward along the value of the
[event.payload][] to the validator's `#validate!` method.

Next, introduce the Active Record-specific
`FactoryBotRails::FactoryValidator::ActiveRecordValidator` class. Only
require the module whenever [Active Record's engine is loaded][on_load].

The `ActiveRecordValidator#validate!` method rejects attributes that
define primary key generation logic for `ActiveRecord::Base`
descendants.

In order to test this behavior, add a development dependency on
`sqlite3` and `activerecord`, along with some model and database table
generating helper methods.

[thoughtbot/factory_bot#1586]: thoughtbot/factory_bot#1586
[thoughtbot/factory_bot#1587]: thoughtbot/factory_bot#1587
[event.payload]: module-ActiveSupport::Notifications-label-Subscribers
[on_load]: https://guides.rubyonrails.org/engines.html#avoid-loading-rails-frameworks

Gemfile.lock

@@ -2,7 +2,7 @@ PATH
   remote: .
   specs:
     factory_bot_rails (6.2.0)
-      factory_bot (~> 6.2.0)
+      factory_bot (~> 6.3.0)
       railties (>= 5.0.0)
 
 GEM
@@ -21,6 +21,11 @@ GEM
       erubi (~> 1.4)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.1, >= 1.2.0)
+    activemodel (7.0.4.3)
+      activesupport (= 7.0.4.3)
+    activerecord (7.0.4.3)
+      activemodel (= 7.0.4.3)
+      activesupport (= 7.0.4.3)
     activesupport (7.0.4.3)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (>= 1.6, < 2)
@@ -69,7 +74,7 @@ GEM
     cucumber-tag-expressions (4.1.0)
     diff-lcs (1.5.0)
     erubi (1.12.0)
-    factory_bot (6.2.1)
+    factory_bot (6.3.0)
       activesupport (>= 5.0.0)
     ffi (1.15.5)
     ffi (1.15.5-java)
@@ -150,6 +155,8 @@ GEM
       rubocop (>= 1.7.0, < 2.0)
       rubocop-ast (>= 0.4.0)
     ruby-progressbar (1.13.0)
+    sqlite3 (1.6.2)
+      mini_portile2 (~> 2.8.0)
     standard (1.27.0)
       language_server-protocol (~> 3.17.0.2)
       rubocop (~> 1.50.2)
@@ -167,12 +174,14 @@ PLATFORMS
   ruby
 
 DEPENDENCIES
+  activerecord (>= 5.0.0)
   appraisal
   aruba
   cucumber
   factory_bot_rails!
   rake
   rspec-rails
+  sqlite3
   standard
 
 BUNDLED WITH

README.md

@@ -152,6 +152,22 @@ rails generate factory_bot:model NAME [field:type field:type] [options]
 
 [default factory template]: https://github.com/thoughtbot/factory_bot_rails/tree/master/lib/generators/factory_bot/model/templates/factories.erb
 
+### Active Record Configuration
+
+By default, FactoryBot will refuse to generate Active Record primary key
+columns. Without additional configuration, an Active Record model treats a
+column named `id` as its primary key.
+
+For example, defining an `id` attribute with `add_attribute(:id)`, `id { ... }`,
+or `sequence(:id)` will raise a `FactoryBot::AttributeDefinitionError`.
+
+You can disable this behavior by adding the following to `config/application.rb`
+or the appropriate environment configuration in `config/environments`:
+
+```ruby
+config.factory_bot.reject_primary_key_attributes = false
+```
+
 ## Contributing
 
 Please see [CONTRIBUTING.md](CONTRIBUTING.md).

factory_bot_rails.gemspec

@@ -14,6 +14,9 @@ Gem::Specification.new do |s|
   s.executables = []
   s.license = "MIT"
 
-  s.add_runtime_dependency("factory_bot", "~> 6.2.0")
+  s.add_runtime_dependency("factory_bot", "~> 6.3.0")
   s.add_runtime_dependency("railties", ">= 5.0.0")
+
+  s.add_development_dependency("sqlite3")
+  s.add_development_dependency("activerecord", ">= 5.0.0")
 end

lib/factory_bot_rails/factory_validator.rb

@@ -0,0 +1,39 @@
+module FactoryBotRails
+  class FactoryValidator
+    def initialize(validators = [])
+      @validators = Array(validators)
+    end
+
+    def add_validator(validator)
+      @validators << validator
+    end
+
+    def run
+      ActiveSupport::Notifications.subscribe("factory_bot.compile_factory", &validate_compiled_factory)
+    end
+
+    private
+
+    def validate_compiled_factory
+      if Rails.version >= "6.0"
+        rails_6_0_support
+      else
+        rails_5_2_support
+      end
+    end
+
+    def rails_6_0_support
+      proc do |event|
+        @validators.each { |validator| validator.validate!(event.payload) }
+      end
+    end
+
+    def rails_5_2_support
+      proc do |*notification_event_arguments|
+        event = ActiveSupport::Notifications::Event.new(*notification_event_arguments)
+
+        rails_6_0_support.call(event)
+      end
+    end
+  end
+end

lib/factory_bot_rails/factory_validator/active_record_validator.rb

@@ -0,0 +1,18 @@
+module FactoryBotRails
+  class FactoryValidator
+    class ActiveRecordValidator
+      def validate!(payload)
+        attributes, for_class = payload.values_at(:attributes, :class)
+        attributes.each do |attribute|
+          if for_class < ActiveRecord::Base && for_class.primary_key == attribute.name.to_s
+            raise FactoryBot::AttributeDefinitionError, <<~ERROR
+              Attribute generates #{for_class.primary_key.inspect} primary key for #{for_class.name}"
+
+              Do not define #{for_class.primary_key.inspect}. Instead, rely on the database to generate it.
+            ERROR
+          end
+        end
+      end
+    end
+  end
+end

lib/factory_bot_rails/railtie.rb

@@ -3,12 +3,15 @@
 require "factory_bot"
 require "factory_bot_rails/generator"
 require "factory_bot_rails/reloader"
+require "factory_bot_rails/factory_validator"
 require "rails"
 
 module FactoryBotRails
   class Railtie < Rails::Railtie
     config.factory_bot = ActiveSupport::OrderedOptions.new
     config.factory_bot.definition_file_paths = FactoryBot.definition_file_paths
+    config.factory_bot.reject_primary_key_attributes = true
+    config.factory_bot.validator = FactoryBotRails::FactoryValidator.new
 
     initializer "factory_bot.set_fixture_replacement" do
       Generator.new(config).run
@@ -18,9 +21,20 @@ class Railtie < Rails::Railtie
       FactoryBot.definition_file_paths = definition_file_paths
     end
 
+    ActiveSupport.on_load :active_record do
+      config = Rails.configuration.factory_bot
+
+      if config.reject_primary_key_attributes
+        require "factory_bot_rails/factory_validator/active_record_validator"
+
+        config.validator.add_validator FactoryValidator::ActiveRecordValidator.new
+      end
+    end
+
     config.after_initialize do |app|
       FactoryBot.find_definitions
       Reloader.new(app).run
+      app.config.factory_bot.validator.run
     end
 
     private

spec/factory_bot_rails/factory_validator/active_record_validator_spec.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+describe FactoryBotRails::FactoryValidator do
+  before { FactoryBot.reload }
+
+  describe "ActiveRecordValidator" do
+    context "when defined with a class that descends from ActiveRecord::Base" do
+      it "raises an error for a sequence generating its primary key" do
+        define_model "Article", an_id: :integer do
+          self.primary_key = :an_id
+        end
+
+        FactoryBot.define do
+          factory :article do
+            sequence(:an_id)
+          end
+        end
+
+        expect { FactoryBot.create(:article) }
+          .to raise_error(FactoryBot::AttributeDefinitionError)
+      end
+    end
+  end
+end

spec/spec_helper.rb

@@ -5,6 +5,8 @@
 require "factory_bot_rails"
 require "fake_app"
 
+Dir["spec/support/**/*.rb"].each { |f| require File.expand_path(f) }
+
 RSpec.configure do |config|
   config.run_all_when_everything_filtered = true
   config.filter_run :focus

spec/support/macros/define_constant.rb

@@ -0,0 +1,67 @@
+require "active_record"
+
+module DefineConstantMacros
+  def define_class(path, base = Object, &block)
+    const = stub_const(path, Class.new(base))
+    const.class_eval(&block) if block
+    const
+  end
+
+  def define_model(name, columns = {}, &block)
+    model = define_class(name, ActiveRecord::Base, &block)
+    create_table(model.table_name) do |table|
+      columns.each do |column_name, type|
+        table.column column_name, type
+      end
+    end
+    model
+  end
+
+  def create_table(table_name, &block)
+    connection = ActiveRecord::Base.connection
+
+    begin
+      connection.execute("DROP TABLE IF EXISTS #{table_name}")
+      connection.create_table(table_name, &block)
+      created_tables << table_name
+      connection
+    rescue Exception => e # rubocop:disable Lint/RescueException
+      connection.execute("DROP TABLE IF EXISTS #{table_name}")
+      raise e
+    end
+  end
+
+  def clear_generated_tables
+    created_tables.each do |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
+    @created_tables ||= []
+  end
+end
+
+RSpec.configure do |config|
+  config.include DefineConstantMacros
+
+  config.before(:all) do
+    ActiveRecord::Base.establish_connection(
+      adapter: "sqlite3",
+      database: ":memory:"
+    )
+  end
+
+  config.after do
+    clear_generated_tables
+  end
+end