Add nested routes support in the RestApiHandler#2454
Add nested routes support in the RestApiHandler#2454lsmith77 wants to merge 3 commits intozenstackhq:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds explicit nested route support to RestApiHandler via a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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
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); |
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.
| 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.
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
|
Hi @lsmith77 , thanks for making this PR and my apologies for the late follow up. I can understand it's problematic that resource mutations cannot be scoped (atomically) to a parent. Since it's not part of JSON:API spec, we'll need to deviate and make an extension. For the URL design, I'm wondering if, instead of having a set of parallel URL patterns for scoped updates, it's simpler to just extend the current ones. We already have today:
Can we just extend the second one to allow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/server/src/api/rest/index.ts (2)
1340-1347:⚠️ Potential issue | 🔴 CriticalNested DELETE paths still perform unscoped delete after a scoped check.
Both flows verify parent membership first, then delete by child ID only (
processDelete). This leaves the same TOCTOU window for concurrent relation changes and can delete outside the requested parent scope.Also applies to: 1373-1378
🤖 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 1340 - 1347, The nested DELETE flow currently checks parent membership using (client as any)[childType].findFirst with mergeFilters(this.makeIdFilter(...), nestedFilter) but then calls processDelete(childType, childId) which deletes by ID only and creates a TOCTOU risk; change the deletion to apply the same combined filter used in the existence check (i.e., perform delete where mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter)) instead of deleting by ID alone—either update processDelete to accept a filter object (or an optional nestedFilter parameter) and route the nested-delete path to call it with the merged filter, or call the client deleteMany on (client as any)[childType] with the merged filter directly so the check-and-delete is atomic with respect to the same scope.
1289-1317:⚠️ Potential issue | 🔴 CriticalNested PATCH still has a TOCTOU race between membership check and mutation.
Line 1290 scopes the existence check to
{ childId + parent filter }, but Line 1316 callsprocessUpdatewhich updates by child ID only. A concurrent re-parent can still let this nested endpoint update a child no longer under the requested parent.🤖 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 1289 - 1317, The nested PATCH has a TOCTOU because you check membership with mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter) but then call processUpdate(client, childType, childId, ...) which updates by childId only; fix by making the update atomic/ scoped to the same merged where-filter: either modify processUpdate to accept a full where filter (e.g., mergedWhere = this.mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter) and pass that instead of childId) or perform an updateMany/update with that mergedWhere and verify affectedRows > 0 before returning; ensure you reference childType, childId, nestedFilter, mergeFilters, makeIdFilter and processUpdate (or use updateMany + check count) so the mutation only succeeds if the child still belongs to the parent.
🤖 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/test/api/rest.test.ts`:
- Around line 4026-4033: Update the misleading test comment above the
mappedHandler call: instead of "Wrong parent → 404" describe the asserted
behavior for mappedHandler when using nestedClient (i.e., the request returns
200 with an empty collection). Locate the block using mappedHandler({ method:
'get', path: '/users/u2/posts', client: nestedClient }) and change the comment
to something like "Wrong parent → 200 with empty collection" so it matches the
expect assertions (expect(...status).toBe(200) and
expect(...body.data).toHaveLength(0)).
---
Duplicate comments:
In `@packages/server/src/api/rest/index.ts`:
- Around line 1340-1347: The nested DELETE flow currently checks parent
membership using (client as any)[childType].findFirst with
mergeFilters(this.makeIdFilter(...), nestedFilter) but then calls
processDelete(childType, childId) which deletes by ID only and creates a TOCTOU
risk; change the deletion to apply the same combined filter used in the
existence check (i.e., perform delete where
mergeFilters(this.makeIdFilter(typeInfo.idFields, childId), nestedFilter))
instead of deleting by ID alone—either update processDelete to accept a filter
object (or an optional nestedFilter parameter) and route the nested-delete path
to call it with the merged filter, or call the client deleteMany on (client as
any)[childType] with the merged filter directly so the check-and-delete is
atomic with respect to the same scope.
- Around line 1289-1317: The nested PATCH has a TOCTOU because you check
membership with mergeFilters(this.makeIdFilter(typeInfo.idFields, childId),
nestedFilter) but then call processUpdate(client, childType, childId, ...) which
updates by childId only; fix by making the update atomic/ scoped to the same
merged where-filter: either modify processUpdate to accept a full where filter
(e.g., mergedWhere = this.mergeFilters(this.makeIdFilter(typeInfo.idFields,
childId), nestedFilter) and pass that instead of childId) or perform an
updateMany/update with that mergedWhere and verify affectedRows > 0 before
returning; ensure you reference childType, childId, nestedFilter, mergeFilters,
makeIdFilter and processUpdate (or use updateMany + check count) so the mutation
only succeeds if the child still belongs to the parent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9751e9b4-14be-4e1e-80f8-56d78a3629e9
📒 Files selected for processing (2)
packages/server/src/api/rest/index.tspackages/server/test/api/rest.test.ts
|
Thanks for the feedback! I've reworked the implementation to follow your suggestion. The nested route URLs now extend the existing :type/:id/:relationship pattern rather than introducing a parallel set:
The config also got simplified: the second-level key is now the relation field name on the parent (e.g. posts on User) instead of the child model name, and the explicit relation field is dropped since the reverse relation is derived automatically from the schema's opposite field. So Three things worth flagging before this is finalised:
|
| * (e.g., `posts` for `User.posts`). This matches the URL segment used in nested routes: | ||
| * `/:parentType/:parentId/:relationName` and `/:parentType/:parentId/:relationName/:childId`. | ||
| */ | ||
| nestedRoutes?: Record< |
There was a problem hiding this comment.
Maybe we can make the parent-scoped handling always enabled, and don't require a configuration? What do you think?
There was a problem hiding this comment.
So we would always offer both the flat and nested API? For me that is fine. Just wonder if someone with super high load would have concerns over the TOCTOU race conditions. But this could be handled by a single global config option.
There was a problem hiding this comment.
I have pushed a refactoring to use a single boolean flag to enable/disable nestedRoutes
|
@ymc9 I have hopefully addressed all of your feedback. The PR now contains 3 commits:
So let me know, if you don't want either 2) or 3), or do not even want any flag to enable/disable this feature and I will update the PR accordingly. Otherwise you can squash merge to get it all into a single commit, depending on your preference. |
2 participants
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
TOCTOU = Time-of-Check to Time-of-Use
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