-
-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(model): trigger error post hook on bulkwrite when pre-hook throws an error #15882
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes bulkWrite() to call error post hooks when a pre-hook throws an error, ensuring consistency with insertMany() behavior. The fix addresses issue #15881 where error post hooks were not being triggered when pre-hooks failed.
Key changes:
- Modified error handling in
bulkWrite()to callexecPostwith error context when pre-hooks throw - Added comprehensive test coverage for pre-hook error scenarios in both
bulkWrite()andinsertMany()
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/model.js | Updated bulkWrite() error handling to call error post hooks when pre-hooks throw, replacing .catch() chain with try-catch block |
| test/model.middleware.test.js | Added test suite verifying bulkWrite() error propagation, error post hook invocation, and operation prevention when pre-hooks fail |
| test/model.insertMany.test.js | Added parallel test suite for insertMany() to document and verify consistent behavior with bulkWrite() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hasezoey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| throw options.preHookError; | ||
| }); | ||
|
|
||
| if (options.postHook) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createTextContext() is fine, but in this case it feels like you're building a mini-framework for this test suite, which hurts readability and doesn't meaningfully reduce SLOC. Fine for now, but let's avoid creating too many abstractions in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkarpov15
My aim is to have the it('...') blocks be as minimal as possible and only showing the important parts to understand what a test does, with building the "common" blocks in createTestContext, which I think you find acceptable.
Do you think instead of having a generic createTestContext, we should try to have multiple createTestContexts specific to each use-case even if they're only slightly different?
Summary
Fix
bulkWrite()to call error post hooks when a pre-hook throws an error. Fixes #15881Changes
execPostwith{ error }whenbulkWritepre-hook throws, matchinginsertManybehaviorBefore
After