Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating from 6.2.0 -> 6.4.2 is a breaking change for us: "Do not define "id". Instead, rely on the database to generate it." #1602

Open
MarcusRiemer opened this issue Nov 24, 2023 · 7 comments
Labels

Comments

@MarcusRiemer
Copy link

Description

We are modeling tree structures in with UUID primary keys. We specifically chose UUID as type because among other benefits we can generate those IDs in the application and send whole trees to the database in a single transaction without having to go back and forth for each new node we are inserting.

Reproduction Steps

We have this factory:

factory :root_node, class: 'Tree' do
  id { SecureRandom.uuid }
  root_id { id }
end

Expected behavior

The instance should be constructed with identical values for id and root_id (as was the case with FactoryBot 6.2.0). Alternatively we would be fine with overriding this behavior but browsing the Readme or the web for "Do not define "id". Instead, rely on the database to generate it." didn't yield any helpful results.

Actual behavior

Creating instances worked just fine with FactoryBot 6.2.0, but since at latest 6.4.2 this results in the following error:

     FactoryBot::AttributeDefinitionError:
       Attribute generates "id" primary key for Tree"
       Do not define "id". Instead, rely on the database to generate it.

System configuration

factory_bot version: 6.4.2
rails version: 7.0.8
ruby version: 3.2.2

@pyromaniac
Copy link

https://github.com/thoughtbot/factory_bot_rails/blob/54c72541ac8d9afa2c94274d80597c24112ce054/README.md?plain=1#L168C1-L168C57 should help.

@StanBright
Copy link

Within the context @pyromaniac, I think we can close this "bug". It's more like a configurable feature.

p.s. this feature was a surprise to me, too.

@BiggerNoise
Copy link

I'd suggest a more prominent announcement, e.g., at the top of the README, to close this one out.
Call it an inadvertent breaking change and point to the docs for how to get the old behavior.

@MarcusRiemer
Copy link
Author

I'm happy that there is a feasible way to revert back to the old behavior and this has addressed my personal needs. I do however agree with @BiggerNoise : a more prominent mention would be very helpful. What about either:

  • Mentioning the config value in the error message?
  • Mention the error message in the Readme?

@BiggerNoise
Copy link

Agree that being able to revert to the previous behavior has met my needs as well.

I think @MarcusRiemer documentation suggestions would be very helpful, would a P/R be helpful?

@krisleech
Copy link

One issue with turning off via config.factory_bot.reject_primary_key_attributes = false is that for Rails engines this needs to go in the dummy app, not in say spec_helper.

@jagthedrummer
Copy link

Seems like the kind of breaking change that should warrant a major version number increment.

jagthedrummer added a commit to bullet-train-co/bullet_train that referenced this issue Nov 29, 2023
It seems that `factory_bot_rails` unexpectedly introduced a breaking
change. thoughtbot/factory_bot#1602

For now I'm just going to avoid that version while we decide what to do
about it: bullet-train-co/bullet_train-core#707
jagthedrummer added a commit to bullet-train-co/bullet_train that referenced this issue Nov 29, 2023
It seems that `factory_bot_rails` unexpectedly introduced a breaking
change. thoughtbot/factory_bot#1602

For now I'm just going to avoid that version while we decide what to do
about it: bullet-train-co/bullet_train-core#707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants