Overrides don't respect attribute definition order #140

Closed
ladelfa opened this Issue May 14, 2011 · 15 comments

Comments

Projects
None yet
4 participants

ladelfa commented May 14, 2011

First, we're using Factory Girl 1.3.3.

I have a model which requires that one attribute be set before another is; the second one raises a "you can't set me" error if some condition based on the first attribute isn't met.

Factories work fine when the attribute values are hard-coded in the define block; I have them defined in the required order.

But when the attribute values are passed in to Factory.create() or .build() via hash, as overrides, they intermittently fail because they're sometimes being called on the model in the wrong order.

The problem seems to be that the overrides hash has no order to it, and so its elements are processed in indeterminate, random order, without regard to the order of the attributes were defined.

I've attempted to address this in my version below, which loops first through the attributes, processing any overrides that apply to each one, and then processing any overrides which remain.

class Factory
  def run (proxy_class, overrides) #:nodoc:
    proxy = proxy_class.new(build_class)

    # The original way, overrides are processed in whatever order the hash offers them, e.g. random.
    #
    # overrides = symbolize_keys(overrides)
    # overrides.each {|attr, val| proxy.set(attr, val) }
    # passed_keys = overrides.keys.collect {|k| Factory.aliases_for(k) }.flatten
    # @attributes.each do |attribute|
    #   unless passed_keys.include?(attribute.name)
    #     attribute.add_to(proxy)
    #   end
    # end


    # But this way, overrides are processed in the same order the attributes are defined, e.g. deliberate.
    #
    remaining_overrides = symbolize_keys(overrides)
    @attributes.each do |attribute|
      # Take out whatever overrides apply to this attribute
      overrides_for_this_attribute, remaining_overrides = remaining_overrides.partition do |key, val|
        Factory.aliases_for(key).include?(attribute.name)
      end

      if overrides_for_this_attribute.empty?
        attribute.add_to(proxy)
      else
        overrides_for_this_attribute.each do |attr, val|
          proxy.set(attr, val)
        end
      end
    end    

    # Any that are left didn't apply to a defined attribute; process them now, in indeterminate order.
    remaining_overrides.each {|attr, val| proxy.set(attr, val) }

    proxy.result
  end
end
Owner

jferris commented May 23, 2011

Do you want to put together a patch with tests? I don't have time to put it together myself, but if you send a pull request I'll make time to review it.

Owner

joshuaclayton commented Jun 30, 2011

@ladelfa, were you still interested in this? Could you put together a pull request with this functionality?

ladelfa commented Jun 30, 2011

I'm sorry I won't have any resources to devote to this in the immediate
future. The code I posted does solve the problem; we are using the patched
version and it works as desired. Please feel free to take what I posted and
utilize it as you see fit.

DL

-----Original Message-----
From: joshuaclayton
Sent: Thursday, June 30, 2011 11:49 AM
To: dave@ladelfa.net
Subject: Re: [factory_girl] Overrides don't respect attribute definition
order (#140)

@lafelda, were you still interested in this? Could you put together a pull
request with this functionality?

Reply to this email directly or view it on GitHub:
#140 (comment)

Owner

joshuaclayton commented Jun 30, 2011

Glad to hear that code is working - would it be possible for you to at least provide a test that we can use that recreates your issue? I don't want to assume anything and would like to make sure that we're able to reproduce the exact problem in a test before applying the above patch. If you're not able to, I'll try and see if I can get something failing. Thanks.

Contributor

metaskills commented Jul 19, 2011

I think I was experiencing a similar issue using v2.0.0.rc4. I wrote this gist to demonstrate the attribute race condition.

class Merchant < ActiveRecord::Base
  def foobarable?
    # ...
  end
end

class Customer < ActiveRecord::Base
  belongs_to :merchant
  def foobar=(something_else)
    self[:foobar] = something_else if merchant.foobarable?
  end
end

FactoryGirl.define do
  factory :merchant do
  end
  factory :customer do
    association :merchant
  end
end

# How can I make sure that the merchant attribute/association is set first so 
# that the foobar attribute checks my passed in merchant?
c = FactoryGirl.create :customer, :merchant => @some_merchant, :foobar => 'xyz'

Is this something that people have ran into often and is there a workaround?

Contributor

metaskills commented Jul 19, 2011

I can confirm that a #run method like this using @ladelfa's example not only passes all the specs but my particular use case.

def run(proxy_class, overrides) #:nodoc:
  proxy = proxy_class.new(build_class)
  remaining_overrides = symbolize_keys(overrides)
  @attributes.each do |attribute|
    overrides_for_this_attribute, remaining_overrides = remaining_overrides.partition do |key, val|
      FactoryGirl.aliases_for(key).include?(attribute.name)
    end
    if overrides_for_this_attribute.empty?
      attribute.add_to(proxy)
    else
      overrides_for_this_attribute.each do |attr, val|
        proxy.set(attr, val)
      end
    end
  end
  remaining_overrides.each {|attr, val| proxy.set(attr, val) }
  proxy.result(@to_create_block)
end

I just forked the repo and will dig around to see where an appropriate place to write a test for this may be.

Contributor

metaskills commented Jul 19, 2011

As @joshuaclayton stated on twitter, ordered hashes in 1.9 would be a cure, but then again, I can not think of all the places I would have to apply ActiveSupport::OrderedHash to fix this when it may or may not show it's ugly head. Would you be opposed to a patch? or should I be encouraged to just monkey patch this on my own till we move to 1.9?

Contributor

metaskills commented Jul 20, 2011

I have had some time to think about this as I have explained the issue to a few people and I have come to the opinion that even if you are running ruby 1.9, you should not have to think back to what default attributes/associations your factory implemented and then make sure you always code your factory overrides hash in such a way that puts the overrides before any extra attributes.

For this reason, I do think this should be something tested and patched for as mentioned above. I would be willing to write a test for this and try to make the patch cleaner if you could point me to the spec file you think this belongs in.

Owner

joshuaclayton commented Jul 29, 2011

@metaskills, the more I think about it, I totally agree - the order attributes are defined in the factory should be the order attributes are applied when a hash is passed to create. Obviously, if the attributes aren't defined in the factory, the attribute order is dependent on the version of Ruby (1.8 is unordered unless passing an ordered hash, and Ruby 1.9 will be ordered after respecting attribute definition order).

One thing to note is that there's another ticket that places a higher precedence in static attributes - that is, we'll be introducing attribute sorting by default so static attributes are calculated before dynamic ones (blocks, sequences, etc.) Here's the ticket: #159

I'm not sure if that code would effectively resolve this issue, so you may want to hold off until we get that merged in before spending some time on this.

Contributor

metaskills commented Jul 29, 2011

Thanks, sounds good to me. Just shout out when you think I could be of help writing and testing a patch.

Owner

joshuaclayton commented Jul 29, 2011

Cool, will do

Owner

joshuaclayton commented Aug 5, 2011

@metaskills I just merged in 28f541e which gives precedence to static attributes being evaluated first. If that doesn't solve the issue, please submit a pull request with a test that's able to reproduce the failure and I'll get it merged in. Thanks!

Contributor

metaskills commented Aug 5, 2011

Thanks @joshuaclayton, I just did a quick test and I think #159's solution does not work for me because my attribute/association happens to not be static. The general concept of my needs is that both factory static/dynamic attributes are always applied first before any overrides in general since it is assumed that both static/dynamic factory attributes are the bare minimum to get a valid object before the overrides are applied. I will try to write a test.

Owner

joshuaclayton commented Aug 5, 2011

Awesome, thanks

Contributor

metaskills commented Aug 7, 2011

OK, I have opened up a pull request.
#169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment