Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #8713 (closed defect: fixed)

Opened 1 year ago

Last modified 1 month ago

ActiveRecord associations auto-load implicitly on save.

Reported by: blaine Assigned to: david
Priority: high Milestone: 1.2.7
Component: ActiveRecord Version: 1.2.6
Severity: major Keywords: associations
Cc:

Description

This problem is a serious defect in the ActiveRecord association after_save callback chain. A simple conditional bug meant that Rails passively loads assocations that have not been loaded before the save() call.

The impact of this bug will vary on a per-application basis; on the database side, this bug will manifest as a full select from the association without any limits. For a has_many association, this means:

SELECT * FROM association_table WHERE fkey_id = self.id

When your association includes thousands of records, this can be an extremely painful operation (never mind spuriously loading the corresponding ActiveRecord objects and calling save on each of them in turn).

Attachments

prevent_associations_from_autoloading_on_save.patch (1.5 kB) - added by blaine on 06/21/07 20:25:34.
test and fix for assocation loading on save bug.
always_construct_new_sql_on_save.patch (1.9 kB) - added by brynary on 10/16/07 06:33:03.
0001-Fix-regression-in-r7076-to-save-built-records-on-an.patch (3.0 kB) - added by shoe on 02/25/08 00:24:12.
test and fix for regression to save records built on unloaded association
test_for_edge.patch (1.4 kB) - added by shoe on 02/25/08 00:28:20.
add a test_save_without_validations_after_build_without_loading_association

Change History

06/21/07 20:25:34 changed by blaine

  • attachment prevent_associations_from_autoloading_on_save.patch added.

test and fix for assocation loading on save bug.

06/21/07 20:47:30 changed by bitsweat

  • status changed from new to closed.
  • resolution set to fixed.

(In [7075]) Save associated records only if the association is already loaded. Closes #8713.

06/21/07 20:51:00 changed by bitsweat

(In [7076]) Merge [7075] to stable: save associated records only if the association is already loaded. References #8713.

06/21/07 21:20:01 changed by blaine

thanks for the quick commit! :-)

10/10/07 05:42:59 changed by nzkoz

(In [7823]) Add deprecation warning for calling .create on has_many associations with an unsaved owner.

Fix regression introduced by [7076] by restoring 1.2.3 behaviour for saving associated records [Bryan Helmkamp]. References #8713

10/10/07 06:45:16 changed by nzkoz

(In [7824]) Ensure that 'autosaving' works when associations aren't loaded [Bryan Helmkamp] References #8713

10/16/07 06:32:29 changed by brynary

  • status changed from closed to reopened.
  • resolution deleted.

[7823] introduced a regression where the association would not construct new finder SQL on save causing bogus queries for "WHERE owner_id = NULL" even after owner was saved. Patch against trunk attached.

10/16/07 06:33:03 changed by brynary

  • attachment always_construct_new_sql_on_save.patch added.

10/16/07 07:26:21 changed by bitsweat

  • status changed from reopened to closed.
  • resolution set to fixed.

[7940] 1-2-stable, [7942] trunk

10/16/07 07:35:06 changed by bitsweat

  • milestone changed from 1.x to 1.2.6.

02/25/08 00:22:13 changed by shoe

  • status changed from closed to reopened.
  • version changed from edge to 1.2.6.
  • resolution deleted.

I believe that there remains a quite serious regression from [7075] and [7076] that was not entirely fixed in [7823]/[7824] and is unrelated to the regression fixed by [7940] and [7942].

The regression is that records that are built on an association that is not loaded are not saved when the parent record is saved. Note that because the association validation callback loaded the association, the easiest way to trigger this is to save without validations.

This regression was (accidentally?) fixed on edge in [8504], but it appears the original author of [8504] only intended it as an optimization to avoid the loading of the association, without realizing that it also fixed this regression. This is probably why it wasn't considered for backport.

I've attached a patch for the stable-1-2 branch, with a test case and a fix. The style of the fix is not what I would normally do from scratch, but it most closely resembles what is currently on edge.

I will also attach a test case to apply to edge, since I couldn't find one.

02/25/08 00:24:12 changed by shoe

  • attachment 0001-Fix-regression-in-r7076-to-save-built-records-on-an.patch added.

test and fix for regression to save records built on unloaded association

02/25/08 00:28:20 changed by shoe

  • attachment test_for_edge.patch added.

add a test_save_without_validations_after_build_without_loading_association

02/25/08 00:33:18 changed by shoe

  • milestone changed from 1.2.6 to 1.2.7.

03/11/08 22:00:06 changed by shoe

  • status changed from reopened to closed.
  • resolution set to fixed.

I opened a new ticket #11329 for the regression caused by the code here, so I'm closing this back out.

05/30/08 17:57:51 changed by airforce1

thanks get a cool air force ones