executor: 100% unit test coverage & fix dead blocks#744
Conversation
📝 WalkthroughWalkthroughThis PR improves executor robustness by fixing fragment condition matching to handle nil-derived types and simplifying tag-matching logic, then validates the changes and executor behavior broadly through two new comprehensive test suites covering edge cases, error paths, and complex execution scenarios. ChangesExecutor Internals: Fixes and Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor.go (1)
367-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the new nil-type guard in planner fragment matching to avoid a remaining panic path.
Line 368 and Line 389 correctly handle
conditionalType == nil, butplanFragmentMatches(plan.go:509-534) still only checkserr != niland then dereferencesconditionalType.Name(). That leaves the same nil-pointer risk in the planning path for missing type conditions.Suggested mirror fix (plan.go)
func planFragmentMatches(schema Schema, typeConditionAST *ast.Named, runtime *Object) bool { if typeConditionAST == nil { return true } conditionalType, err := typeFromAST(schema, typeConditionAST) - if err != nil { + if err != nil || conditionalType == nil { return false } if conditionalType == runtime { return 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 `@executor.go` around lines 367 - 390, The planner's planFragmentMatches function still calls typeFromAST and only checks err, then dereferences conditionalType (conditionalType.Name()) causing a nil-pointer risk; update planFragmentMatches (in plan.go) to mirror executor.go by checking both err != nil and conditionalType == nil after calling typeFromAST(eCtx.Schema, typeConditionAST) and return false (or the same safe value used in executor) before any use of conditionalType, and ensure subsequent type comparisons and IsPossibleType calls use the non-nil conditionalType variable.
🤖 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 `@executor_internal_test.go`:
- Line 732: Remove the unused type declaration customMap from the file to
satisfy linting; locate the type definition "type customMap
map[string]interface{}" and delete that line (or the entire declaration) since
nothing references customMap in executor_internal_test.go.
---
Outside diff comments:
In `@executor.go`:
- Around line 367-390: The planner's planFragmentMatches function still calls
typeFromAST and only checks err, then dereferences conditionalType
(conditionalType.Name()) causing a nil-pointer risk; update planFragmentMatches
(in plan.go) to mirror executor.go by checking both err != nil and
conditionalType == nil after calling typeFromAST(eCtx.Schema, typeConditionAST)
and return false (or the same safe value used in executor) before any use of
conditionalType, and ensure subsequent type comparisons and IsPossibleType calls
use the non-nil conditionalType variable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83cee6f6-7acb-45b3-8558-6479d8e36116
📒 Files selected for processing (3)
executor.goexecutor_extended_test.goexecutor_internal_test.go
| // DefaultResolveFn – reflect.Map with func() interface{} value | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| type customMap map[string]interface{} |
There was a problem hiding this comment.
Remove unused customMap declaration to keep lint clean.
Line 732 declares customMap but nothing references it in this file.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 732-732: type customMap is unused
(unused)
🤖 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 `@executor_internal_test.go` at line 732, Remove the unused type declaration
customMap from the file to satisfy linting; locate the type definition "type
customMap map[string]interface{}" and delete that line (or the entire
declaration) since nothing references customMap in executor_internal_test.go.
Source: Linters/SAST tools
1 participant
Details
executor: 100% unit test coverage & fix dead blocksTest Plan
Summary by CodeRabbit
Bug Fixes
Tests