This project is archived and is in readonly mode.

#4521 new
Lawrence Pit

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.

Comments and changes to this ticket

  • Eloy Duran

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

    • Assigned user set to “José Valim”
  • 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.

  • Lawrence Pit

    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.

  • Lawrence Pit

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

    Updated version with extra tests to show association target isn't loaded when not necessary.

  • Lawrence Pit

    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!

  • Lawrence Pit
  • José Valim

    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

    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?

    1. Is the purpose of this new addition related closely enough to the existing :autosave purpose/behavior to warrant them both using the same option?
    2. 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

    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.)

  • Lawrence Pit

    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.

  • Lawrence Pit

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

    • Tag changed from activerecord 3.0, rails3 to activerecord 3.0, patch, rails3
  • Jeremy Kemper
  • Ryan Bigg

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

    • Tag cleared.
    • Importance changed from “” to “Low”

    Automatic cleanup of spam.

  • Ryan Bigg

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

    Automatic cleanup of spam.

  • Jeremy Kemper

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

    • Milestone set to 3.0.2
  • Ryan Bigg

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

    Automatic cleanup of spam.

  • Santiago Pastorino
  • Santiago Pastorino
  • Santiago Pastorino

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

    • Milestone changed from 3.0.5 to 3.0.6
  • klkk

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>

Pages