Skip to content

Loading…

Factory.create caches serialized fields #121

Closed
evtuhovich opened this Issue · 10 comments

3 participants

@evtuhovich

This spec fails

class Batch < ActiveRecord::Base
  serialize :options, Hash
end

    batch = Factory :batch
    batch.options[:test].should == nil

    batch.options[:test] = 'test'

    batch2 = Factory :batch

    batch2.options[:test].should == nil

I reproduce this bug, https://github.com/evtuhovich/factory_girl_test

ruby 1.9.1p376 (2009-12-07 revision 26041) [x86_64-linux]

@joshuaclayton
thoughtbot, inc. member

Can you try this without using factory girl and confirm that it works correctly with ActiveRecord? Thanks.

@gogolok

+1 (same here)

@joshuaclayton
thoughtbot, inc. member

@gogolok, could you confirm that it works correctly by just using ActiveRecord and not FactoryGirl? I never got a response from @evtuhovich.

@evtuhovich

Sorry, i just forgot about this.

This is not a AR problem


    b = Batch.create :options => {1 => 2}
    b.options[:test] = 'test'
    b.save

    b2 = Batch.create :options => {1 => 2}
    p b2.options

    => {1=>2}

@evtuhovich

Problem is that, you cache the initial value

Factory.define :batch, :class => Batch do |batch|
  batch.type 'Batch'
  batch.options({:domain => 'ce.test.com'}) # this hash is cached
end

I create another example

h = { 1 => 2 }
b = Batch.create :options => h

b.options[:test] = 'test'

b.save

b2 = Batch.create :options => h

p b2.options
# return {:test=>"test", 1=>2}

Yes, we could use Hash.new in Factory definition, but this is not intuitive, i think

@joshuaclayton
thoughtbot, inc. member

@evtuhovich this example you just pasted isn't using FactoryGirl, so I'm really confused. Above, when you set b.options[:test] = 'test' on line 3. you're also modifying h. So, when you assign b2, b2.options SHOULD have both keys (test and 1).

Could you upgrade to the latest factory_girl and let me know if this is still an issue? I'm trying to figure out what's actually related to FactoryGirl. When you upgrade, if you could make Batch#options a dynamic attribute in the factory, that'd be great.

FactoryGirl.define do
  factory :batch do
    options { { :domain => "ce.test.com" } }
  end
end

If it is a caching thing (because you're modifying the reference by overriding the defaults), I think a dynamic attribute (one that's evaluated with the block, so it's calculated when it's executed) should resolve this.

@evtuhovich

Yes, i now understand the root of problem, but at the time i write this report - this was a problem.

May be this is a good idea to create a new Hash inside Factory Girl in situations, like this (because some people also add +1 to this problem)

Factory.define :batch, :class => Batch do |batch|
  batch.type 'Batch'
  batch.options({:domain => 'ce.test.com'}) # i think this hash should recreated on every Factory.build call
end
@joshuaclayton
thoughtbot, inc. member

In my opinion, this isn't so much a problem with factory girl as it is a misunderstanding of how Ruby works. If you declare a hash and assign it, it will maintain certain references. Factory Girl's job is not to prevent Ruby from working like Ruby. The solution here is to assign the hash dynamically so that the reference isn't maintained.

@gogolok

@joshuaclayton You're right, have to blame AR or rather an extension to AR that caused the error. Sorry about the noise!

@joshuaclayton
thoughtbot, inc. member

@gogolok No worries!

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.