Configurable REST API id divider #1785
Conversation
thomassnielsen
changed the title
- Update test to use postgres db - Changes url pattern option from `segmentNameCharset` to `segmentValueCharset` - Made sure ID key sent to Prisma is always joined with '_' instead of user-configured divider. I've renamed methods for clarity.
|
Hi @thomassnielsen , I've pushed a commit with the following changes:
Please help check if they make sense. Thanks! |
Looks good to me! Thanks! |
📝 Walkthrough<details>
<summary>📝 Walkthrough</summary>
## Walkthrough
The pull request introduces significant modifications to the `RequestHandler` class within the REST API. A new optional property, `idDivider`, is added to the `Options` type, allowing for customizable separation of compound ID fields in URLs. The static `urlPatterns` object is replaced by a dynamic method, `buildUrlPatterns`, which generates URL patterns based on the `idDivider`. Additionally, internal methods related to ID handling are updated for consistency. The test suite is also enhanced to validate the handling of compound IDs with a custom separator, including database setup and teardown for testing.
## Changes
| File Path | Change Summary |
|------------------------------------------------|---------------------------------------------------------------------------------------------------|
| packages/server/src/api/rest/index.ts | - Added `idDivider?: string;` to `Options` type.<br>- Introduced `buildUrlPatterns(idDivider: string)` method.<br>- Renamed `makeIdFilter` to `makePrismaIdFilter`.<br>- Updated methods to use `idDivider`. |
| packages/server/tests/api/rest.test.ts | - Added tests for compound IDs with a custom separator.<br>- Included database setup/teardown logic using `createPostgresDb` and `dropPostgresDb`. |
## Possibly related PRs
- **#1754**: Enhances the `RequestHandler` class to support compound IDs, directly relating to the changes made regarding ID handling and the `idDivider` property.
- **#1784**: Modifies the `RequestHandler` to improve serialization for entities with compound IDs, aligning with the focus on managing compound IDs in the main PR.
## Suggested reviewers
- ymc9
</details>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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (2)
- packages/server/src/api/rest/index.ts (24 hunks)
- packages/server/tests/api/rest.test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/server/src/api/rest/index.ts (1)
216-216: Verify URL encoding of special characters inidDivider.If
idDividercontains special characters that may require URL encoding, ensure thatsegmentValueCharsetaccommodates them appropriately to prevent parsing issues.Run the following script to check for special characters in
idDivider:
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/server/src/api/rest/index.ts (2)
224-235: LGTM: Well-implementedbuildUrlPatternsmethod.The
buildUrlPatternsmethod is correctly implemented. It uses the providedurlSegmentNameCharsetfor creating URL patterns, which should handle custom characters in IDs properly.Consider adding a comment explaining why the
idDividerparameter is not directly used in this method. It might be helpful for future maintainers to understand that it's implicitly used through theurlSegmentNameCharset.
Line range hint
1230-1264: LGTM: Updated ID-related methods to use configurable divider.The ID-related methods have been updated to use the configurable
this.idDividerin most cases, which is correct. ThemakePrismaIdKeymethod still uses a hardcoded underscore, which aligns with the PR comment about ensuring the ID key sent to Prisma is always joined with an underscore.Consider adding a comment in the
makePrismaIdKeymethod explaining why it uses a hardcoded underscore instead ofthis.idDivider. This will help future maintainers understand the intentional difference.private makePrismaIdKey(idFields: FieldInfo[]) { // TODO: support `@@id` with custom name + // Note: We always use underscore for Prisma, regardless of the configured idDivider return idFields.map((idf) => idf.name).join('_'); }packages/server/tests/api/rest.test.ts (1)
2525-2564: LGTM! Well-structured test suite setup.The setup for the "REST server tests - compound id with custom separator" test suite is comprehensive and well-structured. It correctly configures the test environment with a custom schema, PostgreSQL database, and the necessary options for testing compound IDs with a custom separator.
Consider adding a comment explaining the purpose of the
urlSegmentNameCharsetoption, as it's not immediately clear why it's needed for this test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/server/src/api/rest/index.ts (24 hunks)
- packages/server/tests/api/rest.test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/server/src/api/rest/index.ts (10)
42-53: LGTM: New options for configurable ID divider and URL segment charset.The additions of
idDividerandurlSegmentNameCharsetto theOptionstype are well-documented and provide clear explanations of their purpose. The default values are sensible, allowing for backward compatibility while enabling customization.
212-216: LGTM: New private properties for ID divider and URL patterns.The addition of private
idDividerandurlPatternsproperties to theRequestHandlerclass is appropriate. These properties will store the configurable ID divider and dynamically generated URL patterns, which are essential for the new functionality.
219-222: LGTM: Proper initialization of ID divider and URL patterns.The constructor correctly initializes the
idDividerandurlPatternsproperties. It uses the values from the options object if provided, otherwise falling back to sensible defaults. ThebuildUrlPatternsmethod is called with the appropriate parameters to generate the URL patterns.
Line range hint
269-293: LGTM: Correct implementation of dynamic URL patterns in GET handler.The GET method handler has been updated to use the dynamic
this.urlPatternsinstead of static patterns. This change correctly implements the use of the custom ID divider in URL matching while maintaining the existing logic for different types of GET requests (single resource, fetch related, read relationship, collection read).
Line range hint
307-322: LGTM: Proper use of dynamic URL patterns in POST handler.The POST method handler has been correctly updated to use the dynamic
this.urlPatterns. This change ensures that the custom ID divider is respected in URL matching for POST requests, while maintaining the existing logic for resource creation and relationship creation.
Line range hint
337-362: LGTM: Correct implementation of dynamic URL patterns in PUT/PATCH handler.The PUT/PATCH method handler has been properly updated to use the dynamic
this.urlPatterns. This change ensures that the custom ID divider is respected in URL matching for update requests, while preserving the existing logic for resource updates and relationship updates.
Line range hint
369-386: LGTM: Proper use of dynamic URL patterns in DELETE handler.The DELETE method handler has been correctly updated to use the dynamic
this.urlPatterns. This change ensures that the custom ID divider is respected in URL matching for delete requests, while maintaining the existing logic for resource deletion and relationship deletion.
777-777: LGTM: Consistent usage of updated ID-related methods.The changes consistently use the renamed
makePrismaIdKeyandmakePrismaIdFiltermethods in various parts of the code, including relationship operations and filter operations. This maintains consistency with the earlier changes and ensures that the configurable ID divider is respected throughout the codebase.Also applies to: 785-785, 794-794, 1587-1591
Line range hint
1-1740: Summary: Well-implemented configurable REST API ID dividerThe changes in this file successfully implement a configurable ID divider for the REST API. Key points:
- The
Optionstype now includesidDividerandurlSegmentNameCharsetproperties.- Dynamic URL patterns are used throughout the request handlers to accommodate the custom ID divider.
- ID-related methods have been updated to use the configurable divider, with the exception of
makePrismaIdKeywhich intentionally uses an underscore for Prisma compatibility.- The changes are consistent and maintain the existing functionality while adding the new configurability.
The implementation is solid and achieves the goal of making the REST API ID divider configurable. The code is well-structured and the changes are applied consistently throughout the file.
415-415: LGTM: Updated method name for clarity.The change from
makeIdFiltertomakePrismaIdFilterimproves clarity about the method's purpose.Please ensure that the
makePrismaIdFiltermethod correctly uses the newidDividerproperty. Let's verify its implementation:
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/server/tests/api/rest.test.ts (1)
2541-2565: Robust test setup with room for minor improvementThe test suite setup is well-structured and includes important practices:
- Creating a fresh PostgreSQL database for each test run, ensuring a clean testing environment.
- Custom handler setup with specific options, allowing for comprehensive testing of the custom ID separator functionality.
- Inclusion of potentially problematic characters in urlSegmentNameCharset, which is excellent for thorough testing.
One minor suggestion for improvement:
Consider adding error handling for the database creation and dropping processes. This could help identify any issues with the database setup or teardown more easily.
Example:
beforeAll(async () => { try { const dbUrl = await createPostgresDb(dbName); // ... rest of the setup } catch (error) { console.error('Failed to set up test database:', error); throw error; } }); afterAll(async () => { try { await dropPostgresDb(dbName); } catch (error) { console.error('Failed to drop test database:', error); } });packages/server/src/api/rest/index.ts (2)
1237-1238: Address TODO: Support@@idwith custom names inmakePrismaIdFilterThe current implementation doesn't support models with
@@idcomposite keys that have custom names. This might lead to incorrect query behavior for such models. Consider implementing support for custom composite ID names.Would you like assistance in implementing this feature? I can help provide a solution or open a GitHub issue to track this task.
1260-1262: Address TODO: Support@@idwith custom names inmakePrismaIdKeyThe method
makePrismaIdKeycurrently doesn't handle custom names for composite IDs defined using@@id. This could result in incorrect key generation when interfacing with Prisma for models using custom composite IDs.Would you like assistance in implementing this support? I can help draft the necessary changes or open a GitHub issue to track this enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/server/src/api/rest/index.ts (25 hunks)
- packages/server/tests/api/rest.test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/server/tests/api/rest.test.ts (1)
2524-2540: Well-structured test suite setupThe test suite is well-organized and demonstrates good practices:
- Clear definition of the User model with a compound primary key.
- Use of enums for the Role field, improving type safety.
- Custom idDivider (':') to avoid potential conflicts in compound IDs.
- Use of PostgreSQL for testing, which is a robust choice for a database.
These choices contribute to a more reliable and maintainable test suite.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/server/src/api/rest/index.ts (2)
1258-1267: LGTM: Separated API and Prisma ID key generation.The changes in
makeIdKey,makePrismaIdKey, andmakeCompoundIdmethods correctly implement the separation of concerns between API and Prisma ID handling. This approach maintains Prisma compatibility while allowing API flexibility.Consider adding a comment explaining why
prismaIdDivideris used inmakePrismaIdKeyto improve code clarity:private makePrismaIdKey(idFields: FieldInfo[]) { // TODO: support `@@id` with custom name + // Use prismaIdDivider to maintain compatibility with Prisma's internal ID handling return idFields.map((idf) => idf.name).join(prismaIdDivider); }
Line range hint
1233-1247: LGTM: Updated makePrismaIdFilter for compatibility and flexibility.The
makePrismaIdFiltermethod now correctly usesprismaIdDividerfor Prisma compatibility andthis.idDividerfor API flexibility. This approach ensures proper handling of IDs across both the API and Prisma layers.Consider adding a comment to explain the dual use of dividers for improved clarity:
private makePrismaIdFilter(idFields: FieldInfo[], resourceId: string) { if (idFields.length === 1) { return { [idFields[0].name]: this.coerce(idFields[0].type, resourceId) }; } else { return { // TODO: support `@@id` with custom name + // Use prismaIdDivider for Prisma compatibility and this.idDivider for API flexibility [idFields.map((idf) => idf.name).join(prismaIdDivider)]: idFields.reduce( (acc, curr, idx) => ({ ...acc, [curr.name]: this.coerce(curr.type, resourceId.split(this.idDivider)[idx]), }), {} ), }; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/server/src/api/rest/index.ts (25 hunks)
- packages/server/tests/api/rest.test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/server/src/api/rest/index.ts (10)
42-54: LGTM: New options for ID divider and URL segment charset.The additions of
idDividerandurlSegmentCharsetto the Options type are well-documented and provide the necessary flexibility for configuring the REST API handler.
96-96: LGTM: Constant for Prisma ID divider.The addition of
prismaIdDividerconstant is a good practice for maintaining consistency with Prisma's internal ID handling.
215-225: LGTM: Constructor updated to handle new options.The changes in the constructor properly initialize the new
idDividerproperty and dynamically generateurlPatterns. This approach provides the desired configurability while maintaining backward compatibility.
227-238: LGTM: Dynamic URL pattern generation.The
buildUrlPatternsmethod is a well-implemented solution for generating flexible URL patterns based on the provided options. This enhances the configurability of the API.
Line range hint
272-296: LGTM: Updated handleRequest to use dynamic URL patterns.The changes in the
handleRequestmethod correctly utilize the new dynamicurlPatterns, ensuring consistency with the configuredidDivider.
310-316: LGTM: POST request handling updated for dynamic URL patterns.The changes in POST request handling correctly use the new dynamic
urlPatterns, maintaining consistency with the GET request handling modifications.
Line range hint
340-354: LGTM: PUT/PATCH request handling updated for dynamic URL patterns.The changes in PUT/PATCH request handling correctly use the new dynamic
urlPatterns, maintaining consistency across all HTTP methods.
372-378: LGTM: DELETE request handling updated for dynamic URL patterns.The changes in DELETE request handling correctly use the new dynamic
urlPatterns, ensuring consistency across all CRUD operations.
1137-1137: LGTM: getId method updated to use configurable ID key.The
getIdmethod now correctly usesthis.makeIdKey(ids), ensuring consistency with the configuredidDivider.
Line range hint
1-1733: Overall assessment: Well-implemented configurable ID divider feature.The changes in this file consistently implement the new configurable ID divider feature across all relevant methods and operations. The approach taken maintains compatibility with Prisma's internal ID handling while allowing flexibility in the API layer. The separation of concerns between API and Prisma ID handling is well-executed.
Key points:
- New options (
idDividerandurlSegmentCharset) added to allow configuration.- Dynamic URL pattern generation implemented to support the configurable divider.
- All CRUD operations updated to use the new dynamic URL patterns.
- ID key generation methods updated to use the configurable divider for API operations while maintaining Prisma compatibility.
The implementation is thorough and consistent, with only minor suggestions for improving code clarity through additional comments.
Make id divider in the REST API generator configurable, in case the values contain the default
_divider.