Add nested routes support in the RestApiHandler#2454
Open
lsmith77 wants to merge 1 commit intozenstackhq:devfrom
Open
Add nested routes support in the RestApiHandler#2454lsmith77 wants to merge 1 commit intozenstackhq:devfrom
lsmith77 wants to merge 1 commit intozenstackhq:devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds explicit nested route support to RestApiHandler via a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/server/test/api/rest.test.ts (1)
3932-3972: Strengthen mapped nested-route coverage with mutation + link assertions.This block currently checks mapped GET only. Adding mapped POST/PATCH/DELETE assertions (plus nested
links.self) would protect against mapped-segment dispatch regressions.🧪 Example assertion to add
+ const created = await mappedHandler({ + method: 'post', + path: '/users/u1/posts', + client: nestedClient, + requestBody: { + data: { type: 'Post', attributes: { title: 'mapped-created' } }, + }, + }); + expect(created.status).toBe(201); + expect(created.body.data.links.self).toContain('/users/u1/posts/');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/api/rest.test.ts` around lines 3932 - 3972, The test only asserts mapped GET behavior; add assertions that exercise mapped nested-route mutations and link metadata to prevent dispatch regressions: use RestApiHandler with modelNameMapping and nestedRoutes (the mappedApi and mappedHandler helpers) to send POST to /users/u1/posts (create a child), PATCH to update that child via /users/u1/posts/{id}, and DELETE to remove it, asserting status codes and that nested list changes accordingly; additionally assert the response includes nested links.self pointing to the mapped nested URL (e.g., /users/{parentId}/posts) after each relevant response to verify link wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rest/index.ts`:
- Around line 1562-1578: The code currently only rejects setting the
parent-controlled relation via relationships but still allows scalar FK
attributes; update the validation inside the forcedParentRelation block to also
detect any scalar FK fields from forcedParentRelation.parentIdFields present in
createPayload.data and return the same makeError when found. In practice, in the
forcedParentRelation branch check createPayload.data for any keys matching each
entry in forcedParentRelation.parentIdFields (in addition to the existing
relationships check) and call this.makeError('invalidPayload', ...) if any are
present; otherwise proceed to set
createPayload.data[forcedParentRelation.relation] = { connect:
this.makeIdConnect(...) } and include the select via this.makeDefaultIdKey(...)
as before.
- Around line 1303-1311: After building the nested parent filter in the
nested-create flow, add an explicit existence check for the parent before
performing the mutation: use getModelInfo(parentType) to get the parent model
metadata, reuse the filter produced by buildNestedParentFilter(parentType,
parentId, ...) to query the underlying data store via your model/adapter read
method, and if the parent row is not found return a 404-style error (consistent
with your API) instead of proceeding; this ensures nested POST returns an
explicit "parent not found" response rather than relying on downstream ORM
errors.
- Around line 1366-1373: The current guard only checks body.data.relationships
and uses the `in` operator unsafely; update the nested-patch guard in the
handler that inspects requestBody to (1) ensure body?.data?.relationships is an
object before using `in` (e.g., typeof === 'object' && body.data.relationships
!== null) or use hasOwnProperty, and (2) also inspect body?.data?.attributes for
scalar FK fields that could re-parent (cover common patterns for the FK name
derived from nestedRouteConfig.relation such as
`${nestedRouteConfig.relation}Id` and `${nestedRouteConfig.relation}_id`) and
call this.makeError('invalidPayload', ...) if any of those are present;
reference nestedRouteConfig.relation, requestBody/body, body.data.relationships,
body.data.attributes, and this.makeError to locate and update the logic.
---
Nitpick comments:
In `@packages/server/test/api/rest.test.ts`:
- Around line 3932-3972: The test only asserts mapped GET behavior; add
assertions that exercise mapped nested-route mutations and link metadata to
prevent dispatch regressions: use RestApiHandler with modelNameMapping and
nestedRoutes (the mappedApi and mappedHandler helpers) to send POST to
/users/u1/posts (create a child), PATCH to update that child via
/users/u1/posts/{id}, and DELETE to remove it, asserting status codes and that
nested list changes accordingly; additionally assert the response includes
nested links.self pointing to the mapped nested URL (e.g.,
/users/{parentId}/posts) after each relevant response to verify link wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28be15c8-cc27-4592-8c39-1e2203f23b8d
📒 Files selected for processing (3)
packages/server/src/api/rest/index.tspackages/server/test/api/options-validation.test.tspackages/server/test/api/rest.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/api/rest/index.ts (1)
1373-1390:⚠️ Potential issue | 🟠 MajorHarden nested PATCH FK guard using schema-derived FK fields (not naming heuristics).
Line 1383 only checks
authorId/author_id-style keys. Custom or compound FK fields can still re-parent throughattributes, bypassing nested route constraints.💡 Proposed fix
const rel = nestedRouteConfig.relation; @@ -const fkCandidates = [`${rel}Id`, `${rel}_id`]; +const fkCandidates = [ + ...(this.schema.models[typeInfo.name]?.fields[rel]?.relation?.fields ?? []), + ...Object.values(typeInfo.fields) + .filter((f) => f.foreignKeyFor?.includes(rel)) + .map((f) => f.name), +]; if ( typeof body?.data?.attributes === 'object' && body.data.attributes !== null && fkCandidates.some((fk) => Object.prototype.hasOwnProperty.call(body.data.attributes, fk)) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rest/index.ts` around lines 1373 - 1390, The nested PATCH guard currently only checks heuristic FK names and can be bypassed; update the check in the handler that uses requestBody/body to compute FK candidate fields from the schema/nestedRouteConfig instead of hardcoded `${rel}Id`/`${rel}_id`: call the schema or ORM helper that returns actual foreign-key field names for nestedRouteConfig.relation (e.g., a method like schema.getForeignKeyFieldsForRelation or nestedRouteConfig.foreignKeys), then test those real FK field names against body.data.attributes and body.data.relationships and return this.makeError('invalidPayload', ...) if any are present; keep the existing early-return behavior and use nestedRouteConfig.relation and makeError symbols to locate and update the logic.
🧹 Nitpick comments (1)
packages/server/test/api/rest.test.ts (1)
4022-4062: Add mapped nested-create coverage to prove mapped 3-segment nested routing.This block currently validates mapped
GET /users/:id/posts, which can still pass via non-nested fallback behavior. Add a mapped nested create assertion (POST /users/:id/posts) and verify nested self-links to ensure mapped nested routing is truly exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/api/rest.test.ts` around lines 4022 - 4062, Add a mapped nested-create assertion and self-link checks to ensure mapped 3-segment nested routing is exercised: use the existing mappedHandler created from RestApiHandler/mappedApi to POST to '/users/u1/posts' (create a post for u1), assert the response status is 201 (or created) and the returned resource contains the expected attributes (title 'mapped-post' or similar), then fetch the collection via GET '/users/u1/posts' (or inspect the POST response) and assert the nested self-links/relationships reference the mapped route (e.g., self or related links include '/users/u1/posts') and that the created post appears in the collection; reference mappedApi, mappedHandler and nestedClient when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/server/src/api/rest/index.ts`:
- Around line 1373-1390: The nested PATCH guard currently only checks heuristic
FK names and can be bypassed; update the check in the handler that uses
requestBody/body to compute FK candidate fields from the
schema/nestedRouteConfig instead of hardcoded `${rel}Id`/`${rel}_id`: call the
schema or ORM helper that returns actual foreign-key field names for
nestedRouteConfig.relation (e.g., a method like
schema.getForeignKeyFieldsForRelation or nestedRouteConfig.foreignKeys), then
test those real FK field names against body.data.attributes and
body.data.relationships and return this.makeError('invalidPayload', ...) if any
are present; keep the existing early-return behavior and use
nestedRouteConfig.relation and makeError symbols to locate and update the logic.
---
Nitpick comments:
In `@packages/server/test/api/rest.test.ts`:
- Around line 4022-4062: Add a mapped nested-create assertion and self-link
checks to ensure mapped 3-segment nested routing is exercised: use the existing
mappedHandler created from RestApiHandler/mappedApi to POST to '/users/u1/posts'
(create a post for u1), assert the response status is 201 (or created) and the
returned resource contains the expected attributes (title 'mapped-post' or
similar), then fetch the collection via GET '/users/u1/posts' (or inspect the
POST response) and assert the nested self-links/relationships reference the
mapped route (e.g., self or related links include '/users/u1/posts') and that
the created post appears in the collection; reference mappedApi, mappedHandler
and nestedClient when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32ecfa26-53fe-43ca-944c-46d23f105cae
📒 Files selected for processing (3)
packages/server/src/api/rest/index.tspackages/server/test/api/options-validation.test.tspackages/server/test/api/rest.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/test/api/options-validation.test.ts
lsmith77
force-pushed
the
nested-routes
branch
2 times, most recently
from
80c9dd6 to
8c9a4a1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server/test/api/rest.test.ts (1)
3996-4036: Add a regression test for mapped child names withoutnestedRoutes.This block validates mapped nested routes, but it doesn’t cover the flat-route fallback case when
modelNameMappingis set andnestedRoutesis absent. Please add a case likeGET /users/:id/posts(withPost -> postsmapping, no nested config) to ensure regularfetchRelatedstays intact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/api/rest.test.ts` around lines 3996 - 4036, Add a new regression test in the same suite that verifies mapped child names work when modelNameMapping is present but nestedRoutes is omitted: instantiate RestApiHandler with schema: nestedClient.$schema, endpoint, and modelNameMapping: { User: 'users', Post: 'posts' } but no nestedRoutes, create users and a post via nestedClient.user.create, then call the handler (reuse mappedHandler pattern) with method 'get' and path `/users/u1/posts` and assert status 200 and that the returned collection contains the created post, and also call `/users/u2/posts` and assert it returns an empty list; this ensures the flat-route fallback (fetchRelated behavior) works for mapped child names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rest/index.ts`:
- Around line 509-513: The code is mutating match.relationship with a model-name
alias which breaks flat 3-segment routes and the processFetchRelated fallback;
instead of rewriting match.relationship, compute a local mappedRelationship
(e.g. const mappedRelationship =
this.reverseModelNameMapping[match.relationship] || match.relationship) and use
that local value only when resolving nested-route model dispatch, leaving
match.relationship untouched so processFetchRelated still receives the original
route segment; apply the same non-mutating change to the other occurrences
around the blocks that reference reverseModelNameMapping (the spots analogous to
the current match.relationship rewrite).
- Line 594: The variable declaration uses let for values that are never
reassigned (e.g., nestedMatch created from this.matchUrlPattern(path,
UrlPatterns.FETCH_RELATIONSHIP)); change these to const. Update the three
occurrences in this module where a value is assigned from this.matchUrlPattern
or similar pattern-matching calls (including the nestedMatch declaration and the
two other let declarations reported) to use const instead of let so they satisfy
the prefer-const lint rule.
---
Nitpick comments:
In `@packages/server/test/api/rest.test.ts`:
- Around line 3996-4036: Add a new regression test in the same suite that
verifies mapped child names work when modelNameMapping is present but
nestedRoutes is omitted: instantiate RestApiHandler with schema:
nestedClient.$schema, endpoint, and modelNameMapping: { User: 'users', Post:
'posts' } but no nestedRoutes, create users and a post via
nestedClient.user.create, then call the handler (reuse mappedHandler pattern)
with method 'get' and path `/users/u1/posts` and assert status 200 and that the
returned collection contains the created post, and also call `/users/u2/posts`
and assert it returns an empty list; this ensures the flat-route fallback
(fetchRelated behavior) works for mapped child names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5737dc7f-5d4d-47f4-bd8d-c5698cabdc0a
📒 Files selected for processing (3)
packages/server/src/api/rest/index.tspackages/server/test/api/options-validation.test.tspackages/server/test/api/rest.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/test/api/options-validation.test.ts
| if (!requestBody) { | ||
| return this.makeError('invalidPayload'); | ||
| } | ||
| let nestedMatch = this.matchUrlPattern(path, UrlPatterns.FETCH_RELATIONSHIP); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 700 packages/server/src/api/rest/index.ts | tail -n 120 | cat -nRepository: zenstackhq/zenstack
Length of output: 6416
🏁 Script executed:
sed -n '590,600p;640,650p;680,690p' packages/server/src/api/rest/index.ts | cat -nRepository: zenstackhq/zenstack
Length of output: 1948
Change let to const for variables that are never reassigned.
Lines 594, 644, and 685 declare variables with let that are never reassigned after initialization. Change these to const to comply with the prefer-const linting rule.
Changes needed
- let nestedMatch = this.matchUrlPattern(path, UrlPatterns.FETCH_RELATIONSHIP);
+ const nestedMatch = this.matchUrlPattern(path, UrlPatterns.FETCH_RELATIONSHIP);
@@
- let nestedPatchMatch = this.matchUrlPattern(path, UrlPatterns.NESTED_SINGLE);
+ const nestedPatchMatch = this.matchUrlPattern(path, UrlPatterns.NESTED_SINGLE);
@@
- let nestedDeleteMatch = this.matchUrlPattern(path, UrlPatterns.NESTED_SINGLE);
+ const nestedDeleteMatch = this.matchUrlPattern(path, UrlPatterns.NESTED_SINGLE);📝 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.
Suggested change
| let nestedMatch = this.matchUrlPattern(path, UrlPatterns.FETCH_RELATIONSHIP); | |
| const nestedMatch = this.matchUrlPattern(path, UrlPatterns.FETCH_RELATIONSHIP); |
🧰 Tools
🪛 GitHub Actions: Build and Test
[error] 594-594: ESLint: 'nestedMatch' is defined but never reassigned. Use 'const' instead. (prefer-const)
🪛 GitHub Check: build-test (22.x, mysql)
[failure] 594-594:
'nestedMatch' is never reassigned. Use 'const' instead
🪛 GitHub Check: build-test (22.x, postgresql)
[failure] 594-594:
'nestedMatch' is never reassigned. Use 'const' instead
🪛 GitHub Check: build-test (22.x, sqlite)
[failure] 594-594:
'nestedMatch' is never reassigned. Use 'const' instead
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/api/rest/index.ts` at line 594, The variable declaration
uses let for values that are never reassigned (e.g., nestedMatch created from
this.matchUrlPattern(path, UrlPatterns.FETCH_RELATIONSHIP)); change these to
const. Update the three occurrences in this module where a value is assigned
from this.matchUrlPattern or similar pattern-matching calls (including the
nestedMatch declaration and the two other let declarations reported) to use
const instead of let so they satisfy the prefer-const lint rule.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rest/index.ts`:
- Around line 1369-1375: The existence check uses (client as
any)[childType].findFirst with where:
mergeFilters(makeIdFilter(typeInfo.idFields, childId), nestedFilter) but the
subsequent mutation (e.g., (client as any)[childType].update / delete) only uses
the child ID; to prevent TOCTOU, change the mutation's where clause to the same
merged filter (use this.mergeFilters(this.makeIdFilter(typeInfo.idFields,
childId), nestedFilter)) so the update/delete is atomically scoped to the parent
filter (apply the same change to the other occurrence around the update/delete
call referenced in the diff).
- Around line 1434-1442: The current nested DELETE checks membership with
findFirst then calls this.processDelete(client, childType, childId), which
introduces a TOCTOU race; instead delete using a parent-scoped where that
combines the child's id filter and the nestedFilter in a single operation.
Update the call site to pass a merged where (use
this.mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter))
into the delete operation (or adjust processDelete to accept and use a where
object rather than just childId) so deletion is atomic and scoped to the parent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e8f8d98-65fb-43b0-8e70-8ee926068b46
📒 Files selected for processing (3)
packages/server/src/api/rest/index.tspackages/server/test/api/options-validation.test.tspackages/server/test/api/rest.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/server/test/api/rest.test.ts
- packages/server/test/api/options-validation.test.ts
Comment on lines
+1369
to
+1375
| // Verify child belongs to parent before updating | ||
| const exists = await (client as any)[childType].findFirst({ | ||
| where: this.mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter), | ||
| }); | ||
| if (!exists) { | ||
| return this.makeError('notFound'); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid TOCTOU in nested PATCH by scoping mutation atomically to parent filter.
Line 1369 checks parent-scoped existence, but Line 1401 performs update by child ID only. A concurrent re-parent between those calls can let this nested endpoint update a child no longer under parentId.
💡 Suggested direction
- // Verify child belongs to parent before updating
- const exists = await (client as any)[childType].findFirst({ where: scopedWhere });
- if (!exists) return this.makeError('notFound');
- return this.processUpdate(client, childType, childId, query, requestBody, linkers);
+// Perform scoped mutation in a single parent-constrained operation
+// (e.g. scoped updateMany + fetch-for-response, or a dedicated scoped update helper)
+return this.processNestedScopedUpdate(
+ client,
+ childType,
+ childId,
+ scopedWhere,
+ query,
+ requestBody,
+ linkers
+);The key is: mutation where must include both child ID and nestedFilter.
Also applies to: 1401-1404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/api/rest/index.ts` around lines 1369 - 1375, The
existence check uses (client as any)[childType].findFirst with where:
mergeFilters(makeIdFilter(typeInfo.idFields, childId), nestedFilter) but the
subsequent mutation (e.g., (client as any)[childType].update / delete) only uses
the child ID; to prevent TOCTOU, change the mutation's where clause to the same
merged filter (use this.mergeFilters(this.makeIdFilter(typeInfo.idFields,
childId), nestedFilter)) so the update/delete is atomically scoped to the parent
filter (apply the same change to the other occurrence around the update/delete
call referenced in the diff).
Comment on lines
+1434
to
+1442
| // Verify child belongs to parent before deleting | ||
| const exists = await (client as any)[childType].findFirst({ | ||
| where: this.mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter), | ||
| }); | ||
| if (!exists) { | ||
| return this.makeError('notFound'); | ||
| } | ||
|
|
||
| return this.processDelete(client, childType, childId); |
Contributor
There was a problem hiding this comment.
Avoid TOCTOU in nested DELETE by deleting with parent-scoped where.
Line 1434 validates membership first, then Line 1442 deletes by child ID only. This has the same race window as PATCH and can delete a child after it’s re-parented.
💡 Suggested fix
- // Verify child belongs to parent before deleting
- const exists = await (client as any)[childType].findFirst({
- where: this.mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter),
- });
- if (!exists) {
- return this.makeError('notFound');
- }
-
- return this.processDelete(client, childType, childId);
+const deleteResult = await (client as any)[childType].deleteMany({
+ where: this.mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter),
+});
+if (!deleteResult.count) {
+ return this.makeError('notFound');
+}
+return {
+ status: 200,
+ body: { meta: {} },
+};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/api/rest/index.ts` around lines 1434 - 1442, The current
nested DELETE checks membership with findFirst then calls
this.processDelete(client, childType, childId), which introduces a TOCTOU race;
instead delete using a parent-scoped where that combines the child's id filter
and the nestedFilter in a single operation. Update the call site to pass a
merged where (use this.mergeFilters(this.makeIdFilter(typeInfo.idFields,
childId), nestedFilter)) into the delete operation (or adjust processDelete to
accept and use a where object rather than just childId) so deletion is atomic
and scoped to the parent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #2453
Note: this was largely AI developed. I wanted to offer it as a more concrete proposal than just the above referenced ticket. If you agree this feature has merit, I will do a more detailed analysis of the proposed code changes.
TOCTOU Race Condition in Nested Mutations
The nested update/delete operations verify child-parent membership using a merged filter, then perform the mutation using only the child ID. In a concurrent scenario where the parent relationship changes between check and mutation, the operation could target an orphaned or reassigned resource.
Options Explored:
Why Not Fixed:
Mitigation:
The authorization check prevents unauthorized cross-parent access (the primary security concern). The window for this race is extremely narrow and requires concurrent parent-relationship changes—a rare operation pattern.
Summary by CodeRabbit
New Features
Bug Fixes
Tests