Reject `sequence` definitions for Active Record primary keys #1586

seanpdoyle · 2023-08-10T15:44:20Z

It might be useful to define sequence(:id) attributes for models representing objects in external systems. It might be tempting to apply that same pattern to Active Record-backed models that are internal to the system under test.

However, a factory that generates its own sequence of id (or any other primary key attribute) risks collision or desynchronization with the underlying datastore (most likely an SQL database with its own primary key generation strategy and "next" value).

This commit introduces a check in the FactoryBot::Definition#compile method to ensure that descendants of ActiveRecord::Base cannot declare a sequence to generate their own primary key.

It might be useful to define `sequence(:id)` attributes for models representing objects in external systems. It might be tempting to apply that same pattern to Active Record-backed models that are internal to the system under test. However, a factory that generates its own sequence of `id` (or any other primary key attribute) risks collision or desynchronization with the underlying datastore (most likely an SQL database with its own primary key generation strategy and "next" value). This commit introduces a check in the `FactoryBot::Definition#compile` method to ensure that descendants of `ActiveRecord::Base` cannot declare a sequence to generate their own primary key.

seanpdoyle · 2023-08-10T15:47:10Z

lib/factory_bot/definition.rb

+
+    def ensure_attribute_not_generating_primary_key!(attribute, for_class:)
+      case attribute
+      when FactoryBot::Attribute::Sequence


Does it need to be a sequence? Should this raise the same error if there's a dynamic attribute like:

id { 1 }

While I'm skeptical of a legitimate (intentional!) use case for taking over control of a table's primary key, I think it might be helpful to provide an override for test suites that want to.

In those circumstances, would it be helpful to encourage the use of an escape hatch that combines a dynamic attribute and a call to generate?

sequence(:a_primary_key_column_name) # ... id { generate(:a_primary_key_column_name) }

louis-antonopoulos · 2023-08-10T15:50:42Z

lib/factory_bot/definition.rb

+      case attribute
+      when FactoryBot::Attribute::Sequence
+        if for_class < ActiveRecord::Base && for_class.primary_key == attribute.name.to_s
+          raise AttributeDefinitionError, "Attribute generates an Active Record primary key"


Will the error message the user sees provide enough information to determine which factory and attribute needs to change? For example, would they be able to tell that their SomeModel factory's some_id property is the issue at hand?

I think also that it could be good to mention that this only applies to sequences (depending on whether that will be the case), maybe with a warning that while you can silence the error by using e.g. id { 1 }, that's rarely a good idea.

And that extra information wouldn't necessarily need to be in the error message itself; it could be somewhere in the docs.

mike-burns · 2023-08-10T17:42:10Z

I love this idea.

In terms of execution, what do you think of adding it as a lint instead of completely forbidden?

That's a direction I want to go as well: use the linting mechanism to push people into use FB correctly.

(The linked method will likely need some refactoring.)

louis-antonopoulos · 2023-08-10T18:46:07Z

@mike-burns We spent hours troubleshooting bugs caused by this issue on a client project, and while I discovered FactoryBot.lint as an option as part of figuring out what was happening, I think of linting (in general) as something that you do to make your code more consistent in terms of readability, not something that is required for it to function correctly.

While I agree that we should push people to use FB correctly via lint, this particular instance seems worthy of being more restrictive and not just a "hey, you might want to fix this" kind of warning.

mike-burns · 2023-08-10T19:06:33Z

OK, I cannot think of a valid use of a sequence for an AR primary key. I'm fine with this.

That said, I think this needs to go into factory_bot_rails. AR is not a runtime dependency of factory_bot.

seanpdoyle · 2023-08-10T20:37:41Z

OK, I cannot think of a valid use of a sequence for an AR primary key. I'm fine with this.

That's great news!

That said, I think this needs to go into factory_bot_rails. AR is not a runtime dependency of factory_bot.

@mike-burns I had a similar thought, but wasn't sure where would be appropriate. Most of factory_bot_rails is focused on generators and dependency loading sequences. Would it be appropriate to re-open one of these classes in the gem's Railtie?

Alternatively, since Active Record is a development dependency, would it be appropriate to guard this code with a defined?(ActiveRecord::Base) guard?

mike-burns · 2023-08-10T22:25:30Z

What if we added a new notification instrument in the compile method, and then add a subscription for that in f_b_r?

In lib/factory_bot/factory_runner.rb we notify on factory_bot.run_factory, so maybe we could notify on factory_bot.compile_factory too.

The #compile method is a bit of a mess -- it mostly warms memoization caches, I suppose -- but maybe we could do something just before the end:

instrument_data = {
  attributes: declarations.attributes,
  traits: base_traits + additional_traits,
  klass: klass,
}

ActiveSupport::Notifications.instrument('factory_bot.compile_factory', instrument_data) do
  @compiled = true
end

ActiveSupport::Notifications.subscribe('factory_bot.compiled_factory') do |name, start, finish, id, payload|
  payload[:attributes].each do |attribute|
    ensure_attribute_not_generating_primary_key! attribute, for_class: payload[:klass]
  end
end

I haven't tried this or played with instrumentation in a very long time, so maybe this won't work at all.

What do you think?

Related to [comment on #1586][] --- Publish Active Support Notifications under the `factory_bot.compile_factory` key. They payload for that event is a `Hash` with keys: * `name:` - the name of the Factory * `class:` - the Ruby class the Factory constructs instances of * `attributes:` - a `FactoryBot::AttributesList` instance for the available attributes * `traits:` - an Array of `FactoryBot::Trait` instances for the available traits [comment on #1586]: #1586 (comment)

seanpdoyle · 2023-08-11T18:04:28Z

I haven't tried this or played with instrumentation in a very long time, so maybe this won't work at all.

What do you think?

Thank you for sketching out that pseudo-code. I've opened #1587 to explore what it'd take to publish that notification.

Related to [comment on #1586][] --- Publish Active Support Notifications under the `factory_bot.compile_factory` key. They payload for that event is a `Hash` with keys: * `name:` - the name of the Factory * `class:` - the Ruby class the Factory constructs instances of * `attributes:` - a `FactoryBot::AttributesList` instance for the available attributes * `traits:` - an Array of `FactoryBot::Trait` instances for the available traits [comment on #1586]: #1586 (comment)

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

seanpdoyle · 2023-08-12T19:45:10Z

I've opened thoughtbot/factory_bot_rails#419 as an alternative.

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

Related to [comment on #1586][] --- Publish Active Support Notifications under the `factory_bot.compile_factory` key. They payload for that event is a `Hash` with keys: * `name:` - the name of the Factory * `class:` - the Ruby class the Factory constructs instances of * `attributes:` - a `FactoryBot::AttributesList` instance for the available attributes * `traits:` - an Array of `FactoryBot::Trait` instances for the available traits [comment on #1586]: #1586 (comment)

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

seanpdoyle · 2023-09-05T16:39:55Z

Closing in favor of thoughtbot/factory_bot_rails#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

seanpdoyle requested a review from mike-burns August 10, 2023 15:44

seanpdoyle commented Aug 10, 2023

View reviewed changes

louis-antonopoulos reviewed Aug 10, 2023

View reviewed changes

seanpdoyle mentioned this pull request Aug 11, 2023

Publish factory_bot.compile_factory notification #1587

Merged

seanpdoyle mentioned this pull request Aug 12, 2023

Reject sequence definitions for Active Record primary keys thoughtbot/factory_bot_rails#419

Merged

seanpdoyle closed this Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject `sequence` definitions for Active Record primary keys #1586

Reject `sequence` definitions for Active Record primary keys #1586

seanpdoyle commented Aug 10, 2023

seanpdoyle Aug 10, 2023

seanpdoyle Aug 10, 2023 •

edited

louis-antonopoulos Aug 10, 2023

thoughtbot-summer Aug 10, 2023

mike-burns commented Aug 10, 2023 •

edited

louis-antonopoulos commented Aug 10, 2023

mike-burns commented Aug 10, 2023

seanpdoyle commented Aug 10, 2023

mike-burns commented Aug 10, 2023

seanpdoyle commented Aug 11, 2023

seanpdoyle commented Aug 12, 2023

seanpdoyle commented Sep 5, 2023

Reject sequence definitions for Active Record primary keys #1586

Reject sequence definitions for Active Record primary keys #1586

Conversation

seanpdoyle commented Aug 10, 2023

seanpdoyle Aug 10, 2023

Choose a reason for hiding this comment

seanpdoyle Aug 10, 2023 • edited

Choose a reason for hiding this comment

louis-antonopoulos Aug 10, 2023

Choose a reason for hiding this comment

thoughtbot-summer Aug 10, 2023

Choose a reason for hiding this comment

mike-burns commented Aug 10, 2023 • edited

louis-antonopoulos commented Aug 10, 2023

mike-burns commented Aug 10, 2023

seanpdoyle commented Aug 10, 2023

mike-burns commented Aug 10, 2023

seanpdoyle commented Aug 11, 2023

seanpdoyle commented Aug 12, 2023

seanpdoyle commented Sep 5, 2023

Reject `sequence` definitions for Active Record primary keys #1586

Reject `sequence` definitions for Active Record primary keys #1586

seanpdoyle Aug 10, 2023 •

edited

mike-burns commented Aug 10, 2023 •

edited