Conversation
## Walkthrough
This change introduces support for accessing both top-level and subgraph GraphQL response bodies in expression-based logging and configuration. It adds new AST visitors, context fields, and detection logic for `response.body` and `subgraph.response.body` usage in expressions. The server middleware and engine hooks are updated to capture and store response bodies when required. Comprehensive unit and integration tests are included, and dependencies are updated. Additionally, minor cleanup removes debug print statements in tests and metric code.
## Changes
| Files/Groups | Change Summary |
|------------------------------------------------------------------------------|---------------|
| `router/internal/expr/expr.go` | Adds `Response` field to `Context`, introduces `Response`, `SubgraphResponse` structs, updates `Request` and `Subgraph` structs to support response body access in expressions. |
| `router/internal/expr/visitor_group.go` | Renames and exports `visitorGroup` to `VisitorGroup`, adds new visitor kinds and methods for detecting `response.body` and `subgraph.response.body` usage in expressions, updates factory and method names. |
| `router/internal/expr/uses_response_body.go`<br>`.../uses_response_body_test.go` | Introduces `UsesResponseBody` AST visitor and corresponding tests to detect `response.body` usage in expressions. |
| `router/internal/expr/uses_subgraph_response_body.go`<br>`.../uses_subgraph_response_body_test.go` | Introduces `UsesSubgraphResponseBody` AST visitor and corresponding tests for `subgraph.response.body` usage detection. |
| `router/internal/expr/visitor_group_test.go` | Adds tests for `VisitorGroup` methods detecting response body and subgraph response body usage in various expression forms. |
| `router/internal/expr/expr_manager.go` | Updates `Manager` to use exported `VisitorGroup` type, changes initialization to use new factory function. |
| `router/internal/expr/expr_manager_test.go` | Updates tests to use new method `IsRequestBodyUsedInExpressions()` instead of the old method. |
| `router/core/engine_loader_hooks.go` | Adds `storeSubgraphResponseBody` field and parameter, conditionally stores subgraph response body in expression context during request finalization. |
| `router/core/graph_server.go` | Refactors access log attribute handling, adds middleware to capture HTTP response body when needed, passes new boolean to engine hooks for subgraph response body capture. |
| `router/core/graphql_prehandler.go` | Switches to new method `IsRequestBodyUsedInExpressions()` for request body capture logic. |
| `router-tests/structured_logging_test.go` | Adds new subtests covering logging of expression-evaluated fields for response bodies and subgraph response bodies. |
| `router-tests/circuit_breaker_test.go`<br>`router/pkg/metric/prom_metric_store.go` | Removes unused "fmt" imports and debug print statements from circuit breaker test and metric store code. |
| `router/go.mod`<br>`router-tests/go.mod` | Updates `github.com/wundergraph/graphql-go-tools/v2` dependency from `v2.0.0-rc.200` to `v2.0.0-rc.201`. |📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
router/internal/expr/expr.go (1)
41-61: Update Clone method to handle new Response fields.The
Clone()method doesn't handle the newResponseandSubgraphResponsefields. While these currently only contain simpleBodystructs with string fields, the method should be updated for consistency and future-proofing.func (copyCtx Context) Clone() *Context { // the method receiver copyCtx is already a copy // so we just need to make sure any pointer values are copied scopes := make([]string, len(copyCtx.Request.Auth.Scopes)) copy(scopes, copyCtx.Request.Auth.Scopes) copyCtx.Request.Auth.Scopes = scopes claims := make(map[string]any, len(copyCtx.Request.Auth.Claims)) for k, v := range copyCtx.Request.Auth.Claims { claims[k] = v } copyCtx.Request.Auth.Claims = claims query := make(map[string]string, len(copyCtx.Request.URL.Query)) for k, v := range copyCtx.Request.URL.Query { - claims[k] = v + query[k] = v } copyCtx.Request.URL.Query = query + // Note: Response and SubgraphResponse currently contain only value types, + // but this ensures consistency if they're extended with pointer fields + // copyCtx.Response and copyCtx.Subgraph.Response are already copied by value return ©Ctx }Note: I also fixed a bug where
claims[k] = vshould bequery[k] = von line 56.
🧹 Nitpick comments (1)
router-tests/structured_logging_test.go (1)
2943-2991: Comprehensive subgraph response body test with detailed validation.This test effectively validates the
subgraph.responseBody.rawexpression across multiple subgraph calls. The assertions comprehensively check all expected response bodies from different subgraphs, which is excellent for ensuring the feature works correctly across complex GraphQL operations.The hardcoded response body strings are quite long but necessary for precise validation. Consider if these could be extracted to constants or helper functions to improve readability, though this follows the existing pattern in the test file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
router-tests/structured_logging_test.go(3 hunks)router/core/engine_loader_hooks.go(5 hunks)router/core/graph_server.go(3 hunks)router/core/graphql_handler.go(5 hunks)router/core/graphql_prehandler.go(2 hunks)router/internal/expr/expr.go(4 hunks)router/internal/expr/expr_manager.go(1 hunks)router/internal/expr/expr_manager_test.go(3 hunks)router/internal/expr/uses_response_body.go(1 hunks)router/internal/expr/uses_response_body_test.go(1 hunks)router/internal/expr/uses_subgraph_response_body.go(1 hunks)router/internal/expr/uses_subgraph_response_body_test.go(1 hunks)router/internal/expr/visitor_group.go(1 hunks)router/internal/expr/visitor_group_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
router/internal/expr/expr_manager.go (1)
router/internal/expr/visitor_group.go (1)
VisitorGroup(17-20)
router/internal/expr/visitor_group_test.go (2)
router-tests/testenv/testenv.go (1)
Run(104-121)router/internal/expr/expr_manager.go (1)
CreateNewExprManager(19-23)
router/internal/expr/uses_subgraph_response_body_test.go (1)
router/internal/expr/uses_subgraph_response_body.go (1)
UsesSubgraphResponseBody(10-12)
router-tests/structured_logging_test.go (3)
router-tests/testenv/testenv.go (5)
Run(104-121)Config(279-334)LogObservationConfig(380-383)Environment(1715-1751)GraphQLRequest(1883-1891)router/pkg/config/config.go (3)
Config(932-1006)CustomAttribute(43-47)CustomDynamicAttribute(36-41)router/internal/expr/expr.go (1)
Body(92-94)
router/internal/expr/visitor_group.go (4)
router/internal/expr/use_body.go (1)
UsesBody(7-9)router/internal/expr/uses_subgraph_trace.go (1)
UsesSubgraphTrace(9-11)router/internal/expr/uses_response_body.go (1)
UsesResponseBody(10-12)router/internal/expr/uses_subgraph_response_body.go (1)
UsesSubgraphResponseBody(10-12)
router/core/graph_server.go (5)
router/internal/requestlogger/access_log_expression.go (1)
CleanupExpressionAttributes(42-52)router/internal/requestlogger/requestlogger.go (6)
Option(35-35)WithDefaultOptions(101-109)WithNoTimeField(88-93)WithFields(95-99)WithAttributes(64-68)IPAnonymizationConfig(37-40)router/internal/requestlogger/subgraphlogger.go (2)
NewSubgraphAccessLogger(36-48)SubgraphOptions(28-34)router/pkg/trace/meter.go (1)
IPAnonymizationConfig(30-33)router/core/request_context_fields.go (1)
SubgraphAccessLogsFieldHandler(116-132)
🪛 GitHub Actions: Router CI
router/core/engine_loader_hooks.go
[error] 156-156: go vet error: 'responseInfo.ResponseBody' undefined (type *resolve.ResponseInfo has no field or method ResponseBody)
🔇 Additional comments (25)
router/internal/expr/expr_manager_test.go (1)
142-142: LGTM! Consistent method rename for improved clarity.The method name change from
IsBodyUsedInExpressions()toIsRequestBodyUsedInExpressions()provides better clarity by explicitly indicating that it checks for request body usage, distinguishing it from response body usage detection.Also applies to: 160-160, 173-173
router/core/graphql_prehandler.go (1)
303-303: LGTM! Consistent with the expression manager refactoring.The method rename maintains consistency across the codebase and improves clarity by explicitly indicating request body usage detection.
Also applies to: 335-335
router/internal/expr/expr_manager.go (1)
16-16: LGTM! Proper export and naming improvements.The changes export the
VisitorGrouptype and standardize the factory function name, making the visitor management functionality more accessible and consistently named.Also applies to: 21-21
router/internal/expr/visitor_group_test.go (1)
9-126: LGTM! Comprehensive test coverage for new visitor functionality.The test suite provides excellent coverage of the new response body detection methods with:
- Multiple expression patterns (dot notation, bracket notation, mixed access)
- Both positive and negative test cases
- Parallel execution for efficiency
- Clear test case naming and structure
The test cases appropriately validate that the visitor manager correctly identifies when expressions reference
response.bodyandsubgraph.response.bodyfields.router/core/engine_loader_hooks.go (2)
45-45: LGTM! Proper integration of expression visitor manager.The addition of
exprVisitorManagerfield and its initialization in the constructor follows the established pattern and correctly integrates the visitor management functionality.Also applies to: 59-59, 81-81
155-157: Fix compilation error: use correctResponseInfofield for response bodyFile:
router/core/engine_loader_hooks.go(lines 155–157)if f.exprVisitorManager.IsSubgraphResponseBodyUsedInExpression() { exprCtx.Subgraph.Response.Body.Raw = responseInfo.ResponseBody }The
ResponseInfotype from
github.com/wundergraph/graphql-go-tools/v2/pkg/engine/resolve
does not define aResponseBodyfield. Please update this assignment to use the actual field or getter onResponseInfo, for example:
- If the field is named
RawResponse:exprCtx.Subgraph.Response.Body.Raw = responseInfo.RawResponse- If there’s a method
GetResponseBody():exprCtx.Subgraph.Response.Body.Raw = responseInfo.GetResponseBody()Consult the
ResponseInfodefinition in the GraphQL tools library and adjust accordingly to resolve the compilation error.router/core/graphql_handler.go (2)
9-9: LGTM: Clean import addition.The import for the internal expr package is correctly placed and necessary for the new expression visitor functionality.
202-207: LGTM: Response body capture logic is well-implemented.The conditional logic correctly:
- Checks if response body is used in expressions before capturing
- Uses
respBuf.String()which safely returns the entire buffer content- Assigns to the appropriate expression context field
The comment explaining the buffer offset behavior is helpful for maintainability.
router/internal/expr/uses_response_body.go (1)
1-63: LGTM: Well-implemented visitor pattern for response body detection.The implementation correctly:
- Uses early returns to optimize traversal when target is found or node is nil
- Properly detects the
response.bodyaccess pattern through member node analysis- Handles both StringNode and IdentifierNode property types
- Recursively traverses nested member access structures
The code is clean, well-commented, and follows good practices for AST visitor patterns.
router/core/graph_server.go (2)
929-929: LGTM: Proper configuration immutability preserved.The use of
CleanupExpressionAttributescorrectly creates local copies of the configuration attributes, preserving the original configuration while filtering out expression-based attributes for traditional logging. This maintains configuration immutability and separation of concerns.Also applies to: 935-935, 961-961, 969-969
1172-1174: LGTM: Consistent expression visitor manager integration.The expression visitor manager is correctly passed to both the engine loader hooks and the handler options, ensuring consistent access to expression visitor state across components.
router/internal/expr/uses_response_body_test.go (1)
1-135: LGTM: Comprehensive test coverage for response body visitor.The test suite excellently covers:
- Edge cases (nil nodes, non-member nodes)
- Positive detection scenarios (
response.body,response.body.raw)- Negative scenarios (wrong property names, wrong base objects)
- Different AST node types (StringNode vs IdentifierNode properties)
- Optimization behavior (early return when already detected)
- Nested member access patterns
This thorough testing ensures the visitor behaves correctly across all expected use cases.
router/internal/expr/uses_subgraph_response_body.go (1)
1-75: LGTM: Well-implemented subgraph response body visitor.The implementation correctly handles the three-level property chain detection for
subgraph.response.body:
- Properly validates each level of the property chain in reverse order
- Uses consistent error handling and early returns
- Maintains the same code structure and patterns as the related
UsesResponseBodyvisitor- Includes appropriate type checking for nested member nodes
The step-by-step validation ensures accurate detection while maintaining code readability.
router/internal/expr/expr.go (5)
36-36: LGTM! Response field addition follows existing patterns.The addition of the
Responsefield to theContextstruct is well-structured and follows the same pattern as the existingRequestfield.
69-74: LGTM! Body type generalization and field reordering.The generalization from
RequestBodytoBodymakes sense since the same structure is now used for both request and response bodies. Moving theErrorfield to the end is a minor structural improvement.
76-78: LGTM! Response struct follows established patterns.The new
Responsestruct is simple and consistent with the existing architecture, providing a clean way to access response body data in expressions.
92-94: LGTM! Body struct generalization.Renaming
RequestBodytoBodyis a good generalization that allows the same structure to be used for both request and response bodies.
130-144: LGTM! Subgraph response support is well-structured.The addition of
SubgraphResponsestruct and theResponsefield to theSubgraphstruct provides comprehensive response body access for subgraph expressions.router/internal/expr/uses_subgraph_response_body_test.go (2)
1-264: Excellent comprehensive test coverage for UsesSubgraphResponseBody visitor.This test file provides thorough coverage of the
UsesSubgraphResponseBodyvisitor with well-structured test cases covering:
- Positive cases: Correct detection of
subgraph.response.bodyaccess patterns- Negative cases: Proper rejection of similar but incorrect patterns
- Edge cases: Handling of different AST node types and structures
- Deep nesting: Support for accessing nested properties like
subgraph.response.body.rawThe test cases are well-organized, each has a clear purpose, and the AST node construction accurately represents the expression structures being tested.
146-163: Visitor intentionally supports both StringNode and IdentifierNode for propertiesThe
getPropertyNamehelper explicitly handles both*ast.StringNodeand*ast.IdentifierNode, so allowing non-string property nodes in your tests is correct and aligns with the implementation. No changes required.router-tests/structured_logging_test.go (1)
2535-2565: LGTM! Well-structured response body expression test.This test properly validates the new
response.body.rawexpression capability. The configuration is correct, the assertions are specific, and it follows the established test patterns.router/internal/expr/visitor_group.go (4)
8-12: LGTM! Good expansion of visitor kinds for response body support.The renaming of
usesBodyKeytousesRequestBodyKeyprovides better clarity, and the addition ofusesResponseBodyKeyandusesSubgraphResponseBodyKeyappropriately extends the framework to support the new response payload logging feature.
17-17: LGTM! Appropriate export of the struct.Making
VisitorGroupexported allows external packages to use this functionality, which aligns with the PR objectives to enable response payload logging across the codebase.
22-31: LGTM! Proper integration of new visitor types.The factory function correctly initializes all visitor types including the new response body visitors. The mapping between visitor kinds and their corresponding visitor structs is consistent and follows the established pattern.
33-36: LGTM! Improved method naming for clarity.The rename from
IsBodyUsedInExpressionstoIsRequestBodyUsedInExpressionsprovides better specificity about which type of body is being checked, improving code readability.
SkArchon
changed the title
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/internal/expr/visitor_group_test.go (1)
15-50: Consider adding edge case test scenarios.The current test cases cover the main access patterns well. Consider adding tests for:
- Invalid/malformed expressions to verify error handling
- Complex expressions combining multiple patterns
- Expressions that don't reference response/subgraph at all
- Empty expressions
Example additional test cases:
+ { + name: "complex expression with body", + expression: "response.body.data && request.headers['content-type']", + expectedResult: true, + }, + { + name: "no response reference", + expression: "request.method == 'POST'", + expectedResult: false, + },
📜 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 (6)
router-tests/go.mod(1 hunks)router/core/engine_loader_hooks.go(5 hunks)router/core/graphql_handler.go(5 hunks)router/go.mod(1 hunks)router/internal/expr/visitor_group.go(1 hunks)router/internal/expr/visitor_group_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- router/go.mod
- router-tests/go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- router/core/engine_loader_hooks.go
- router/core/graphql_handler.go
- router/internal/expr/visitor_group.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
router/internal/expr/visitor_group_test.go (1)
router/internal/expr/expr_manager.go (1)
CreateNewExprManager(19-23)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
🔇 Additional comments (3)
router/internal/expr/visitor_group_test.go (3)
1-127: Excellent test coverage for visitor manager functionality.This test file demonstrates solid testing practices with comprehensive coverage of the VisitorManager's body usage detection capabilities. The test structure is well-organized, uses proper isolation, and covers various expression syntaxes effectively.
56-62: Test execution pattern is well-structured.The pattern of creating a new expression manager for each test case ensures proper test isolation. The compilation step followed by visitor state checking correctly validates the detection logic.
70-110: Comprehensive subgraph response body test coverage.The test cases appropriately verify that only expressions accessing
subgraph.response.body(and its nested properties) return true, while shorter paths likesubgraphorsubgraph.responsecorrectly return false.
SkArchon
force-pushed
the
milinda/eng-7440-allow-to-log-response-payload-of-router-and-subgraph-via
branch
from
3f1a476 to
729f6ee
Compare
…w-to-log-response-payload-of-router-and-subgraph-via
SkArchon
force-pushed
the
milinda/eng-7440-allow-to-log-response-payload-of-router-and-subgraph-via
branch
from
a7a241f to
a2a8ea5
Compare
…ad-of-router-and-subgraph-via
…w-to-log-response-payload-of-router-and-subgraph-via
SkArchon
deleted the
milinda/eng-7440-allow-to-log-response-payload-of-router-and-subgraph-via
branch
2 participants
Summary by CodeRabbit
Depends on wundergraph/graphql-go-tools#1203
Docs: wundergraph/cosmo-docs#110
Checklist