fix!: implement validation for types in fragment condition#692
fix!: implement validation for types in fragment condition#692stuebingerb wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesFragment validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Fragments can now only be defined on types that are actually possible at their position, e.g. you cannot define a fragment on `Cat` when the return type is known to be `Dog`. Similarly, fragments on interfaces and unions must be for one of the known `possibleTypes`. Fixes #683 BREAKING CHANGE: invalid fragment conditions were previously simply ignored and now cause the query to fail.
c49a64a to
8e3892e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt (1)
605-719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a named-fragment-spread variant for the impossible interface/union cases.
The new impossible-type tests for interfaces/unions/intersections only exercise inline fragments (
... on X). The named-fragment-spread message format ("...of type '$fragmentName'...") is only covered for the plain object case (Lines 466-480). Adding one spread-based case for the interface/union path would close a small coverage gap in the new validation surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 66f615e9-884a-4695-a12c-c146bf95e51e
📒 Files selected for processing (5)
kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/InterfacesSpecificationTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/ObjectsSpecificationTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt
💤 Files with no reviewable changes (1)
- kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/ObjectsSpecificationTest.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt">
<violation number="1" location="kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt:323">
P1: Invalid fragment conditions on union fields are still accepted and silently yield no selected fields, because `handleUnion` bypasses `validateForFragment`. Route its fragment/inline-fragment handling through the same applicability validation before building `memberChildren`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return when (type.kind) { | ||
| TypeKind.OBJECT -> listOf(type) | ||
| TypeKind.INTERFACE -> checkNotNull(type.possibleTypes) | ||
| TypeKind.UNION -> checkNotNull(type.possibleTypes) |
There was a problem hiding this comment.
P1: Invalid fragment conditions on union fields are still accepted and silently yield no selected fields, because handleUnion bypasses validateForFragment. Route its fragment/inline-fragment handling through the same applicability validation before building memberChildren.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt, line 323:
<comment>Invalid fragment conditions on union fields are still accepted and silently yield no selected fields, because `handleUnion` bypasses `validateForFragment`. Route its fragment/inline-fragment handling through the same applicability validation before building `memberChildren`.</comment>
<file context>
@@ -311,6 +314,27 @@ internal class RequestInterpreter(private val schemaModel: SchemaModel) {
+ return when (type.kind) {
+ TypeKind.OBJECT -> listOf(type)
+ TypeKind.INTERFACE -> checkNotNull(type.possibleTypes)
+ TypeKind.UNION -> checkNotNull(type.possibleTypes)
+ else -> emptyList()
+ }
</file context>
There was a problem hiding this comment.
That might actually be correct. I have an upcoming refactoring for union types prepared that would simply re-use regular fragments, though, so I'd like to postpone that topic. Please keep this comment in mind and check again with that upcoming refactoring PR.
There was a problem hiding this comment.
@cubic-dev-ai Please create a test case that would currently fail to verify the behavior is correct after the refactoring.
There was a problem hiding this comment.
@stuebingerb I can help — what did you want me to do?
- Manage learnings: reply in the relevant thread and say
@cubic add this to our learnings(I'll extract the learning from context). For delete/update, paste the learning link from Settings → AI review → Learnings. - Make a code change:
@cubic fix this and push commits(oropen a PR) - Ask a question:
@cubic why is this needed?
There was a problem hiding this comment.
@cubic create a test that demonstrates the problem of missing fragment validation for union fields
There was a problem hiding this comment.
@stuebingerb Fix with cubic is available during trial and on the Pro plan. Upgrade your plan to use this feature. https://www.cubic.dev/settings?tab=subscription
There was a problem hiding this comment.
Marvellous, thanks for suggesting it then.
Alright, @cubic why is this needed?
There was a problem hiding this comment.
@stuebingerb The original comment flagged that handleUnion processes its own fragments directly (matching by type name against possibleTypes) instead of routing through validateForFragment. So if you write:
{
unions {
... on Actor { name } # Actor is NOT a member of the union
}
}handleUnion silently returns emptyList() for that fragment — no error, no warning, just missing fields. The new validateForFragment logic you added would catch that with:
Invalid type 'Actor' in type condition on inline fragment of type 'TopUnion'; must be one of '[Union1, Union2]'
But handleUnion never calls it, so those invalid conditions slip through.
Same issue applies to FragmentSpreadNode inside union selections — handleUnion calls ctx.getFragment() directly which doesn't validate against the enclosing union type.
That said — you mentioned you have a refactoring coming that will unify union fragment handling with the regular path. If that refactoring routes union fragments through findFragmentType (which calls validateForFragment), then the gap closes automatically and this comment becomes obsolete. The test I suggested was just to make the gap visible before the refactoring.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
Bencher Report
stuebingerb
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #692 +/- ##
==========================================
- Coverage 84.06% 84.05% -0.01%
==========================================
Files 151 151
Lines 5008 5024 +16
Branches 864 870 +6
==========================================
+ Hits 4210 4223 +13
- Misses 492 493 +1
- Partials 306 308 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Fragments can now only be defined on types that are actually possible at their position, e.g. you cannot define a fragment on
Catwhen the return type is known to beDog. Similarly, fragments on interfaces and unions must be for one of the knownpossibleTypes.Fixes #683