feat: add ability to map model names in the URLS/JSON response #2187
Conversation
📝 Walkthrough""" WalkthroughThe changes introduce configurable model name mappings in the REST API request handler. URL pattern matching and URL generation are updated to use these mappings consistently, including adding an enum for URL patterns and private methods for matching URLs and reversing model name mappings. The OpenAPI generator is also updated to reflect these mappings in generated paths and operation IDs. A new test suite verifies the correct routing behavior with model name mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST API Handler
participant Internal Logic
Client->>REST API Handler: Sends request with external model name in URL
REST API Handler->>REST API Handler: matchUrlPattern applies modelNameMapping to resolve internal model name
REST API Handler->>Internal Logic: Processes request using internal model name
Internal Logic-->>REST API Handler: Returns response data
REST API Handler->>REST API Handler: mapModelName converts internal model name to external for URLs
REST API Handler->>Client: Sends response with URLs using external model names
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/api/rest/index.ts(19 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/server/src/api/rest/index.ts (1)
packages/runtime/src/local-helpers/lower-case-first.ts (1)
lowerCaseFirst(1-3)
🔇 Additional comments (3)
packages/server/src/api/rest/index.ts (3)
54-55: LGTM! Clean addition of optional configuration fields.The new optional properties follow the existing pattern and are well-documented in the PR objectives.
1200-1202: LGTM! Correctly includes prefix in generated URLs.The implementation properly prepends the prefix to all generated link URLs.
250-256: LGTM! Well-structured initialization of mappings.The constructor properly initializes the model name mappings with appropriate defaults and efficiently pre-computes the reverse mapping.
primary use case is pluralization ie. model User exposed as /users it is also useful in case the internal and external names should be different. TODO: adapt openapi plugin
lsmith77
force-pushed
the
model-name-mapping
branch
from
eb9332e to
c05e542
Compare
|
|
||
| const match = pattern.match(path); | ||
| if (match) { | ||
| match.type = this.modelNameMapping[match.type] ?? match.type; |
There was a problem hiding this comment.
When a mapping is provided, say "users" => "user", should access via unmapped path name like "POST /api/user" fail? I tend to think so - configuring a mapping requires you to only use the mapped name.
What do you think? Currently I believe both are valid.
There was a problem hiding this comment.
Indeed. It should fail. We only had the case in mind where only some of the models are mapped but not all.
So should we rather throw an exception if a model is not mapped if there are any mappings?
There was a problem hiding this comment.
I think supporting partial mapping is a good feature to have. What I meant is that, for the models that are configured with a mapping, the original name shouldn't be accessible anymore. E.g., if model "user" is mapped to "users", only "/api/users/..." should be valid, and "/api/user/..." should not.
There was a problem hiding this comment.
Ok. I guess we will check if the type is included in the reverseModelNameMapping. In that case I would return null so that it is handled as no match found.
There was a problem hiding this comment.
I now did some testing. At least with Nest.js it already returns a 404, since there is no /user route defined. Now if someone would then also define a /user route, indeed without change it would then work but at that point that seems to be intended by the user. So imho there is no need to handle this case inside the RequestHandler.
There was a problem hiding this comment.
My understanding is the mapping (in the latest code) is inverted.
I tested the latest code with this mapping:
{
user: 'myUser',
post: 'myPost',
}And this test is failing because of returning 200:
await expect(
handler({
method: 'post',
path: '/user',
query: {},
requestBody: { data: { type: 'user', attributes: { name: 'User1' } } },
prisma,
})
).resolves.toMatchObject({
status: 400,
});
It seems I can't push the test case commit directly. I've put it as a PR here: digital-sustainability#1 |
lsmith77
force-pushed
the
model-name-mapping
branch
2 times, most recently
from
486a165 to
e2573cf
Compare
I added one comment. Shouldn't using the unmapped model name cause a 404 and not a 400? |
I chose 400 originally based on the understanding that you're passing an "invalid" endpoint (not a valid but not-available one). On the hindsight, maybe 404 is better suited, but it'll be an unnecessary breaking change ... |
primary use case is pluralization ie. model User exposed as /users it is also useful in case the internal and external names should be different. TODO: adapt openapi plugin
lsmith77
force-pushed
the
model-name-mapping
branch
from
e2573cf to
7d03fdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/tests/api/rest.test.ts (1)
3049-3075: Test implementation is correct but could validate response content more thoroughly.The test logic correctly verifies that original model names fail (400) and mapped names succeed (201). However, it only validates status codes without checking response content.
Consider validating the response body content to ensure the mapping works correctly in all aspects:
- ).resolves.toMatchObject({ - status: 201, - }); + ).resolves.toMatchObject({ + status: 201, + body: { + data: { + type: 'user', + attributes: { name: 'User1' }, + links: { + self: expect.stringContaining('/myUser/') + } + } + } + });This would ensure that:
- The response body contains the expected data
- URLs in the response use the mapped model names
- The
typefield still uses the original model name (as per the retrieved learnings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/api/rest/index.ts(18 hunks)packages/server/tests/api/rest.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ymc9
PR: zenstackhq/zenstack#2187
File: packages/server/src/api/rest/index.ts:287-289
Timestamp: 2025-07-16T03:09:04.681Z
Learning: In REST API URL pattern matching with model name mapping, only the `type` parameter (representing model names) should be mapped using `modelNameMapping`. The `relationship` parameter represents field names on models and should not be mapped, as field names are distinct from model names in the API structure.
packages/server/tests/api/rest.test.ts (1)
<retrieved_learning>
Learnt from: ymc9
PR: #2187
File: packages/server/src/api/rest/index.ts:287-289
Timestamp: 2025-07-16T03:09:04.681Z
Learning: In REST API URL pattern matching with model name mapping, only the type parameter (representing model names) should be mapped using modelNameMapping. The relationship parameter represents field names on models and should not be mapped, as field names are distinct from model names in the API structure.
</retrieved_learning>
packages/server/src/api/rest/index.ts (1)
Learnt from: ymc9
PR: zenstackhq/zenstack#2187
File: packages/server/src/api/rest/index.ts:287-289
Timestamp: 2025-07-16T03:09:04.681Z
Learning: In REST API URL pattern matching with model name mapping, only the `type` parameter (representing model names) should be mapped using `modelNameMapping`. The `relationship` parameter represents field names on models and should not be mapped, as field names are distinct from model names in the API structure.
🧬 Code Graph Analysis (1)
packages/server/src/api/rest/index.ts (1)
packages/runtime/src/local-helpers/lower-case-first.ts (1)
lowerCaseFirst(1-3)
🔇 Additional comments (16)
packages/server/src/api/rest/index.ts (16)
54-54: LGTM! Model name mapping option added correctly.The optional
modelNameMappingproperty is properly typed as a record mapping strings to strings, which is appropriate for mapping external API model names to internal model names.
70-81: Well-structured type definitions for URL pattern matching.The simplified
Matchtype andUrlPatternsenum provide clean abstractions for URL pattern matching. The enum values clearly correspond to different REST API route patterns.
238-241: Appropriate private fields for URL pattern and model name mapping.The private fields properly encapsulate the URL pattern map and both forward and reverse model name mappings, which are needed for bidirectional mapping during request processing and URL generation.
247-252: Correct initialization of model name mappings.The constructor properly initializes both forward and reverse mappings, with the reverse mapping correctly constructed by swapping keys and values from the forward mapping. The URL pattern map is built with the appropriate charset configuration.
254-270: Clean URL pattern building with proper path construction.The
buildUrlPatternMapmethod correctly constructs URL patterns for different REST API endpoints. ThebuildPathhelper function properly joins path segments with forward slashes.
272-287: Correct implementation of model name mapping logic.The
reverseModelNameMapmethod properly handles cases where no mapping exists by returning the original type. ThematchUrlPatternmethod correctly applies forward mapping only to thetypeparameter (line 284), which aligns with the retrieved learning that field names should not be mapped.
320-347: Consistent use of centralized URL pattern matching.The GET request handling correctly uses the new
matchUrlPatternmethod for all URL pattern types, maintaining consistency across different endpoint patterns.
356-396: Proper URL pattern matching in POST request handling.The POST request handling consistently uses the centralized
matchUrlPatternmethod for both collection and relationship endpoints.
405-432: Correct URL pattern matching in PUT/PATCH request handling.The PUT/PATCH request handling properly uses the centralized URL pattern matching for both single resource and relationship endpoints.
436-456: Consistent URL pattern matching in DELETE request handling.The DELETE request handling correctly uses the centralized URL pattern matching approach.
572-577: Correct reverse mapping for URL generation.The reverse mapping is properly applied to generate URLs with the external model name for the document linker. This ensures API responses contain URLs using the mapped model names.
624-638: Proper reverse mapping in relationship URL generation.The code correctly uses reverse mapping to generate URLs for relationship endpoints, ensuring consistency in URL structure throughout the API responses.
723-724: Correct reverse mapping for collection URL generation.The reverse mapping is properly applied when generating paginated collection URLs, maintaining consistency with the external model name mapping.
1053-1059: Proper reverse mapping in relationship CRUD operations.The reverse mapping is correctly applied when generating URLs for relationship CRUD operation responses, ensuring consistent URL structure.
1204-1213: Correct reverse mapping in serializer building.The reverse mapping is properly applied when building serializers to ensure resource links use the external model names consistently throughout the API.
1257-1279: Consistent reverse mapping in relator URL generation.The reverse mapping is correctly applied when building relator URLs for relationship links, ensuring all relationship URLs use the mapped model names.
| describe('REST server tests - model name mapping', () => { | ||
| const schema = ` | ||
| model User { | ||
| id String @id @default(cuid()) | ||
| name String | ||
| posts Post[] | ||
| } | ||
| model Post { | ||
| id String @id @default(cuid()) | ||
| title String | ||
| author User? @relation(fields: [authorId], references: [id]) | ||
| authorId String? | ||
| } | ||
| `; | ||
| beforeAll(async () => { | ||
| const params = await loadSchema(schema); | ||
| prisma = params.prisma; | ||
| zodSchemas = params.zodSchemas; | ||
| modelMeta = params.modelMeta; | ||
|
|
||
| const _handler = makeHandler({ | ||
| endpoint: 'http://localhost/api', | ||
| modelNameMapping: { | ||
| myUser: 'user', | ||
| myPost: 'post', | ||
| }, | ||
| }); | ||
| handler = (args) => | ||
| _handler({ ...args, zodSchemas, modelMeta, url: new URL(`http://localhost/${args.path}`) }); | ||
| }); | ||
|
|
||
| it('works with name mapping', async () => { | ||
| // using original model name | ||
| await expect( | ||
| handler({ | ||
| method: 'post', | ||
| path: '/user', | ||
| query: {}, | ||
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | ||
| prisma, | ||
| }) | ||
| ).resolves.toMatchObject({ | ||
| status: 400, | ||
| }); | ||
|
|
||
| // using mapped model name | ||
| await expect( | ||
| handler({ | ||
| method: 'post', | ||
| path: '/myUser', | ||
| query: {}, | ||
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | ||
| prisma, | ||
| }) | ||
| ).resolves.toMatchObject({ | ||
| status: 201, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good foundation for model name mapping tests, but coverage could be expanded.
The test suite correctly validates the basic model name mapping functionality. However, the test coverage is quite limited and could be expanded to ensure the feature works comprehensively.
Consider expanding the test coverage to include:
- Test the second mapping: The
myPostmapping is configured but not tested - Test other HTTP methods: Currently only POST is tested
- Test relationship endpoints: Verify that
/myUser/{id}/postsworks correctly - Validate response consistency: Ensure URLs in response bodies use mapped names
- Test error scenarios: What happens when accessing unmapped model names
+ it('works with both mappings and all HTTP methods', async () => {
+ // Create user with mapped name
+ const userResponse = await handler({
+ method: 'post',
+ path: '/myUser',
+ query: {},
+ requestBody: { data: { type: 'user', attributes: { name: 'User1' } } },
+ prisma,
+ });
+ expect(userResponse.status).toBe(201);
+ const userId = userResponse.body.data.id;
+
+ // Create post with mapped name
+ const postResponse = await handler({
+ method: 'post',
+ path: '/myPost',
+ query: {},
+ requestBody: { data: { type: 'post', attributes: { title: 'Post1', authorId: userId } } },
+ prisma,
+ });
+ expect(postResponse.status).toBe(201);
+
+ // Test GET with mapped names
+ const getUserResponse = await handler({
+ method: 'get',
+ path: '/myUser',
+ prisma,
+ });
+ expect(getUserResponse.status).toBe(200);
+
+ // Test relationship endpoints
+ const relationshipResponse = await handler({
+ method: 'get',
+ path: `/myUser/${userId}/posts`,
+ prisma,
+ });
+ expect(relationshipResponse.status).toBe(200);
+ });
+
+ it('validates response URLs use mapped names', async () => {
+ const response = await handler({
+ method: 'post',
+ path: '/myUser',
+ query: {},
+ requestBody: { data: { type: 'user', attributes: { name: 'User1' } } },
+ prisma,
+ });
+
+ expect(response.body.data.links.self).toContain('/myUser/');
+ expect(response.body.data.relationships.posts.links.related).toContain('/myUser/');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('REST server tests - model name mapping', () => { | |
| const schema = ` | |
| model User { | |
| id String @id @default(cuid()) | |
| name String | |
| posts Post[] | |
| } | |
| model Post { | |
| id String @id @default(cuid()) | |
| title String | |
| author User? @relation(fields: [authorId], references: [id]) | |
| authorId String? | |
| } | |
| `; | |
| beforeAll(async () => { | |
| const params = await loadSchema(schema); | |
| prisma = params.prisma; | |
| zodSchemas = params.zodSchemas; | |
| modelMeta = params.modelMeta; | |
| const _handler = makeHandler({ | |
| endpoint: 'http://localhost/api', | |
| modelNameMapping: { | |
| myUser: 'user', | |
| myPost: 'post', | |
| }, | |
| }); | |
| handler = (args) => | |
| _handler({ ...args, zodSchemas, modelMeta, url: new URL(`http://localhost/${args.path}`) }); | |
| }); | |
| it('works with name mapping', async () => { | |
| // using original model name | |
| await expect( | |
| handler({ | |
| method: 'post', | |
| path: '/user', | |
| query: {}, | |
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | |
| prisma, | |
| }) | |
| ).resolves.toMatchObject({ | |
| status: 400, | |
| }); | |
| // using mapped model name | |
| await expect( | |
| handler({ | |
| method: 'post', | |
| path: '/myUser', | |
| query: {}, | |
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | |
| prisma, | |
| }) | |
| ).resolves.toMatchObject({ | |
| status: 201, | |
| }); | |
| }); | |
| }); | |
| describe('REST server tests - model name mapping', () => { | |
| const schema = ` | |
| model User { | |
| id String @id @default(cuid()) | |
| name String | |
| posts Post[] | |
| } | |
| model Post { | |
| id String @id @default(cuid()) | |
| title String | |
| author User? @relation(fields: [authorId], references: [id]) | |
| authorId String? | |
| } | |
| `; | |
| beforeAll(async () => { | |
| const params = await loadSchema(schema); | |
| prisma = params.prisma; | |
| zodSchemas = params.zodSchemas; | |
| modelMeta = params.modelMeta; | |
| const _handler = makeHandler({ | |
| endpoint: 'http://localhost/api', | |
| modelNameMapping: { | |
| myUser: 'user', | |
| myPost: 'post', | |
| }, | |
| }); | |
| handler = (args) => | |
| _handler({ ...args, zodSchemas, modelMeta, url: new URL(`http://localhost/${args.path}`) }); | |
| }); | |
| it('works with name mapping', async () => { | |
| // using original model name | |
| await expect( | |
| handler({ | |
| method: 'post', | |
| path: '/user', | |
| query: {}, | |
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | |
| prisma, | |
| }) | |
| ).resolves.toMatchObject({ | |
| status: 400, | |
| }); | |
| // using mapped model name | |
| await expect( | |
| handler({ | |
| method: 'post', | |
| path: '/myUser', | |
| query: {}, | |
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | |
| prisma, | |
| }) | |
| ).resolves.toMatchObject({ | |
| status: 201, | |
| }); | |
| }); | |
| it('works with both mappings and all HTTP methods', async () => { | |
| // Create user with mapped name | |
| const userResponse = await handler({ | |
| method: 'post', | |
| path: '/myUser', | |
| query: {}, | |
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | |
| prisma, | |
| }); | |
| expect(userResponse.status).toBe(201); | |
| const userId = userResponse.body.data.id; | |
| // Create post with mapped name | |
| const postResponse = await handler({ | |
| method: 'post', | |
| path: '/myPost', | |
| query: {}, | |
| requestBody: { data: { type: 'post', attributes: { title: 'Post1', authorId: userId } } }, | |
| prisma, | |
| }); | |
| expect(postResponse.status).toBe(201); | |
| // Test GET with mapped names | |
| const getUserResponse = await handler({ | |
| method: 'get', | |
| path: '/myUser', | |
| prisma, | |
| }); | |
| expect(getUserResponse.status).toBe(200); | |
| // Test relationship endpoints | |
| const relationshipResponse = await handler({ | |
| method: 'get', | |
| path: `/myUser/${userId}/posts`, | |
| prisma, | |
| }); | |
| expect(relationshipResponse.status).toBe(200); | |
| }); | |
| it('validates response URLs use mapped names', async () => { | |
| const response = await handler({ | |
| method: 'post', | |
| path: '/myUser', | |
| query: {}, | |
| requestBody: { data: { type: 'user', attributes: { name: 'User1' } } }, | |
| prisma, | |
| }); | |
| expect(response.body.data.links.self).toContain('/myUser/'); | |
| expect(response.body.data.relationships.posts.links.related).toContain('/myUser/'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In packages/server/tests/api/rest.test.ts between lines 3017 and 3076, the
existing test only covers POST requests for the 'myUser' mapped model and does
not test the 'myPost' mapping or other HTTP methods. Expand the test suite by
adding tests for the 'myPost' mapping, include GET, PUT, DELETE methods, and
test relationship endpoints like '/myUser/{id}/posts'. Also, add assertions to
verify that response URLs use the mapped model names consistently and include
tests for error scenarios when accessing unmapped model names to ensure
comprehensive coverage of the model name mapping feature.
|
@ymc9 I have now also updated the openapi plugin. As part of this I realized it would make more sense to have the model mapping look like this: rather than |
lsmith77
force-pushed
the
model-name-mapping
branch
from
15cfdc4 to
92fdd9a
Compare
|
@ymc9 is there anything else you need from us before you can merge this? |
implements #2178
this PR also adds support for a
prefixsimilar to the openapi plugin. note openapi plugin itself still needs to be updated, to also use this model name mapping.also we did not add any new tests. would appreciate any hints on that.
finally we will of course also be happy to provide documentation for this.
/cc @tobiasbrugger