Bryan Helmkamp
2007-Oct-10 01:35 UTC
Assocation saving backward incompatibility in Rails 1.2.4
Today I ran across test failures in my app when upgrading from Rails
1.2.3 to 1.2.4. I determined the issue is caused by r7076 (http://
dev.rubyonrails.org/changeset/7076) which tries to save associated
records if and only if the association is already loaded. Here is a
simple test case that passes in Rails 1.2.3 and fails in Rails 1.2.4:
class Computer < ActiveRecord::Base
has_many :change_logs
before_save do |computer|
computer.change_logs.create(:message => "Computer created!") if
computer.new_record?
end
end
class ChangeLog < ActiveRecord::Base
end
def test_save_on_parent_saves_child_even_when_not_loaded
computer = Computer.create!
assert_equal "Computer created!", computer.change_logs.first.message
end
Upon further investigation I discovered r7511 (http://
dev.rubyonrails.org/changeset/7511) which adds a specific check and
exception when a user attempts to use the behavior that r7076
eliminated. That changeset was never backported to the 1.2 branch.
I see three options for resolving this:
1. Backport r7511 to the 1.2 branch. This would give users appropriate
feedback if they trip over this behavior, but still break backwards
compatibility for AR.
2. Revert r7076 to preserve full backwards compatibility in the 1.2
branch.
3. Attempt to find a solution that addresses the concern in ticket
#8713 (http://dev.rubyonrails.org/ticket/8713) while still maintaining
backward compatibility with Rails 1.2.3. To this end, I have been
toying with the following patch which passes all existing AR tests:
Index: lib/active_record/associations.rb
==================================================================---
lib/active_record/associations.rb (revision 7820)
+++ lib/active_record/associations.rb (working copy)
@@ -1072,7 +1072,7 @@
after_callback = <<-end_eval
association instance_variable_get("@#{association_name}")
- if association.respond_to?(:loaded?) &&
association.loaded?
+ if (association.respond_to?(:loaded?) &&
association.loaded?) || (@new_record_before_save &&
association.respond_to?(:any?) && association.any?)
if @new_record_before_save
records_to_save = association
else
Depending on the preference expressed here, I can work up an
appropriate ticket/patch to execute one of those options. My
preference at the moment is for either #2 or #3. (Of course I have a
vested interest in a Rails 1.2 app that depends on this behavior ;-) )
What do you guys think?
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---
Michael Koziarski
2007-Oct-10 03:28 UTC
Re: Assocation saving backward incompatibility in Rails 1.2.4
On 10/10/07, Bryan Helmkamp <brynarylists@gmail.com> wrote:> > Today I ran across test failures in my app when upgrading from Rails > 1.2.3 to 1.2.4. I determined the issue is caused by r7076 (http:// > dev.rubyonrails.org/changeset/7076) which tries to save associated > records if and only if the association is already loaded. Here is a > simple test case that passes in Rails 1.2.3 and fails in Rails 1.2.4: > > class Computer < ActiveRecord::Base > has_many :change_logs > > before_save do |computer| > computer.change_logs.create(:message => "Computer created!") if > computer.new_record? > end > end > > class ChangeLog < ActiveRecord::Base > end > > def test_save_on_parent_saves_child_even_when_not_loaded > computer = Computer.create! > assert_equal "Computer created!", computer.change_logs.first.message > end > > Upon further investigation I discovered r7511 (http:// > dev.rubyonrails.org/changeset/7511) which adds a specific check and > exception when a user attempts to use the behavior that r7076 > eliminated. That changeset was never backported to the 1.2 branch. > > I see three options for resolving this: > > 1. Backport r7511 to the 1.2 branch. This would give users appropriate > feedback if they trip over this behavior, but still break backwards > compatibility for AR.I''d prefer not to do this.> 2. Revert r7076 to preserve full backwards compatibility in the 1.2 > branch.The performance implications can be considerable, so lets try and avoid this too.> 3. Attempt to find a solution that addresses the concern in ticket > #8713 (http://dev.rubyonrails.org/ticket/8713) while still maintaining > backward compatibility with Rails 1.2.3. To this end, I have been > toying with the following patch which passes all existing AR tests:This approach seems most promising> Depending on the preference expressed here, I can work up an > appropriate ticket/patch to execute one of those options. My > preference at the moment is for either #2 or #3. (Of course I have a > vested interest in a Rails 1.2 app that depends on this behavior ;-) ) > > What do you guys think?A patch for the 3rd option would be the best bet, assuming the current behaviour makes sense. Can you whip up a patch for stable which has a failing test? -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---