feat(router): provide field arguments in operation context #2385
Conversation
WalkthroughAdds runtime collection and exposure of GraphQL field arguments via a new Arguments type and OperationContext.Arguments(), an AST visitor to map field-path → argument values, wires mapping into prehandler/websocket/graph build flows, and adds tests validating hook access (one duplicated subtest present). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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 1.49% 61.66% +60.16%
==========================================
Files 292 231 -61
Lines 46926 23921 -23005
Branches 431 0 -431
==========================================
+ Hits 703 14750 +14047
+ Misses 45940 7926 -38014
- Partials 283 1245 +962 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
router/core/operation_planner.go
Outdated
| func mapFieldArguments(doc *ast.Document, vars *astjson.Value, remapVariables map[string]string) map[string]map[string]any { | ||
| // TODO: Currently we only map root field arguments. I.e. | ||
| // map["rootfield1"]["arg1"] | ||
| // map["rootfield2"]["arg1"] | ||
| // Needs to extended to support subfields, like | ||
| // map["rootfield1.subfield1"]["arg1"] | ||
| selectionSet := doc.OperationDefinitions[0].SelectionSet | ||
| fields := doc.SelectionSetFieldSelections(selectionSet) | ||
|
|
||
| args := make(map[string]map[string]any, len(fields)) | ||
|
|
||
| for _, field := range fields { | ||
| fieldRef := doc.Selections[field].Ref | ||
| fieldName := doc.FieldNameString(fieldRef) | ||
| fieldArgs := doc.FieldArguments(fieldRef) | ||
|
|
||
| for _, fieldArg := range fieldArgs { | ||
| m := make(map[string]any, len(fieldArgs)) | ||
| args[fieldName] = m | ||
|
|
||
| argName := doc.ArgumentNameString(fieldArg) | ||
| val := doc.Arguments[fieldArg].Value | ||
| args[fieldName][argName] = getArgValue(doc, val, vars, remapVariables) | ||
| } | ||
| } | ||
|
|
||
| return args | ||
| } |
There was a problem hiding this comment.
The idea is not bad. I would be careful when adding this to the hot path. There are potentially big queries that will add some latency here.
Maybe this could be a config value or can we cache that?
Also I would rather recommend that you use our astvisitor.Walker and register an EnterField visitor. With the walker you will always get the path walker.Path.
There was a problem hiding this comment.
Yeah I will see if I can come with some sort of an opt-in solution: Only if you register your custom module and need access to args then this will be executed. Also will switch to using a visitor 👍
There was a problem hiding this comment.
Fixed. Now we're using a walker/visitor and it's only triggered when Cosmo Streams hooks are actually used by the user.
router/core/operation_planner.go
Outdated
|
|
||
| argName := doc.ArgumentNameString(fieldArg) | ||
| val := doc.Arguments[fieldArg].Value | ||
| args[fieldName][argName] = getArgValue(doc, val, vars, remapVariables) |
There was a problem hiding this comment.
map[string]map[string]any might be a bit hard to use. Maybe we could have another data structure and some receiver functions to improve the DX?
You could also have your own implementation for seq.Iter
There was a problem hiding this comment.
Yeah, the map is not the best idea. Also it allows a hook developer to write to it, which is intented and could even cause problems. Will change this to something else.
There was a problem hiding this comment.
The map is now hidden behind a new type Arguments. A user needs to use the Get method to access the values.
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."
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/core/field_argument_visitor.go (1)
90-90: Typo: "automically" should be "automatically".Apply this diff:
- // The operation parser automically creates variables and maps them to arguments, + // The operation parser automatically creates variables and maps them to arguments,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router/core/arguments.go(1 hunks)router/core/context.go(3 hunks)router/core/field_argument_visitor.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
router/core/context.go (1)
router/core/arguments.go (1)
Arguments(6-12)
router/core/field_argument_visitor.go (1)
router/core/arguments.go (1)
Arguments(6-12)
⏰ 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). (12)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/core/field_argument_visitor.go (1)
114-128: Clarify behavior for “missing variable” vs “explicit null”.Right now
return v.variables.Get(originalVarName), nil(Line 127) likely makes “variable not provided” indistinguishable from “variable is JSON null” (depending onastjson.Value.Getbehavior). Either:
- use a presence check API (if available) to return a distinct error on missing, or
- document that
nilmeans “missing OR null”.In github.com/wundergraph/astjson, what are the semantics of (*astjson.Value).Get(key string)? Specifically: does it return nil when the key is missing, when the value is JSON null, or both? Is there an API to distinguish presence (e.g., Has/Exists/GetOk)?
🧹 Nitpick comments (1)
router/core/field_argument_visitor.go (1)
30-59: Avoid leaving emptyfieldArguments[fieldPath]entries when resolution fails.You create
v.fieldArguments[fieldPath](Line 43-45) beforeresolveArgValue; on error you return, leaving an empty map for that fieldPath which can be confusing during debugging.func (v *fieldArgumentsVisitor) EnterArgument(ref int) { @@ fieldPath := v.dotPathFromAncestors() - - if v.fieldArguments[fieldPath] == nil { - v.fieldArguments[fieldPath] = make(map[string]*astjson.Value) - } @@ resolvedArgVal, err := v.resolveArgValue(argVal) if err != nil { @@ return } + if v.fieldArguments[fieldPath] == nil { + v.fieldArguments[fieldPath] = make(map[string]*astjson.Value) + } v.fieldArguments[fieldPath][argName] = resolvedArgVal }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/core/field_argument_visitor.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/field_argument_visitor.go (1)
router/core/arguments.go (1)
Arguments(6-12)
⏰ 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 (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
router/core/field_argument_visitor.go (2)
61-84: Field path construction looks good (alias-aware).Using response key (alias else name) in
dotPathFromAncestors()/fieldResponseKey()should prevent collisions for aliased selections and is the expected behavior for “field path” lookups.
138-170: Good: safe logger default + surfacing walker/report failures.Defaulting to
zap.NewNop()and loggingreport.HasErrors()avoids nil-logger panics and makes “empty args” diagnosable without changing the return signature.
…uments-in-cosmo-streams-hooks
@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 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:
But this assumes the user is in charge of its clients. This is not always the case, especially on public api's.
This leaves them only with the option to use the remapped variables, with all it's 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
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.To implement this feature I added a new field to
core.operationContextto hold the map. The map is created during operation parsing, so it's ready to use when hooks call it. The implementation uses a new walker to walk the AST and find all field arguments. Since this adds complexity to the hot path, when operations are normalized, this is only done when users actually register Cosmo Streams custom modules.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.