fix(polymorphism): support orderBy with base fields
#1086
Conversation
WalkthroughWalkthroughThe recent updates focus on enhancing the management of inherited fields, especially in sorting operations ( Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/runtime/src/enhancements/delegate.ts (6 hunks)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (1 hunks)
Additional comments: 3
packages/runtime/src/enhancements/delegate.ts (3)
- 860-868: The modifications to the
aggregatemethod to processcursor,orderBy, andwherearguments using thebuildWhereHierarchymethod are well-aligned with the PR's objectives. These changes ensure that aggregation operations can correctly handle fields inherited from base types, enhancing the support for polymorphic models. As with other methods that manipulate object properties, ensure to guard against prototype pollution risks.- 888-892: The updates to the
countmethod to processcursorandwherearguments using thebuildWhereHierarchymethod are consistent with the PR's objectives. These changes ensure that counting operations can correctly handle fields inherited from base types, further enhancing polymorphism support. Continue to be vigilant about prototype pollution risks when manipulating object properties based on user input.- 928-928: The modifications to the
groupBymethod to process thewhereargument using thebuildWhereHierarchymethod align with the PR's objectives. These changes ensure that groupBy operations can correctly handle fields inherited from base types. The proactive error handling forbyfields inherited from base types is a good practice, clearly communicating the current limitations to the users.
| private buildWhereHierarchy(model: string, where: any) { | ||
| if (!where) { | ||
| return undefined; | ||
| } | ||
|
|
||
| where = deepcopy(where); | ||
| Object.entries(where).forEach(([field, value]) => { | ||
| const fieldInfo = resolveField(this.options.modelMeta, this.model, field); | ||
| const fieldInfo = resolveField(this.options.modelMeta, model, field); | ||
| if (!fieldInfo?.inheritedFrom) { | ||
| return; | ||
| } | ||
|
|
||
| let base = this.getBaseModel(this.model); | ||
| let base = this.getBaseModel(model); |
There was a problem hiding this comment.
The implementation of buildWhereHierarchy method correctly handles the transformation of where conditions to support fields inherited from base types. This is key to achieving the PR's objective of enhancing polymorphism support. However, there's a potential risk of prototype pollution due to direct manipulation of object properties without validating their names against a list of disallowed prototype properties. This could lead to unintended side effects if a user-controlled input is passed directly to this method.
Consider adding a check to ensure that field names do not correspond to prototype properties, or use a Map for handling dynamic keys safely.
| it('order by base fields', async () => { | ||
| const { db, user } = await setup(); | ||
|
|
||
| await expect( | ||
| db.video.findMany({ | ||
| orderBy: { viewCount: 'desc' }, | ||
| }) | ||
| ).resolves.toHaveLength(1); | ||
|
|
||
| await expect( | ||
| db.ratedVideo.findMany({ | ||
| orderBy: { duration: 'asc' }, | ||
| }) | ||
| ).resolves.toHaveLength(1); | ||
|
|
||
| await expect( | ||
| db.user.findMany({ | ||
| orderBy: { assets: { _count: 'desc' } }, | ||
| }) | ||
| ).resolves.toHaveLength(1); | ||
|
|
||
| await expect( | ||
| db.user.findUnique({ | ||
| where: { id: user.id }, | ||
| include: { | ||
| ratedVideos: { | ||
| orderBy: { | ||
| viewCount: 'desc', | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| ).toResolveTruthy(); | ||
| }); |
There was a problem hiding this comment.
The new test case order by base fields correctly tests the functionality of ordering by base fields in different scenarios, which aligns with the PR's objectives to enhance polymorphism support. However, there are a few points to consider for improvement:
-
Assertion Specificity: The test case uses
.resolves.toHaveLength(1)to assert the outcome of thefindManyandfindUniqueoperations. While this checks that the query returns a result, it does not verify that the ordering is correctly applied. It would be beneficial to enhance the assertions to check the order of the returned records explicitly, ensuring that theorderByfunctionality works as expected. -
Error Handling: The test case uses
.toResolveTruthy()for thefindUniqueoperation, which is not a standard Jest matcher. This might be a typo or a custom matcher not defined in the provided context. If it's intended to check that the promise resolves successfully, consider using.resolves.toBeTruthy()or another appropriate Jest matcher for clarity and correctness. -
Test Coverage: The test case covers ordering by
viewCountanddurationforvideoandratedVideomodels, and a count-based order forassetsrelated to auser. To further ensure robustness, consider adding more scenarios that cover edge cases, such as ordering by fields that might have null values, or combiningorderBywith other query parameters likewhereandincludeto test more complex queries. -
Documentation: Adding comments to explain the setup and the specific scenarios being tested can improve the readability and maintainability of the test case. This is especially useful for future developers who might work on this test suite.
Overall, the test case is a good start towards validating the new functionality but could be enhanced for more comprehensive testing and clarity.
- Consider enhancing the assertions to explicitly check the order of the returned records.
- Verify the usage of
.toResolveTruthy()and replace it with a standard Jest matcher if necessary. - Expand the test coverage to include more scenarios and edge cases.
- Add comments to document the setup and scenarios being tested for better readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/integration/tests/cli/plugins.test.ts (1 hunks)
Additional comments: 1
tests/integration/tests/cli/plugins.test.ts (1)
- 119-119: Adding
skipLibCheck: trueto the TypeScript compiler options can significantly speed up the compilation process by skipping type checking of declaration files. This change is generally acceptable in test environments where speed might be prioritized over the thoroughness of type checks. However, it's important to weigh the trade-offs, as this could potentially mask type errors in third-party libraries or custom type definitions.Ensure that this change aligns with the overall testing and quality assurance strategy of the project. It might be beneficial to periodically run type checks with
skipLibCheckdisabled to catch any hidden issues.Consider periodically running type checks with
skipLibCheckdisabled in a separate CI job or before releases to ensure that potential type errors in declaration files are not overlooked.
Summary by CodeRabbit
orderByfields to support correct ordering by inherited fields in queries.