fix(mongoose): fix prehook when insertMany is used to hook method #2959
Conversation
WalkthroughThe pull request introduces significant type improvements and refinements to the Mongoose ORM decorators and interfaces. The changes primarily focus on enhancing type safety and flexibility for pre and post hooks in the Mongoose schema definition. A new Changes
Sequence DiagramsequenceDiagram
participant Developer
participant PreHook/PostHook Decorator
participant SchemaOptions
participant MongooseSchema
Developer->>PreHook/PostHook Decorator: Apply decorator with method
PreHook/PostHook Decorator->>SchemaOptions: Generate schema options
SchemaOptions->>MongooseSchema: Configure hook with validated method
MongooseSchema-->>Developer: Return configured schema
The sequence diagram illustrates the enhanced process of applying pre and post hooks to Mongoose schemas, highlighting the improved method validation and schema configuration workflow. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Romakita
force-pushed
the
fix-mongoose-hooks-arguments
branch
from
21c8c5d to
7e7f415
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/orm/mongoose/src/decorators/postHook.ts (1)
Line range hint
52-71: Further type safety improvements possible.The implementation could benefit from stronger typing:
- The
paramsspread parameter could be typed more specifically based on the overloads- The
argsparameter in the returned function could leverage the decorator parameter typesWould you like me to propose a more strongly-typed implementation?
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/orm/mongoose/src/decorators/postHook.spec.ts(2 hunks)packages/orm/mongoose/src/decorators/postHook.ts(2 hunks)packages/orm/mongoose/src/decorators/preHook.spec.ts(4 hunks)packages/orm/mongoose/src/decorators/preHook.ts(2 hunks)packages/orm/mongoose/src/interfaces/MongooseSchemaOptions.ts(1 hunks)packages/orm/mongoose/src/utils/schemaOptions.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/orm/mongoose/src/decorators/postHook.ts
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/orm/mongoose/src/decorators/preHook.ts
[error] 55-55: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-specs (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: test-graphql (20.12.2)
🔇 Additional comments (4)
packages/orm/mongoose/src/interfaces/MongooseSchemaOptions.ts (1)
7-27: LGTM! Comprehensive list of Mongoose methods.The new
MongooseMethodtype properly enumerates all supported Mongoose operations, includinginsertMany. This improves type safety by preventing invalid method names.packages/orm/mongoose/src/decorators/preHook.spec.ts (1)
Line range hint
1-70: Add test coverage for insertMany hook.While the tests cover the basic hook functionality, they don't specifically test the
insertManyscenario that this PR aims to fix.Add a test case to verify the fix:
it("should handle insertMany hook with multiple documents", () => { const fn = vi.fn(); @PreHook("insertMany", fn) class Test {} const options = schemaOptions(Test); expect(options.pre[0].method).toBe("insertMany"); expect(options.pre[0].fn).toBeInstanceOf(Function); });packages/orm/mongoose/src/decorators/postHook.spec.ts (1)
Line range hint
1-45: Add test coverage for post-insertMany hook.Similar to the pre-hook tests, add coverage for the
insertManyscenario to ensure complete test coverage of the fix.Add a test case:
it("should handle post-insertMany hook", () => { const fn = vi.fn(); @PostHook("insertMany", fn) class Test {} const options = schemaOptions(Test); expect(options.post[0].method).toBe("insertMany"); expect(options.post[0].fn).toBeInstanceOf(Function); });✅ Verification successful
Review comment is correct - insertMany hook test coverage is needed
The verification confirms that:
insertManyis a valid mongoose method in the type definition- No existing test coverage for
insertManyhooks exists- The suggested test implementation follows the established patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find MongooseMethod type definition ast-grep --pattern 'type MongooseMethod = $$$' # Search for existing insertMany hook tests rg "insertMany.*hook" -B2 -A2 # Check postHook decorator implementation ast-grep --pattern 'export function PostHook($$$) { $$$ }'Length of output: 1725
Script:
#!/bin/bash # Check preHook tests for insertMany coverage rg "insertMany" packages/orm/mongoose/src/decorators/preHook.spec.tsLength of output: 68
packages/orm/mongoose/src/decorators/preHook.ts (1)
58-66: Verify that the static method binding works withinsertMany.The PR aims to fix prehook with insertMany. The static method binding implementation looks correct, but we should verify it works specifically with insertMany.
Let's check the usage of insertMany with PreHook in the codebase:
✅ Verification successful
Static method binding implementation is correct for
insertManyThe binding mechanism is method-agnostic and correctly implemented using
bind(args[0]). While there are no specific tests forinsertMany, the implementation will work as it uses the same binding logic that's verified for other mongoose methods.
- Consider adding specific tests for
insertManyto improve test coverage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PreHook usage with insertMany rg -B 2 -A 2 '@PreHook.*insertMany' # Search for test cases involving insertMany rg -B 2 -A 2 'insertMany.*test'Length of output: 71
Script:
#!/bin/bash # Search for PreHook decorator usage rg -l '@PreHook' # Search for insertMany usage in the codebase rg -B 2 -A 2 'insertMany' # Look for test files containing insertMany fd -e test.ts -e spec.ts -x rg -l 'insertMany' {} # Find static method declarations that might use insertMany ast-grep --pattern 'static $_ ($$$) { $$$ insertMany $$$ }'Length of output: 1034
Script:
#!/bin/bash # Check PreHook test file content cat packages/orm/mongoose/src/decorators/preHook.spec.ts # Look for insertMany examples in documentation cat docs/tutorials/snippets/mongoose/pre-hook.ts # Check PreHook implementation cat packages/orm/mongoose/src/decorators/preHook.tsLength of output: 4419
Romakita
force-pushed
the
fix-mongoose-hooks-arguments
branch
2 times, most recently
from
a78ca8f to
db4e89d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/orm/mongoose/src/decorators/preHook.ts (1)
67-76: 🧹 Nitpick (assertive)Consider adding validation for decorator arguments in the class decorator case.
In the else branch, when
PreHookis used as a class decorator, the code assumes thatdecoratorArgs[0]is a function (fn) without validating it. If an incorrect argument is passed, it could lead to runtime errors. Adding validation to check ifdecoratorArgs[0]is a function would enhance robustness.Apply this diff to add validation:
} else { + if (typeof decoratorArgs[0] !== 'function') { + throw new Error('PreHook requires a function as the second argument when used as a class decorator'); + } schemaOptions(args[0], { pre: [ { method, fn: decoratorArgs[0], options: decoratorArgs[1] } ] }); }packages/orm/mongoose/src/interfaces/MongooseSchemaOptions.ts (1)
30-31: 🧹 Nitpick (assertive)Consider adding JSDoc comments for hook callback parameters.
The
MongoosePreHookCBandMongoosePostHookCBtypes now accept variable arguments with...args: unknown[], increasing flexibility. Documenting the expected arguments for each hook method would improve developer experience and maintainability.Add JSDoc comments to describe the expected arguments:
/** * Pre-hook callback that receives the document and additional method-specific arguments. * @param doc - The Mongoose document. * @param args - Additional arguments specific to the pre-hook method. */ export type MongoosePreHookCB<T = any> = (doc: T | MongooseDocument<T>, ...args: unknown[]) => Promise<void> | void; /** * Post-hook callback that receives the document and additional method-specific arguments. * @param doc - The Mongoose document. * @param args - Additional arguments specific to the post-hook method. */ export type MongoosePostHookCB<T = any> = (doc: T | MongooseDocument<T>, ...args: unknown[]) => Promise<void> | void;packages/orm/mongoose/src/utils/schemaOptions.ts (1)
42-44: 🧹 Nitpick (assertive)Consider adding a reference to the Mongoose issue.
While the explicit passing of arguments fixes the hook parameters issue, it would be helpful to add a link to the related Mongoose issue for future reference.
- // we need to explicitly gives args to avoid a bug with mongoose + // Explicitly pass args to avoid Mongoose hook arguments bug + // See: https://github.com/Automattic/mongoose/issues/[issue-number]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/orm/mongoose/src/decorators/postHook.spec.ts(2 hunks)packages/orm/mongoose/src/decorators/postHook.ts(2 hunks)packages/orm/mongoose/src/decorators/preHook.spec.ts(4 hunks)packages/orm/mongoose/src/decorators/preHook.ts(2 hunks)packages/orm/mongoose/src/interfaces/MongooseSchemaOptions.ts(1 hunks)packages/orm/mongoose/src/utils/schemaOptions.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-specs (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-graphql (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: lint (20.12.2)
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
packages/orm/mongoose/src/decorators/preHook.spec.ts (2)
Line range hint
10-19: Tests updated correctly to reflect method name changes.The method name in the
@PreHookdecorator has been updated from"method"to"save", aligning the test with the actual Mongoose lifecycle method. The expected schema options now correctly reflect this change.
Line range hint
35-44: Tests accurately adjusted for the method name update.The second test case also updates the method name to
"save"in the@PreHookdecorator, ensuring consistency and correctness in the test suite.packages/orm/mongoose/src/decorators/postHook.ts (1)
49-52: LGTM! Good type safety improvements.The changes improve type safety in several ways:
- Restricting the
methodparameter to valid Mongoose hook methods- Making the return type more specific with
ClassDecorator | StaticMethodDecoratorpackages/orm/mongoose/src/decorators/postHook.spec.ts (1)
11-11: LGTM! Test updated to use valid hook method.The test has been correctly updated to use a valid Mongoose hook method ("save") instead of the generic "method" string, aligning with the new type restrictions.
Also applies to: 20-20
Romakita
force-pushed
the
fix-mongoose-hooks-arguments
branch
from
db4e89d to
91fac3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/orm/mongoose/src/decorators/postHook.spec.ts(2 hunks)packages/orm/mongoose/src/decorators/postHook.ts(2 hunks)packages/orm/mongoose/src/decorators/preHook.spec.ts(4 hunks)packages/orm/mongoose/src/decorators/preHook.ts(2 hunks)packages/orm/mongoose/src/interfaces/MongooseSchemaOptions.ts(1 hunks)packages/orm/mongoose/src/utils/schemaOptions.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/orm/mongoose/src/utils/schemaOptions.ts
[error] 43-43: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-graphql (20.12.2)
- GitHub Check: test-specs (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: lint (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
packages/orm/mongoose/src/interfaces/MongooseSchemaOptions.ts (1)
30-31: Consider documenting the expected arguments in the hook callbacks.Since the hook callbacks now use
...args: unknown[], it would be helpful to document the expected arguments for each hook type.packages/orm/mongoose/src/decorators/preHook.ts (1)
67-76: Consider adding validation for invalid decorator usage.In the else branch, the function assumes that
decoratorArgs[0]is a function. Consider adding validation to ensure thatdecoratorArgs[0]is indeed a function to prevent runtime errors when the decorator is used incorrectly.packages/orm/mongoose/src/utils/schemaOptions.ts (2)
43-43: Avoid using 'Function' as a type; specify a more precise function signature.Using 'Function' as a type reduces type safety and can be a source of bugs. Consider defining the specific function type expected for the
nextparameter to enhance type safety.🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
43-44: Good fix for handling Mongoose hook arguments.Explicitly passing
arg1andarg2fixes issues with Mongoose hooks not receiving all parameters, especially important for methods likeinsertMany. This ensures that hooks receive all necessary context.🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/orm/mongoose/src/decorators/postHook.ts (2)
3-3: LGTM! Import of MongooseMethods type improves type safety.The addition of the MongooseMethods type import helps restrict hook methods to valid Mongoose operations.
49-52: Great improvement in type safety!The changes improve type safety in two ways:
- Restricting the
methodparameter to valid Mongoose methods usingMongooseMethodstype- Providing a more specific return type
ClassDecorator | StaticMethodDecoratorinstead ofFunctionpackages/orm/mongoose/src/decorators/postHook.spec.ts (1)
11-11: LGTM! Test updated to use valid Mongoose method.The test case now correctly uses "save" as a valid Mongoose method, aligning with the new type restrictions.
Also applies to: 20-20
packages/orm/mongoose/src/decorators/preHook.spec.ts (1)
10-10: LGTM! Tests updated consistently to use valid Mongoose method.All test cases now correctly use "save" as a valid Mongoose method, maintaining consistency with the new type restrictions.
Also applies to: 19-19, 35-35, 44-44
|
🎉 This PR is included in version 8.4.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes