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

attributes_for should honor associations defined in the factory #359

Closed
nbt opened this issue Apr 24, 2012 · 38 comments
Closed

attributes_for should honor associations defined in the factory #359

nbt opened this issue Apr 24, 2012 · 38 comments

Comments

@nbt
Copy link

nbt commented Apr 24, 2012

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.)

@joshuaclayton
Copy link
Contributor

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
Copy link

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...

@sevenseacat
Copy link

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
Copy link

gamov commented Nov 8, 2012

+1

@garrettlancaster
Copy link

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

@joshuaclayton
Copy link
Contributor

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
Copy link

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
Copy link

gamov commented Dec 10, 2012

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
Copy link

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
Copy link

gamov commented Dec 10, 2012

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

@brycesenz
Copy link

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.

@joshuaclayton
Copy link
Contributor

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

@ghost
Copy link

ghost commented Jul 23, 2013

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!

@joshuaclayton
Copy link
Contributor

@BigNerdRanchDan 👍, love it!

@esbanarango
Copy link

@BigNerdRanchDan 👌, thanks!

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

@ghost
Copy link

ghost commented Sep 17, 2013

@esbanarango 👍

@jbussdieker
Copy link

@BigNerdRanchDan just used it thanks!

@sineed
Copy link

sineed commented Feb 28, 2014

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

@mrbrdo
Copy link

mrbrdo commented Mar 17, 2014

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.

@joshuapinter
Copy link

As follow-up to ghost (thank you by the way!), here's a couple tips to help you use this new method:

  1. Put the module ControllerMacros code in a new (or existing) file at ./spec/support/controller_macros.rb.
  2. Don't call FactoryGirl.attributes_with_foreign_keys, just call attributes_with_foreign_keys directly.

Hope that helps.

@geo1004
Copy link

geo1004 commented Aug 4, 2015

Thank you, it works perfectly. Also, don't forget to require the module in rails_helper.rb (require 'support/controller_macros')

@craigweston
Copy link

One thing I found with @Ghosts (thank you btw) approach is that FactoryGirl.build(*args).attributes will convert ActiveRecord enums into integers in the outputted attributes hash.

"content_type"=>1

If using this for POST params in testing, it will break with something similar to:

     Failure/Error: post :create, comment: attributes_with_foreign_keys(:comment)
     ArgumentError:
       '1' is not a valid content_type

(given an enum called content_type on the comment model)

This enum conversion difference between the two functions was brought up by @dankohn in FactoryGirl.attributes converting enum to integer - Issue #680

In my company model: enum auth_type: { default: 1, ny: 2 }
In my company factory: auth_type "ny"
In rails console:

[1] pry(main)> FactoryGirl.attributes_for(:company)[:auth_type]
:ny
[2] pry(main)> FactoryGirl.build(:company).attributes['auth_type']
2

I've added the following to @ghost's code to set the enum attributes back ("content_type"=>"publication"), allowing params to continue to work:

inst.class.defined_enums.each { |k, v| attrs[k] = inst.send(k) }

See below...

def attributes_with_foreign_keys(*args)
  inst = FactoryGirl.build(*args)
  attrs = inst.attributes
  attrs.delete_if do |k, v| 
    ["id", "type", "created_at", "updated_at"].member?(k)
  end
  inst.class.defined_enums.each { |k, v| attrs[k] = inst.send(k) }
  return attrs
end

Hopefully this helps someone who, like me, is using this method and also has enums defined in their model.

@sadaf2605
Copy link

Some of you who has come across via google may find this post/code-snippet useful:

https://www.sadafnoor.com/blog/little-hack-to-get-factorygirl-return-more-getpost-friendly-nested-attributes/

@sadaf2605
Copy link

Or you can use gem 'nested_attr': https://github.com/sadaf2605/nested_attr

@shrugs
Copy link

shrugs commented Mar 25, 2016

I was looking to create nested attributes (for example, because my model validates the presence of a has_one association). I ended up writing this code snippet:

  def nested_attributes_for(*args)
    attrs = FactoryGirl.attributes_for(*args)
    klass_name = args.first.to_s.camelize.constantize
    klass_name.reflect_on_all_associations(:has_one).each do |r|
        attrs["#{r.name}_attributes".to_sym] = FactoryGirl.attributes_for(r.name)
    end

    attrs
  end

So, given a factory definition like

  factory :location do
    name 'Location Name'
    address
  end

nested_attributes_for returns

{:name=>"Location Name",
 :address_attributes =>
  {:country=>"USA",
   :state=>"New York",
   :city=>"New York",
   :zip_code=> 11001,
   :address_line_one=>"Union Square",
   :address_line_two=>""}}

@Ghosts
Copy link

Ghosts commented Mar 26, 2016

Every time someone adds an s to @ghost I feel like I'm actually contributing to an important project :)

morynicz added a commit to morynicz/Ippon that referenced this issue Sep 16, 2016
FG does not put associations to hash generated using
attributes_for.

Provide a helper with attributes_with_foreign_keys copied from
thoughtbot/factory_bot#359 (comment)
@nwshane
Copy link

nwshane commented Dec 13, 2016

After stealing @craigweston and @ghost's code snippet above, I was briefly tripped up by the fact that the attribute keys in the hash returned by attributes_with_foreign_keys are strings and not symbols:

person_attr = attributes_with_foreign_keys(:person, name: 'Bob')
person_attr[:name] // -> nil
person_attr['name'] // -> 'Bob'

This is different from the FactoryGirl.attributes_for method, which returns a hash with the attribute keys as symbols:

person_attr = attributes_for(:person, name: 'Bob')
person_attr[:name] // -> 'Bob'

I didn't want to deal with that inconsistency, so I tweaked the attributes_with_foreign_keys to transform all hash keys to symbols:

  def attributes_with_foreign_keys(*args)
    inst = FactoryGirl.build(*args)
    attrs = inst.attributes
    attrs.delete_if do |k, v|
      ["id", "type", "created_at", "updated_at"].member?(k)
    end
    inst.class.defined_enums.each { |k, v| attrs[k] = inst.send(k) }
    return attrs.deep_transform_keys(&:to_sym)
  end

@gosukiwi
Copy link

gosukiwi commented May 9, 2017

@nwshane why not just do .with_indifferent_access?

@atz
Copy link

atz commented May 12, 2017

@gosukiwi because the poster wants it to be the same as attributes_for.

@brunofacca
Copy link

brunofacca commented Jul 3, 2017

Another possible solution is:

FactoryGirl.build(:foo).attributes.except('id', 'created_at', 'updated_at').symbolize_keys

Limitations:

  • It does not generate attributes for HMT and HABTM associations (as these associations are stored in a join table, not an actual attribute).
  • Association strategy in the factory must be create, as in association :user, strategy: :create. This strategy can make your factory very slow if you don't use it wisely.

@mbedarff
Copy link

As @joshuaclayton proposed earlier I defined an own strategy :attributes_with_foreign_keys that includes the foreign keys in the the hash:

module FactoryBot
  module Strategy
    class AttributesWithForeignKeys
      def initialize
        @strategy = FactoryBot.strategy_by_name(:attributes_for).new
      end

      def result(evaluation)
        attributes = @strategy.result(evaluation)

        # iterate over associations defined in factory and find matching belongs_to association in model
        # => don't iterate over belongs_to associations defined in model since there maybe more associations defined in the model than in the factory
        belongs_to_associations = build_class(evaluation).reflect_on_all_associations(:belongs_to)
        get_association_names(evaluation).each do |association_name|
          belongs_to_association = belongs_to_associations.find {|a| a.name == association_name}
          # set id of association target in attributes hash if belongs_to association was found in the model and the attribute was not overridden in the factory
          unless belongs_to_association.blank? || get_overrides(evaluation).include?(association_name)
            target = get_evaluator(evaluation).send(association_name)
            attributes[belongs_to_association.foreign_key.to_sym] = target&.id
          end
        end

        attributes
      end

      def association(runner)
        # restore usage of default strategy defined in factory
        runner.run
      end

      private

        def build_class(evaluation)
          evaluation.instance_variable_get(:@attribute_assigner).instance_variable_get(:@build_class)
        end

        def get_association_names(evaluation)
          evaluation.instance_variable_get(:@attribute_assigner).instance_variable_get(:@attribute_list).associations.names
        end

        def get_evaluator(evaluation)
          @evaluator ||= evaluation.instance_variable_get(:@attribute_assigner).instance_variable_get(:@evaluator)
        end

        def get_overrides(evaluation = nil)
          @overrides ||= get_evaluator(evaluation).instance_variable_get(:@overrides).symbolize_keys
        end
    end
  end

  register_strategy(:attributes_with_foreign_keys, Strategy::AttributesWithForeignKeys)
end

In order to work properly the association in the factory needs to use the :create strategy. Otherwise there isn't any id to add to the attributes hash.

@JorgeGarciaxyz
Copy link

@joshuapinter The @ghost solution can be included in any part of the documentation ?

@Jeehut
Copy link

Jeehut commented Jun 8, 2018

Solution of @ghost is working fine for me, I changed a few things though:

  1. Added a _for at the end to be consistent with the naming of attributes_for
  2. Updated to the new name of FactoryBot (instead of FactoryLint)
  3. Inclusion through FactoryBot::Syntax::Methods instead of an own module limited to controllers
  4. Replaced double quotes with single quotes
module FactoryBot::Syntax::Methods
  def attributes_with_foreign_keys_for(*args)
    FactoryBot.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 FactoryBot::Syntax::Methods
end

@danielricecodes
Copy link

Nice @Dschee. FYI the @ghost is me - BigNerdRanchDan was my old github username :-)

@jamesst20
Copy link

jamesst20 commented Feb 19, 2019

Something else that could possibly work with belongs_to, has_many and has_one could be something like this. However, keep in mind that in case of belongs_to relationship, it creates a new record, except if the belongs_to match the parent has_many.

spec/support/factorybot_macros.rb

module FactoryBot
  module Syntax
    module Methods
      def nested_attributes_for(*args)
        skip = args[1].delete :_skip if args.length > 1

        attrs = FactoryBot.attributes_for(*args)
        klass_name = args.first.to_s.camelize.constantize

        # Process has_one
        klass_name.reflect_on_all_associations(:has_one).each do |r|
          attrs["#{r.name}_attributes"] = nested_attributes_for(r.class_name.underscore)
        end

        # Process belongs_to
        klass_name.reflect_on_all_associations(:belongs_to).each do |r|
          next if r.name == skip || attrs["#{r.name}_id"].present?

          attrs["#{r.name}_id"] = FactoryBot.create(r.class_name.underscore).id
        end

        # Process has_many
        klass_name.reflect_on_all_associations(:has_many).each do |r|
          next if r.class_name == "PaperTrail::Version"

          attrs["#{r.name}_attributes"] = [nested_attributes_for(r.class_name.underscore, _skip: args[0])]
        end

        attrs
      end
    end
  end
end

Models

class Expert < ApplicationRecord
  has_paper_trail

  belongs_to :region

  has_many :expert_partners, inverse_of: :expert, dependent: :destroy
  has_many :expert_operating_regions, inverse_of: :expert, dependent: :destroy

  accepts_nested_attributes_for :expert_partners, reject_if: :all_blank, allow_destroy: true
  accepts_nested_attributes_for :expert_operating_regions, reject_if: :all_blank, allow_destroy: true
end

class ExpertPartner < ApplicationRecord
  has_paper_trail

  belongs_to :expert

  validates :name, presence: true
end

class ExpertOperatingRegion < ApplicationRecord
  has_paper_trail

  belongs_to :expert
  belongs_to :region

  validates :expert, :region, presence: true
end

Output

nested_attributes_for :expert
{
    "email": "MyString",
    "name": "MyString",
    "region_id": 22,
    "expert_partners_attributes": [
        {
            "name": "MyString"
        }
    ],
    "expert_operating_regions_attributes": [
        {
            "region_id": 23
        }
    ]
}

PS: To be honnest, I havn't tested has_one relationship

@davegson
Copy link

davegson commented Jul 8, 2019

A clean version for the most basic functionality for Rails 5+

This creates :belongs_to associations and adds their id (and type if :polymorphic)
to the attributes.

spec/support/factory_bot_macros.rb

module FactoryBot::Syntax::Methods
  def nested_attributes_for(*args)
    attributes = attributes_for(*args)
    klass = args.first.to_s.camelize.constantize

    klass.reflect_on_all_associations(:belongs_to).each do |r|
      association = FactoryBot.create(r.class_name.underscore)
      attributes["#{r.name}_id"] = association.id
      attributes["#{r.name}_type"] = association.class.name if r.options[:polymorphic]
    end

    attributes
  end
end

this is an adapted version of @jamesst20 - kudos to him 👏

@loqimean
Copy link

loqimean commented Oct 9, 2022

A clean version for the most basic functionality for Rails 5+

This creates :belongs_to associations and adds their id (and type if :polymorphic) to the attributes.

spec/support/factory_bot_macros.rb

module FactoryBot::Syntax::Methods
  def nested_attributes_for(*args)
    attributes = attributes_for(*args)
    klass = args.first.to_s.camelize.constantize

    klass.reflect_on_all_associations(:belongs_to).each do |r|
      association = FactoryBot.create(r.class_name.underscore)
      attributes["#{r.name}_id"] = association.id
      attributes["#{r.name}_type"] = association.class.name if r.options[:polymorphic]
    end

    attributes
  end
end

this is an adapted version of @jamesst20 - kudos to him 👏

module FactoryBot::Syntax::Methods
  def nested_attributes_for(*args)
    attributes = attributes_for(*args)
    klass = args.first.to_s.camelize.constantize

    klass.reflect_on_all_associations(:belongs_to).each do |r|
      association = FactoryBot.create(r.class_name.underscore)
      attributes[:"#{r.name}_id"] = association.id
      attributes[:"#{r.name}_type"] = association.class.name if r.options[:polymorphic]
    end

    attributes
  end
end

You forgot to add symbol transformation, because of attributes_for returns keys as symbols

@oniram88
Copy link

A clean version for the most basic functionality for Rails 5+

This creates :belongs_to associations and adds their id (and type if :polymorphic) to the attributes.
spec/support/factory_bot_macros.rb

module FactoryBot::Syntax::Methods
  def nested_attributes_for(*args)
    attributes = attributes_for(*args)
    klass = args.first.to_s.camelize.constantize

    klass.reflect_on_all_associations(:belongs_to).each do |r|
      association = FactoryBot.create(r.class_name.underscore)
      attributes["#{r.name}_id"] = association.id
      attributes["#{r.name}_type"] = association.class.name if r.options[:polymorphic]
    end

    attributes
  end
end

this is an adapted version of @jamesst20 - kudos to him 👏

module FactoryBot::Syntax::Methods
  def nested_attributes_for(*args)
    attributes = attributes_for(*args)
    klass = args.first.to_s.camelize.constantize

    klass.reflect_on_all_associations(:belongs_to).each do |r|
      association = FactoryBot.create(r.class_name.underscore)
      attributes[:"#{r.name}_id"] = association.id
      attributes[:"#{r.name}_type"] = association.class.name if r.options[:polymorphic]
    end

    attributes
  end
end

You forgot to add symbol transformation, because of attributes_for returns keys as symbols

don't know if its with new FactoryBot Versions but for finding the correct class I use directly FactoryBot::Factory#build_class and so the helper is:

module FactoryBot::Syntax::Methods
  def nested_attributes_for(*args)
    attributes = attributes_for(*args)
    klass = FactoryBot::Internal.factory_by_name(args.first).build_class

    klass.reflect_on_all_associations(:belongs_to).each do |r|
      association = FactoryBot.create(r.class_name.underscore)
      attributes[:"#{r.name}_id"] = association.id
      attributes[:"#{r.name}_type"] = association.class.name if r.options[:polymorphic]
    end

    attributes
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests