fix(router): ignore header rules for pubsub trigger sources - #3135
Conversation
WalkthroughThe router excludes PubSub subscription triggers from propagated headers and header hashes. Tests cover PubSub and non-PubSub hash behavior, Authorization-based trigger deduplication, and header-specific nested fetches. ChangesPubSub subscription header handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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/core/header_rule_engine_buildheader_test.go (1)
409-428: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the required zero aggregate hash.
Lines 425-428 only prove that two client header values produce equal hashes. They do not prove that a PubSub-only plan produces the required empty hash. Store one result and assert that it is zero before comparing it with the second result.
Proposed test update
- assert.Equal(t, - SubgraphHeadersBuilder(newCtx("va"), ht, pubSubOnlyPlan()).HashAll(), - SubgraphHeadersBuilder(newCtx("vb"), ht, pubSubOnlyPlan()).HashAll(), - ) + hashA := SubgraphHeadersBuilder(newCtx("va"), ht, pubSubOnlyPlan()).HashAll() + hashB := SubgraphHeadersBuilder(newCtx("vb"), ht, pubSubOnlyPlan()).HashAll() + + assert.Zero(t, hashA) + assert.Equal(t, hashA, hashB)🤖 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/core/header_rule_engine_buildheader_test.go` around lines 409 - 428, Update the test case around SubgraphHeadersBuilder.HashAll to store the first hash result, assert that the PubSub-only plan produces the required zero aggregate hash, then compute and compare the second hash result for equality. Preserve the existing use of different propagated client headers and pubsubOnlyPlan.router-tests/events/trigger_test.go (1)
129-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
WaitGroup.Gofor both subscription goroutines.router-tests/go.modtargets Go 1.25.0, so replacedone.Add(2)and the deferreddone.Done()calls at lines 129-143 and 175-177 withdone.Go(...).🤖 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/events/trigger_test.go` around lines 129 - 143, Update both subscription goroutine launches in the surrounding test to use done.Go(...) instead of separate done.Add(2) bookkeeping and deferred done.Done() calls. Remove the manual Add call and preserve each goroutine’s existing body and synchronization behavior.Source: Learnings
🤖 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/events/trigger_test.go`:
- Around line 129-143: Update both subscription goroutine launches in the
surrounding test to use done.Go(...) instead of separate done.Add(2) bookkeeping
and deferred done.Done() calls. Remove the manual Add call and preserve each
goroutine’s existing body and synchronization behavior.
In `@router/core/header_rule_engine_buildheader_test.go`:
- Around line 409-428: Update the test case around
SubgraphHeadersBuilder.HashAll to store the first hash result, assert that the
PubSub-only plan produces the required zero aggregate hash, then compute and
compare the second hash result for equality. Preserve the existing use of
different propagated client headers and pubsubOnlyPlan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bcf4946-8c1a-4c66-b6fa-057e1894a517
📒 Files selected for processing (3)
router-tests/events/trigger_test.gorouter/core/context.gorouter/core/header_rule_engine_buildheader_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3135 +/- ##
===========================================
+ Coverage 48.94% 62.37% +13.42%
===========================================
Files 1130 262 -868
Lines 157571 31003 -126568
Branches 10883 0 -10883
===========================================
- Hits 77130 19337 -57793
+ Misses 78588 10158 -68430
+ Partials 1853 1508 -345
🚀 New features to boost your workflow:
|
…es-on-nats-with-durable-consumers
Fixes the single flight test assumption that edfs triggers are not deduplicated on different headers. In contrast to normal subscriptions they now will. The test now expects one trigger will still ensuring deduplication to subgraphs during resolving still works. Also added another websocket subscription test to make sure that for non-edfs and edfs triggers the deduplication based on headers work as expected.
|
By the way there is a very similar pull request, which fixed a very similar issue #2950 . In contrast to this one it fixed any headers from the websocket |
9 LOC actual bug fix, the remainder is test code.
Fixes a bug on the routers header rule builder. When a user configures the router to propagate headers to all subgraphs
it adds this rule to every subgraph, which includes the Cosmo Streams trigger source. Using the demo environment and performing a query
The header builder builds available headers per subgraph in its map
The first one is the EDFS trigger source and it does not make sense. In reality this is a message broker. When the router connects to them it does not forward any headers. So having header rules for it does not make sense.
The fix is to avoid the first map entry but keep the others. This way subgraph fetches during resolving still send headers.
This caused a bug, which is how I became aware of it. Headers for trigger sources are used as part of the trigger id hash. Two clients on the same subscription with different headers landed on different triggers. It meant they listened for the same events on the same message queue with two different triggers. Having seperate triggers is necesarry for normal subscriptions but not for Cosmo Streams. Removing the above mentioned first line avoids the hash generation in the engine from considering header values.
EDIT: Had to fix a single flight test assumption that edfs triggers are not deduplicated on different headers.
In contrast to normal subscriptions they now will. The test now expects one trigger will still ensuring deduplication
to subgraphs during resolving still works.
Summary by CodeRabbit
Bug Fixes
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.