refactor!: refactor union properties handling#701
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughUnion-specific field and execution classes are removed. Union properties now use generic function fields and execution paths, while fragment validation rejects impossible or invalid union type conditions. ChangesUnion execution simplification
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Refactors handling of union properties and processes them just like regular fragments, thereby benefitting from all recent bugfixes and simplifying code. Fixes #697 BREAKING CHANGE: removed `Type.Union`, `Field.Union`, and `Execution.Union`. BREAKING CHANGE: invalid fragment conditions in union properties were previously simply ignored and now cause the query to fail.
626bac4 to
dbec446
Compare
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 935f9711-9e5f-41fa-ad6c-09d59f9d8b42
📒 Files selected for processing (10)
kgraphql/api/kgraphql.apikgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt
💤 Files with no reviewable changes (5)
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.kt
- kgraphql/api/kgraphql.api
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.kt
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.kt
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
=======================================
Coverage 84.05% 84.06%
=======================================
Files 151 151
Lines 5024 4925 -99
Branches 870 850 -20
=======================================
- Hits 4223 4140 -83
+ Misses 493 488 -5
+ Partials 308 297 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 10 files
Architecture diagram
sequenceDiagram
participant Client
participant SC as SchemaCompilation
participant RI as RequestInterpreter
participant Exe as Execution.Node
participant PRE as ParallelRequestExecutor
participant Resolver
Note over SC,Resolver: Schema Build Time
SC->>SC: handleUnionProperty(unionProperty)
SC->>SC: CHANGED: create Field.Function instead of Field.Union
Note over Client,PRE: Query Execution Time
Client->>RI: POST /graphql (query with union field + fragments)
RI->>RI: parse selection set for union field
RI->>RI: lookup field: returns Field.Function (returnType=Type.Union)
loop each selection (fragment spread / inline fragment)
RI->>RI: validate fragment type condition against union's possibleTypes
alt Invalid type condition
RI-->>Client: ValidationException (error)
else Valid
RI->>Exe: CHANGED: create Execution.Node<br>with children as Execution.Fragment<br>per typed fragment
end
end
RI->>PRE: pass execution tree
PRE->>PRE: createNode(value, node, returnType)
PRE->>Resolver: invoke union property resolver
Resolver-->>PRE: union value (e.g. Actor instance)
loop each child Execution.Fragment
PRE->>PRE: check if value's type matches fragment's type condition
alt Match
PRE->>PRE: execute fragment's field selections
else No match
PRE->>PRE: skip fragment
end
end
PRE-->>Client: JSON result with fields from matching fragment
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.kt (1)
190-192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore runtime type validation for union returns.
By deleting
createUnionOperationNodeand routing unions through this generic path, the executor no longer validates that the resolved value is actually a member of the union'spossibleTypes. This introduces two specification violations for abstract types:
- Type Leakage: If a union resolver returns an invalid object (e.g.,
Directorinstead ofActororFilm), querying__typenamewill successfully return"Director", leaking a type that isn't defined as a valid member of the union into the response.- Silent Empty Objects: If the invalid object is queried with inline fragments, no fragments will match, and the executor will silently return
{}instead of raising a field error as required by the GraphQL specification for invalid abstract type resolutions.Additionally, because primitive checks (like
value is String) happen beforenode.children.isNotEmpty(), returning a primitive from a union resolver will incorrectly serialize as a scalar, bypassing the selection set entirely.Please consider restoring a runtime validation check in this block to ensure that
valueconforms toreturnType.unwrapped().possibleTypesfor abstract types before proceeding to create the object node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bb5979a-20f6-4a53-846f-0b6e57a2c54b
📒 Files selected for processing (10)
kgraphql/api/kgraphql.apikgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.ktkgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.ktkgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt
💤 Files with no reviewable changes (5)
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.kt
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.kt
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.kt
- kgraphql/api/kgraphql.api
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt
|
CodeRabbit is right, the actual return type is no longer validated against the possible union types. I have a draft implementation but I'm not sure if I want to restore that check now, and if to what extent - I'd expect that invalid return types are possible in other places as well. Edit: in fact, everything except the |
1 participant
Refactors handling of union properties and processes them just like regular fragments, thereby benefitting from all recent bugfixes and simplifying code.
Fixes #697
BREAKING CHANGE: removed
Type.Union,Field.Union, andExecution.Union.BREAKING CHANGE: invalid fragment conditions in union properties were previously simply ignored and now cause the query to fail.