Prevent double firing the before save callback of new object when the parent association saved in the callback #28640
Conversation
|
Thanks for this fix. This behavior was driving me crazy with my test suite. |
|
I really like how neat this feels! But adding the block parameter to the public |
|
Actually we don't need to change the https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/suppressor.rb#L41-L48 But the deepest |
| @@ -29,14 +29,9 @@ def handle_dependency | |||
| end | |||
| end | |||
|
|
|||
| def insert_record(record, validate = true, raise = false) | |||
| def insert_record(record, validate = true, raise = false, &block) | |||
There was a problem hiding this comment.
Do we need to capture the block? I don't seems used.
| @@ -35,15 +35,11 @@ def concat_records(records) | |||
| records | |||
| end | |||
|
|
|||
| def insert_record(record, validate = true, raise = false) | |||
| def insert_record(record, validate = true, raise = false, &block) | |||
There was a problem hiding this comment.
Same about capturing the block
| @@ -121,8 +121,8 @@ def persisted? | |||
| # | |||
| # Attributes marked as readonly are silently ignored if the record is | |||
| # being updated. | |||
| def save(*args) | |||
| create_or_update(*args) | |||
| def save(*args, &block) | |||
There was a problem hiding this comment.
This is the public method and like @matthewd pointed it is not good to add a block to this method. Since we need it can we at least remove &block from the method signature on rdoc using the :args: directive?
kamipo
force-pushed
the
prevent_double_firing_before_save_callback
branch
from
b3ed928 to
271ed22
Compare
| @@ -100,6 +100,10 @@ def persisted? | |||
| !(@new_record || @destroyed) | |||
| end | |||
|
|
|||
| ## | |||
| # :call-seq: | |||
| # save(*args) | |||
There was a problem hiding this comment.
I tried using :args: to remove &block but didn't work at least in my local.
So I used :call-seq: instead, then it worked properly.
… parent association saved in the callback Related rails#18155, rails#26661, 268a5bb, rails#27434, rails#27442, and rails#28599. Originally rails#18155 was introduced for preventing double insertion caused by the after save callback. But it was caused the before save issue (rails#26661). 268a5bb fixed rails#26661, but it was caused the performance regression (rails#27434). rails#27442 added new record to `target` before calling callbacks for fixing rails#27434. But it was caused double firing before save callback (rails#28599). We cannot add new object to `target` before saving the object. This is improving rails#18155 to only track callbacks after `save`. Fixes rails#28599.
kamipo
force-pushed
the
prevent_double_firing_before_save_callback
branch
from
c4f5f1a to
c0038f7
Compare
|
Squashed! |
…ve_callback Prevent double firing the before save callback of new object when the parent association saved in the callback
|
Backported in 9d112c2 |
|
This is also an issue for 5-0-stable. |
|
@kamipo could you create a backport PR? It doesn't apply clearly. |
|
Sure, will do. |
…re_save_callback Prevent double firing the before save callback of new object when the parent association saved in the callback
Related #18155, #26661, 268a5bb, #27434, #27442, and #28599.
Originally #18155 was introduced for preventing double insertion caused
by the after save callback. But it was caused the before save issue
(#26661). 268a5bb fixed #26661, but it was caused the performance
regression (#27434). #27442 added new record to
targetbefore callingcallbacks for fixing #27434. But it was caused double firing before save
callback (#28599). We cannot add new object to
targetbefore savingthe object.
This is improving #18155 to only track callbacks after
save.Fixes #28599.