refactor!: refactor union properties handling - #701
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughUnion-specific field and execution classes are removed. Union properties now use generic function fields and execution paths, with runtime type validation and fragment-condition checks for invalid union targets. ChangesUnion execution simplification
Sequence Diagram(s)sequenceDiagram
participant RequestInterpreter
participant ParallelRequestExecutor
participant UnionResolver
participant RuntimeTypeCheck
RequestInterpreter->>ParallelRequestExecutor: create generic property execution node
ParallelRequestExecutor->>UnionResolver: resolve union-backed property
UnionResolver-->>ParallelRequestExecutor: return runtime value
ParallelRequestExecutor->>RuntimeTypeCheck: validate possible union/interface type
RuntimeTypeCheck-->>ParallelRequestExecutor: accept value or raise ExecutionError
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
=======================================
Coverage 84.05% 84.05%
=======================================
Files 151 151
Lines 5024 4930 -94
Branches 870 853 -17
=======================================
- Hits 4223 4144 -79
+ Misses 493 488 -5
+ Partials 308 298 -10 ☔ 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 |
dbec446 to
6a3fee3
Compare
Restored validation: https://github.com/stuebingerb/KGraphQL/compare/dbec446f2fef5d4ab41c185a86e0bb46aee33873..6a3fee323624be328e5bac84c100cbb01f30d147 |
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-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate composite results before serializing primitives.
A resolver returning
"invalid"for a union/interface selection hits theStringbranch before Line 190, so it produces a scalar JSON value without checkingpossibleTypes. Move the deferred/object-selection handling ahead of primitive serialization, then add a scalar-invalid-union regression case.Proposed fix
+ value is Deferred<*> -> createNode(ctx, value.await(), node, returnType) + + node.children.isNotEmpty() -> createObjectNode(ctx, value, node, returnType) + value is String -> CompletableDeferred(jsonNodeFactory.textNode(value)) value is Int -> CompletableDeferred(jsonNodeFactory.numberNode(value)) value is Float -> CompletableDeferred(jsonNodeFactory.numberNode(value)) value is Double -> CompletableDeferred(jsonNodeFactory.numberNode(value)) value is Boolean -> CompletableDeferred(jsonNodeFactory.booleanNode(value)) value is Long -> CompletableDeferred(jsonNodeFactory.numberNode(value)) value is Short -> CompletableDeferred(jsonNodeFactory.numberNode(value)) - - value is Deferred<*> -> createNode(ctx, value.await(), node, returnType) - - node.children.isNotEmpty() -> createObjectNode(ctx, value, node, returnType)Also applies to: 229-238
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 741ca9a7-d553-4c53-bef0-8a75d6bf159d
📒 Files selected for processing (11)
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/language/VariablesSpecificationTest.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/Field.kt
- 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/RequestInterpreter.kt
- kgraphql/api/kgraphql.api
🚧 Files skipped from review as they are similar to previous changes (3)
- kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.kt
- kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.kt
- kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt
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.
6a3fee3 to
542d4c1
Compare
There was a problem hiding this comment.
No issues found across 11 files
Architecture diagram
sequenceDiagram
participant Client
participant SchemaComp as Schema Compilation
participant RequestInterp as Request Interpreter
participant Validation
participant Executor as Parallel Request Executor
participant FieldFunc as Field.Function
participant ExecNode as Execution.Node
participant FragmentHandler as Fragment Logic
participant TypeCheck as Type Checking
Note over Client,TypeCheck: Current architecture after union property refactor
Client->>SchemaComp: Define schema with union property
SchemaComp->>SchemaComp: handleUnionProperty()
Note over SchemaComp: NEW: Compiles union property to Field.Function (instead of Field.Union)
SchemaComp-->>SchemaComp: Type resolved as union/interface
Client->>RequestInterp: Execute query (includes union field with fragments)
RequestInterp->>RequestInterp: interpretField() for union property
Note over RequestInterp: No special handleUnion() exists now
RequestInterp->>RequestInterp: Use getSelectionSet() like any object field
RequestInterp->>Validation: validateFragmentConditions(inline/spread)
alt Valid fragment conditions
Validation-->>RequestInterp: OK
RequestInterp->>ExecNode: Create Execution.Node with fragment children
else Invalid type condition (e.g., unknown type, wrong kind)
Validation-->>RequestInterp: NEW: Throws ValidationException (previously ignored)
RequestInterp-->>Client: Error response
end
Executor->>ExecNode: resolveNode() for union field
ExecNode->>FieldFunc: Invoke union property resolver (T -> Any?)
FieldFunc-->>ExecNode: value (Deferred or raw)
ExecNode->>ExecNode: createObjectNode() or createNode()
Note over ExecNode: Now enters common path for object fields
ExecNode->>TypeCheck: Check if returnType is UNION/INTERFACE
alt Value type does not match any possible type
TypeCheck-->>ExecNode: NEW: ExecutionError("Unexpected value type...")
ExecNode-->>Client: Error response
end
ExecNode->>FragmentHandler: For each child (fragment nodes)
FragmentHandler->>FragmentHandler: handleFragment() with isExpectedType() check
Note over FragmentHandler: Reuses existing fragment logic (no special union handling)
FragmentHandler-->>ExecNode: Merged children for matching type
ExecNode-->>Client: JSON result
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.