This project is archived and is in readonly mode.
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 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 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 -
-
Dan Pickett May 9th, 2010 @ 07:21 PM
- Tag changed from “activemodel, dirty” to “activemodel, bugmash, dirty”
-
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.
-
-
-
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 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 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 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 May 15th, 2010 @ 09:24 PM
- Tag changed from “activemodel, bugmash, dirty, patch” to “activemodel, bugmash-review, dirty, patch”
-
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”
-
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 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 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.
-
-
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 May 16th, 2010 @ 02:51 PM
Last assertion should be
assert !cloned_topic.group_changed?offcourse. -
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.
-
-
-
-
-
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”
-
-
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>
People watching this ticket
Arpit Jain
Federico Brubacher
Jeremy Kemper
pleax
Rafael Mendonça França
Ryan Bigg
Attachments
Referenced by
-
4614
Cloned object mimic changed_attributes from creator after being clonde
Quick question: how does this interact with the issue fro...
-
2919
Activerecord::Base#clone does not set dirty bits
[#2919 state:committed]