-
Notifications
You must be signed in to change notification settings - Fork 207
feat: client response expressions #2472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds client-side header propagation: new 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
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (80.76%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2472 +/- ##
===========================================
+ Coverage 36.06% 61.87% +25.81%
===========================================
Files 944 229 -715
Lines 123477 23889 -99588
Branches 4951 0 -4951
===========================================
- Hits 44530 14782 -29748
+ Misses 77386 7851 -69535
+ Partials 1561 1256 -305
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist