#4521 HABTM: ability to link and unlink records with :autosave => true - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4521 new

HABTM: ability to link and unlink records with :autosave => true

Reported by Lawrence Pit | May 2nd, 2010 @ 10:19 AM | in 3.0.6

Attached patch for rails 3 allows to link and unlink records from a HABTM relation with autosave. Meaning the association updates will only be reflected in the database when the parent is saved.

Summary of new possibilities:

   class Pirate < ActiveRecord::Base
     has_and_belongs_to_many :parrots, :autosave => true
   end

   # Query existing situation
   @pirate = Pirate.first
   assert @pirate.parrots == [@parrot1, @parrot2]

   # Add some and delete one
   @pirate.parrots << [@parrot3, @parrot4]
   @pirate.parrots.delete(@parrot1)
   assert @pirate.parrots == [@parrot2, @parrot3, @parrot4]

   # The parrots are not saved to the database yet:
   assert Pirate.first.parrots == [@parrot1, @parrot2]

   # check validity of @pirate and @pirate.parrots
   # if not valid, no worries, nothing happened in the db yet

   @pirate.save!

   # And now they are saved:
   assert Pirate.first.parrots == [@parrot2, @parrot3, @parrot4]

Note: I plan to submit a patch for has_many relations as well.

habtm_autosave.diff 19.8 KB

Comments and changes to this ticket

Eloy Duran May 2nd, 2010 @ 11:38 AM
- Assigned user set to “José Valim”
José Valim May 2nd, 2010 @ 11:48 AM
- Milestone cleared.
Hey Lawrence, thanks for the patch! Just two questions:

1) Can you please describe what is the current behavior? Today it automatically saves at the moment we add or delete the record?

2) Is there a need to create a new association class? Because we would likely need another one for has_many and possibly another one for has_many :through. Ideally, the autosave behavior should be just a module that we should include/extend in an autosave association.
You flagged this item as spam.
Lawrence Pit May 2nd, 2010 @ 11:27 PM
- Assigned user cleared.
1) The current behavior, with or without :autosave => true, is that when you add or delete a record to a collection association, the link is immediately saved to the database, if the owner of the association already exists in the database.

Related is the method +accepts_nested_attributes_for+, which sets :autosave => true automatically on the collection association. Although I believe the implementation of this for habtm associations is wrong (I mean unwanted), I have not changed that behavior with this patch. The behaviour of +accepts_nested_attributes_for+ is that you are able to modify and destroy the record on the other end of the association, nothing happens to the links between those records and the owner of the association.

2) I thought about that, but then I question why has_many :through is a separate association class, and not just a module? Furthermore, I think it's even correct that there would be a separate one for has_many and one for has_many :through. I imagine the behavior for each is distinctly different, which cannot be captured by one module.
You flagged this item as spam.
Lawrence Pit May 3rd, 2010 @ 12:53 AM
Updated version with extra tests to show association target isn't loaded when not necessary.
- habtm_autosave.diff 28 KB
You flagged this item as spam.
Lawrence Pit May 3rd, 2010 @ 03:03 AM
And another updated version. Now without an extra association class (and without an extra module). This version also supports linking / unlinking of records to has_many relations!
You flagged this item as spam.
Lawrence Pit May 3rd, 2010 @ 04:12 AM
- no changes were found...
- autosave_link_unlink.diff 18.9 KB
José Valim May 3rd, 2010 @ 08:46 AM
- Assigned user set to “Jeremy Kemper”
Assigning to Jeremy Kemper since he is definitely more familiar with AR code.

Tyler Rick May 19th, 2010 @ 11:59 PM

I think it could be nice to see something like this added. I wrote a plugin that did something similar a long time ago (http://github.com/TylerRick/has_and_belongs_to_many_with_deferred_save ), but Lawrence's implementation is much better. I haven't actually tried out these patches yet, but at least from reading through them, they look fantastic.

Question about AutosaveAssociation

1) The current behavior, with or without :autosave => true, is that when you add or delete a record to a collection association, the link is immediately saved to the database, if the owner of the association already exists in the database.

That sounds correct. But AutosaveAssociation also has mark_for_destruction:

  # Destroying one of the associated models members, as part of the parent's
  # save action, is as simple as marking it for destruction:
  #
  #   post.comments.last.mark_for_destruction
  #   post.comments.last.marked_for_destruction? # => true
  #   post.comments.length # => 2
  #
  # Note that the model is _not_ yet removed from the database:
  #   id = post.comments.last.id
  #   Comment.find_by_id(id).nil? # => false
  #
  #   post.save
  #   post.reload.comments.length # => 1
  #
  # Now it _is_ removed from the database:
  #   Comment.find_by_id(id).nil? # => true

Couldn't we simply use that instead of keeping a separate pending_link_deletions array? What's the difference?

When used with accepts_nested_attributes_for, doesn't the :autosave behavior already not do any inserts, deletes, or updates to nested/associated models until you call save on the parent? Can't we use that somehow?

Different semantics?

It looks like you're adding on some new behavior (to handle the case where we add a new, not-already-associated object to the association collection, or delete a member from that association) to the existing :autosave => true option (which I believe only handled the case of auto-saving (updating) changes made to existing, already-associated associated objects when you call save before).

My next question is, should the existing :autosave option be extended with this new behavior or should a new option with a different name be used instead?

Is the purpose of this new addition related closely enough to the existing :autosave purpose/behavior to warrant them both using the same option?
Would we ever want to enable one behavior but not the other?

This, IIRC, is the current behavior (and the purpose/primary use case for) :autosave:

:autosave => false (default) :autosave => true

	`:autosave => false` (default)	`:autosave => true`
Modifying existing associated objects when you call save)	`post = Post.first post.comments[0].subject # => 'Old' post.comments[0].subject = 'New' post.save post.reload post.comments[0].subject # => 'Old' # Changes were not saved -- at all`	`post = Post.first post.comments[0].subject # => 'Old' post.comments[0].subject = 'New' post.save post.reload post.comments[0].subject # => 'New' # Changes were saved`
Destroy members marked for destruction, when saving the parent object	Not applicable?	`post.comments.last.mark_for_destruction post.comments.last.marked_for_destruction? # => true post.comments.length # => 2 # The model is not yet removed from the database post.save # Now it is removed post.reload.comments.length # => 1`

Modifying existing associated objects when you call save)

post = Post.first
post.comments[0].subject # => 'Old'
post.comments[0].subject = 'New'
post.save
post.reload
post.comments[0].subject # => 'Old'
# Changes were not saved -- at all

post = Post.first
post.comments[0].subject # => 'Old'
post.comments[0].subject = 'New'
post.save
post.reload
post.comments[0].subject # => 'New'
# Changes were saved

Destroy members marked for destruction, when saving the parent object

Not applicable?

post.comments.last.mark_for_destruction
post.comments.last.marked_for_destruction? # => true
post.comments.length # => 2
# The model is not yet removed from the database
post.save
# Now it is removed
post.reload.comments.length # => 1

And this I think is the proposed behavior to be added:

	Disabled (default)	Enabled
Adding a new, previously-not-associated object (existing or new_record doesn't matter) to an association collection	`post = Post.first post.comments.count # => 1 post.comments << [@new_comment1, @new_comment2] # The new comments are saved to the database IMMEDIATELY post.reload post.comments.count # => 3`	`post = Post.first post.comments.count # => 1 post.comments << [@new_comment1, @new_comment2] # The new comments are not saved to the database yet post.save post.reload post.comments.count # => 3`
	Disabled (default)	Enabled
Deleting a member from that association	`post = Post.first post.comments.count # => 3 post.comments.delete(post.comments[1]) # The comment is deleted from the database IMMEDIATELY [? -- not tested] post.reload post.comments.count # => 2`	`post = Post.first post.comments.count # => 3 post.comments.delete(post.comments[1]) # The comment is not deleted from the database yet post.reload post.comments.count # => 3`

To summarize, I find that :autosave => false and :autosave => true make sense to describe the existing difference in behavior...

Enabled	Disabled
Changes are not saved -- at all -- even when you call save on the parent object	Changes are saved -- but only when you call save on the parent object

...but somewhat unintuitive to describe the proposed new meaning:

Disabled	Enabled
The new objects are saved to the database (linked/associated with parent_object) IMMEDIATELY when added to association collection	The new objects are only saved to the database (linked/associated with parent_object) when you call save on the parent

It seems to me like we're doing the exact opposite of automatically saving in this case -- we're actually deferring the automatic save (that would normally happen) until later on.

The "enabled" column of both tables is similar enough, and I can sort of see why the name might make sense for that.
But in the "disabled" column of the one table, we're not saving at all and in the other, we're saving immediately.

It's not intuitive that :autosave => false would mean "save immediately".

And the docs (at least the 2.3.5 docs -- couldn't find 3.0 docs) say:

Unless you set the :autosave option on a has_one, belongs_to, has_many, or has_and_belongs_to_many association [sentence incomplete]. Setting it to true will always save the members, whereas setting it to false will never save the members.

So, a different name?

Here are some ideas...

:deferred_save => true
:save => :when_parent_saved
:dont_link_records_until_parent_saved => true

Or...

Rename :autosave => false to :save_existing => false
Rename :autosave => true to :save_existing => :when_parent_saved
Add :save_new_links => :immediately (used for unlinks too) and :save_new_links => :when_parent_saved
(Yuck! I know...)

A tiny change to a test

  def test_should_have_pending_link_creations
+   assert_equal 2, @pirate.send(@association_name).count
    @pirate.send(@association_name) << [@new_child_1, @new_child_2]
    assert_equal [@new_child_1, @new_child_2], @pirate.send(@association_name).pending_link_creations
    @pirate.reload
    assert_equal 2, @pirate.send(@association_name).count
  end

I would suggest adding the first assert_equal so we can see what it is before we start making changes (and see that it does not end up changing). (When I first read the test, I was assuming incorrectly that it started out at 0 and was changed to 2 when [@new_child_1, @new_child_2] were added, but I was wrong since the setup method adds 2 records before we get here.)

Tiny question about accepts_nested_attributes_for

Can you clarify what you mean by "nothing happens to the links between those records and the owner of the association" when you said:

The behaviour of accepts_nested_attributes_for is that you are able to modify and destroy the record on the other end of the association, nothing happens to the links between those records and the owner of the association.

What could or should happen to those links? Your patch wouldn't even change anything about the way accepts_nested_attributes_for works, would it? (Does accepts_nested_attributes_for even work with habtm? I don't recall.)

You flagged this item as spam.
Lawrence Pit May 20th, 2010 @ 05:51 AM
Tyler, thanks for your input. Below some answers.

mark_for_destruction

+mark_for_destruction+ sets an instance var on a model instance, so it would destroy the instance. We don't want to destroy the instance, we only want to break the link between two model instances.

For example, suppose a pirate has many books and has many favorite_books. Suppose Book X is in both collections. If you want to remove Book X from the favorite_books collection, but not from the books collection, then you cannot use +mark_for_destruction+ on the Book X object, because a) you don't want it to disappear from both collections and b) you don't want to actually destroy Book X.

Technically speaking you'd want to do +mark_for_destruction+ on the object that sits between two model instances, in case of a habtm. However, such an object does not really exist in the rails implementation of habtm. Hence the need for +pending_link_deletions+.

Note the subtle difference in case of a has_many. Suppose a Pirate has_many :birds, :autosave => true, and one of the birds in that collection is Tweety. The current behaviour is that you can call +mark_for_destruction+ on the Tweety object, and the bird will be killed when you call pirate.save. However, if you only want to release the bird without killing it, and defer this action until you call pirate.save, then this is currently not possible. The patch I've attached does make this possible.

Note that the +pending_link_deletions+ and +pending_link_creations+ are extremely useful for validation purposes and for re-rendering views in a correct state in case validation fails.

accepts_nested_attributes_for

+accepts_nested_attributes_for+ only CRUDs model instances. It does not CRUD links between model instances, it happens as a side effect when required. Specifically in the case of habtm I believe its implementation is flawed: it will update the attributes on the other end of the habtm relation or even delete the model instance on the other end. I don't think anyone would ever want to do that. What you want to do is link or unlink to/from a model instance on the other end, not modify it's attributes or destroy it.

intention of :autosave

My next question is, should the existing :autosave option be extended with this new behavior or should a new option with a different name be used instead?

A very good question. I believe the intention is the same. :autosave is really about deferring a save until the parent object is saved. That's exactly what I want. However, not only do I want to defer saves of changes to attributes on the other end of a has_many for example, but also I want to defer saves of changes made to what is and what is not included in an association collection.

rename :autosave ?

It seems to me like we're doing the exact opposite of automatically saving in this case -- we're actually deferring the automatic save (that would normally happen) until later on. [...] It's not intuitive that :autosave => false would mean "save immediately".

I totally agree. I always found the term "autosave" confusing. It's about deferring the save until the parent is saved.

:deferred_save => true

That would be my preference. But I leave this renaming exercise to another ticket.

Clarify test

Good suggestion to clarify the test. Will do.

changes to accepts_nested_attributes_for ?

What could or should happen to those links?

What I mean to explain is that accepts_nested_attributes_for does not allow you to link to an existing object nor does it allow to unlink from an existing object without destroying it.

The issue is however not with accepts_nested_attributes_for. I view accepts_nested_attributes_for as a sort of macro on top of the :autosave => true feature. I don't really need accepts_nested_attributes_for if all I want is deferred saves. Especially because accepts_nested_attributes_for is geared towards being a helper method for HTML form submissions, but it's unsuitable for e.g. XML or JSON submissions imo.

Your patch wouldn't even change anything about the way accepts_nested_attributes_for works, would it?

My patch changes nothing about the way accepts_nested_attributes_for works. All tests for accepts_nested_attributes_for are untouched.

Note that when this patch is accepted it would be logical to add something like accepts_nested_attributes_for that works for linking/unlinking objects. That too is for a different ticket.

(Does accepts_nested_attributes_for even work with habtm? I don't recall.)

It does, but as mentioned previously, it's unusable in its current state imo.
You flagged this item as spam.
Lawrence Pit June 8th, 2010 @ 05:24 AM
- Tag changed from “activerecord 3.0, rails3” to “activerecord 3.0, patch, rails3”
Jeremy Kemper August 30th, 2010 @ 04:10 AM
- Milestone cleared.
[bulk edit]
Ryan Bigg October 9th, 2010 @ 10:11 PM
- Tag cleared.
- Importance changed from “” to “Low”
Automatic cleanup of spam.
Ryan Bigg October 11th, 2010 @ 12:12 PM
Automatic cleanup of spam.
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]
Ryan Bigg October 21st, 2010 @ 03:35 AM
Automatic cleanup of spam.
Santiago Pastorino November 15th, 2010 @ 09:04 PM
- Milestone cleared.
[bulk edit]
Santiago Pastorino February 12th, 2011 @ 09:58 PM
- Milestone set to “3.0.5”
[bulk edit]
Santiago Pastorino February 27th, 2011 @ 03:15 AM
- Milestone changed from “3.0.5” to “3.0.6”
[bulk edit]
You flagged this item as spam.
klkk May 23rd, 2011 @ 02:54 AM
www.louisvuittonwarehouse.com

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>

Shared Ticket Bins (Sort)

↓↑ drag 455 Open Bugs
↓↑ drag 0 Recent Open Tickets
↓↑ drag 87 Open Patches
↓↑ drag 0 Open Doc Patches
↓↑ drag 542 Stale Tickets
↓↑ drag 3 Verified Patches
↓↑ drag 87 Stale Patches
↓↑ drag 0 Yesterday's Tickets
↓↑ drag 0 Two Days Ago
↓↑ drag 0 This Week
↓↑ drag 0 Last Week
↓↑ drag 7 Bugmash
↓↑ drag 0 Bugmash review queue
↓↑ drag 5 Rails 3 High Priority Tickets

Rails Ruby on Rails → 3.0.6

HABTM: ability to link and unlink records with :autosave => true

Comments and changes to this ticket

Eloy Duran May 2nd, 2010 @ 11:38 AM

José Valim May 2nd, 2010 @ 11:48 AM

Lawrence Pit May 2nd, 2010 @ 11:27 PM

Lawrence Pit May 3rd, 2010 @ 12:53 AM

Lawrence Pit May 3rd, 2010 @ 03:03 AM

Lawrence Pit May 3rd, 2010 @ 04:12 AM

José Valim May 3rd, 2010 @ 08:46 AM

Tyler Rick May 19th, 2010 @ 11:59 PM

Question about AutosaveAssociation

Different semantics?

So, a different name?

A tiny change to a test

Tiny question about accepts_nested_attributes_for

Lawrence Pit May 20th, 2010 @ 05:51 AM

mark_for_destruction

accepts_nested_attributes_for

intention of :autosave

rename :autosave ?

Clarify test

changes to accepts_nested_attributes_for ?

Lawrence Pit June 8th, 2010 @ 05:24 AM

Jeremy Kemper August 30th, 2010 @ 04:10 AM

Ryan Bigg October 9th, 2010 @ 10:11 PM

Ryan Bigg October 11th, 2010 @ 12:12 PM

Jeremy Kemper October 15th, 2010 @ 11:01 PM

Ryan Bigg October 21st, 2010 @ 03:35 AM

Santiago Pastorino November 15th, 2010 @ 09:04 PM

Santiago Pastorino February 12th, 2011 @ 09:58 PM

Santiago Pastorino February 27th, 2011 @ 03:15 AM

klkk May 23rd, 2011 @ 02:54 AM

Create your profile

Shared Ticket Bins (Sort)

People watching this ticket

Attachments

Pages

Rails Ruby on Rails → 3.0.6

Keyword searching

HABTM: ability to link and unlink records with :autosave => true

Comments and changes to this ticket

Question about AutosaveAssociation

Different semantics?

So, a different name?

A tiny change to a test

Tiny question about accepts_nested_attributes_for

mark_for_destruction

accepts_nested_attributes_for

intention of :autosave

rename :autosave ?

Clarify test

changes to accepts_nested_attributes_for ?

Create your profile

Shared Ticket Bins (Sort)

People watching this ticket

Attachments

Pages