feat: add Variables to OperationContext#2045
Conversation
alepane21
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughThe Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
Router image scan passed✅ No security vulnerabilities found in image: |
alepane21
force-pushed
the
ale/eng-7598-add-variables-to-requestcontextoperation
branch
from
86ceafe to
755cf67
Compare
|
Do we need some tests here? |
I've added some integration tests, thanks for the suggestion! |
![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
🧹 Nitpick comments (3)
router-tests/modules/verify_operation_context_values_test.go (3)
72-72: Hardcoded normalized query assertion may be brittle.The assertion uses a hardcoded normalized query string which could break if the GraphQL normalization algorithm changes. Consider using a more flexible approach.
Consider making the content assertion more flexible:
-assert.Equal(t, captured.Content, "query GetEmployee($a: Int!){employee(id: $a){id details {forename surname} tag}}", "Operation content should be set") +assert.Contains(t, captured.Content, "GetEmployee", "Operation content should contain the operation name") +assert.Contains(t, captured.Content, "employee(id:", "Operation content should contain the main field")This makes the test more resilient to changes in query normalization while still verifying the content is meaningful.
189-191: Potential issue with string variable handling.The test uses
GetStringBytes()and converts to string, which might not handle all edge cases like escaped characters or Unicode properly.Consider using the more direct string access method if available:
-newTagBytes := newTagVar.GetStringBytes() -assert.Equal(t, "Updated by test", string(newTagBytes), "newTag should be 'Updated by test'") +assert.Equal(t, "Updated by test", newTagVar.String(), "newTag should be 'Updated by test'")This approach is more direct and handles string conversion internally.
21-21: Consider extracting common test setup.The channel creation and configuration setup is duplicated across all three test cases. This could be extracted into a helper function to reduce duplication.
Consider extracting the common setup:
+func setupTestModule(t *testing.T) (chan verifyModule.CapturedOperationValues, config.Config) { + resultsChan := make(chan verifyModule.CapturedOperationValues, 1) + cfg := config.Config{ + Graph: config.Graph{}, + Modules: map[string]any{ + "verifyOperationContextValues": verifyModule.VerifyOperationContextValuesModule{ + ResultsChan: resultsChan, + }, + }, + } + return resultsChan, cfg +}Then use it in each test:
-resultsChan := make(chan verifyModule.CapturedOperationValues, 1) -cfg := config.Config{...} +resultsChan, cfg := setupTestModule(t)Also applies to: 94-94, 139-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/modules/verify-operation-context-values/module.go(1 hunks)router-tests/modules/verify_operation_context_values_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
router-tests/modules/verify-operation-context-values/module.go (4)
router/core/modules.go (4)
ModuleContext(160-164)Module(52-54)ModuleInfo(44-50)RouterMiddlewareHandler(102-105)router/core/context.go (1)
RequestContext(60-136)router/core/batch.go (1)
Handler(46-58)router/internal/expr/expr.go (1)
Request(65-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
🔇 Additional comments (7)
router-tests/modules/verify-operation-context-values/module.go (4)
13-23: Well-structured data capture struct.The
CapturedOperationValuesstruct effectively captures all necessary operation context values. The inclusion of bothVariablesas*astjson.ValueandVariablesJSONas string provides flexibility for different testing scenarios.
31-37: Proper initialization with defensive programming.The
Provisionmethod correctly initializes the logger and defensively creates the channel only if it's nil. This prevents overwriting an existing channel that might be configured externally.
52-58: Handle potential nil variables gracefully.The nil check for
captured.Variablesand fallback to empty JSON object"{}"is appropriate for handling cases where no variables are provided in the GraphQL request.
60-64: Non-blocking channel send prevents deadlocks.The use of
selectwith adefaultcase prevents the middleware from blocking when the channel is full. This is crucial for production scenarios where the test consumer might not be draining the channel.router-tests/modules/verify_operation_context_values_test.go (3)
17-89: Comprehensive test coverage for operation context values.The first test effectively verifies all major operation context values including the new
Variables()method. The assertions cover operation name, type, hash, content, and variables in both parsed and JSON string formats.
91-134: Good coverage for edge case with empty variables.The test properly verifies that the
Variables()method returns a non-nil value even when no variables are provided, and correctly handles the empty variables scenario.
136-194: Thorough testing of mutation operations.The mutation test effectively verifies that the operation context correctly identifies mutation operations and provides access to mutation variables with proper type handling (both integer and string variables).
Co-authored-by: Ludwig Bedacht <ludwig.bedacht@gmail.com>
alepane21
deleted the
ale/eng-7598-add-variables-to-requestcontextoperation
branch
3 participants
As specified in this RFC https://github.com/wundergraph/cosmo/blob/35e80778b0d0f0c51ffcbb4676d17bf711a8aafa/rfc/cosmo-streams-v1.md, we need to provide access to the variables for custom modules.
The operationContext struct already exposes the variables, what is missing is simply adding the corresponding method to the OperationContext interface.
Checklist
Summary by CodeRabbit
New Features
Tests