MONGOID-5658 Fix callbacks for embedded #5727
Conversation
| @@ -137,6 +137,16 @@ module Config | |||
| # See https://jira.mongodb.org/browse/MONGOID-5542 | |||
| option :prevent_multiple_calls_of_embedded_callbacks, default: true | |||
|
|
|||
| # When this flag is true, callbacks for embedded documents will not be | |||
| # called. This is the default in 9.0. | |||
There was a problem hiding this comment.
I feel like the name of the option might be backwards, if this is the case. I would expect around_callbacks_for_embeds = true to mean that around callbacks are invoked. If we want a true value to mean they are not invoked, perhaps the option should be called something like omit_around_callbacks_for_embeds?
There was a problem hiding this comment.
Updated, thank you!
| @@ -149,6 +149,14 @@ def run_callbacks(kind, with_children: true, skip_if: nil, &block) | |||
| # | |||
| # @api private | |||
| def _mongoid_run_child_callbacks(kind, children: nil, &block) | |||
| if Mongoid::Config.around_callbacks_for_embeds | |||
There was a problem hiding this comment.
The logic here seems to contradict the documentation for the around_callbacks_for_embeds option. Maybe the documentation just has the logic backwards?
There was a problem hiding this comment.
You are right, thank you. Adjusted the docs.
| @@ -162,6 +170,46 @@ def _mongoid_run_child_callbacks(kind, children: nil, &block) | |||
| end | |||
There was a problem hiding this comment.
(GitHub won't let me leave a comment on a line of code that's not actually part of the PR, so I'm leaving it here)
A few lines above this, _mongoid_run_child_callbacks is invoked, which is fine. However, since we already know that around callbacks for embeds are enabled, we might as well just invoke _mongoid_run_child_callbacks_with_around, and avoid adding an extra call to the stack.
There was a problem hiding this comment.
Good catch, thank you. Fixed.
lib/mongoid/interceptable.rb
Outdated
| @@ -162,6 +170,46 @@ def _mongoid_run_child_callbacks(kind, children: nil, &block) | |||
| end | |||
| end | |||
|
|
|||
| def _mondoid_run_child_before_callbacks(kind, children: [], callback_list: []) | |||
There was a problem hiding this comment.
Fixed, thank you. Mondoid sounds kind of nice, though...
There was a problem hiding this comment.
Mondoid does sound kind of nice
lib/mongoid/interceptable.rb
Outdated
| callback_list | ||
| end | ||
|
|
||
| def _mondoid_run_child_after_callbacks(callback_list: []) |
This is a band-aid for MONGOID-5658
The existing implementation of callbacks guarantees a sane order of execution; however, it causes stack overflow if there are many embeddeds (even in a flat structure). To fix this issue we suggest ignoring around callbacks for embedded documents. This allows us to patch the existing implementation of the callbacks and keep the proper order of callbacks.
If something still needs to be done around a database operation for an embedded document, it can be done in corresponding callback of the parent document.
We should discuss a necessity of callbacks for embedded documents in general. If we still feel that we should keep this functionality, the callbacks implementation in general should be revised.