feat: custom module to propagate headers from subgraphs to federated response - #3121
feat: custom module to propagate headers from subgraphs to federated response#3121alepane21 wants to merge 2 commits into
Conversation
federated response
WalkthroughChangesCache tag aggregation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
router-tests/modules/cache_tags_test.go (1)
281-301: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
sync.WaitGroup.Gofor the completion group.
donecan drop the manualAdd/defer Donebookkeeping (readystill needs manual handling as a start barrier).Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically."
♻️ Proposed refactor
var ready, done sync.WaitGroup ready.Add(requestCount) - done.Add(requestCount) trigger := make(chan struct{}) errs := make(chan error, requestCount) responses := make([]*testenv.TestResponse, requestCount) for i := range requestCount { - go func() { - defer done.Done() + done.Go(func() { ready.Done() <-trigger response, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{Query: query}) if err != nil { errs <- err return } responses[i] = response - }() + }) }🤖 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 `@router-tests/modules/cache_tags_test.go` around lines 281 - 301, Update the goroutine launch in this test to use sync.WaitGroup.Go for the done completion group, removing done.Add and the corresponding defer done.Done bookkeeping. Keep the manual ready WaitGroup handling and start barrier unchanged.Source: Learnings
router-tests/modules/cache-tags/module.go (1)
161-189: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider
s-maxage/must-revalidatein the restrictive merge.A subgraph sending
s-maxage=30, must-revalidatecurrently contributes nothing, so the federated policy ends up less restrictive than the subgraph asked for. Fine if intentionally out of scope for this test module — otherwise fold them in.🤖 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 `@router-tests/modules/cache-tags/module.go` around lines 161 - 189, Extend parseCacheControl to recognize s-maxage and must-revalidate during restrictive policy merging, ensuring their values contribute to the returned cacheControlPolicy rather than being ignored. Reuse the existing max-age parsing and restrictive-minimum behavior for s-maxage, and update the policy representation as needed for must-revalidate while preserving current directives and return semantics.
🤖 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.
Nitpick comments:
In `@router-tests/modules/cache_tags_test.go`:
- Around line 281-301: Update the goroutine launch in this test to use
sync.WaitGroup.Go for the done completion group, removing done.Add and the
corresponding defer done.Done bookkeeping. Keep the manual ready WaitGroup
handling and start barrier unchanged.
In `@router-tests/modules/cache-tags/module.go`:
- Around line 161-189: Extend parseCacheControl to recognize s-maxage and
must-revalidate during restrictive policy merging, ensuring their values
contribute to the returned cacheControlPolicy rather than being ignored. Reuse
the existing max-age parsing and restrictive-minimum behavior for s-maxage, and
update the policy representation as needed for must-revalidate while preserving
current directives and return semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45c5cbfa-9df2-4a39-bc5a-111f24d4ddd0
📒 Files selected for processing (2)
router-tests/modules/cache-tags/module.gorouter-tests/modules/cache_tags_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3121 +/- ##
==========================================
+ Coverage 61.34% 62.20% +0.86%
==========================================
Files 279 262 -17
Lines 32328 30868 -1460
==========================================
- Hits 19831 19201 -630
+ Misses 10924 10158 -766
+ Partials 1573 1509 -64 🚀 New features to boost your workflow:
|
This PR contains a module that allows to propagate cache tags from subgraph responses to the federated response.
The cache tags will be merged and the Cache-Control header will use the most restrictive settings from all the subgraph responses.
This module will disable the subgraph request deduplication (https://cosmo-docs.wundergraph.com/router/request-deduplication#layer-1-subgraph-request-deduplication).
Summary by CodeRabbit
New Features
max-age.Bug Fixes
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.