Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Dup'ed ActiveRecord objects may not share the errors object #7371

Merged
merged 1 commit into from

6 participants

@csmuc

Call super to nullify the reference to the original errors object in the dup'ed object (call ActiveModel::Validations#initialize_dup)

@rafaelfranca

Seems good.

Could you add a CHANGELOG entry?

activerecord/lib/active_record/timestamp.rb
@@ -42,6 +42,7 @@ module Timestamp
def initialize_dup(other)
clear_timestamp_attributes
+ super if defined?(super)

I don't think we need to check for defined?, since Ruby 1.9 has initialize_dup, or not?

@csmuc
csmuc added a note

Maybe it makes sense to marge into 3_2 as well? Then the defined? check would make more sense.

In there it might be needed yeah, but here it's not. If the decision to backport happens, then the responsible can add it I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@csmuc

oh well the super call is required, meant the super check

activerecord/test/cases/dup_test.rb
@@ -107,5 +107,20 @@ def test_dup_after_initialize_callbacks
assert Topic.after_initialize_called
end
+ def test_dup_validity_is_independent
+ Topic.validates_presence_of :title
+ topic = Topic.new("title" => "Litterature")
+ topic.valid?
+
+ duped = topic.dup
+ duped.title = nil
+ assert duped.invalid?
+
+ topic.title = nil
+ duped.title = 'Mathematics'
+ assert topic.invalid?
+ assert duped.valid?
+ end
+

Please squash your commits and remove this blank line, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva

Also important to note that this will close #7291.

@csmuc

removed the blank line, rebased and squashed the commits (sorry for delay, didn't notice the sqash comment)

@csmuc

I just verified that the bug still exists in master and this fix works. You guys want to merge it in?

@spastorino
Owner

@csmuc as @carlosantoniodasilva mentioned, could you add Closes #7291 to the commit message so it references the issue and github automatically close it ?.
Thank you for your work and help :)

@csmuc csmuc Call super to nullify the reference to the original errors object in …
…the dup'ed object (call ActiveModel::Validations#initialize_dup). Closes #7291
fb66521
@csmuc

added "Closes #7291" to the commit message

@spastorino spastorino merged commit c432c74 into from
@csmuc

wow, I just added one line of production code to Rails :)

@steveklabnik
Collaborator

@csmuc now just check out http://contributors.rubyonrails.org/contributors/Christian-Seiler/commits and hit f5f5f5f5 ;) Keep going!

@rafaelfranca

Guys remember to backport the fixes to 3-2-stable

@spastorino
Owner

@rafaelfranca did you backport this one? if not I will

@rafaelfranca

Yes, 9a38e73

@spastorino
Owner

Cool

@csmuc

@rafaelfranca Ruby 1.8 doesn't have the Object#initialize_dup method. So by just looking at the Timestamp module you actually can't be sure that a super method exists. It exists because of ActiveModel, but maybe you should check if it's defined?

@rafaelfranca

@csmuc all the tests are green. https://travis-ci.org/#!/rails/rails/builds/2833973

Or we are missing tests that should catch this failure, or it is working

@csmuc

I don't know if the idea is that modules like ActiveRecord::Timestamp can be used outside of ActiveRecord::Base (which depends on ActiveModel, which adds #initialize_dup).

If ActiveRecord::Timestamp is used outside of ActiveRecord::Base, then it could be possible that the method doesn't exist in case of Ruby 1.8.

In Ruby 1.8 there is some sort of implicit assumption that the method was defined somewhere else. Not sure if it's a real problem, though.

@rafaelfranca

ActiveRecord::Timestamp should not be used outside of ActiveRecord::Base, so the issue doesn't exist

@rochefort

Is this line required?
If required, I consider that it is better to add assert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 16, 2012
  1. @csmuc

    Call super to nullify the reference to the original errors object in …

    csmuc authored
    …the dup'ed object (call ActiveModel::Validations#initialize_dup). Closes #7291
This page is out of date. Refresh to see the latest.
View
5 activerecord/CHANGELOG.md
@@ -269,6 +269,11 @@
*Ari Pollak*
+* Fix AR#dup to nullify the validation errors in the dup'ed object. Previously the original
+ and the dup'ed object shared the same errors.
+
+ * Christian Seiler*
+
* Raise `ArgumentError` if list of attributes to change is empty in `update_all`.
*Roman Shatsov*
View
1  activerecord/lib/active_record/timestamp.rb
@@ -42,6 +42,7 @@ module Timestamp
def initialize_dup(other) # :nodoc:
clear_timestamp_attributes
+ super
end
private
View
14 activerecord/test/cases/dup_test.rb
@@ -107,5 +107,19 @@ def test_dup_after_initialize_callbacks
assert Topic.after_initialize_called
end
+ def test_dup_validity_is_independent
+ Topic.validates_presence_of :title
+ topic = Topic.new("title" => "Litterature")
+ topic.valid?
+
+ duped = topic.dup
+ duped.title = nil
+ assert duped.invalid?
+
+ topic.title = nil
+ duped.title = 'Mathematics'
+ assert topic.invalid?
+ assert duped.valid?
+ end
end
end
Something went wrong with that request. Please try again.