Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

test case to illustrate callbacks definitions and HABTM association are order dependent #8674

Closed
wants to merge 1 commit into from

5 participants

@senny
Owner

WIP

The source issue is: #8672

This PR is only a failing test case as I am not completely sure how to tackle the problem.

I investigated and can confirm that the order in which after_create and has_and_belongs_to_many associations are defined does impact the functionality.

If the callback is defined before the association every HABTM record created inside the callback will be inserted twice. The reason is AutosaveAssociation#save_collection_association. It will pick up the already created records and save them again. This happens because @new_record_before_save will be true and since it's an after_create callback, the associations are inserted directly with <<.

My test case results in the following failure:

  1) Failure:
test_after_create_before_habtm_definition(HasAndBelongsToManyAssociationsTest) [test/cases/associations/has_and_belongs_to_many_associations_test.rb:616]:
Expected: 1
  Actual: 2
@senny
Owner

@jonleighton @rafaelfranca is it known and expected that HABTM associations and callbacks are order dependent? If this is the expected behavior, feel free to close. Otherwise we should look for a possible solution.

@jonleighton
Collaborator

Definitely looks like unexpected behaviour to me.

@tehgeekmeister

No clue if I can squash this or not, but I'm starting to investigate. I'll update in here with whatever I find, at the least.

@JonRowe

Was any progress made on this?

@tehgeekmeister

I'll try to spend a little more time on it tonight, but no promises if I'll get anywhere. I was just trying to figure it out when I had to give up on it last time.

@tehgeekmeister

Status update: spent hours learning all the surrounding code, figured out that @senny is right on the money. No idea for a fix yet, but I'll give that a stab tomorrow.

@senny
Owner

I was thinking about this issue lately and it could be a tricky one to solve. Let me know if you find an angle.

@pixeltrix
Owner

@senny the problem is that AutosaveAssociation is using the public callback API to implement its functionality which means that it's subject to the definition order. There used to be a lot of these problems but most of them went away when @jonleighton refactored the association code.

The belongs_to :foo, :touch => true code is also using the callbacks api so that may be definition order dependent as well.

@tehgeekmeister

I basically came to the same conclusion as @pixeltrix. I think the best solution is to have separate callback chains for these purposes. Here's why:

  1. If we try to make save_collection_association smart enough to handle this situation, it'll be really ugly. The hack is certainly possible, but it's inexcusably ugly.
  2. If we try to force the right ordering by some sort of special case, that doesn't help at all in solving other instances where there might be problems of this sort.

The downsides I can see:

  1. By default, all callback chains are publicly exposed. If people start using the callback chains we add in their code, these sorts of problems could happen again. Then it turns into a sort of arms race for which callback chain is REALLY first or last.
  2. Aside from the public use scenario, in general proliferation of callback chains inside the same callback management code seems inelegant to me.

One final alternative that presents itself to me is to have an entirely separate, not publicly exposed, set of callback chains. Of course, this could be enforced more by code (of course, anyone can actually access them if they really want) or more by convention (documented as "don't touch unless you really know what you're doing"). A "separate set" is flexible concept, of course, so it could mean separate objects managing it, or a conceptually distinct set, managed by the same code, but distinct by convention.

I'm working on the separate by convention approach right now to see how the code turns out. If everyone is radically opposed to that, I'll stop and move on to something else.

@senny
Owner

I'm closing this one as it is just a test-case without a fix. It will remain linked in the issue.

@senny senny closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 1, 2013
  1. @senny
This page is out of date. Refresh to see the latest.
View
24 activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -38,6 +38,23 @@ def add_david
end
end
+class ProjectWithAfterCreateHookBeforeHABTM < ActiveRecord::Base
+ self.table_name = 'projects'
+
+ after_create :add_david
+
+ has_and_belongs_to_many :custom_developers,
+ :class_name => "DeveloperForProjectWithAfterCreateHook",
+ :join_table => "developers_projects",
+ :foreign_key => "project_id",
+ :association_foreign_key => "developer_id"
+
+ def add_david
+ david = DeveloperForProjectWithAfterCreateHook.find_by_name('David')
+ custom_developers << david
+ end
+end
+
class DeveloperForProjectWithAfterCreateHook < ActiveRecord::Base
self.table_name = 'developers'
has_and_belongs_to_many :projects,
@@ -592,6 +609,13 @@ def test_new_with_values_in_collection
assert project.developers.include?(david)
end
+ def test_after_create_before_habtm_definition
+ project = ProjectWithAfterCreateHookBeforeHABTM.create! :name => 'Getting things done'
+ project.save!
+
+ assert_equal 1, project.custom_developers.count
+ end
+
def test_find_in_association_with_options
developers = projects(:active_record).developers.to_a
assert_equal 3, developers.size
Something went wrong with that request. Please try again.