Rest api sparse fieldsets #2252
Conversation
📝 WalkthroughWalkthroughAdds sparse fieldset support to the REST RequestHandler: introduces buildPartialSelect to parse fields[] and return Prisma select objects, threads this into buildRelationSelect and multiple read/relationship handlers, and includes new tests covering sparse fields with includes, relationships, pagination, and JSON:API responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant REST_Handler as REST Handler
participant BuildRelation as buildRelationSelect
participant BuildPartial as buildPartialSelect
participant Prisma
Client->>REST_Handler: GET /resource?include=rel&fields[type]=a,b
REST_Handler->>BuildRelation: buildRelationSelect(type, include, query)
BuildRelation->>BuildPartial: buildPartialSelect(relatedType, query)
BuildPartial-->>BuildRelation: { select } or { error }
alt valid select
BuildRelation-->>REST_Handler: relation select merged
REST_Handler->>Prisma: query with select/include
Prisma-->>REST_Handler: data
REST_Handler-->>Client: 200 + JSON:API response
else error
BuildRelation-->>REST_Handler: error
REST_Handler-->>Client: 400 + error body
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/server/tests/api/rest-partial.test.ts (3)
411-445: Typo: “efect” → “effect”.Fix test titles for clarity.
Apply this diff:
- it('does not efect toplevel filtering', async () => { + it('does not effect toplevel filtering', async () => {And similarly for the next test below.
446-478: Typo: “efect” → “effect”.Same correction for the sorting test name.
Apply this diff:
- it('does not efect toplevel sorting', async () => { + it('does not effect toplevel sorting', async () => {
479-505: Typo and add a guardrail test.
- Fix “efect” → “effect”.
- Recommend adding a case for GET /user/:id/posts without fields[post] to catch regressions for default related selection (it would have exposed the boolean-spread bug).
Do you want me to add that test snippet?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/api/rest/index.ts(12 hunks)packages/server/tests/api/rest-partial.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/server/tests/api/rest-partial.test.ts (2)
packages/testtools/src/schema.ts (1)
loadSchema(172-248)packages/server/src/api/rest/index.ts (2)
makeHandler(2096-2099)makeHandler(2101-2101)
packages/server/src/api/rest/index.ts (1)
packages/runtime/src/local-helpers/lower-case-first.ts (1)
lowerCaseFirst(1-3)
🔇 Additional comments (6)
packages/server/src/api/rest/index.ts (6)
180-183: Good addition: explicit error for duplicated fields parameter.Clear error semantics for duplicated fields params will help clients debug malformed requests.
519-527: Include handling now respects per-relation sparse fieldsets.Passing the query into buildRelationSelect and merging its select into args.include looks correct. Ensure nested includes still serialize as expected when combined with relationship ID inclusion.
If you have complex nested includes, please run a quick manual check for a case like include=posts.author with fields[post] and fields[user] both set, to verify the merged select/include shape.
529-541: Converting include into select when sparse fieldsets are used is correct.Prisma disallows mixing select and include at the same level; your conversion avoids that. Nice defensive merge order so relation selections win over placeholders.
739-747: Include merge for collection reads looks good.buildRelationSelect’s output merged into include preserves prior relationship ID selection and is compatible with later select conversion.
749-761: Top-level sparse fieldsets: correct select/include reconciliation.Merging select first, then folding include into select, avoids Prisma select/include conflicts and preserves relation selections.
1861-1914: buildRelationSelect: integrates sparse fieldsets for included relations.Logic is sound. One caution: when a leaf inclusion has no fields[...] override, you set the leaf to true. If this result is later passed to injectRelationQuery (as in processFetchRelated), it will need to be normalized to an object (addressed in the earlier fix).
Please re-test GET /:type/:id/:relationship without any fields[...] to ensure filtering/sorting/pagination on the related collection still works after applying the fix.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/server/src/api/rest/index.ts (2)
806-831: buildPartialSelect: honor modelNameMapping and validate fields to fail fast.
Currently only fields[internalType] is read and arbitrary field names pass through until Prisma errors. Support mapped names and reject unknown fields early.Apply:
- private buildPartialSelect(type: string, query: Record<string, string | string[]> | undefined) { - const selectFieldsQuery = query?.[`fields[${type}]`]; + private buildPartialSelect(type: string, query: Record<string, string | string[]> | undefined) { + // accept fields[Internal], fields[internal], and fields[Mapped] + const internal = lowerCaseFirst(type); + const mapped = this.mapModelName(internal); + const keys = [`fields[${type}]`, `fields[${internal}]`, `fields[${mapped}]`]; + const present = keys.filter((k) => query?.[k] !== undefined); + const selectFieldsQuery = present.length ? (query as any)[present[0]] : undefined; if (!selectFieldsQuery) { return { select: undefined, error: undefined }; } - if (Array.isArray(selectFieldsQuery)) { + // duplicated fields[...] (e.g., both mapped and internal provided) + if (Array.isArray(selectFieldsQuery) || present.length > 1) { return { select: undefined, - error: this.makeError('duplicatedFieldsParameter', `duplicated fields query for type ${type}`), + error: this.makeError('duplicatedFieldsParameter', `duplicated fields query for type ${type}`), }; } - const typeInfo = this.typeMap[lowerCaseFirst(type)]; + const typeInfo = this.typeMap[internal]; if (!typeInfo) { return { select: undefined, error: this.makeUnsupportedModelError(type) }; } - const selectFieldNames = selectFieldsQuery.split(',').filter((i) => i); + const selectFieldNames = selectFieldsQuery.split(',').map((s: string) => s.trim()).filter((i: string) => i); + // validate requested fields + const unknown = selectFieldNames.filter((f: string) => !typeInfo.fields[f]); + if (unknown.length) { + return { + select: undefined, + error: this.makeError('invalidValue', `unknown field(s) for ${type}: ${unknown.join(', ')}`), + }; + } const fields = selectFieldNames.reduce((acc, curr) => ({ ...acc, [curr]: true }), {}); return { select: { ...this.makeIdSelect(typeInfo.idFields), ...fields }, }; }If you’d like, I can add tests for:
- fields[mappedName] acceptance
- invalid field name → 400 invalid-value with bad field list
1861-1919: buildRelationSelect: preserve existing branch shape when multiple include paths meet.
When assembling multiple paths, if a relation key already exists with select/include, reusing that container avoids overwriting earlier work.Apply:
for (let i = 0; i < parts.length; i++) { const relation = parts[i]; const relationInfo = currType.relationships[relation]; @@ - // handle partial results for requested type + // handle partial results for requested type const { select, error } = this.buildPartialSelect(lowerCaseFirst(relationInfo.type), query); if (error) return { select: undefined, error }; - if (i !== parts.length - 1) { - if (select) { - currPayload[relation] = { select: { ...select } }; - currPayload = currPayload[relation].select; - } else { - currPayload[relation] = { include: { ...currPayload[relation]?.include } }; - currPayload = currPayload[relation].include; - } - } else { + if (i !== parts.length - 1) { + // if branch exists, descend into it + const existing = currPayload[relation]; + if (existing?.select) { + currPayload = existing.select; + } else if (existing?.include) { + currPayload = existing.include; + } else if (select) { + currPayload[relation] = { select: { ...select } }; + currPayload = currPayload[relation].select; + } else { + currPayload[relation] = { include: { ...(existing?.include ?? {}) } }; + currPayload = currPayload[relation].include; + } + } else { currPayload[relation] = select ? { select: { ...select }, } : true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/api/rest/index.ts(12 hunks)packages/server/tests/api/rest-partial.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/tests/api/rest-partial.test.ts
🧰 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 (4)
packages/server/src/api/rest/index.ts (4)
180-183: Good: explicit error for duplicated fields parameter.
Clear, typed entry; paramCase yields a sensible code.
519-527: Top‑level sparse select wiring looks correct.
Passing query into buildRelationSelect and lifting include→select when fields[...] is present is consistent with Prisma and JSON:API expectations. No blockers.Please confirm you have a test asserting select with nested include at leaf level works (select: { posts: { include: { author: true } } }). If missing, I can add one.
Also applies to: 529-541
577-586: Defensive default for related selection to avoid shape mismatches (and align with prior feedback).
When no fields[...] is provided and there’s no include, select[relationship] becomes boolean true. Later augmentation assumes an object. Prefer always using an object and provide a sensible default (IDs + scalar fields).Apply:
- // handle partial results for requested type - if (!select) { - const { select: partialFields, error } = this.buildPartialSelect(lowerCaseFirst(relationInfo.type), query); - if (error) return error; - - select = partialFields ? { [relationship]: { select: { ...partialFields } } } : { [relationship]: true }; - } + // handle partial results for requested type + if (!select) { + const relatedType = lowerCaseFirst(relationInfo.type); + const { select: partialFields, error } = this.buildPartialSelect(relatedType, query); + if (error) return error; + if (partialFields) { + select = { [relationship]: { select: { ...partialFields } } }; + } else { + const relatedTypeInfo = this.typeMap[relatedType]; + const scalarSelect = Object.fromEntries( + Object.entries(relatedTypeInfo.fields) + .filter(([, f]) => !f.isDataModel) + .map(([name]) => [name, true]) + ); + select = { + [relationship]: { + select: { + ...this.makeIdSelect(relationInfo.idFields), + ...scalarSelect, + }, + }, + }; + } + }Also applies to: 588-595
739-747: Collection read: sparse fieldsets + pagination integration LGTM.
- include→select coercion when fields[...] present is sound.
- Promise.all for data+count is good.
Also applies to: 749-761, 782-785
2 participants
implements sparse fieldsets as shown in the specification https://jsonapi.org/format/#fetching-sparse-fieldsets