#2919 Activerecord::Base#clone does not set dirty bits - Ruby on Rails

You do not have access to this ticket

This project is archived and is in readonly mode.

#2919 ✓committed

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

You flagged this item as spam.
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.
You flagged this item as spam.
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 May 4th, 2010 @ 06:48 PM
- Milestone changed from “2.x” to “3.x”
[bulk edit]
Dan Pickett May 9th, 2010 @ 07:21 PM
- Tag changed from “activemodel, dirty” to “activemodel, bugmash, dirty”
You flagged this item as spam.
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.
- clones_marked_dirty.diff 1.7 KB
You flagged this item as spam.
Simon Jefford May 15th, 2010 @ 07:07 PM
verified +1
You flagged this item as spam.
Anil Wadghule May 15th, 2010 @ 07:12 PM
+1 Verified.

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

You flagged this item as spam.
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
You flagged this item as spam.
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.
You flagged this item as spam.
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.
You flagged this item as spam.
Federico Brubacher May 15th, 2010 @ 09:24 PM
- Tag changed from “activemodel, bugmash, dirty, patch” to “activemodel, bugmash-review, dirty, patch”
You flagged this item as spam.
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 May 16th, 2010 @ 02:22 AM
- Tag changed from “activemodel, bugmash-review, dirty, patch” to “activemodel, bugmash, dirty, patch”
You flagged this item as spam.
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...
You flagged this item as spam.
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.
You flagged this item as spam.
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.
You flagged this item as spam.
Federico Brubacher May 16th, 2010 @ 01:09 PM
Added forgotten patch
- clones_marked_dirty.diff 1.9 KB

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

You flagged this item as spam.
pleax May 16th, 2010 @ 02:51 PM
Last assertion should be assert !cloned_topic.group_changed? offcourse.
You flagged this item as spam.
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.
- clones_marked_dirty.diff 2.5 KB
You flagged this item as spam.
pleax May 16th, 2010 @ 10:56 PM
I've attached patch with bunch of test-cases and my version of fix.
You flagged this item as spam.
pleax May 16th, 2010 @ 11:14 PM
The patch itself :)
- 2919-ar-clone-dirtybits.diff 3.8 KB
Rizwan Reza May 16th, 2010 @ 11:16 PM
Please test this and report back so it can be committed.
You flagged this item as spam.
Rafael Mendonça França May 17th, 2010 @ 12:39 AM
+1

All tests pass.
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 May 17th, 2010 @ 12:09 PM
- Tag changed from “activemodel, bugmash, dirty, patch” to “activemodel, dirty, patch”
Ryan Bigg October 9th, 2010 @ 09:47 PM
- Tag cleared.
Automatic cleanup of spam.
You flagged this item as spam.
bingbing March 26th, 2011 @ 01:23 AM
gucci watches