Conversation
WalkthroughAdds router-level response header rules: new config/schema types and fixtures, splits compiled header expressions into request vs router-response maps, wires HeaderPropagation into Router and GraphQL handler, and adds unit and integration tests for router response header evaluation, error handling, and logging. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2472 +/- ##
==========================================
+ Coverage 61.60% 61.63% +0.03%
==========================================
Files 229 229
Lines 23860 23894 +34
==========================================
+ Hits 14698 14728 +30
- Misses 7923 7924 +1
- Partials 1239 1242 +3
🚀 New features to boost your workflow:
|
![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
🤖 Fix all issues with AI agents
In `@router/core/header_rule_engine.go`:
- Around line 248-277: The response rule "expression" field is never compiled or
evaluated; update compileExpressionRules to also compile response rules (call
processExpression for each response rule and store programs in a
compiledResponseRules map on HeaderPropagation using the same expr.Manager), and
update applyResponseRule to evaluate the compiled program for the response rule
(look up hf.compiledResponseRules[rule.Expression] and execute via the expr VM
to decide rule application), or alternatively validate and reject any response
rules that contain an Expression by returning an error from
compileExpressionRules; reference functions: compileExpressionRules,
processExpression, applyResponseRule and the compiledResponseRules map on
HeaderPropagation when implementing the fix.
🧹 Nitpick comments (1)
router/core/router_config.go (1)
99-101: Include client header rules in usage tracking.Now that client rules exist,
Usage()can reportheader_rulesas disabled when onlyheaders.clientis configured. Consider countingClientas well.✅ Suggested update
- usage["header_rules"] = c.headerRules != nil && (c.headerRules.All != nil || len(c.headerRules.Subgraphs) > 0) + usage["header_rules"] = c.headerRules != nil && + (c.headerRules.All != nil || len(c.headerRules.Subgraphs) > 0 || len(c.headerRules.Client) > 0)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/header_rule_engine.go (1)
629-642:⚠️ Potential issue | 🟡 MinorInconsistent error formatting with
getRequestRuleExpressionValue.Line 639 explicitly calls
err.Error()while the equivalent line 624 ingetRequestRuleExpressionValueuses%swith the error directly. Both work correctly, but for consistency, use the same style.🔧 Suggested fix for consistency
func (h *HeaderPropagation) getClientRuleExpressionValue(rule *config.ClientHeaderRule, reqCtx *requestContext) (value string, err error) { if reqCtx == nil { return "", fmt.Errorf("context cannot be nil") } program, ok := h.compiledClientRules[rule.Expression] if !ok { return "", fmt.Errorf("expression %s not found in compiled rules for header rule %s", rule.Expression, rule.Name) } value, err = expr.ResolveStringExpression(program, reqCtx.expressionContext) if err != nil { - return "", fmt.Errorf("unable to resolve expression %q for header rule %s: %s", rule.Expression, rule.Name, err.Error()) + return "", fmt.Errorf("unable to resolve expression %q for header rule %s: %s", rule.Expression, rule.Name, err) } return }
🧹 Nitpick comments (2)
router/pkg/config/config.go (1)
333-337: Consider adding documentation comments for consistency.Unlike
RequestHeaderRuleandResponseHeaderRulewhich have detailed comments explaining each field,ClientHeaderRulelacks documentation. Adding comments would improve maintainability.📝 Suggested documentation
type ClientHeaderRule struct { - // Set header options - Name string `yaml:"name"` - Expression string `yaml:"expression"` + // Name is the name of the header to set on the client response + Name string `yaml:"name"` + // Expression is the Expr Lang expression to evaluate for dynamic header values. + // Expressions are evaluated once after all subgraph requests complete, + // allowing access to errors, response headers, and other post-subgraph information. + Expression string `yaml:"expression"` }router/core/header_rule_engine.go (1)
265-278: Consider renaming thehfparameter for clarity.The parameter name
hfinprocessExpressionis misleading since it's amap[string]*vm.Program, not aHeaderPropagationinstance. A more descriptive name would improve readability.♻️ Suggested rename
-func processExpression(expression string, hf map[string]*vm.Program, manager *expr.Manager) error { +func processExpression(expression string, compiledRules map[string]*vm.Program, manager *expr.Manager) error { if expression == "" { return nil } - if _, ok := hf[expression]; ok { + if _, ok := compiledRules[expression]; ok { return nil } program, err := manager.CompileExpression(expression, reflect.String) if err != nil { return fmt.Errorf("error compiling expression %s for header rule: %w", expression, err) } - hf[expression] = program + compiledRules[expression] = program return nil }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@router/core/header_rule_engine.go`:
- Around line 703-719: ApplyClientResponseHeaderRules currently dereferences
h.rules.Client and can panic if the receiver h is nil; add the same nil-receiver
guard used by methods like HasRequestRules/HasResponseRules at the start of
ApplyClientResponseHeaderRules (e.g., if h == nil || h.rules == nil ||
len(h.rules.Client) == 0 { return nil }) so the method safely returns when h or
h.rules is nil and only iterates rules when present; keep the existing error
handling and header setting logic unchanged.
🧹 Nitpick comments (1)
router/core/header_rule_engine.go (1)
265-278: Consider renaming thehfparameter to avoid confusion.The parameter
hf map[string]*vm.Programshadows the common pattern wherehfrefers toHeaderPropagationelsewhere in this file. Consider renaming tocacheorcompiledRulesfor clarity.♻️ Suggested rename
-func processExpression(expression string, hf map[string]*vm.Program, manager *expr.Manager) error { +func processExpression(expression string, compiledRules map[string]*vm.Program, manager *expr.Manager) error { if expression == "" { return nil } - if _, ok := hf[expression]; ok { + if _, ok := compiledRules[expression]; ok { return nil } program, err := manager.CompileExpression(expression, reflect.String) if err != nil { return fmt.Errorf("error compiling expression %s for header rule: %w", expression, err) } - hf[expression] = program + compiledRules[expression] = program return nil }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@router/core/header_rule_engine.go`:
- Around line 703-718: ApplyRouterResponseHeaderRules in HeaderPropagation
currently deletes the header when getRouterResponseRuleExpressionValue returns
an empty string; instead treat empty as "no mutation". Modify the loop over
h.rules.Router.Response (and the handling around rule.Name and value) so that
when value == "" you do nothing (no Header().Del), only call
w.Header().Set(rule.Name, value) when value is non-empty; keep existing continue
for rule.Expression == "" and preserve error handling from
getRouterResponseRuleExpressionValue.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/header_rule_engine.go (1)
147-216:⚠️ Potential issue | 🟡 MinorInclude router response rules in
hasResponseRulesflag.
hasResponseRulesonly reflects subgraph response rules. If a config has only router response rules and no subgraph rules, the flag will be false, preventingOnOriginResponsefrom being registered as a post-origin handler even though router rules will be applied separately. Update the flag assignment to include router rules:Suggested fix
- hf.hasResponseRules = len(rhrrs) > 0 + hf.hasResponseRules = len(rhrrs) > 0 || len(rrs) > 0
🧹 Nitpick comments (1)
router/core/header_rule_engine.go (1)
251-280: RenamerouterRequestRulestorouterResponseRulesfor clarity.The function compiles router response rules; a more precise name reduces confusion.
Suggested rename
-func (hf *HeaderPropagation) compileExpressionRules(subgraphRequestRules []*config.RequestHeaderRule, routerRequestRules []*config.RouterResponseHeaderRule) error { +func (hf *HeaderPropagation) compileExpressionRules(subgraphRequestRules []*config.RequestHeaderRule, routerResponseRules []*config.RouterResponseHeaderRule) error { manager := expr.CreateNewExprManager() for _, rule := range subgraphRequestRules { if err := processExpression(rule.Expression, hf.compiledRequestRules, manager); err != nil { return fmt.Errorf("error compiling header %s: %w", rule.Name, err) } } - for _, rule := range routerRequestRules { + for _, rule := range routerResponseRules { if err := processExpression(rule.Expression, hf.compiledRouterResponseRules, manager); err != nil { return fmt.Errorf("error compiling header %s: %w", rule.Name, err) } }
2 participants
This PR allows for the use of response client expressions, currently we only support header propagation from the subgraph level. Even for cases like setting a static value like "test" for a header on responses, this would run for every subgraph. The problem with this is that the requestContext's expression context has not been updated with information such as errors, response headers, etc. As this is processed after the subgraph level round trippers. (The use case was that the user wants to access the
errorattribute in expressions, which is not populated at this level).In order to mitigate this we introduce client expressions, which runs once before returning the response.
Summary by CodeRabbit
New Features
Tests
Checklist