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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningspackages/server/src/api/rest/index.ts (1)🧬 Code Graph Analysis (1)packages/server/src/api/rest/index.ts (1)
🔇 Additional comments (12)
✨ 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
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.
|
@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? |
Hi @lsmith77, it seems the newly added test case is still failing. Also I believe the mapping direction in the test case should be updated based on the latest changes. Could you help fix that, then I think we can merge it. I'll make a follow up PR to add more test cases. |
Ah yes, sorry. I didn't take ownership of the test within the PR. I have updated the mapping. I have also now added explicit code to check for the old path (which IMHO should be done by the framework's router and not really here). But still I am unable to get the tests to pass. Maybe I am running the tests incorrectly. What I did was: Which results in: |
That's what I meant in a previous comment: even with the mapping "user" -> "myUser", access to route "/user" is still valid given the current implementation, which should actually fail. If a model name is mapped, only the mapped route is accessible. What do you think? |
|
This aspect I do not understand.
Now for 2), I guess maybe that logic is not sufficient and rather it should trigger an error response directly. But due to 1), I am not even convinced this case should even be handled explicitly at all. |
I think I understand where the gap is 😄. The rest api handler is framework agnostic and is supposed to work with different frameworks. For Nest.js, people would explicitly define "myUser" route, however for other frameworks, e.g., Express.js, one would simply mount the api handler to a parent route and let it handle whatever request is sent to it: // forward all route under "/api/model" to the underlying request handler
app.use(
'/api/model',
ZenStackMiddleware({
...
})
);That's why I think the api handler itself should have a consistent handling of mapped model names, regardless the outer framework rejects an invalid route or not. I.e., if "user" is mapped to "myUser", then "/user" route shouldn't handle any request anymore, at the api handler level. |
The relationship between framework and api handler is depicted here: https://zenstack.dev/docs/reference/server-adapters/api-handlers/ |
lsmith77
force-pushed
the
model-name-mapping
branch
from
3729fce to
5bc08d6
Compare
|
OK, I am now throwing an exception in this case and the tests are now passing. |
3 participants
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