From 0040292890541736d0065c30ab2f798e8608787d Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Tue, 5 Sep 2023 12:45:30 -0400 Subject: [PATCH] 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]: https://github.com/thoughtbot/factory_bot/pull/1586 [thoughtbot/factory_bot#1587]: https://github.com/thoughtbot/factory_bot/pull/1587 [event.payload]: module-ActiveSupport::Notifications-label-Subscribers [on_load]: https://guides.rubyonrails.org/engines.html#avoid-loading-rails-frameworks --- Gemfile.lock | 13 +++- README.md | 16 +++++ factory_bot_rails.gemspec | 5 +- lib/factory_bot_rails/factory_validator.rb | 39 +++++++++++ .../active_record_validator.rb | 18 +++++ lib/factory_bot_rails/railtie.rb | 14 ++++ .../active_record_validator_spec.rb | 24 +++++++ spec/spec_helper.rb | 2 + spec/support/macros/define_constant.rb | 67 +++++++++++++++++++ 9 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 lib/factory_bot_rails/factory_validator.rb create mode 100644 lib/factory_bot_rails/factory_validator/active_record_validator.rb create mode 100644 spec/factory_bot_rails/factory_validator/active_record_validator_spec.rb create mode 100644 spec/support/macros/define_constant.rb diff --git a/Gemfile.lock b/Gemfile.lock index f7d69e42..7b4399ac 100644 --- a/Gemfile.lock +++ b/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 diff --git a/README.md b/README.md index 63f29c7a..7e2ba39a 100644 --- a/README.md +++ b/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). diff --git a/factory_bot_rails.gemspec b/factory_bot_rails.gemspec index b4c518f9..7ffc6c30 100644 --- a/factory_bot_rails.gemspec +++ b/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 diff --git a/lib/factory_bot_rails/factory_validator.rb b/lib/factory_bot_rails/factory_validator.rb new file mode 100644 index 00000000..b40ad647 --- /dev/null +++ b/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 diff --git a/lib/factory_bot_rails/factory_validator/active_record_validator.rb b/lib/factory_bot_rails/factory_validator/active_record_validator.rb new file mode 100644 index 00000000..6ad2cc12 --- /dev/null +++ b/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 diff --git a/lib/factory_bot_rails/railtie.rb b/lib/factory_bot_rails/railtie.rb index 8dca2f4f..5512f239 100644 --- a/lib/factory_bot_rails/railtie.rb +++ b/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 diff --git a/spec/factory_bot_rails/factory_validator/active_record_validator_spec.rb b/spec/factory_bot_rails/factory_validator/active_record_validator_spec.rb new file mode 100644 index 00000000..000476cc --- /dev/null +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5cbcde8a..1c77212d 100644 --- a/spec/spec_helper.rb +++ b/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 diff --git a/spec/support/macros/define_constant.rb b/spec/support/macros/define_constant.rb new file mode 100644 index 00000000..2621dd1e --- /dev/null +++ b/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