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.

Sign up for GitHub

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

Jump to bottom

Factory.build + association should not hit the db? #66

Closed
shwoodard opened this issue Jul 3, 2010 · 4 comments
Closed

Factory.build + association should not hit the db? #66

shwoodard opened this issue Jul 3, 2010 · 4 comments
Labels
invalid

Comments

@shwoodard
Copy link

Ordinarily when using Factory.build one does not expect FG will hit the db? Should FG possibly create and populate the associated records when Factory.build is used on a factory (as opposed to Factor.create) that has associations, but not create the records? Would this be possible?
@mcmire
Copy link

mcmire commented Jul 13, 2010

I've overridden this behavior in my app to do exactly that, but a few times I've run into problems where when I finally save the record and Rails goes to auto-save the associations, id's are lost. So it is possible, but it's buggy, at least from my perspective.

@mcmire
Copy link

mcmire commented Jul 13, 2010

Also see #64

@jferris
Copy link
Member

jferris commented Sep 7, 2010

If Factory.build doesn't save its dependencies, there won't be any foreign keys set on the result. This means that if you use validate_presence_of with a foreign key, you'll build an invalid instance. Take a look at this example: http://gist.github.com/569231

I'm open to ideas of how to make this work, but currently I'd rather err on the side of building a valid instance and saving some extra records.

@jferris
Copy link
Member

jferris commented Nov 4, 2010

I'm going to close this issue, since it's a duplicate of #64.

@bradx3 bradx3 mentioned this issue Feb 22, 2015
Closed
composerinteralia added a commit that referenced this issue Dec 7, 2018
@composerinteralia
WIP: Always use parent strategy
297d3f5 
Closes #1236

See #961, #749, #66, #64
@composerinteralia composerinteralia mentioned this issue Dec 7, 2018
Closed
composerinteralia added a commit that referenced this issue Dec 17, 2018
@composerinteralia
Always use parent strategy
e720c28 
Closes #1236

Background:

In issues #64 and #66 (from 2010) people expressed surprise
that using the `build` strategy would still `create` any associations.
I remember being similarly surprised by that behavior when I first
started using factory_bot.

The main reason for this behavior is to ensure the built instance will
be valid if there were any foreign key validations
(like `validates_presence_of :user_id`). If we don't save the
associations, there won't be any ids present.

PR #191 (from 2011) offers a workaround for people who don't want
records to be saved automatically. Passing `strategy: :build`
(originally `method: :build`, but later renamed) when declaring the
association will prevent it from being saved when using `build`.

But #191 isn't really a complete solution (as discussed on the PR
after it was merged). `strategy: :build` may do
the right thing when building objects with the `build` strategy, but it
can cause new problems when using the `create` strategy.

A better solution would be something like:
`strategy: <whatever strategy I was already using>`.
PRs #749 and #961 (merged in 2016) introduce something like that,
with the `use_parent_strategy` configuration option.
With this turned on `build` end up being generally [a little faster][]
than `build_stubbed`, since it no longer needs to hit the database for
each association.

[a little faster]: https://gist.github.com/composerinteralia/d4796df9140f431e36f88dfb6fe9733a

I have set `use_parent_strategy` on several projects now. I also added
it to suspenders in thoughtbot/suspenders#952.
On newer projects I have not run into any problems. On existing
projects I have seen the occasional test failure, which are easy enough
to fix by changing `build` to `create`.

Unfortunately I don't think `use_parent_strategy` is widely known,
since it wasn't documented until #1234 (about a month ago).
I also learned in #1234 that the `use_parent_strategy` setting gets
wiped out with `FactoryBot.reload`, which can be problematic when using
factory_bot_rails.

To summarize, we have been exploring having a `build` strategy that uses
`build` all the way down since 2010, but there still isn't a totally
reliable way to get that.

This PR:

* Default to using the parent strategy for factory_bot 5, as suggested
in [this comment][]
* Get rid of the `use_parent_strategy` option, since it doesn't play
well with `FactoryBot.reload`

[this comment]: #749 (comment)

I think this PR will also make the `strategy` option unnecessary, but I
would want to explore deprecating that in a separate PR.

If dealing with foreign key validations is important, maybe we can
rethink that from the ground up.
composerinteralia added a commit that referenced this issue Jan 5, 2019
@composerinteralia
Default use_parent_strategy to true
d0208ed 
Background
---

In issues #64 and #66 (from 2010) people expressed surprise
that using the `build` strategy would still `create` any associations.
I remember being similarly surprised by that behavior when I first
started using factory_bot.

The main reason for this behavior is to ensure the built instance will
be valid if there were any foreign key validations
(like `validates_presence_of :user_id`). If we don't save the
associations, there won't be any ids present.

PR #191 (from 2011) offers a workaround for people who don't want
records to be saved automatically. Passing `strategy: :build`
(originally `method: :build`, but later renamed) when declaring the
association will prevent it from being saved when using `build`.

But #191 isn't really a complete solution (as discussed on the PR
after it was merged). `strategy: :build` may do
the right thing when building objects with the `build` strategy, but it
can cause new problems when using the `create` strategy.

A better solution would be something like:
`strategy: <whatever strategy I was already using>`.
PRs #749 and #961 (merged in 2016) introduce something like that,
with the `use_parent_strategy` configuration option.
With this turned on `build` end up being generally [a little faster][]
than `build_stubbed`, since it no longer needs to hit the database for
each association.

[a little faster]: https://gist.github.com/composerinteralia/d4796df9140f431e36f88dfb6fe9733a

I have set `use_parent_strategy` on several projects now. I also added
it to suspenders in thoughtbot/suspenders#952.
On newer projects I have not run into any problems. On existing
projects I have seen the occasional test failure, which are easy enough
to fix by changing `build` to `create`.

Unfortunately I don't think `use_parent_strategy` is widely known,
since it wasn't documented until #1234 (about a month ago).
I also learned in #1234 that the `use_parent_strategy` setting gets
wiped out with `FactoryBot.reload`, which can be problematic when using
factory_bot_rails.

To summarize, we have been exploring having a `build` strategy that uses
`build` all the way down since 2010, but there still isn't a totally
reliable way to get that.

This PR
---

* Default to using the parent strategy for factory_bot 5, as suggested
in #749.
* In the original PR for this (#1240) I had removed
`use_parent_strategy`, but to make the transition smoother I ended up
fixing the reload problem in #1244

Once factory\_bot 5 is released and the dust has settled, it may make
sense to deprecate `use_parent_strategy`, and maybe also the strategy
option for associations.
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
invalid
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcmire @jferris @shwoodard