Skip to content
This repository

attributes_for should honor associations defined in the factory #359

Closed
nbt opened this Issue · 19 comments

12 participants

NBT Ventures Joshua Clayton artem-mindrov Rebecca Skinner gamov garrettlancaster Naomarik Bryce Senz Esteban Arango Medina Joshua Bussdieker Denis Jan Berdajs
NBT Ventures
nbt commented

I've looked over the issues surrounding attributes_for (#316, #274, #261) and I think there's still a problem.

As currently documented in Getting Started, attributes_for

Returns a hash of attributes that can be used to build a[n ...] instance

But as currently implemented, attributes_for elides any associations. For example, if a factory is defined as:

factory :membership do
  user
  club
end

... attributes_for returns an empty hash:

>> FactoryGirl.attributes_for(:membership)
=> {}

This is surprising. @joshuaclayton says that this is to keep attributes_for ORM agnostic. The suggested workaround is to use build and attributes:

>> FactoryGirl.build(:membership).attributes
=> {"id"=>nil, "user_id"=>3, "club_id"=>6, "created_at"=>nil, "updated_at"=>nil}

but this isn't useful for testing.

I'd like to propose that the contract of attributes_for is to return a hash of exactly the fields defined in the factory, i.e.:

>> FactoryGirl.attributes_for(:membership)
=> {:user_id => 1002, :club_id => 1004}

(I naively imagine that you can avoid hitting the DB by calling build_stubbed on the associated objects, but I suspect it's more complicated than that.)

Joshua Clayton
Owner

The attributes_for strategy isn't going to change to build associations: that's one of the benefits of attributes_for! It never touches any models, ever. This allows it to be blazing fast.

Regarding returning a hash of exactly the fields defined in the factory, that'd be fine - just make sure you declare your factory as:

factory :membership do
  user_id 1002
  club_id 1004
end

Deriving associations and setting _id attributes is not something that'd be intuitive; something we've discussed is some sort of way to define foreign keys.

Earlier today, I released FG 3.2.0, which allows registering custom strategies. I'd recommend looking into changing the attributes_for strategy to a different class that you've changed the behavior for.

class CustomAttributesForStrategy
  def initialize
    @strategy = FactoryGirl::Strategy::AttributesFor
  end

  delegate :result, to: :@strategy

  def association(runner)
    runner.run(FactoryGirl.strategy_by_name(:build)) # or whatever other strategy name you'd like
  end
end

FactoryGirl.register_strategy(:attributes_for, CustomAttributesForStrategy)
artem-mindrov

I was trying to find a way to properly construct params with nested_attributes for my controller specs. Since attributes_for wouldn't give me params entries for _attributes, I've put together a helper method:

  def params_for(factory_name)
    exclude_params = [ "id", "created_at", "updated_at" ]
    f = FactoryGirl.build(factory_name)

    params = f.attributes.except(*exclude_params).dup

    f.reflections.select { |k,v|
      v.macro == :has_many && !v.instance_of?(ActiveRecord::Reflection::ThroughReflection)
    }.each_key do |k|
      assoc_collection = f.send(k)

      unless assoc_collection.empty?
        params["#{k.to_s}_attributes"] = {}

        assoc_collection.each_with_index do |assoc_obj,idx|
          params["#{k.to_s}_attributes"][idx.to_s] = assoc_obj.attributes.except(*exclude_params << "#{f.class.name.underscore}_id")
        end
      end
    end

    params
  end

It's aimed to help only in one specific case (one-to-many associations), but it worked for me so far...

Rebecca Skinner
karpah commented

Having some easily documented way to implement this very common use case (I personally think it should be implemented as mentioned in the original issue) would be nice.

gamov

+1

garrettlancaster

+1 Any meaningful app is going to have nested attributes and likely deeply nested as well...

Joshua Clayton

I've been keeping ActiveRecord-specific code out of FactoryGirl since it can be used with all sorts of ORMs/ODMs. Is it possible that this nested attributes solution can work for ActiveRecord, Mongoid, MongoMapper, and Sequel (for each of the largest tiny releases per minor release which supports Rails 3)? If not, I think allowing developers to craft their own solutions is still best. That said, with custom build strategies as I mentioned above, it should be a bit easier.

Naomarik

This would definitely help me out a lot. My tests are rife with factories that build nested models and I've no clean way to test the create/update actions on my controllers without merging the attributes_for hashes manually. Passing in FactoryGirl.attributes_for() works brilliantly when the model being made is flat, but it'd save a lot of time if it would build a hash for models that use accepts_nested_attributes_for.

gamov

Funny, I just wrote this:

FactoryGirl.attributes_for(:item_variant).merge({:item_family_id => ItemFamily.first.id})

seconds before receiving the email update! :o) I wish I could just:

Factory.attributes_for(:item_variant, :create_associations => true)

or something.

Naomarik

I think in your specific case you could have just wrote

FactoryGirl.attributes_for(:item_variant, :item_family_id => ItemFamily.first.id)

since that's just a flat hash.

In order to get a hash similar to this:

"taskable"=>
{"name"=>"Test Poll",
    "question"=>"What's one?",
    "choices_attributes"=>
{"1355130889386"=>{"choice"=>"1"},
    "1355130890641"=>{"choice"=>"2"},   
    "1355130895744"=>{"choice"=>"3"}}}

Which I've configured to automatically make everything if I simply do FactoryGirl.create(:poll)

In my controller spec I currently have to do:

poll = FactoryGirl.attributes_for(:poll, :name => "Test Poll", :question => "What's one?")
choice1 = FactoryGirl.attributes_for(:poll_choice, :choice => 1)
choice2 = FactoryGirl.attributes_for(:poll_choice, :choice => 2)
taskable = {:taskable => poll.merge({:choice_attributes => {'1' => choice1, '2' => choice2}})}
gamov

Oh yes, good call. I tried but with :item_family => instead of the id.

Bryce Senz

I've been facing this problem for the last two days and my two cents is this: First, I agree with @joshuaclayton that this probably shouldn't be a build in feature. Between different types of relationships, protected attributes, accessors, etc., there's not a great way to write a master function that can handle all cases. And if you try (which I have), you need to pass so many parameters to handle each edge case that simply calling the function feels ugly.

I came across this post :http://stackoverflow.com/questions/10032760/how-to-define-an-array-hash-in-factory-girl, and it's worked wonders - for each model, I also define a factory with the Hash class, and building that Factory gives me the attributes that I need with no hassle.

Joshua Clayton

@brycesenz that reminds me of an article I posted on the Giant Robots blog - similar ideas with different uses. Thanks for posting this!

Deleted user
ghost commented

Maybe this will help some folks who are ending up here by googling this issue (like me). I heeded @joshuaclayton's advice and implemented my own solution, specific to Rspec and ActiveRecord 4.0.0.

module ControllerMacros
  def attributes_with_foreign_keys(*args)
    FactoryGirl.build(*args).attributes.delete_if do |k, v| 
      ["id", "type", "created_at", "updated_at"].member?(k)
    end
  end
end

RSpec.configure do |config|
  config.include ControllerMacros, :type => :controller
end

Then all I have to do is invoke attributes_with_foreign_keys in my specs and voila. DRY!

Joshua Clayton
Owner

@BigNerdRanchDan :thumbsup:, love it!

Esteban Arango Medina

@BigNerdRanchDan :ok_hand:, thanks!

Note: You forgot to close the do |k,v|.

Joshua Bussdieker

@BigNerdRanchDan just used it thanks!

Denis

If anybody interested I've upgraded @BigNerdRanchDan solution a little bit to support nested attributes: https://gist.github.com/sineed/9267389

Jan Berdajs

I stumbled upon this issue when I tried providing the associated record in the attributes_for call.
E.g. attributes_for(:post, blog: blog).
This fails silently. I understand the concerns as to why this does not work, but can we make this raise an exception or at least produce a warning? It's not exactly straightforward to figure out why this is not working, you simply get a hash back without the property you passed in. I think the solution is either to raise an exception or return everything that was passed in. Simply removing stuff that I passed in is really counter-intuitive.
Returning whatever you put in there should work fine too, since most ORMs will complain if you try to pass them attributes that don't exist. The point is to get an error somewhere.

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.