This project is archived and is in readonly mode.

#2100 ✓stale
Kenneth Kalmer

Change behaviour of association's after_add callback

Reported by Kenneth Kalmer | February 28th, 2009 @ 03:18 PM | in 3.x

The current behavior of ActiveRecord's association callback "after_add" is to be called irrespective of the association getting persisted or not.

As discussed with Eloy on the core list (http://groups.google.com/group/r... I set out to change the behavior so that after_add only gets called when the association is persisted to the database.

The associated path solves the issue, with a lot of extra (possibly redundant) tests to make sure the hole stays plugged.

Comments and changes to this ticket

  • Kenneth Kalmer

    Kenneth Kalmer February 28th, 2009 @ 03:21 PM

    Argh, one failing test remains. Will add new patch in a bit.

  • Kenneth Kalmer

    Kenneth Kalmer February 28th, 2009 @ 05:01 PM

    Fixes the broken has_many :through test cases with association callbacks.

    This did however bring up another issue: (ActiveRecord tests)

    
    ted = post.people_with_callbacks.build(:first_name => "Ted")
    assert_equal [
      [:added, :after, "Lary"],
      [:added, :before, "Ted"]
    ], log.last(2)
        
    ted.save
    assert_equal [
      [:added, :before, "Ted"],
      [:added, :after, "Ted"]
    ], log.last(2)
    # WARNING: This is flawed!?
    assert post.people_with_callbacks.include?( ted )
    

    All but the last assertion pass, which is confusing. From the the second assertion we would assume a join model instance was saved because the callback was called, but looking closer it is in fact not the case.

  • Eloy Duran

    Eloy Duran March 1st, 2009 @ 12:47 PM

    • Assigned user changed from “Eloy Duran” to “Michael Koziarski”
  • Pratik

    Pratik March 9th, 2009 @ 01:03 PM

    • Assigned user changed from “Michael Koziarski” to “Pratik”
    • Title changed from “[PATCH] Change behaviour of association's after_add callback” to “Change behaviour of association's after_add callback”
  • Nathaniel Bibler

    Nathaniel Bibler May 14th, 2009 @ 07:32 PM

    I'm not sure whether or not his patch addresses this concern, but I'd rather bring it up here rather than start a new ticket:

    It is highly unexpected that the following triggers an association callback:

    class Blog
      has_many :posts, :after_add  => :do_something
    
      def do_something(post)
        post.profit!
      end
    end
    
    blog.posts.create(:title => "I'm an in-valid? post")
    

    This example is a bit contrived, but when creating an invalid child object through an association which carries an add callback, the callback is fired regardless of whether or not the child is valid (and therefor persisted).

    The current implementation of these call backs requires some variation of the following to work as expected:

    class Blog
      def do_something(post)
        post.profit! unless post.new_record?
      end
    end
    

    Was this intended? And if so, what's the point of being notified when an invalid object was attempted to be included in an associated collection? Obviously it's not being persisted (which is to the point of this ticket), so ... ???

  • Michael Koziarski

    Michael Koziarski May 15th, 2009 @ 04:25 AM

    Invalid associated objects are still added to the association in memory. So I suppose a case can be made the after_add should still be fired.

  • Nathaniel Bibler

    Nathaniel Bibler May 15th, 2009 @ 04:07 PM

    I would agree that there could be cases for wanting to execute a callback after any element is added to the association, regardless of whether or not it is actually valid or persisted. Although, honestly, the only fair use I can think of for that functionality is basically logging bad attempts.

    It would be terrible form to try to catch an object after it was submitted to the association, check for validity, then if invalid .. modify the object and try again. I guess you could potentially do something crazy like .. if the object isn't really supposed to be in the original association, you could - by means of this callback - attempt to throw it into a different association. But again, these are things you should never do in practice. ;)

    I don't want to start an argument about it - because really, it's fairly insignificant - so I'll probably end the "ranting" with this post just saying: My feeling is that if it's called after_add or after_remove it should be after the element is actually added. If the object is invalid .. it certainly isn't actually added to the association. So, maybe it's just a disconnect in the current naming of the callback. The current after_add functionality speaks more to what I would expect from a before_add callback, meaning I get the ability to manipulate the object, regardless of state; granted the current functionality doesn't then attempt to insert it afterward.

    So, maybe the shortest distance is to just fill out the current documentation on the subject (see "Association callbacks") in a more meaningful way.

  • Michael Koziarski

    Michael Koziarski May 16th, 2009 @ 12:37 AM

    Take a shot at filling out the documentation a bit and we'll see what
    the final result looks like. If the documentation reads too much like
    a justification for a bad decision, we can look at reversing that
    decision :)

  • Pratik

    Pratik May 17th, 2009 @ 05:42 PM

    • Assigned user changed from “Pratik” to “Michael Koziarski”
  • Nathaniel Bibler

    Nathaniel Bibler May 17th, 2009 @ 05:55 PM

    As soon as I'm done with what I'm working on, I'll patch the documentation and post it here.

    BTW: I apologize to Kenneth for hijacking this ticket. :-\ Certainly not my intention. I'm discontinuing posting to this thread and have created ticket #2664.

  • CancelProfileIsBroken

    CancelProfileIsBroken August 6th, 2009 @ 02:42 PM

    • Tag changed from associations, callbacks, patch to associations, bugmash, callbacks, patch
  • Brennan Dunn

    Brennan Dunn August 18th, 2009 @ 01:50 AM

    I've unintentionally spawned another thread on the core list dealing with this issue: http://groups.google.com/group/rubyonrails-core/browse_thread/threa...

    +1 to either modifying the documentation to be more explicit, or changing the way association callbacks are performed.

  • Lawrence Pit

    Lawrence Pit September 28th, 2009 @ 01:43 AM

    I don't think the behaviour of before_add/after_add/before_remove/after_remove should be changed, even though many times I was tempted to do the same.

    Instead I propose 4 extra callbacks that do what this ticket requests: before_create/after_create/before_destroy/after_destroy.

    The add/remove callbacks are related to additions/removals from the collection in memory, the create/destroy callbacks are related to additions/removals from the collection in the datastore.

    I filed a ticket earlier for this: see #3277

  • Rizwan Reza

    Rizwan Reza February 12th, 2010 @ 12:46 PM

    • Tag changed from associations, bugmash, callbacks, patch to associations, callbacks, patch
  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:54 PM

    • State changed from “new” to “open”
    • Importance changed from “” to “”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:54 PM

    • State changed from “open” to “stale”
  • Michael Kintzer

    Michael Kintzer February 2nd, 2011 @ 06:52 PM

    Reproduces with Rails 3.0.3.

    +1 for modifying after_add to not get called if the added_record is not persisted, or for adding an alternative callbacks for 'after_persisted' or 'after_create' which would guarantee a valid, persisted record.

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>

Referenced by

Pages