Don't update a child destroyed via relation

[brendon]

@swanandp, I've cobbled together a solution. It's actually largely inspired from the Rails code where they don't update the counter cache if they know the child is being deleted via its parent's dependent: :destroy.

I've become a bit stuck in how to test it. I want to test that the callback method either is or isn't called but apparently this isn't very straightforward in MiniTest. Do you have any suggestions?

@scottsherwood can you bundle this branch in your application and see if it works for you? If you're not the right Scott Sherwood, please let me know and I'll try the other one :)

[brendon]

It's failing on Rails 3.2 due to them not having the right functions. I'll fix that up later.

[swanandp]

Thanks @brendon , will go through this today

[swanandp]

Looks good.

[brendon]

Thanks @swanandp, have you had any experience with testing wether a callback is called or not with MiniTest?

[swanandp]

No, I mostly test side effects in such cases.

[brendon]

Unfortunately there is no detectable side effect in this case because wether the item's siblings are shuffled or not can't be detected after the whole destroy process has run its course (since it happens as part of the callbacks). I'll keep investigating.

[brendon]

Managed to fix things up a bit so that callback order isn't relied upon anymore. Also, we're now testing that the callback is called/not called via a mock. In Rails 3.2 we always shuffle because the functionality of knowing wether the parent initiated the destroy doesn't exist.

[brendon]

@scottsherwood, all tests are passing now so I'm happy to merge this. Let me know once you've done your test this week and we can go from there :)

[scottsherwood]

Hi @brendon

Sorry for the delay in checking this, after bundling commit 62f0a71 into my app, this is what I found:

If you have two models involved

TodoList -> HAS MANY -> TodoItem (The acts_as_list is on this final TodoItem class scoped by TodoList, like in your mocha tests)

I my app I actually found that it reduced the sql queries down by 3 including the one which re-set the position. Which can easily cause mysql deadlocks due to trying to delete and update at the same time. So this works great :)

If you have an 3 models involved

TodoList -> HAS MANY -> TodoSection -> HAS MANY -> TodoItem ( The acts_as_list is on this final child class scoped by TodoSection)

If you called TodoList.destroy when it gets down the chain to destroy the TodoItem, it does still do the position update sql before deleting the TodoItem.

So as it is, there are some good efficiency gains here, but there are more to be had if wanted. (and if its possible to detect the destroy further up the chain)

I've got the raw sql output should you want to see this. (Please just drop me an email.)

Scott

[brendon]

Hi @scottsherwood, no worries about the delay. Turns out the testing isn't working. When callbacks are executed, new objects are created for the children representing the records in the database. Those objects differ from the ones that I've already loaded into instance variables in the tests so I don't think there's a way to detect wether the callbacks are run as I can't see a way to hook into the objects that the destroy process is creating.

Testing that a method is never invoked gives a false positive in this case because of course the other object never received the method call.

Interesting about the grandparent problem. Theoretically it should work provided you're dependent: :destroying through both relationships. Can you check this for me? I assume you are since the records are getting deleted anyway. I'll look into it more here.

[brendon]

Hi @scottsherwood, upon further digging, I created a test that simulated the grandparent relationship with dependent: :destroy from grandparent -> parent -> child; so on both of those arrows and it's functioning as expected (no extra shuffling SQL):

D, [2017-03-13T10:06:54.918299 #9851] DEBUG -- :    (0.1ms)  begin transaction
D, [2017-03-13T10:06:54.937998 #9851] DEBUG -- :   DestructionTodoList Load (0.2ms)  SELECT "destruction_todo_lists".* FROM "destruction_todo_lists" WHERE "destruction_todo_lists"."destruction_todo_list_super_id" = ?  [["destruction_todo_list_super_id", 1]]
D, [2017-03-13T10:06:54.940498 #9851] DEBUG -- :   DestructionTodoItem Load (0.2ms)  SELECT "destruction_todo_items".* FROM "destruction_todo_items" WHERE "destruction_todo_items"."destruction_todo_list_id" = ?  [["destruction_todo_list_id", 1]]
D, [2017-03-13T10:06:54.941754 #9851] DEBUG -- :   SQL (0.2ms)  DELETE FROM "destruction_todo_items" WHERE "destruction_todo_items"."id" = ?  [["id", 1]]
D, [2017-03-13T10:06:54.942326 #9851] DEBUG -- :   SQL (0.0ms)  DELETE FROM "destruction_todo_items" WHERE "destruction_todo_items"."id" = ?  [["id", 2]]
D, [2017-03-13T10:06:54.942832 #9851] DEBUG -- :   SQL (0.0ms)  DELETE FROM "destruction_todo_items" WHERE "destruction_todo_items"."id" = ?  [["id", 3]]
D, [2017-03-13T10:06:54.944828 #9851] DEBUG -- :   DestructionTadaItem Load (0.2ms)  SELECT "destruction_tada_items".* FROM "destruction_tada_items" WHERE "destruction_tada_items"."destruction_todo_list_id" = ?  [["destruction_todo_list_id", 1]]
D, [2017-03-13T10:06:54.946702 #9851] DEBUG -- :   SQL (0.1ms)  DELETE FROM "destruction_tada_items" WHERE "destruction_tada_items"."id" = ?  [["id", 1]]
D, [2017-03-13T10:06:54.947317 #9851] DEBUG -- :   SQL (0.0ms)  DELETE FROM "destruction_tada_items" WHERE "destruction_tada_items"."id" = ?  [["id", 2]]
D, [2017-03-13T10:06:54.947817 #9851] DEBUG -- :   SQL (0.0ms)  DELETE FROM "destruction_tada_items" WHERE "destruction_tada_items"."id" = ?  [["id", 3]]
D, [2017-03-13T10:06:54.948504 #9851] DEBUG -- :   SQL (0.1ms)  DELETE FROM "destruction_todo_lists" WHERE "destruction_todo_lists"."id" = ?  [["id", 1]]
D, [2017-03-13T10:06:54.949114 #9851] DEBUG -- :   SQL (0.1ms)  DELETE FROM "destruction_todo_list_supers" WHERE "destruction_todo_list_supers"."id" = ?  [["id", 1]]
D, [2017-03-13T10:06:54.949335 #9851] DEBUG -- :    (0.1ms)  commit transaction
D, [2017-03-13T10:06:54.950315 #9851] DEBUG -- :    (0.1ms)  DROP TABLE "destruction_todo_list_supers"
D, [2017-03-13T10:06:54.950583 #9851] DEBUG -- :    (0.2ms)  DROP TABLE "destruction_todo_lists"
D, [2017-03-13T10:06:54.950832 #9851] DEBUG -- :    (0.1ms)  DROP TABLE "destruction_todo_items"
D, [2017-03-13T10:06:54.951074 #9851] DEBUG -- :    (0.1ms)  DROP TABLE "destruction_tada_items"

I'm hoping it's just an implementation detail on your end that's stopping it from working. Can you show me the acts_as_list declarations? You need to be using the symbol or array method for specifying the scope. If you use anything else (e.g. a string) then we can't detect wether the foreign_key of the parent relationship matches the string scope (it could be some complex SQL).

If you're using a string to specify a non-id scope then you can also do this: acts_as_list scope: [:my_non_id_scope].

[brendon]

Ok, I've settled that we can't easily test if the callbacks are called in the dependent situation, but we can test at least that they are still called in the individual destruction situation which is the important case. Whether they are or not otherwise isn't too important other than in terms of extra SQL issued.

I'm ready to merge once @scottsherwood confirms his info RE the grandparents.

[brendon]

@scottsherwood, I'm happy with the testing for this one now. Let me know if you still have trouble. I've merged it into master and will release a new version once I hear from you :)

[scottsherwood]

@brendon I've looked into this further and confirm that the issue with the grandparent is on my-side and the updates look great.

Thanks (I will drop you an email now)

[brendon]

@scottsherwood, I've just released 0.9.3 with this change in it.