RESTApiHandler support compound ids (@@id) #1754
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Database
Client->>API: Request to create a PostLike
API->>Database: Validate and process request with makeIdFilter
Database-->>API: Confirmation of PostLike creation
API-->>Client: Response with created PostLike
Client->>API: Request to fetch PostLikes
API->>Database: Query PostLikes with makeIdSelect
Database-->>API: Return PostLikes data
API-->>Client: Response with PostLikes data
sequenceDiagram
participant Client
participant API
participant Database
Client->>API: Request to update a PostLike
API->>Database: Validate and process request with makeCompoundId
Database-->>API: Confirmation of PostLike update
API-->>Client: Response with updated PostLike
Client->>API: Request to delete a PostLike
API->>Database: Validate and process request with makeIdKey
Database-->>API: Confirmation of PostLike deletion
API-->>Client: Response confirming deletion
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (7)
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 (
|
There was a problem hiding this comment.
Hi @thomassnielsen , many thanks for making this PR. It's a great start! I've left some comments there, please help take a look.
I'm not sure how much time you have to continue working on this. Another option is we merge it first, and I can follow up with a separate PR to continue polishing it.
I'd love to include it in the next release!
I have some time today and tomorrow so I'll try to fix at least what's been discussed in this PR and add some more tests. When's the next release cutoff point? |
Thank you! Current goal of v2.7 is 10/15. Please take your time 😄. |
|
Marking this ready for review now and moving on to looking at the openapi plugin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/server/src/api/rest/index.ts (1)
1481-1484: Simplify sorting logic for related models with composite IDsIn the
buildSortmethod, the logic for sorting on related models with composite IDs can be simplified by leveraging themakeIdSelectmethod or by ensuring consistent handling of ID fields.Consider refactoring the sorting logic to reuse existing methods for selecting ID fields, improving code readability and maintainability.
packages/server/tests/api/rest.test.ts (1)
331-331: UseidDividerwhen specifying compound IDsIn the response assertion, you've hardcoded the compound ID as
'1_user1'. For consistency and maintainability, use theidDividervariable when constructing compound IDs.Apply this diff to fix:
- data: [{ type: 'postLike', id: '1_user1' }], + data: [{ type: 'postLike', id: `1${idDivider}user1` }],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/server/src/api/rest/index.ts (39 hunks)
- packages/server/tests/api/rest.test.ts (11 hunks)
🔇 Additional comments (1)
packages/server/src/api/rest/index.ts (1)
1568-1569: Verify composite ID filters inmakeFilterValueWhen filtering by composite IDs, ensure that the
makeIdFilterfunction constructs the correct Prismawhereclause. Incorrect filtering may lead to unexpected query results.Run the following script to check how composite ID filters are constructed:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/server/src/api/rest/index.ts (2)
35-36: LGTM! Consider adding a comment for clarity.The addition of the
idDividerconstant is a good approach for handling compound IDs. It matches Prisma's default and allows for potential future configurability.Consider adding a brief comment explaining the purpose of this constant, e.g.:
// Separator used for joining multiple ID fields in compound IDs export const idDivider = '_';
1198-1226: LGTM! Well-implemented utility methods for compound ID handling.The new methods
makeIdFilter,makeIdSelect,makeIdKey, andmakeCompoundIdare well-implemented and provide essential functionality for consistent handling of compound IDs throughout the class. These methods ensure that Prisma queries are constructed correctly for both single and compound IDs, and that compound IDs are consistently represented and manipulated.Consider adding JSDoc comments to these methods to improve code documentation. For example:
/** * Creates a Prisma filter object for the given ID fields and resource ID. * @param idFields - Array of FieldInfo objects representing the ID fields. * @param resourceId - The resource ID (compound or single). * @returns A Prisma filter object. */ private makeIdFilter(idFields: FieldInfo[], resourceId: string) { // ... existing implementation ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/server/src/api/rest/index.ts (30 hunks)
🔇 Additional comments (6)
packages/server/src/api/rest/index.ts (6)
58-58: LGTM! Good update for compound ID support.The changes to
RelationshipInfoandModelInfotypes, replacingidField: stringwithidFields: FieldInfo[], are well-implemented. This modification allows for multiple ID fields and provides more detailed information about each ID field, which is essential for supporting compound IDs.Also applies to: 64-64
756-757: LGTM! Proper handling of compound IDs in relationships.The changes in the
processCreatemethod correctly implement compound ID support for relationship connections. The use ofmakeIdKeyensures that the proper key is used for both single and compound IDs. This modification maintains consistency and correctness when creating entities with relationships that may have compound IDs.Also applies to: 764-766, 773-773
810-814: LGTM! Consistent handling of compound IDs in relationship CRUD.The modifications to the
processRelationshipCRUDmethod properly implement compound ID support for relationship operations. The use ofmakeIdFilterandmakeIdSelectensures that both single and compound IDs are handled correctly in the where clause and select statement. This change maintains consistency across different types of relationship operations.Also applies to: 845-845
910-910: LGTM! Proper handling of compound IDs in update operations.The changes in the
processUpdatemethod correctly implement compound ID support for update operations. The use ofmakeIdFilterandmakeIdKeyensures that both single and compound IDs are handled properly in the where clause and when setting relationships. This modification maintains consistency and correctness when updating entities with potential compound IDs.Also applies to: 929-929, 937-939, 944-944
963-963: LGTM! Consistent handling of compound IDs in delete operations.The modification to the
processDeletemethod properly implements compound ID support for delete operations. The use ofmakeIdFilterensures that both single and compound IDs are handled correctly in the where clause. This change maintains consistency with other CRUD operations and supports the overall goal of compound ID implementation.
983-983: LGTM! Proper handling of compound IDs in type map construction.The modifications to the
buildTypeMapmethod correctly implement compound ID support when constructing the type map. By usingidFieldsinstead of a singleidField, the method now properly handles both single and compound IDs. This change is crucial as it provides the correct type information for other methods that rely on this data, ensuring consistency throughout the API handler.Also applies to: 1003-1003
|
Testing with an updated openapi plugin it looks like there's something still missing here, so converting it back to a draft. |
|
Never mind, forgot to build before publishing the latest to verdaccio 🤦 |
There was a problem hiding this comment.
Thanks for continuing to work on this @thomassnielsen ! I've left two minor comments and the code overall looks very good to me now.
#1748
Adds support for the Prisma @@id compound ids to the RESTApiHandler.
Discord thread