This project is archived and is in readonly mode.

#2919 ✓committed
Fjan

Activerecord::Base#clone does not set dirty bits

Reported by Fjan | July 16th, 2009 @ 10:52 PM | in 3.x

A cloned object does not get marked dirty:

obj = Person.first
obj2 = obj.clone
obj2.changed? # => false

This might trip people up who limit expensive validations to changed attributes.
For example, this is the failure that I ran into:

Class Person
validates_uniqueness_of :name, :if => Proc.new { |u| u.name_changed? } # cloned objects are not checked! end

One could argue that the behaviour is correct since #new() also results in #changed?() being false. In any case, I would suggest updating the documentation to point out the behaviour.

Comments and changes to this ticket

  • Arpit Jain

    Arpit Jain September 12th, 2009 @ 01:41 PM

    • Tag set to activemodel, dirty

    I ran into the same problem when my records could not be saved on cloning as i used #{attr}_changed? to locate the changes in new/cloned objects.

    I would still say that this behavior is wrong because if you do

    u = User.new(:email => "someemail@email.com") u.email_changed? # => true

    So, if you instantiate a new user with initial hash, dirty bits are set. Isn't cloning the same thing ? We are just duplicating a new object, meaning we are creating a new object and instantiating with values of older object. Hence, dirty bits should be set.

  • Arpit Jain

    Arpit Jain September 12th, 2009 @ 01:43 PM

    Sorry the code block didn't come correctly in previous comment.
    Here is what I meant :

      user = User.new(:email => "someemail@email.com")
      u.email_changed? #=> true
    
  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Dan Pickett

    Dan Pickett May 9th, 2010 @ 07:21 PM

    • Tag changed from activemodel, dirty to activemodel, bugmash, dirty
  • Federico Brubacher

    Federico Brubacher May 15th, 2010 @ 06:00 PM

    • Tag changed from activemodel, bugmash, dirty to activemodel, bugmash, dirty, patch

    I've attached a patch.

    I agree with Arpit, from the docs: Cloned objects have no id assigned and are treated as new records

    Here is a patch that fixes this behavior.

  • Simon Jefford
  • Anil Wadghule
  • pleax

    pleax May 15th, 2010 @ 07:15 PM

    -1 A problem with the patch is illustrated by given test case:

      test "cloned object should threat as changed only modified attributes" do
        token = Topic.new :content => 'foo bar baz'
        assert !token.title_changed?
        
        token.save
        assert !token.title_changed?
        
        cloned_token = token.clone
        assert !cloned_token.title_changed?
      end
    
  • elcuervo

    elcuervo May 15th, 2010 @ 07:29 PM

    +1

    @pleax I i disagree with your comment, as Federico and Arpit mentioned, the cloned objects shall be treated as new records then it follows that cloned objects should be dirty

  • pleax

    pleax May 15th, 2010 @ 07:37 PM

    @elcuervo But it doesn't! As you can see from test-case I provided, some attributes of cloned object behaves differently from new one.

  • Federico Brubacher

    Federico Brubacher May 15th, 2010 @ 08:05 PM

    Pleax if you read the thread in this ticket you will see what is the behavior that is bugging me and some of the other users.

    What the thread starter and Armit are saying is that for them the clone method is doing is creating a new Object and copying all the attributes from the original. Even if those were blank in the first object.

    So that's basically the idea of this ticket. To repeat Fian : One could argue that the behaviour is correct since #new() also results in #changed?() being false.

    Doesn't make sense for me to copy the dirty attribute flag from the first object.

  • Federico Brubacher

    Federico Brubacher May 15th, 2010 @ 09:24 PM

    • Tag changed from activemodel, bugmash, dirty, patch to activemodel, bugmash-review, dirty, patch
  • pleax

    pleax May 15th, 2010 @ 10:20 PM

    Still not clear to me.

    You're trying to say that new objects doesn't treat modified attributes as changed?. But they do:

    t = Topic.new :title => 'test'
    t.changed? # => true
    t.title_changed? # => true
    t.content_changed? # => false (!)
    

    And yes. There is no sence to copy dirty-flags from original object. There should be different implementation.

    I'm trying to say that attributes of clonned object should be threated as changed? only if they differ from defaults.

    Also I've found strange behavior with the patch applied:

    t = Topic.new
    t.changed? # => false
    t.clone
    t.changed? # => true (!)
    
  • Rizwan Reza

    Rizwan Reza May 16th, 2010 @ 02:22 AM

    • Tag changed from activemodel, bugmash-review, dirty, patch to activemodel, bugmash, dirty, patch
  • Federico Brubacher

    Federico Brubacher May 16th, 2010 @ 11:29 AM

    I've attached a patch.

    "I'm trying to say that attributes of clonned object should be threated as changed? only if they differ from defaults."

    I think I can agree with that, I added a test and a fix. Please confirm. Also next time upload a patch so i can give you credit.

    About the strange issue, I'm opening a new ticket now. Here : https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

  • Federico Brubacher

    Federico Brubacher May 16th, 2010 @ 11:32 AM

    I meant to say in the previous comment that the issue was present in master before my fix.

  • pleax

    pleax May 16th, 2010 @ 12:05 PM

    I added a test and a fix. Please confirm.

    Don't see it.

    I meant to say in the previous comment that the issue was present in master before my fix.

    Agree.

  • Federico Brubacher
  • pleax

    pleax May 16th, 2010 @ 02:49 PM

    I'm not sure if check for blank? is acceptable here. Because model attributes may have non-empty defaults.

      test "non-empty defaults should not threated as changed" do
        topic = Topic.new
        assert_equal "non-empty-default", topic.group
        assert !topic.group_changed?
        
        cloned_topic = topic.clone
        assert_equal "non-empty-default", cloned_topic.group
        assert !topic.group_changed?
      end
    
  • pleax

    pleax May 16th, 2010 @ 02:51 PM

    Last assertion should be assert !cloned_topic.group_changed? offcourse.

  • Eugene Bolshakov

    Eugene Bolshakov May 16th, 2010 @ 05:18 PM

    We have updated the patch so that it takes default values into account. Cloning now involves creation of another model object against which attributes of the clone are checked (and marked as dirty if necessary), which is not great, but it was the best we could come up with.

  • pleax

    pleax May 16th, 2010 @ 10:56 PM

    I've attached patch with bunch of test-cases and my version of fix.

  • pleax
  • Rizwan Reza

    Rizwan Reza May 16th, 2010 @ 11:16 PM

    Please test this and report back so it can be committed.

  • Rafael Mendonça França
  • Repository

    Repository May 17th, 2010 @ 02:03 AM

    • State changed from “new” to “committed”

    (from [4db10bce7b5ca794c4d408de3b8002bf58233bb7]) AR::Base#clone fixed to set dirty bits for cloned object

    [#2919 state:committed]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
    http://github.com/rails/rails/commit/4db10bce7b5ca794c4d408de3b8002...

  • Rizwan Reza

    Rizwan Reza May 17th, 2010 @ 12:09 PM

    • Tag changed from activemodel, bugmash, dirty, patch to activemodel, dirty, patch
  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 09:47 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • bingbing

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages