feat!: support fragments on query level - #650
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMarks ChangesExecution Internalization, Plan Refactor, and Behavioral Fixes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
==========================================
+ Coverage 84.22% 84.23% +0.01%
==========================================
Files 151 151
Lines 5000 4991 -9
Branches 857 858 +1
==========================================
- Hits 4211 4204 -7
+ Misses 491 489 -2
Partials 298 298 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ParallelRequestExecutor.kt`:
- Around line 373-374: The ktlint formatter is failing due to missing whitespace
formatting. In the ParallelRequestExecutor.kt file, locate the checkAccess
method call on node.field where takeUnless is used with a lambda expression. The
lambda expression `{ it is Type.OperationObject}` is missing a space before the
closing brace. Add a space before the `}` to match ktlint formatting standards,
changing it to `{ it is Type.OperationObject }`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 540d2d32-3692-4efa-89d2-cabb438346e6
📒 Files selected for processing (8)
kgraphql/api/kgraphql.apikgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ExecutionPlan.ktkgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ParallelRequestExecutor.ktkgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/RequestExecutor.ktkgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/structure/RequestInterpreter.ktkgraphql/src/test/kotlin/com/apurebase/kgraphql/integration/BaseSchemaTest.ktkgraphql/src/test/kotlin/com/apurebase/kgraphql/integration/QueryTest.ktkgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/language/FragmentsSpecificationTest.kt
💤 Files with no reviewable changes (1)
- kgraphql/api/kgraphql.api
stuebingerb
force-pushed
the
feature/support-fragments-on-query
branch
from
June 14, 2026 15:52
a068dce to
8e1e2ae
Compare
Bencher Report![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 8 files
Architecture diagram
sequenceDiagram
participant Client as GraphQL Client
participant Exe as Schema execution
participant Int as RequestInterpreter
participant Plan as ExecutionPlan
participant Exec as ParallelRequestExecutor
participant Load as Loader cache
Note over Client,Exec: NEW: Query-level fragment support
Client->>Exe: execute request
Exe->>Int: createExecutionPlan(ast, operationName, variables)
Int->>Int: parse operation selectionSet
Int->>Int: CHANGED: handleSelectionFieldOrFragment()<br/>for each selection (FieldNode or FragmentNode)
Int-->>Exe: ExecutionPlan(operations, root, declaredVariables)
Note over Exe,Exec: CHANGED: root and declaredVariables now tracked
Exe->>Exec: suspendExecute(plan, variables, context)
Exec->>Exec: create ExecutionContext with declaredVariables
Exec->>Plan: iterate over operations
loop for each operation
alt Execution.Fragment
Exec->>Exec: handleFragment(ctx, root, operation)
Exec->>Load: check fragment directives (skip/include)
alt fragment included
Exec->>Exec: recursively resolve fragment elements<br/>as property nodes or sub-fragments
Exec-->>Exec: merge results into parent object
end
else Execution.Node
Exec->>Exec: handleProperty(ctx, root, operation, root)
Exec->>Load: check direct field include/skip
alt field included
Exec->>Exec: invoke resolver, create child nodes
Exec-->>Exec: return key-value pair for merging
end
end
end
Exec->>Exec: merge all deferred results into single objectNode
alt executionMode = Normal
Exec->>Exec: mapIndexedParallel() for concurrent execution
else executionMode = Serial
Exec->>Exec: sequential execution
end
alt any non-nullable operation errors
Exec->>Exec: raise ExecutionError → data = null
else success
Exec-->>Exe: serialized JSON result
end
Exe-->>Client: HTTP response with data
stuebingerb
force-pushed
the
feature/support-fragments-on-query
branch
from
June 15, 2026 10:02
8e1e2ae to
dbb44e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/QueryTest.kt (1)
1086-1100: ⚡ Quick winStrengthen the non-nullable root error assertion
This test only checks
data == null(Line [1099]); also assert anerrorsentry exists so the test can’t pass on unrelated null-data regressions.Suggested test hardening
- response.deserialize()["data"] shouldBe null + val deserialized = response.deserialize() + deserialized["data"] shouldBe null + (deserialized["errors"] as? List<*>)?.isNotEmpty() shouldBe true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/QueryTest.kt` around lines 1086 - 1100, The test function `errors during non-nullable operation execution should stop other operations` only validates that the response data is null but does not verify that the errors field is present. Add an additional assertion after the existing data check to confirm that response.deserialize()["errors"] exists and is not null/empty. This will ensure the test cannot pass due to unrelated data-null regressions and truly validates that errors were returned as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.kt`:
- Line 338: The condition at line 338 in the ParallelRequestExecutor.kt file
performs a null-dereference when accessing __typename on a JsonNode. The
expression value["__typename"].textValue() will throw when the __typename field
is absent from delegated or remote payloads. Modify the fragment type matching
logic to safely check if __typename exists and is not null before calling
textValue() on it, such as by verifying the field is present and non-null before
dereferencing, or using a safe navigation approach to handle the case where
__typename is missing.
- Around line 521-523: The subscriptionArgs construction in the map operation at
line 522 unconditionally casts all children to Execution.Node and accesses
aliasOrKey, but fragments in the subscription selection are not Execution.Node
instances and will throw ClassCastException. Filter the children collection to
only process Execution.Node instances (by checking the type before casting or
using filterIsInstance) before mapping to extract their aliasOrKey values,
ensuring fragment children are properly skipped during subscription args
collection.
---
Nitpick comments:
In `@kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/QueryTest.kt`:
- Around line 1086-1100: The test function `errors during non-nullable operation
execution should stop other operations` only validates that the response data is
null but does not verify that the errors field is present. Add an additional
assertion after the existing data check to confirm that
response.deserialize()["errors"] exists and is not null/empty. This will ensure
the test cannot pass due to unrelated data-null regressions and truly validates
that errors were returned as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df6a4bd1-325e-45a3-9de7-91665804ac34
📒 Files selected for processing (8)
kgraphql/api/kgraphql.apikgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ExecutionPlan.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/RequestExecutor.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/BaseSchemaTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/QueryTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt
💤 Files with no reviewable changes (1)
- kgraphql/api/kgraphql.api
✅ Files skipped from review due to trivial changes (1)
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/RequestExecutor.kt
stuebingerb
force-pushed
the
feature/support-fragments-on-query
branch
from
June 15, 2026 12:29
dbb44e7 to
e24fedb
Compare
mervyn-mccreight
approved these changes
Jun 16, 2026
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
2 participants
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.
Resolves #406
Resolves #645
BREAKING CHANGE:
ExecutionPlan,RequestInterpreterandParallelRequestExecutorare no longer public classes