feat(router): provide field arguments in operation context#2385
feat(router): provide field arguments in operation context#2385
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Arguments API and wires field-argument mapping through normalization, operation context, HTTP prehandler and WebSocket flows so hooks (start/publish/receive) can access GraphQL field arguments during operation execution. Includes unit and integration tests and a dependency pseudo-version update. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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. 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. Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
|
Important questions in my head:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2385 +/- ##
==========================================
+ Coverage 61.69% 62.68% +0.98%
==========================================
Files 239 240 +1
Lines 25423 25464 +41
==========================================
+ Hits 15685 15962 +277
+ Misses 8418 8174 -244
- Partials 1320 1328 +8
🚀 New features to boost your workflow:
|
dkorittki
changed the title
…uments-in-cosmo-streams-hooks
|
I did a bunch of changes. We are now using a visitor to register all field arguments, including those of nested fields. Its only triggered when a user registers Cosmo Streams hooks. The User does not access the map directly but instead interfaces with a new type |
dkorittki
requested review from
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/graphql_prehandler.go (1)
910-931: Fix potential nil-deref:mapFieldArgumentsruns before variables parse error check.
requestContext.operation.variables, err = variablesParser.ParseBytes(...)(Line 912) can return a non-nilerrand a nil variables value; the code then callsmapFieldArguments(...)(Lines 914-923) which will dereferencevarsvia.Get(...).Suggested fix:
requestContext.operation.rawContent = operationKit.parsedOperation.Request.Query requestContext.operation.content = operationKit.parsedOperation.NormalizedRepresentation requestContext.operation.variables, err = variablesParser.ParseBytes(operationKit.parsedOperation.Request.Variables) - - if h.mapFieldArguments { - requestContext.operation.fieldArguments = mapFieldArguments(mapFieldArgumentsOpts{ - operation: operationKit.kit.doc, - definition: h.executor.ClientSchema, - vars: requestContext.operation.variables, - remapVariables: requestContext.operation.remapVariables, - logger: h.log, - }) - } - if err != nil { rtrace.AttachErrToSpan(engineNormalizeSpan, err) if !requestContext.operation.traceOptions.ExcludeNormalizeStats { httpOperation.traceTimings.EndNormalize() } engineNormalizeSpan.End() return err } + +if h.mapFieldArguments { + requestContext.operation.fieldArguments = mapFieldArguments(mapFieldArgumentsOpts{ + operation: operationKit.kit.doc, + definition: h.executor.ClientSchema, + vars: requestContext.operation.variables, + remapVariables: requestContext.operation.remapVariables, + logger: h.log, + }) +}
🧹 Nitpick comments (3)
router-tests/modules/stream_receive_test.go (1)
1016-1019: Remove duplicate type definition.The
kafkaSubscriptionArgstype is already defined at the file scope (lines 32-35). This inner redefinition is unnecessary and creates inconsistency with other tests in this file that use the file-scoped type.- type kafkaSubscriptionArgs struct { - dataValue []byte - errValue error - } - subscriptionArgsCh := make(chan kafkaSubscriptionArgs) + subscriptionArgsCh := make(chan kafkaSubscriptionArgs)router/core/field_argument_visitor.go (2)
53-64: Consider alias-aware (or otherwise disambiguated) field paths to prevent overwrites.As-is, repeated fields like
user/user(commonly disambiguated via aliases) will collide becausedotPathFromAncestors()usesFieldNameString(...)only.If the intent is “what the user typed”, consider preferring alias when present (pseudo-API names; adjust to actual graphql-go-tools helpers):
- fieldName := v.operation.FieldNameString(anc.Ref) + fieldName := v.operation.FieldNameString(anc.Ref) + if v.operation.FieldAliasIsDefined(anc.Ref) { + fieldName = v.operation.FieldAliasString(anc.Ref) + }If you intentionally want schema field names, then at least document collision behavior in
Arguments.Get(...)docs/tests.
106-135: Use (or remove)mapFieldArgumentsOpts.loggerand checkoperationreport.Reporterrors.Right now
opts.loggeris unused andreportis ignored, so traversal errors can be silently dropped.Suggested patch:
func mapFieldArguments(opts mapFieldArgumentsOpts) Arguments { @@ visitor := &fieldArgumentsVisitor{ walker: &walker, variables: opts.vars, remapVariables: opts.remapVariables, fieldArguments: make(map[string]map[string]*astjson.Value), + logger: opts.logger, } @@ report := &operationreport.Report{} walker.Walk(opts.operation, opts.definition, report) + if report.HasErrors() { + if opts.logger != nil { + opts.logger.Debug("field-argument mapping walk reported errors", zap.String("report", report.Error())) + } + // Option A: return partial results (current behavior) + // Option B: return empty to avoid confusing partial state + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
router-tests/modules/start_subscription_test.go(1 hunks)router-tests/modules/stream_publish_test.go(1 hunks)router-tests/modules/stream_receive_test.go(1 hunks)router/core/arguments.go(1 hunks)router/core/context.go(3 hunks)router/core/field_argument_visitor.go(1 hunks)router/core/field_argument_visitor_test.go(1 hunks)router/core/graph_server.go(2 hunks)router/core/graphql_prehandler.go(4 hunks)router/core/router_config.go(1 hunks)router/core/websocket.go(8 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-13T10:10:47.680Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2329
File: router/pkg/pubsub/datasource/subscription_event_updater.go:86-88
Timestamp: 2025-11-13T10:10:47.680Z
Learning: In router/pkg/pubsub/datasource/subscription_event_updater.go, the SetHooks method is intentionally designed to only replace hook handlers, not reconfigure timeout or semaphore settings. The timeout and semaphore fields are meant to be set once during construction via NewSubscriptionEventUpdater and remain immutable. If different timeout or concurrency settings are needed, a new updater instance should be created rather than modifying the existing one.
Applied to files:
router/core/router_config.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/core/graph_server.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/core/graph_server.gorouter/core/graphql_prehandler.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/core/graphql_prehandler.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/core/graphql_prehandler.go
🧬 Code graph analysis (4)
router-tests/modules/stream_receive_test.go (2)
router/core/subscriptions_modules.go (1)
StreamReceiveEventHandlerContext(269-292)router/core/arguments.go (1)
Arguments(6-11)
router-tests/modules/start_subscription_test.go (4)
router-tests/modules/start-subscription/module.go (1)
StartSubscriptionModule(14-19)router/core/subscriptions_modules.go (1)
SubscriptionOnStartHandlerContext(15-53)router/core/arguments.go (1)
Arguments(6-11)router/core/router_config.go (1)
Config(53-148)
router-tests/modules/stream_publish_test.go (3)
router-tests/modules/stream-publish/module.go (1)
PublishModule(14-18)router/core/subscriptions_modules.go (1)
StreamPublishEventHandlerContext(306-327)router/core/arguments.go (1)
Arguments(6-11)
router/core/context.go (1)
router/core/arguments.go (1)
Arguments(6-11)
🔇 Additional comments (13)
router/core/router_config.go (1)
49-51: LGTM!The
needFieldArgumentMapping()method correctly determines if field argument mapping is needed by checking if any subscription hook handlers are registered. The implementation is simple and efficient -len()on nil slices safely returns 0.router-tests/modules/stream_receive_test.go (1)
967-1051: Test correctly validates field argument access in receive hooks.The test properly verifies that GraphQL field arguments can be accessed via
ctx.Operation().Arguments(). The synchronization throughWaitForSubscriptionCount, message production, channel awaiting, andclient.Close()ensures thatcapturedEmployeeIDis written before being read in the assertion.router-tests/modules/stream_publish_test.go (1)
362-408: LGTM!The test correctly validates that publish hooks can access GraphQL field arguments via
ctx.Operation().Arguments(). Since the mutation is synchronous, the callback executes and completes beforeMakeGraphQLRequestOKreturns, ensuring proper sequencing of the write and read ofcapturedEmployeeID.router/core/graph_server.go (2)
1473-1473: LGTM!The
MapFieldArgumentsflag is correctly derived fromneedFieldArgumentMapping()and passed to the PreHandler, enabling field argument mapping for HTTP-based GraphQL requests when subscription hooks are registered.
1499-1499: LGTM!Consistent propagation of
MapFieldArgumentsto WebSocket middleware ensures field arguments are available for WebSocket-based subscriptions as well.router/core/arguments.go (1)
1-43: LGTM!The
Argumentstype provides a clean, nil-safe API for accessing GraphQL field arguments. Key design decisions are sound:
- Nil receiver check prevents panics
- Unexported
datafield ensures immutability from consumers- Map lookups safely return
nilfor missing keys- Documentation clearly explains the dot-notation path convention
router-tests/modules/start_subscription_test.go (1)
715-785: LGTM!The test correctly validates that subscription start hooks can access GraphQL field arguments via
ctx.Operation().Arguments(). The synchronization throughWaitForSubscriptionCountensures the callback (which capturesemployeeID) executes before the assertion runs.router/core/field_argument_visitor_test.go (1)
15-289: LGTM!Excellent test coverage for the
mapFieldArgumentsfunction. The test cases comprehensively cover:
- Root field arguments (with and without variables)
- Nested field arguments with dot-notation paths
- Non-existent fields/arguments returning nil
- Multiple root fields in a single operation
- Array and object argument types
The test setup correctly mirrors the router's normalization pipeline, ensuring the tests validate realistic scenarios.
router/core/context.go (2)
485-488: LGTM!The
Arguments()method addition to theOperationContextinterface follows the existing pattern established byVariables(). The documentation comment appropriately describes the purpose.
564-566: LGTM!The implementation correctly returns a pointer to the embedded
fieldArgumentsfield. This is safe because:
- The
operationContextoutlives any usage of the returned*Arguments- The zero value of
Arguments(nildatamap) is handled gracefully byArguments.Get()which returnsnilrouter/core/graphql_prehandler.go (1)
42-75: Good feature-flag plumbing for field-arguments mapping.Also applies to: 77-111, 133-171
router/core/websocket.go (2)
45-91: WiringMapFieldArgumentsthrough the WS stack looks consistent.Also applies to: 241-273, 696-721, 722-762, 775-802
824-978: Schema source is consistent; both HTTP and WebSocket paths use the same executor instance.Both
PreHandler.executorandOperationProcessor.executorare initialized from the sameexecutorvariable ingraph_server.go(lines 1275 and 1446). Therefore,h.executor.ClientSchema(HTTP, line 917) andh.operationProcessor.executor.ClientSchema(WebSocket, line 943) reference identical schema sources—no divergence possible.
This test verifies that values of arguments can be extracted directly from the AST when no variable remapping has taken place. Usually this is not the case in reality but if for whatever reason it will be reality one day, this method is proven to work with that.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
router/core/field_argument_visitor.go (2)
29-34: Fix possible panic:Ancestors[len-1]without length check.
v.walker.Ancestors[len(v.walker.Ancestors)-1]will panic whenAncestorsis empty (even if “unlikely”, visitors can be invoked in unexpected contexts).func (v *fieldArgumentsVisitor) EnterArgument(ref int) { // skip if we don't deal with field arguments (e.g. directive arguments) + if len(v.walker.Ancestors) == 0 { + return + } anc := v.walker.Ancestors[len(v.walker.Ancestors)-1] if anc.Kind != ast.NodeKindField { return }
97-107: Handle nilv.variablesexplicitly (and decide missing-vs-null semantics).
v.variables.Get(...)will panic whenopts.varsis nil. Even if “router always sets{}”, this is an easy footgun in other call paths.func (v *fieldArgumentsVisitor) getArgValueFromVars(val ast.Value) (*astjson.Value, error) { + if v.variables == nil { + return nil, fmt.Errorf("no variables supplied") + } varName := v.operation.VariableValueNameString(val.Ref) originalVarName := varName if v.remapVariables != nil { if original, ok := v.remapVariables[varName]; ok { originalVarName = original } } return v.variables.Get(originalVarName), nil }Also worth clarifying behavior: today
Get(...) == nilconflates “variable missing” and “variable is null” (and callers can’t distinguish).
🧹 Nitpick comments (1)
router/core/field_argument_visitor_test.go (1)
350-355: Test brittleness: avoid asserting “exactly 1 warning log”.
require.Len(t, logs, 1)will fail if future code logs an additional warn (even if behavior is correct). Prefer asserting “at least one warn” and matching on the message/fields you care about.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/core/field_argument_visitor.go(1 hunks)router/core/field_argument_visitor_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/field_argument_visitor_test.go (1)
router/core/arguments.go (1)
Arguments(6-11)
⏰ 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). (14)
- GitHub Check: lint
- GitHub Check: lint
- GitHub Check: lint
- GitHub Check: build-router
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
router/core/field_argument_visitor.go (2)
61-72: Alias collision issue from previous review remains unresolved.As noted in the earlier review,
dotPathFromAncestorsuses only the field name (viaFieldNameString), so selections with aliases likea: user(id: 1)andb: user(id: 2)both map to"user"and overwrite each other. This causes last-write-wins behavior and prevents hooks from accessing arguments for aliased fields independently.Consider incorporating the response key (alias if present, otherwise field name) into the path, or append the ancestor ref to disambiguate:
func (v *fieldArgumentsVisitor) dotPathFromAncestors() string { var pathParts []string for _, anc := range v.walker.Ancestors { if anc.Kind == ast.NodeKindField { // Use alias if present, otherwise field name alias := v.operation.FieldAliasString(anc.Ref) if alias == "" { alias = v.operation.FieldNameString(anc.Ref) } pathParts = append(pathParts, alias) } } return strings.Join(pathParts, ".") }If this behavior is intentional, document it clearly.
145-146: Minor: Walker errors are silently ignored.As flagged in the previous review,
walker.Walkcan accumulate errors inreport, but they're never checked or logged. This can make debugging difficult when field arguments are unexpectedly empty or incomplete.Add error logging without changing the return signature:
report := &operationreport.Report{} walker.Walk(opts.operation, opts.definition, report) + + if report.HasErrors() { + logger.Debug("walker encountered errors while mapping field arguments", + zap.String("errors", report.String()), + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/core/field_argument_visitor.go(1 hunks)router/core/graphql_prehandler.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/core/graphql_prehandler.go
🧬 Code graph analysis (1)
router/core/field_argument_visitor.go (1)
router/core/arguments.go (1)
Arguments(6-11)
⏰ 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). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
router/core/graphql_prehandler.go (2)
70-70: LGTM: Feature flag wiring is clean.The
MapFieldArgumentsoption is correctly threaded throughPreHandlerOptions, stored inPreHandler, and initialized inNewPreHandler. The naming and pattern are consistent with other feature flags in this struct.Also applies to: 106-106, 165-165
923-931: LGTM: Field argument mapping correctly placed in normalization flow.The conditional call to
mapFieldArgumentsis appropriately positioned after variable parsing and remapping succeed, ensuring the visitor has access to fully normalized variables. The parameters passed (operation document, schema, parsed variables, remap map, logger) align with what the visitor needs.router/core/field_argument_visitor.go (2)
30-59: LGTM: Previous safety concerns addressed.The empty
Ancestorscheck (lines 31-33) prevents the panic that was flagged in earlier review, and error logging (lines 52-54) now provides visibility into resolution failures with field path and argument name context.
102-116: LGTM: Nil variables check added.The nil-variables guard (lines 103-105) addresses the previous review concern. Note that
Getreturnsnilfor both missing and explicitly-null variables—this is acceptable for the hook use case, though it means hooks cannot distinguish between "argument not provided" and "argument is null."
…uments-in-cosmo-streams-hooks
| WebSocketConfiguration: s.webSocketConfiguration, | ||
| ClientHeader: s.clientHeader, | ||
| DisableVariablesRemapping: s.engineExecutionConfiguration.DisableVariablesRemapping, | ||
| MapFieldArguments: s.subscriptionHooks.needFieldArgumentMapping(), |
There was a problem hiding this comment.
I think it would be good to also have a disable flag to prevent the visitor from running in general.
There was a problem hiding this comment.
Yeah it probably makes sense.
Would you make this part of the router configuration or part of Custom Modules code, when you define them?
An alternative would be that I hook myself into a visitor, which already visits arguments. I could save a seperate walk here. I believe there was one visitor, which already does that but need to check if it is viable. What do you think of that?
There was a problem hiding this comment.
As now we don't use a dedicated walker anymore (runs inside the engines variable normalizer walker) I think we can omit a new config flag. If someone wants to disable it actively we can always add it afterwards.
|
I set this pull request back to draft. Currently I'm working on another approach which avoids a seperate walker entirely. |
Only temporary until the engine changes have been merged. Then the go.mod files here will be updated again.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@router/core/operation_processor.go`:
- Line 1443: The call to astnormalization.NewVariablesNormalizer currently
passes options.enableFieldArgumentMapping but the constructor takes no
arguments; update the variablesNormalizer initialization to call
astnormalization.NewVariablesNormalizer() with no parameters (remove
options.enableFieldArgumentMapping) so the variablesNormalizer variable is
created by NewVariablesNormalizer() as intended.
🧹 Nitpick comments (4)
router/core/graphql_prehandler.go (1)
824-825: Misplaced comment — the "storing" happens 120 lines later at lines 944–949.The comment on line 824 says "Store the field argument mapping for later use when creating Arguments" but the actual assignment to
requestContext.operation.fieldArgumentshappens much later (lines 944–949). This comment would be less confusing if moved to where the storing actually occurs, or removed here entirely.Suggested fix
- // Store the field argument mapping for later use when creating Arguments engineNormalizeSpan.SetAttributes(otel.WgVariablesNormalizationCacheHit.Bool(cached))And at line 943:
+ // Build field arguments from the mapping produced during variable normalization if h.mapFieldArguments {router-tests/modules/stream_receive_test.go (1)
1016-1019: Remove duplicatekafkaSubscriptionArgstype declaration.This type is already declared at lines 32-35 in the outer
TestReceiveHookscope with the same fields. The inner redeclaration shadows it unnecessarily.♻️ Suggested fix
- type kafkaSubscriptionArgs struct { - dataValue []byte - errValue error - } - subscriptionArgsCh := make(chan kafkaSubscriptionArgs) + subscriptionArgsCh := make(chan kafkaSubscriptionArgs)router/core/arguments_test.go (1)
330-340: Consider adding a nil-receiver test forArguments.Get.Per the
Arguments.Getimplementation, it handles a nil receiver. A test like(*Arguments)(nil).Get("path")would verify that safety net, complementing the nil/empty mapping tests here.🧪 Suggested additional test
func TestArguments_Get_NilReceiver(t *testing.T) { var args *Arguments assert.Nil(t, args.Get("query.user.id")) }router/core/graph_server.go (1)
1313-1313: StoreneedFieldArgumentMapping()result in a local variable to avoid redundant computation.
s.subscriptionHooks.needFieldArgumentMapping()is called three times inbuildGraphMuxwithin different struct initializations. Since it's a pure function with no side effects, storing the result once improves readability.♻️ Suggested refactor
Add near the top of the struct initializations:
+ needFieldArgMapping := s.subscriptionHooks.needFieldArgumentMapping()Then replace the three call sites:
- EnableFieldArgumentMapping: s.subscriptionHooks.needFieldArgumentMapping(), + EnableFieldArgumentMapping: needFieldArgMapping,- MapFieldArguments: s.subscriptionHooks.needFieldArgumentMapping(), + MapFieldArguments: needFieldArgMapping,- MapFieldArguments: s.subscriptionHooks.needFieldArgumentMapping(), + MapFieldArguments: needFieldArgMapping,
|
Refactored the pull request description with better structure and more context. |
…uments-in-cosmo-streams-hooks
|
Awaiting a new review here after adressing feedback. Simultaneously awaiting an approval on wundergraph/graphql-go-tools#1373 . I need to merge both in close order, first the engine and then the router. The engine PR is a breaking change as it changes a method signature used in router. This PR adresses this. So once this is approved I keep the PR open and updated until the engine PR has been approved, merged and released. Once that happened I will update go.mod here to use latest engine version. |
|
@CodeRabbit summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/core/arguments_test.go`:
- Around line 520-538: The test sets operation.Input.Variables to
[]byte(tc.variables) which becomes empty bytes when tc.variables == "" and
causes astjson.ParseBytes to fail; change the setup so when tc.variables is
empty you assign operation.Input.Variables = []byte("{}") (i.e. normalize
missing variables to an empty JSON object) before calling
astnormalization.NewNormalizer/astnormalization.NewVariablesNormalizer and
astjson.ParseBytes; update the code around operation.Input.Variables,
VariablesNormalizerOptions/varNorm.NormalizeOperation and the following
astjson.ParseBytes call to use this non-empty "{}" fallback.
ℹ️ Review info
Configuration used: Organization 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 (9)
router-tests/go.modrouter/core/arguments_test.gorouter/core/context.gorouter/core/graph_server.gorouter/core/graphql_prehandler.gorouter/core/operation_processor.gorouter/core/operation_processor_test.gorouter/core/router_config.gorouter/go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- router/go.mod
- router/core/router_config.go
- router-tests/go.mod
- router/core/operation_processor_test.go
| @@ -295,7 +323,7 @@ func (c *CacheWarmupPlanningProcessor) ProcessOperation(ctx context.Context, ope | |||
| return nil, err | |||
| } | |||
|
|
|||
| _, _, err = k.NormalizeVariables() | |||
| _, _, _, err = k.NormalizeVariables() | |||
3 participants
@CodeRabbit summary
Checklist
--> feat: add field argument usage for Cosmo Streams cosmo-docs#210
Motivation and Context
This pull request provides users of Cosmo Streams hooks the ability to access field arguments by their original name. Usually these get lost during the normalization of the operation, where for each argument the operation kit creates a variable:
In Cosmo Streams hooks, for example the
OnReceiveEventshook, you would need to access this argument by it's mapped variable "a":This is a problem because
One solution is to use variables in the operation:
This way one could access the values of an argument by accessing the variable
id:But this assumes the user is in charge of its clients. This is not always the case, especially on public api's. Clients could choose all kinds of variable names a hook developer can't be aware of.
This leaves them only with the option to use the remapped variables, with all it's mentioned downsides.
Solution
Add
Arguments()method toctx.Operation()on custom modules interfaces of Cosmo Streams hooks.ctx.Operation()provides data of the current operation to hook developers and since field arguments are part of that operation, I think it fits well there.Given an operation
The hook developer can now access the field directly by its path and name
Arguments of nested fields are accessable by using a dot notated path of the field
ctx.Operation().Arguments().Getreturns*astjson.Valueobjects. This way we can expose all kinds of different argument types, including GraphQL arrays and objects. It also makes usingctx.Operation().Arguments().Get()feel similar toctx.Operation().Variables(), which also returns*astjson.Valueobjects. Have a look at the unit tests to see how it looks.Implementation
Now we know how a hook developer can access field arguments but we how do we actually collect and provide them?
To implement this feature I added a new field to
core.operationContextto hold the data as a map. The map is created during variable normalization, so it's ready to use when hooks call it. This implementation involves a change on the engine, where we extend the variable extraction visitor of the variable normalizer. Here is the pull request. That must be merged before we merge this one.So the actual creation of the field argument map is done by the engine. At first the engine wasn't involved in this feature and the router did it itself with a new walker. After discussing this I changed this to the engine for performance reasons (see comments). The router does not have an existing walker which walks field arguments at the right place, so the data is ready when hooks are called. But the engine does have such a visitor: The variable normalizer. Doing it in the engine prevents us from using a dedicated visitor just for that.
After the engine change the variable normalizers method signature changes and returns the field arg mapping. We store this map on
operationContextand that's what thectx.Operation.Arguments()(used in hooks) acesses.Caching
The results of the variable normalizer are cached for every operation. This is functionality that already exists. I added the field arg map to that cache as well. It's now part of the variable normalizer result and is cached with it.
The cache key generation did not change. The only two factors for a variable normalizer cache key are the operations variables and the normalized string representation. This is sufficient because if these factors do not change, the field argument mapping didn't as well.
Other Optimisations
The field arg mapping on the engine will only run if actual Cosmo Streams hooks are registered on the router. This is done via function
needFieldArgumentMapping(). They are the only potential places of accessing field arguments this way, so it wouldn't make sense to process them otherwise.Summary by CodeRabbit
New Features
Tests