Skip to content
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

fix(discriminator): apply built-in plugins to discriminator schema even if mergeHooks and mergePlugins are both false #12833

Merged
merged 4 commits into from Dec 26, 2022

Conversation

vkarpov15
Copy link
Collaborator

Fix #12696

Summary

#12696 correctly points out that if both mergeHooks and mergePlugins are false, then we don't apply built-in plugins. Which is a problem because then we don't call save hooks on subdocs when parent hooks are saved, we don't call validate before save, etc. We use plugins for some of Mongoose's core functionality for dogfooding purposes, so we need to apply our built-in plugins even if the user doesn't want to apply user-defined global plugins to discriminator schemas.

Examples

…en if `mergeHooks` and `mergePlugins` are both false

Fix #12696
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

added a suggestions to fix a lint warning

also why was clearValidating deleted, was it unused?

just to clarify, applyBuiltinPlugins is different from using mongoose._applyPlugins because that function would also apply user-defined global plugins, right?

maybe also consider replacing

mongoose/lib/index.js

Lines 111 to 118 in 2fd0d29

value: [
[saveSubdocs, { deduplicate: true }],
[validateBeforeSave, { deduplicate: true }],
[shardingPlugin, { deduplicate: true }],
[removeSubdocs, { deduplicate: true }],
[trackTransaction, { deduplicate: true }]
]
});
with Object.values(builtinPlugins).map(v => [v, { deduplicate: true }]) to have built-in plugins defined in one place instead of multiple

lib/helpers/schema/applyBuiltinPlugins.js Outdated Show resolved Hide resolved
@vkarpov15 vkarpov15 merged commit 94fbd21 into master Dec 26, 2022
@vkarpov15 vkarpov15 deleted the vkarpov15/gh-12696 branch December 26, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No global plugins applied when using mergePlugins on discriminators
2 participants