fix: plan generator ignores the skip/include directives #2113
Conversation
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughThe changes update the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router-tests/go.mod (1)
30-31: Keep router-tests in sync with router’s dependency graphThe tests mirror the router’s pseudo-version pin. Make sure you run
go mod tidyin both modules so that their transitive graphs stay identical; desynchronisation can surface as hard-to-trace CI failures.router/pkg/plan_generator/plan_generator_test.go (1)
23-24: Consider derivingallFilesdynamicallyManually enumerating
"5.graphql"inallFilesworks now but is error-prone whenever new queries are added.
Reading the directory once (e.g.os.ReadDir(...)) would keep the list self-maintaining and avoid missed test coverage.- allFiles := []string{"1.graphql", "2.graphql", "3.graphql", "4.graphql", "5.graphql"} + entries, _ := os.ReadDir(path.Join(getTestDataDir(), "queries", "base")) + allFiles := make([]string, 0, len(entries)) + for _, e := range entries { + if !e.IsDir() { + allFiles = append(allFiles, e.Name()) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
router-tests/go.mod(1 hunks)router/core/plan_generator.go(1 hunks)router/go.mod(1 hunks)router/pkg/plan_generator/plan_generator_test.go(1 hunks)router/pkg/plan_generator/testdata/plans/base/5.graphql(1 hunks)router/pkg/plan_generator/testdata/plans/base/report.json(1 hunks)router/pkg/plan_generator/testdata/queries/base/5.graphql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in go testing, t.fatal, t.failnow, t.skip* and similar methods cannot be called from goroutines othe...
Learnt from: Noroth
PR: wundergraph/cosmo#2088
File: demo/pkg/subgraphs/projects/src/main_test.go:0-0
Timestamp: 2025-07-29T08:19:55.720Z
Learning: In Go testing, t.Fatal, t.FailNow, t.Skip* and similar methods cannot be called from goroutines other than the main test goroutine, as they will cause a runtime panic. Use channels, t.Error*, or other synchronization mechanisms to communicate errors from goroutines back to the main test thread.
Applied to files:
router/pkg/plan_generator/plan_generator_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: lint
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
router/go.mod (1)
34-35: Confirm the pseudo-version pin is intentional
github.com/wundergraph/graphql-go-tools/v2is now fixed to a commit-based pseudo-version (fa127e0).
Please double-check that:
- This commit is pushed to the public upstream and won’t be garbage-collected.
- All downstream modules (e.g. private CI caches, mirrors) can fetch it.
- No local
replaceoverride is accidentally left active – the commented line 165 looks fine.If a proper tag will be cut shortly, prefer switching to the tag for cleaner
go mod tidyoutput.router/pkg/plan_generator/testdata/plans/base/report.json (1)
1-1: Test fixture updated – LGTMThe report now contains the expected plan for
5.graphql; structure matches the existing entries.router/pkg/plan_generator/testdata/queries/base/5.graphql (1)
1-9: Good edge-case coverage for@skip/@includedirectivesAdding this query ensures the normaliser keeps skipped/included nodes for plan generation – great catch.
router/pkg/plan_generator/testdata/plans/base/5.graphql (1)
1-30: LGTM! Well-structured test data for skip/include directive testing.The GraphQL query plan follows proper federation patterns with appropriate sequencing of fetch operations. The structure correctly represents the expected output for testing skip/include directive handling where all query nodes are preserved during plan generation.
router/core/plan_generator.go (1)
128-136: Excellent refactor to support skip/include directive preservation.The change from a single normalization call to a configurable normalizer with explicit options directly addresses the PR objective. The addition of
WithIgnoreSkipInclude()ensures that query nodes with skip/include directives are preserved during plan generation, allowing full visibility into all query nodes regardless of their execution status.The comprehensive set of normalization options maintains existing functionality while adding the required directive handling capability.
ysmolski
force-pushed
the
yury/eng-7726-skipinclude-should-be-ignored-in-query-plan-generator
branch
from
6691a7d to
0274600
Compare
This allows to print all query nodes, even those that would not be executed because of directives like "skip", "include".
ysmolski
force-pushed
the
yury/eng-7726-skipinclude-should-be-ignored-in-query-plan-generator
branch
from
0274600 to
217e8dc
Compare
ysmolski
deleted the
yury/eng-7726-skipinclude-should-be-ignored-in-query-plan-generator
branch
This allows to print nodes that would not be included because of directives like "skip", "include".
Depends on wundergraph/graphql-go-tools#1261
Summary by CodeRabbit
Summary by CodeRabbit
New Features
@includeand@skipdirectives.Bug Fixes
Chores
Checklist