Skip to content

Loading…

Building an object creates associations #64

Closed
jcf opened this Issue · 15 comments

7 participants

@jcf

When you define and build a factory any associations are created, rather than built. This is slow and feels kinda quirky. Instead the associated objects could be built as you would in Rails, only using memory as opposed to foreign keys and potentially lots of selects and inserts.

I've forked, branched and committed a change, with passing tests that hopefully will fix this in a way you agree is acceptable.

http://github.com/jcf/factory_girl/tree/build_association

Regards,

James

@jferris
thoughtbot, inc. member

I commented on issue #66 about this same problem. If you don't save the dependencies, you'll run into issues with foreign key validations.

@jcf
jcf commented

I think in most cases if you build an object you want associated objects to be built. In your gist you validate the presence of a foreign key but I think your subject is defined incorrectly.

# Create the associated user because we want a `user_id`
describe Post, "factory" do
  subject { Factory.build(:post, :user => Factory.create(:user)) }
  it { should be_valid }
end

# And other tests will work as expected with only a built user…
describe Post do
  it "should have a publisher" do
    post = Factory(:post, :published_at => 2.days.ago)
    post.published_by.should == 'John Smith'
  end
end

FactoryGirl to me is a tool that allows you to manage attributes on ActiveRecord-style objects. I don't think it's up to FactoryGirl to choose to create an associated object to protect me from my own code.

What do you think?

@jferris
thoughtbot, inc. member

One potential solution I came up with for this is to explicitly specify when you need a foreign key in a factory definition - that way, factory_girl won't have to assume that you need to save the dependency. Here's an example (although I'd probably not use this exact API):

  Factory.define :post do |factory|
   factory.association :user
    factory.foreign_key :category_id
  end
  Factory.build(:post)

That would build a user and post without saving them, but would save a category and assign its ID.

What do you think?

@jcf
jcf commented

Love the idea. No need to save unless you define a foreign_key constraint.

@ghost

Great idea. It would make my use of fg much more flexible.

@jkingdon

Creating, rather than building, objects from build strikes me as highly non-intuitive.

As for the gist, one possible solution is validates_presence_of :user rather than validates_presence_of :user_id (I see discussions of this issue, for example at http://www.ruby-forum.com/topic/184462 http://www.rornoob.com/a/nested-objects-and-validates_presence_of/ and http://groups.google.com/group/rubyonrails-talk/browse_thread/thread/8e77960b8d62479a ). The problem created by validates_presence_of :user_id does not appear to be specific to factory girl.

As for the suggestion of factory.foreign_key :category_id, I could see some solution along those lines. The "create the associated objects" functionality is more palatable if one has to ask for it explicitly.

@gamov

jferris, I see your point. However, I still feel the build/create strategy responsibility should still belong to the user, not to factory_girl. A note in the read me about it should be enough (OR a note that says the build strategy is overridden in case of assocations for current version :).
Regarding your example, I do this all the time, pages with nested models that are built in memory and only written to the DB later. They validate, even with their presence validation pointing to instead of .
By the way, validating the presence of doesn't work for me because errors are not reported correctly.

@NZKoz

Defaulting to create seems fine, but we actually needed build for a case where the validations / db constraints are a little circular. Basically a User has an associated Member record, and the members table has a NOT NULL on the user_id column. We ended up doing this horrible thing.

Something like user.association :factory=>:user, :method=>:build would solve our issue nicely without breaking the more common case where you want to create a record.

@joshuaclayton
thoughtbot, inc. member

Was someone interested in providing a patch which introduces the functionality similar to what @jferris suggested in his gist? It'd be pretty cool to get this out in the 2.1 release.

@jkingdon

I've provided a patch at jkingdon@3e19796

I hope people take this as a starting point for discussion; I haven't yet written any documentation nor do I have any strong opinions about whether the API I implemented is the best one.

@jkingdon jkingdon pushed a commit to jkingdon/factory_girl that referenced this issue
Jim Kingdon Now able to specify :method => :build in a factory's association.
See issue #64.
788ded1
@jkingdon jkingdon pushed a commit to jkingdon/factory_girl that referenced this issue
Jim Kingdon Now able to specify :method => :build in a factory's association.
See issue #64.
9037481
@jkingdon

Pull request is in #191.

@joshuaclayton
thoughtbot, inc. member

Closed with 9037481

@gamov

:method => :build is a nice option but I feel it's solving more of an edge case than the main problem here. What would really be great is: :method => :current_strategy, following the strategy invoked on the main factory (FactoryGirl.build or .create). The user should be aware of the potential validation problem.

Even though the Rails guides says (http://guides.rubyonrails.org/active_record_validations_callbacks.html#presence):
If you want to be sure that an association is present, you’ll need to test whether the foreign key used to map the association is present, and not the associated object itself.

However, if you want to create a nested model at the same time than the parent model, you can't do that, you must name the association in the presence validation otherwise you run in the exact same problem.

@urbanautomaton

@gamov, I was just reading the pull request for this feature, #191, and it appears that the :method => :build option actually works in the way you would like - it only applies if the parent factory is built, and does not affect the create or build_stubbed methods. See spec/acceptance/create_spec.rb for an example test. So really, it is indeed a :current_strategy option.

@gamov

@urbanautomaton. Thanks for the precision, I just tested it and works as you say.
I actually find the name ':method => :build' terrible and misleading. Its confusing what will happen if I 'create' an object which has association with ':method => :build'. It sounds like the association will always be built instead of created, even when we 'create' the object... :current_strategy is much better imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.