feat: support fragment parsing in parseFieldNodes #405
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant parseFieldNodes
participant asts as Original AST
participant fragments as resolveInfo.fragments
participant inlined as Inlined Selections
participant reduction as Reduction/Tree Build
parseFieldNodes->>asts: Iterate field nodes
loop For each selection
alt Is FRAGMENT_SPREAD?
parseFieldNodes->>fragments: Resolve fragment definition
fragments-->>inlined: Return selectionSet or []
else Is other selection
asts-->>inlined: Keep as-is
end
end
inlined->>reduction: Process inlined selections
reduction-->>parseFieldNodes: Build resolve-info tree
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts (1)
73-77: Consider adding error handling for missing fragments.When a fragment reference is not found in
resolveInfo.fragments, the code silently returns an empty array. While this might be intentional, it could hide configuration errors or typos in fragment names. Consider logging a warning in development mode to help with debugging.if (ast.kind === Kind.FRAGMENT_SPREAD) { const fragment = resolveInfo.fragments[ast.name.value] if (fragment) { return fragment.selectionSet.selections + } else { + // Optional: log warning for missing fragment + console.warn(`Fragment "${ast.name.value}" not found in resolveInfo.fragments`) } return [] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts
[error] 71-71: Replace ast with (ast)
(prettier/prettier)
[error] 73-73: Delete ;
(prettier/prettier)
[error] 75-75: Delete ;
(prettier/prettier)
[error] 77-77: Delete ;
(prettier/prettier)
[error] 79-79: Delete ;
(prettier/prettier)
🔇 Additional comments (1)
packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts (1)
82-82: Good: Reduce loop now operates on inlined fragments.This change correctly processes the inlined fragment selections instead of the original ASTs. Once the fragment inlining logic is fixed to handle nested spreads and directives (see previous comments), this will properly include all fragment fields in the resolve info.
| const astsWithInlinedFragments = asts.flatMap(ast => { | ||
| if (ast.kind === Kind.FRAGMENT_SPREAD) { | ||
| const fragment = resolveInfo.fragments[ast.name.value]; | ||
| if (fragment) { | ||
| return fragment.selectionSet.selections; | ||
| } | ||
| return []; | ||
| } | ||
| return ast; | ||
| }) |
There was a problem hiding this comment.
Critical: Nested fragment spreads are not handled recursively.
If a fragment contains another fragment spread, the nested spread will be silently ignored. For example:
fragment UserFields on User {
id
...AddressFields # This nested spread gets skipped
}When UserFields is inlined, astsWithInlinedFragments becomes [id, ...AddressFields]. The reduce loop then processes ...AddressFields, but since it's still a FRAGMENT_SPREAD node, getFieldFromAST (line 96) returns undefined, causing it to be skipped at lines 97-99. This results in missing fields from nested fragments.
Fix: Make the inlining recursive to handle nested fragment spreads:
- const astsWithInlinedFragments = asts.flatMap(ast => {
- if (ast.kind === Kind.FRAGMENT_SPREAD) {
- const fragment = resolveInfo.fragments[ast.name.value];
- if (fragment) {
- return fragment.selectionSet.selections;
- }
- return [];
- }
- return ast;
- })
+ function inlineFragments(nodes: ReadonlyArray<SelectionNode>): SelectionNode[] {
+ return nodes.flatMap((ast) => {
+ if (ast.kind === Kind.FRAGMENT_SPREAD) {
+ const fragment = resolveInfo.fragments[ast.name.value]
+ if (fragment) {
+ return inlineFragments(fragment.selectionSet.selections)
+ }
+ return []
+ }
+ return ast
+ })
+ }
+
+ const astsWithInlinedFragments = inlineFragments(asts)🧰 Tools
🪛 ESLint
[error] 71-71: Replace ast with (ast)
(prettier/prettier)
[error] 73-73: Delete ;
(prettier/prettier)
[error] 75-75: Delete ;
(prettier/prettier)
[error] 77-77: Delete ;
(prettier/prettier)
[error] 79-79: Delete ;
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts around
lines 71 to 80, the current flatMap inlining only expands top-level fragment
spreads and ignores nested spreads; replace it with a recursive inliner that,
given an AST selection or array of selections, returns a flat array where each
FRAGMENT_SPREAD is replaced by the fragment's selectionSet selections processed
recursively; ensure you return [] for missing fragments and add a visited
fragment-name Set to prevent infinite recursion on cyclic fragments; use that
recursive function to produce astsWithInlinedFragments so all nested fragment
spreads are resolved before downstream processing.
Major: Directives on fragment spreads are not respected.
Fragment spreads can have @include or @skip directives (e.g., ...UserFields @skip(if: $condition)). The current implementation inlines fragments without checking these directives, causing them to be ignored. This is incorrect because the GraphQL spec requires evaluating directives on fragment spreads before including their fields.
Fix: Check directives on the FRAGMENT_SPREAD node before inlining:
const astsWithInlinedFragments = asts.flatMap((ast) => {
if (ast.kind === Kind.FRAGMENT_SPREAD) {
+ // Check directives on the fragment spread itself
+ if (ast.directives && ast.directives.length) {
+ const { shouldInclude, shouldSkip } = getDirectiveResults(ast, resolveInfo)
+ if (shouldSkip || !shouldInclude) {
+ return []
+ }
+ }
+
const fragment = resolveInfo.fragments[ast.name.value]
if (fragment) {
return fragment.selectionSet.selections
}
return []
}
return ast
})Note: This fix should be combined with the recursive inlining fix from the previous comment.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 71-71: Replace ast with (ast)
(prettier/prettier)
[error] 73-73: Delete ;
(prettier/prettier)
[error] 75-75: Delete ;
(prettier/prettier)
[error] 77-77: Delete ;
(prettier/prettier)
[error] 79-79: Delete ;
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts around
lines 71-80, fragment spreads are inlined unconditionally which ignores
@include/@skip directives; update the FRAGMENT_SPREAD branch to evaluate the
spread's directives before inlining by calling GraphQL's directive inclusion
helper (e.g., shouldIncludeNode or equivalent) with the spread node and
resolveInfo.variableValues and only inline the fragment selectionSet if the node
should be included; ensure this change is applied together with the recursive
inlining fix from the previous comment so nested fragments and directives are
handled correctly.
Fix formatting issues.
The code has several Prettier/ESLint violations that should be fixed for consistency with the codebase style.
Apply these formatting fixes:
- const astsWithInlinedFragments = asts.flatMap(ast => {
+ const astsWithInlinedFragments = asts.flatMap((ast) => {
if (ast.kind === Kind.FRAGMENT_SPREAD) {
- const fragment = resolveInfo.fragments[ast.name.value];
+ const fragment = resolveInfo.fragments[ast.name.value]
if (fragment) {
- return fragment.selectionSet.selections;
+ return fragment.selectionSet.selections
}
- return [];
+ return []
}
- return ast;
+ return ast
})📝 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.
| const astsWithInlinedFragments = asts.flatMap(ast => { | |
| if (ast.kind === Kind.FRAGMENT_SPREAD) { | |
| const fragment = resolveInfo.fragments[ast.name.value]; | |
| if (fragment) { | |
| return fragment.selectionSet.selections; | |
| } | |
| return []; | |
| } | |
| return ast; | |
| }) | |
| const astsWithInlinedFragments = asts.flatMap((ast) => { | |
| if (ast.kind === Kind.FRAGMENT_SPREAD) { | |
| const fragment = resolveInfo.fragments[ast.name.value] | |
| if (fragment) { | |
| return fragment.selectionSet.selections | |
| } | |
| return [] | |
| } | |
| return ast | |
| }) |
🧰 Tools
🪛 ESLint
[error] 71-71: Replace ast with (ast)
(prettier/prettier)
[error] 73-73: Delete ;
(prettier/prettier)
[error] 75-75: Delete ;
(prettier/prettier)
[error] 77-77: Delete ;
(prettier/prettier)
[error] 79-79: Delete ;
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/query-graphql/src/decorators/graphql-resolve-info.utils.ts around
lines 71 to 80, fix Prettier/ESLint formatting issues: add missing semicolons,
ensure consistent spacing and indentation, use parentheses for the arrow
function parameter (ast) if your linter requires them, put braces/newlines so
each return sits on its own line, and ensure consistent trailing commas where
the codebase expects them; reformat the block so it conforms to the project's
Prettier/ESLint rules without changing logic.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many --target=lint --all |
❌ Failed | 16s | View ↗ |
nx run-many --target=e2e --all |
✅ Succeeded | 1m 53s | View ↗ |
nx run-many --target=test --all |
✅ Succeeded | 42s | View ↗ |
nx run-many --target=build --all |
✅ Succeeded | 15s | View ↗ |
nx run workspace:version |
✅ Succeeded | <1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-30 20:37:27 UTC
|
Sorry, seems this topic is a little more complicated. |
1 participant
This PR adds fragment support to nestjs-query-graphql's
simplifyResolveInfomethod.Previously, fields only included in a fragment were missing from the QueryService
opts.resolveInfofield.This does not affect inline fragments, as those are defined using a different
Kind(Kind.INLINE_FRAGMENT).There is no open issue for this PR.
Summary by CodeRabbit