fix(runtime): intercepts $extends to reproxy its result to make sure enhancements persist
#1847
Conversation
…e enhancements persist
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
tests/regression/tests/issue-prisma-extension.test.ts (2)
7-15: Consider adding more test cases for edge casesThe schema definition is clear and concise. However, consider adding test cases for:
- Nested relations (e.g., Post with Comments)
- Multiple extension layers
- Error cases when permissions are denied
34-38: Add negative test case for unpublished postsThe test verifies that published posts are accessible but doesn't explicitly verify that unpublished posts are filtered out. Consider adding an assertion to check that the unpublished post (id: 2) is not included in the results.
tests/integration/tests/enhancements/with-policy/client-extensions.test.ts (3)
47-49: LGTM! Comprehensive test coverage for extension and enhancement combinations.The test cases effectively verify the behavior of
$extendswith different enhancement combinations. They demonstrate that:
- Raw extensions see all records (length 3)
- Enhanced extensions filter records correctly (length 2)
- The order of applying enhancements and extensions maintains consistent behavior
Consider adding test cases for:
- Chaining multiple extensions (
$extends(ext1).$extends(ext2))- Nested extensions (ext1 that calls
$extendsinternally)Also applies to: 83-85
114-118: LGTM! Clear and isolated test cases for client extensions.The test effectively verifies that client extensions work correctly with different enhancement orders. The boolean flag reset ensures proper test isolation.
Consider using a jest spy instead of a boolean flag for better test practices:
const logSpy = jest.fn(); // ... in extension $log: logSpy // ... in test expect(logSpy).toHaveBeenCalledWith('abc');
295-297: LGTM! Good coverage of result modifications.The tests effectively verify that both direct mutations and computed fields persist through enhancement wrapping. The use of
expect.objectContainingmakes the assertions robust.Consider adding test cases for:
- Computed fields that depend on other computed fields
- Mutations that add/remove fields
- Error cases (e.g., computing fields with missing dependencies)
Also applies to: 331-333
packages/runtime/src/enhancements/node/proxy.ts (1)
257-274: Consider extracting common logic for property interceptionThe interception logic for both
$transactionand$extendsproperties shares similarities in how they wrap and proxy new client instances. To enhance maintainability and reduce code duplication, consider refactoring this common functionality into a helper function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/runtime/src/enhancements/node/proxy.ts(1 hunks)tests/integration/tests/enhancements/with-policy/client-extensions.test.ts(9 hunks)tests/regression/tests/issue-prisma-extension.test.ts(1 hunks)
🔇 Additional comments (2)
tests/integration/tests/enhancements/with-policy/client-extensions.test.ts (1)
153-154: LGTM! Thorough coverage of query override scopes.
The test suite comprehensively verifies that query overrides work correctly at all scopes (model-specific, all-models, operation-specific, and global) and persist through enhancement wrapping.
Let's verify that all query override scopes are tested:
Also applies to: 189-190, 225-226, 259-260
✅ Verification successful
Verified coverage of all query override scopes.
All query override patterns are present and properly tested across the specified lines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify coverage of query override scopes
# Look for all query override test patterns in the test file
echo "Checking query override patterns..."
rg -U 'query: \{[^}]+\}' tests/integration/tests/enhancements/with-policy/client-extensions.test.ts
Length of output: 773
packages/runtime/src/enhancements/node/proxy.ts (1)
257-274: Proper handling of $extends to ensure enhancements persist
The added code correctly intercepts the $extends property and re-proxies its result to maintain enhancements. The check to avoid double wrapping using the PRISMA_PROXY_ENHANCER marker is appropriate and ensures that extended Prisma client instances continue to function with the applied proxies.
|
fixes #1859 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/regression/tests/issue-1859.test.ts (2)
4-39: Consider strengthening test assertionsWhile the test effectively validates the core behavior, consider adding these assertions to make it more robust:
- Verify the structure of returned posts (not just length)
- Test with different
argspassed tofindManyListView- Add negative test cases (e.g., invalid args)
await expect(db.post.findMany()).resolves.toHaveLength(1); +await expect(db.post.findMany()).resolves.toMatchObject([ + { id: 1, title: 'post1', published: true } +]); const extended = db.$extends({ model: { post: { findManyListView: async (args: any) => { return { view: true, data: await db.post.findMany(args) }; }, }, }, }); await expect(extended.post.findManyListView()).resolves.toMatchObject({ view: true, data: [{ id: 1, title: 'post1', published: true }], }); + +// Test with args +await expect(extended.post.findManyListView({ + where: { title: 'post1' } +})).resolves.toMatchObject({ + view: true, + data: [{ id: 1, title: 'post1', published: true }], +}); + +// Negative test +await expect(extended.post.findManyListView({ + invalid: 'arg' +})).rejects.toThrow(); await expect(extended.post.findMany()).resolves.toHaveLength(1);
27-27: Improve type safety by avoidinganyThe
argsparameter in bothfindManyListViewmethods is typed asany. Consider using proper Prisma types for better type safety and IDE support.-findManyListView: async (args: any) => { +findManyListView: async (args: Parameters<typeof db.post.findMany>[0]) => {Also applies to: 62-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/regression/tests/issue-1859.test.ts(1 hunks)
🔇 Additional comments (1)
tests/regression/tests/issue-1859.test.ts (1)
1-16: LGTM: Well-structured schema with clear access control rules
The schema setup effectively defines the test scenario with appropriate access control rules that will help validate the enhancement behavior.
No description provided.