New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't update a child destroyed via relation #261

Merged
merged 8 commits into from Mar 13, 2017

Conversation

Projects
None yet
3 participants
@brendon
Collaborator

brendon commented Mar 8, 2017

@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 brendon self-assigned this Mar 8, 2017

@brendon brendon requested a review from swanandp Mar 8, 2017

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 8, 2017

Collaborator

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

Collaborator

brendon commented Mar 8, 2017

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

@swanandp

This comment has been minimized.

Show comment
Hide comment
@swanandp

swanandp Mar 8, 2017

Owner

Thanks @brendon , will go through this today

Owner

swanandp commented Mar 8, 2017

Thanks @brendon , will go through this today

@swanandp

Looks good.

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 10, 2017

Collaborator

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

Collaborator

brendon commented Mar 10, 2017

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

@swanandp

This comment has been minimized.

Show comment
Hide comment
@swanandp

swanandp Mar 10, 2017

Owner

No, I mostly test side effects in such cases.

Owner

swanandp commented Mar 10, 2017

No, I mostly test side effects in such cases.

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 11, 2017

Collaborator

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.

Collaborator

brendon commented Mar 11, 2017

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 added some commits Mar 11, 2017

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 11, 2017

Collaborator

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.

Collaborator

brendon commented Mar 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 11, 2017

Collaborator

@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 :)

Collaborator

brendon commented Mar 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@scottsherwood

scottsherwood Mar 11, 2017

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

scottsherwood commented Mar 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 12, 2017

Collaborator

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.

Collaborator

brendon commented Mar 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 12, 2017

Collaborator

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

Collaborator

brendon commented Mar 12, 2017

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

We can't test if the callbacks are never called
The best we can do is test that they are called when individual items
are deleted.
@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 12, 2017

Collaborator

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.

Collaborator

brendon commented Mar 12, 2017

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 brendon merged commit 64c9438 into master Mar 13, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 13, 2017

Collaborator

@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 :)

Collaborator

brendon commented Mar 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@scottsherwood

scottsherwood Mar 13, 2017

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

scottsherwood commented Mar 13, 2017

@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 brendon deleted the detroyed-via-scope branch Mar 14, 2017

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Mar 14, 2017

Collaborator

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

Collaborator

brendon commented Mar 14, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment