rails 3.2 active_record.clone creates joined objects #6235

Closed
jshow opened this Issue May 9, 2012 · 13 comments

5 participants

@jshow

When ar objects are cloned, modifying one modifies all others

Loading development environment (Rails 3.2.0)
1.9.3p194 :001 > jango = JangoFett.new
 => #<JangoFett id: nil, name: nil, created_at: nil, updated_at: nil> 
1.9.3p194 :002 > boba_1 = jango.clone
 => #<JangoFett id: nil, name: nil, created_at: nil, updated_at: nil> 
1.9.3p194 :003 > jango.name = 'luke'
 => "luke" 
1.9.3p194 :004 > boba_1.name
 => "luke" 
1.9.3p194 :003 > boba_1.name = 'leia'
 => "leia" 
1.9.3p194 :004 > jango.name
 => "leia" 
1.9.3p194 :005 > jango.object_id
 => 2168547760 
1.9.3p194 :006 > boba_1.object_id
 => 2153124180 

$ rails --version
Rails 3.2.0

$ ruby --version
ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-darwin10.8.0]
(also with ruby 1.9.2p290 (2011-07-09 revision 32553) [x86_64-darwin10.4.0] )

$ bundle list
Gems included by the bundle:
  * actionmailer (3.2.0)
  * actionpack (3.2.0)
  * activemodel (3.2.0)
  * activerecord (3.2.0)
  * activeresource (3.2.0)
  * activesupport (3.2.0)
  * arel (3.0.2)
  * builder (3.0.0)
  * bundler (1.1.3)
  * coffee-rails (3.2.2)
  * coffee-script (2.2.0)
  * coffee-script-source (1.3.1)
  * erubis (2.7.0)
  * execjs (1.3.2)
  * hike (1.2.1)
  * i18n (0.6.0)
  * journey (1.0.3)
  * jquery-rails (2.0.2)
  * json (1.7.1)
  * mail (2.4.4)
  * mime-types (1.18)
  * multi_json (1.3.4)
  * polyglot (0.3.3)
  * rack (1.4.1)
  * rack-cache (1.2)
  * rack-ssl (1.3.2)
  * rack-test (0.6.1)
  * rails (3.2.0)
  * railties (3.2.0)
  * rake (0.9.2.2)
  * rdoc (3.12)
  * sass (3.1.17)
  * sass-rails (3.2.5)
  * sprockets (2.1.3)
  * sqlite3 (1.3.6)
  * thor (0.14.6)
  * tilt (1.3.3)
  * treetop (1.4.10)
  * tzinfo (0.3.33)
  * uglifier (1.2.4)
@frodsan

This is an issue in master too.

@frodsan

@carlosantoniodasilva Is this a bug? Apparently, is an expected behaviour:

# Clone all attributes except the pk and any nested ARes
cloned = Hash[attributes.reject {|k,v| k == self.class.primary_key || v.is_a?(ActiveResource::Base)}.map { |k, v| [k, v.clone] }]
# Form the new resource - bypass initialize of resource with 'new' as that will call 'load' which
# attempts to convert hashes into member objects and arrays into collections of objects.  We want
# the raw objects to be cloned so we bypass load by directly setting the attributes hash.
resource.prefix_options = self.prefix_options
resource.send :instance_variable_set, '@attributes', cloned
@erichmenge

Rails AR objects try to mimic Ruby in this regard. Clone produces a shallow copy as per Ruby docs. So your attributes hash still references the original. Use #dup instead, which will copy your attributes hash as well.

irb(main):007:0> user.name.object_id == user.clone.name.object_id
=> true
irb(main):008:0> user.name.object_id == user.dup.name.object_id
=> false
@frodsan

@erichmenge nice explanation. I think that it should be closed.

@jshow

ruby behavior is different (as far as I understand)

$ irb
1.9.3p194 :001 > class Person
1.9.3p194 :002?>     def name
1.9.3p194 :003?>         @name
1.9.3p194 :004?>       end
1.9.3p194 :005?>     def name=(name)
1.9.3p194 :006?>         @name = name
1.9.3p194 :007?>       end
1.9.3p194 :008?>   end
 => nil 
1.9.3p194 :009 > p = Person.new
 => #<Person:0x000001009d34a0> 
1.9.3p194 :010 > clone = p.clone
 => #<Person:0x000001009c56c0> 
1.9.3p194 :011 > p.name = 'flintstone'
 => "flintstone" 
1.9.3p194 :013 > clone.name
 => nil
@drogus drogus closed this May 9, 2012
@jshow

imo, rails clone behavior is unexpected if it's to mimic ruby behavior

what is copied (shallow versus deep) is independent of the copy being "live"

@erichmenge

This isn't quite the same thing. Your attributes hash is being shallow copied. Ruby works the same way:

class Foo
  attr_reader :messages

  def message(msg)
    @messages ||= []
    @messages << msg
  end
end

foo = Foo.new
foo.message "crux"
puts foo.messages.inspect   # ["crux"]
bar = foo.clone
puts bar.messages.inspect   # ["crux"]
puts bar.messages[0]        # crux
bar.messages[0] = "Ooops!"
puts bar.messages[0]        # Ooops!
puts foo.messages[0]        # Ooops!
puts foo.messages.inspect   # ["Ooops!"]

Ruby #clone copies instance variables, so does the Rails implementation. It copies your attributes hash instance variable. Unfortunately the data is not copied, but only the reference. This is the same as Ruby.

In your example, you're reassigning the instance variable to a new string. It won't reassign your original instance variable to point to the new one.

In my example (which is what is going on in Rails), the instance variables point to the array (or hash, as in the case of the Active Record). In your first example, you're modifying the hash contents, but both the original and the cloned object instance variables point to the same hash. So you get mutation.

@jshow

yes - I see

I can only now suggest that a note is made in the docs as this behavior will bite developers - perhaps something more explicit - since rails uses hashes for values, the values will be shared amongst instances.

thanks all.

@drogus
Ruby on Rails member

@fxn @vijaydev can we document methods that are not actually defined? we just implement initialize_dup and initialize_copy, it would be nice to document clone better

@carlosantoniodasilva
Ruby on Rails member

@frodsan Sorry, I'm late for the party, glad that it's already solved :).. I think it'd be good to better document the behavior.

@erichmenge you did a great explanation on the issue and how Ruby works, perhaps you could expand the docs as @drogus commented, wdyt? If you want to, just go ahead and do it in lifo/docrails, or even send a pull request, would be awesome.

Thanks!

@erichmenge

@carlosantoniodasilva I'd be happy to do that. #initialize_dup is already documented, but that doesn't do much for new comers. Where do you think the added documentation should go?

@carlosantoniodasilva
Ruby on Rails member

@erichmenge I can't think of other than add more docs to the method itself, perhaps with some examples as you posted here.

@drogus @vijaydev any other thoughts?

@erichmenge

Okay, here it is: rails/docrails@7dba512 let me know what you think.

@vijaydev vijaydev pushed a commit that referenced this issue May 12, 2012
Erich Menge Better document the difference between #clone and #dup.
Add #nodoc to initialize_dup and use :method: to document the #dup method.
Relates to issue #6235
7dba512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment