fix: operation checks for input and argument usages #2380
Conversation
JivusAyrus
requested review from
Noroth,
StarpTech,
devsergiy,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
WalkthroughAdds IsNull and SubgraphIDs propagation through usage extraction, protobufs, metrics, DB migrations and views; makes router usage extraction plan-aware with variable remapping; refines inspector change classification and tests to prefer parent-pathing and surface nullability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
229-235: Incorrect query filter logic forisNull.The
isNullfilter is only applied when bothisInputandisArgumentare falsy (line 233). However,isNullis specifically relevant whenisInputorisArgumentis true. This condition structure will never apply theisNullfilter for input/argument changes.The logic should apply
isNullalongside (not instead of)isInput/isArgument:if (change.isInput) { where.push(`IsInput = true`); - } else if (change.isArgument) { + } + if (change.isArgument) { where.push(`IsArgument = true`); - } else if (change.isNull !== undefined) { + } + if (change.isNull !== undefined) { where.push(`IsNull = ${change.isNull}`); }
🧹 Nitpick comments (5)
graphqlmetrics/migrations/20251120223910_drop_gql_schema_usage_5m_90d_mv_for_is_null.sql (1)
30-48: DuplicateOperationTypein GROUP BY clause.
OperationTypeappears twice in the GROUP BY clause (lines 34 and 38). While this won't cause errors, it's redundant and should be cleaned up.GROUP BY Timestamp, OperationHash, OperationName, OperationType, FederatedGraphID, RouterConfigVersion, OrganizationID, - OperationType, ClientName, ClientVersion,controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
155-163: Fallback case could mask unexpected scenarios.The else branch returns
OPTIONAL_TO_REQUIRED_SAMEas a fallback, but the comment notes this "shouldn't happen in breaking changes." Consider logging a warning or throwing an error for unexpected combinations (e.g., required-to-optional transitions) to aid debugging.} else { // Edge case: same type, from required, to optional (shouldn't happen in breaking changes) - // Fallback to same type becoming required - return FieldTypeChangeCategory.OPTIONAL_TO_REQUIRED_SAME; + // Log or throw to identify unexpected patterns + throw new Error( + `Unexpected type change pattern: ${fromType} -> ${toType} (fromRequired: ${fromRequired}, toRequired: ${toRequired}, sameType: ${sameType})`, + ); }router/pkg/graphqlschemausage/schemausage.go (2)
826-873: Consider: O(n) deduplication scan on each append.
appendUniqueUsageperforms a linear scan through all existing usage entries. For operations with many input fields, this could become expensive. Consider using a map-based deduplication keyed on a composite ofPath + TypeName + IsNull.However, given typical GraphQL operation sizes, this is likely acceptable.
1012-1035: Minor duplication withtrackImplicitNullArguments.The logic for finding argument definitions on ObjectTypeDefinition and InterfaceTypeDefinition is duplicated between
trackImplicitNullArguments(lines 559-582) andtrackImplicitInputTypeArguments(lines 1012-1035). Consider extracting a shared helper.However, the duplication is minor and the code is clear, so this is optional.
router/pkg/graphqlschemausage/schemausage_test.go (1)
2816-2823: Minor: Empty goroutine in test helper.
source.Start()is an empty method, so launching it in a goroutine does nothing. This appears to be test infrastructure that may have been copied from elsewhere. Consider removing the goroutine or adding a comment explaining its purpose if it's intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
graphqlmetrics/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (16)
controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts(8 hunks)controlplane/src/core/services/SchemaUsageTrafficInspector.ts(8 hunks)graphqlmetrics/core/metrics_service.go(5 hunks)graphqlmetrics/migrations/20251120223520_add_is_null_to_schema_usage.sql(1 hunks)graphqlmetrics/migrations/20251120223529_add_is_null_to_schema_usage_5m_90d.sql(1 hunks)graphqlmetrics/migrations/20251120223537_add_is_null_to_schema_usage_lite_1d_90d.sql(1 hunks)graphqlmetrics/migrations/20251120223910_drop_gql_schema_usage_5m_90d_mv_for_is_null.sql(1 hunks)graphqlmetrics/migrations/20251120223951_recreate_gql_schema_usage_5m_90d_mv_with_is_null.sql(1 hunks)graphqlmetrics/migrations/20251120224005_drop_gql_schema_usage_1d_90d_mv_for_is_null.sql(1 hunks)graphqlmetrics/migrations/20251120224017_recreate_gql_schema_usage_1d_90d_mv_with_is_null.sql(1 hunks)proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.proto(2 hunks)router/core/operation_planner.go(3 hunks)router/demo.config.yaml(2 hunks)router/pkg/graphqlschemausage/schemausage.go(8 hunks)router/pkg/graphqlschemausage/schemausage_bench_test.go(1 hunks)router/pkg/graphqlschemausage/schemausage_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/pkg/graphqlschemausage/schemausage.gorouter/pkg/graphqlschemausage/schemausage_test.go
🧬 Code graph analysis (5)
router/pkg/graphqlschemausage/schemausage_bench_test.go (2)
router/pkg/graphqlschemausage/schemausage_test.go (1)
FakeFactory(2804-2806)router/pkg/graphqlschemausage/schemausage.go (3)
GetTypeFieldUsageInfo(66-75)GetArgumentUsageInfo(79-106)GetInputUsageInfo(111-125)
controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts (2)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
InspectorSchemaChange(166-175)controlplane/src/core/composition/composition.ts (1)
buildSchema(67-69)
router/pkg/graphqlschemausage/schemausage.go (1)
router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (9)
ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)TypeFieldUsageInfo(476-493)TypeFieldUsageInfo(508-508)TypeFieldUsageInfo(523-525)
router/core/operation_planner.go (1)
router/pkg/graphqlschemausage/schemausage.go (2)
GetArgumentUsageInfo(79-106)GetInputUsageInfo(111-125)
router/pkg/graphqlschemausage/schemausage_test.go (2)
router/pkg/graphqlschemausage/schemausage.go (3)
GetArgumentUsageInfo(79-106)GetInputUsageInfo(111-125)TypeFieldUsageInfo(144-152)router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (9)
InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)TypeFieldUsageInfo(476-493)TypeFieldUsageInfo(508-508)TypeFieldUsageInfo(523-525)ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)
🔇 Additional comments (50)
router/pkg/graphqlschemausage/schemausage_bench_test.go (8)
20-105: LGTM! Comprehensive benchmark setup.The setup function follows benchmark best practices with proper use of
b.Helper(), realistic scenario construction, and appropriate error handling.
108-118: LGTM! Proper benchmark structure.The benchmark correctly measures memory allocations with proper timer reset and compiler optimization prevention.
121-134: LGTM! Proper error handling and benchmark structure.The benchmark correctly handles errors and follows Go benchmark conventions.
137-164: LGTM! Proper benchmark implementations.Both benchmarks correctly isolate the measured operations from setup overhead, with
BenchmarkIntoGraphQLMetricsappropriately pre-computingtypeFieldMetricsbefore the timer reset.
168-195: LGTM! Comprehensive end-to-end benchmark.The benchmark effectively simulates a complete schema usage extraction lifecycle, providing valuable performance metrics for the full flow.
199-291: LGTM! Dynamic schema generation for scalability testing.The function correctly generates schemas with varying field counts to test performance characteristics at scale. The string concatenation in setup is acceptable since it's outside the benchmark timer.
295-337: LGTM! Excellent scalability testing.The benchmark suite effectively tests performance across different field counts, helping identify potential algorithmic bottlenecks. The use of sub-benchmarks allows for easy comparison across scenarios.
44-44: No issues found. TheschemaUsageInfoTestSchemaconstant is properly defined inschemausage_test.go(line 27) and is correctly accessible fromschemausage_bench_test.gosince both files are in the same package.router/demo.config.yaml (2)
8-10: LGTM! GraphQL metrics configuration added.The addition of the
graphql_metricsblock with a localhost collector endpoint is appropriate for a demo configuration.
26-26: LGTM! Indentation corrected.The indentation fix properly aligns the redis URLs list item.
router/core/operation_planner.go (3)
117-120: Verify variable type compatibility.Similar to the concern above, ensure that
opContext.variablesmatches the expected*astjson.Valuetype forGetInputUsageInfo.
155-158: Verify variable type compatibility.Ensure that
opContext.variablesis compatible with the*astjson.Valuetype expected byGetInputUsageInfoin the cache-hit path as well.
86-89: Type compatibility is correct; no issue exists.The review comment's concern is unfounded.
ctx.variablesis declared as*astjson.Value(line 530 inrouter/core/context.go), which matches exactly the parameter type expected byGetArgumentUsageInfo(...), which declaresvariables *astjson.Valueas its third parameter.graphqlmetrics/migrations/20251120223520_add_is_null_to_schema_usage.sql (1)
1-7: LGTM! IsNull column addition is well-structured.The migration properly uses guard clauses (
IF NOT EXISTS/IF EXISTS) for idempotency, applies appropriate compression (ZSTD(3)), and provides a sensible default value (false). The up/down pair is correctly structured.graphqlmetrics/migrations/20251120223951_recreate_gql_schema_usage_5m_90d_mv_with_is_null.sql (1)
3-48: Verify the materialized view includes all necessary fields.The materialized view successfully incorporates IsNull tracking. Please verify that all required fields from the base table are included and that the aggregation logic aligns with the intended metrics collection requirements.
graphqlmetrics/migrations/20251120224017_recreate_gql_schema_usage_1d_90d_mv_with_is_null.sql (1)
1-27: LGTM! Lite materialized view correctly includes IsNull.The materialized view properly incorporates IsNull tracking for daily aggregation. The projection is clean, uses appropriate optimizations (
toLowCardinality), and the up/down migration pair is well-structured.graphqlmetrics/migrations/20251120223537_add_is_null_to_schema_usage_lite_1d_90d.sql (1)
1-7: LGTM! Consistent IsNull column addition.The migration follows the same well-structured pattern as the base table migration, with proper guards, compression, and default value.
graphqlmetrics/migrations/20251120223529_add_is_null_to_schema_usage_5m_90d.sql (1)
1-7: LGTM! Consistent IsNull column addition.The migration follows the established pattern with proper guards, compression, and default value for the 5m aggregation table.
graphqlmetrics/migrations/20251120224005_drop_gql_schema_usage_1d_90d_mv_for_is_null.sql (1)
1-26: LGTM! Migration pair correctly handles view transition.This migration works in tandem with
20251120224017_recreate_gql_schema_usage_1d_90d_mv_with_is_null.sqlto transition the view:
- Up: Drops the old view (this file) → Next migration recreates with IsNull
- Down: Next migration drops IsNull version → This file recreates the old version
The down migration correctly recreates the view without IsNull, matching the previous schema state.
graphqlmetrics/core/metrics_service.go (3)
341-345: LGTM!The boolean flags are correctly set for field metrics:
IsArgument=false,IsInput=false, andIsNull=false(not applicable for fields). The inline comments clearly document the purpose of each flag.
354-382: LGTM!The argument metrics handling correctly:
- Sorts
SubgraphIDsfor cardinality reduction- Uses
argumentUsage.SubgraphIDsinstead of empty slice- Propagates
argumentUsage.IsNullto the batch
391-419: LGTM!The input metrics handling mirrors the argument metrics pattern correctly, with proper sorting and propagation of
inputUsage.SubgraphIDsandinputUsage.IsNull.proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.proto (2)
92-96: LGTM!The new fields for
ArgumentUsageInfoare well-documented and use correct sequential field numbers. The comment explains the critical purpose ofIsNullfor detecting breaking changes when optional arguments become required.
110-114: LGTM!Consistent with
ArgumentUsageInfo, theInputUsageInfomessage now includesSubgraphIDsandIsNullwith appropriate documentation. Field numbers are correctly assigned.controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts (4)
22-30: LGTM!The test correctly validates that adding a required argument is detected as a breaking change with the path pointing to the field containing the argument.
85-108: LGTM!Good test coverage for removing optional arguments. The expectation of
isNull: falseis correct since we need to find operations that actually used this argument (with a non-null value) to determine breaking impact.
110-134: LGTM!Correctly tests the
OPTIONAL_TO_REQUIRED_SAMEcategory whereisNull: trueidentifies operations that didn't provide the argument (which will now break since it's required).
476-493: LGTM!The helper function correctly maps schema changes to inspector changes and filters out null results. The logic is clean and straightforward.
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (3)
6-27: LGTM!The
FieldTypeChangeCategoryenum clearly categorizes the four possible type change scenarios with helpful JSDoc comments and examples.
35-66: LGTM!The type extraction and normalization helpers handle the various GraphQL type formats correctly (arrays, required indicators, nested structures).
589-618: LGTM!The
FieldArgumentAddedandFieldArgumentRemovedhandlers correctly differentiate between required and optional arguments, applying appropriate path and flag combinations for breaking change detection.graphqlmetrics/migrations/20251120223910_drop_gql_schema_usage_5m_90d_mv_for_is_null.sql (1)
3-3: The review comment is incorrect—the code already uses the proper ClickHouse syntax.According to official ClickHouse documentation,
DROP VIEWis the documented and correct approach for dropping materialized views. WhileDROP TABLEalso works for views and materialized views,DROP VIEWis the explicit, canonical syntax shown in the ClickHouse DROP statements documentation. The code at line 3 correctly usesDROP VIEW IF EXISTS gql_metrics_schema_usage_5m_90d_mv;and requires no changes.Likely an incorrect or invalid review comment.
router/pkg/graphqlschemausage/schemausage.go (13)
1-45: Excellent package documentation.The documentation clearly explains the architecture, the challenge being solved, and how the components work together. This is valuable for maintainability and onboarding.
47-59: LGTM.Imports are appropriate and well-organized.
77-106: LGTM.The function correctly creates the infrastructure components, walks the AST, and returns collected usage with proper error handling.
170-207: LGTM.The visitor correctly traverses the execution plan, builds field paths, and handles both direct and indirect interface fields. The lazy initialization and path copying are implemented correctly.
213-240: LGTM.Clean, focused implementation of a reusable path stack. The empty-stack guard in
pop()prevents panics.
275-280: Verify: Missing variable returnsfalsefor null check.When
variables.Get(originalVarName)returnsnil(variable not found in JSON),isVariableNullreturnsfalse. This is correct because a missing variable is handled separately inprocessVariableDefinition(line 894). Just confirming this is intentional design.
340-356: LGTM.The collector correctly traverses the resolve tree and builds the field-to-subgraph mapping using a path stack.
414-441: LGTM.The
mergeSubgraphIDsfunction efficiently combines and deduplicates subgraph IDs with early returns for empty inputs.
460-483: LGTM.Stack management is correct - push enclosing node and nil arguments map on enter, pop both on leave. The lazy allocation pattern for the arguments map is efficient.
533-613: LGTM.The implicit null argument tracking correctly identifies arguments defined in the schema but not provided in the operation. The implementation handles both object and interface type definitions.
619-663: LGTM.Clean type resolution for input objects with proper nil-safety checks.
688-717: LGTM.The traverse method correctly implements null propagation - when
isNullis true, it tracks the usage and stops traversal (doesn't recurse into nested fields). This aligns with the documented behavior.
879-920: LGTM.The variable definition processing correctly handles:
- Variables not provided in JSON (tracks with
IsNull: true)- Variable name remapping for normalized operations
- Both null and non-null variable values
This is critical for breaking change detection.
router/pkg/graphqlschemausage/schemausage_test.go (5)
214-229: LGTM.Test call signatures correctly updated to use the new API with
mergedvariables andnilremap map.
1358-1657: Excellent test coverage for null propagation.This table-driven test comprehensively covers the null propagation behavior described in the package documentation, including:
- Explicit null at different nesting levels
- Implicit null (empty objects) at different nesting levels
- Full value chains with no nulls
The descriptive test case names and comments make it easy to understand each scenario.
1758-1965: Excellent real-world test case.This test validates the critical variable remapping feature by:
- Using the actual
VariablesMapperfrom normalization- Demonstrating correct behavior with remapping
- Showing the failure mode without remapping
This provides confidence that the feature works in production scenarios.
2397-2598: Comprehensive multi-subgraph test.This test validates that when an input object variable is used by multiple fields from different subgraphs:
- Each argument is attributed to its specific subgraph
- Input usage is correctly merged across all subgraphs
- Implicit null fields also get merged subgraph IDs
The assertions using
ElementsMatchfor order-independent comparison is appropriate.
7-7:stringsimport is used in the file.The
stringsimport is necessary—strings.Join()is called at line 2562 to join path elements with a dot separator. The import is correctly added and utilized.
controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts
Outdated
Show resolved
Hide resolved
graphqlmetrics/migrations/20251120223951_recreate_gql_schema_usage_5m_90d_mv_with_is_null.sql
Show resolved
Hide resolved
…493-schema-usage-issues
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2380 +/- ##
=======================================
Coverage ? 31.28%
=======================================
Files ? 212
Lines ? 23111
Branches ? 0
=======================================
Hits ? 7231
Misses ? 14975
Partials ? 905 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
router/pkg/graphqlschemausage/schemausage.go (3)
312-372: Confirm alias vs. field-name consistency for subgraph path mapping
subgraphIDCollectorbuilds keys fromfield.Info.Name, while the AST-side collectors (variableSubgraphCollector, argument visitors, etc.) build keys fromoperation.FieldNameStringviapathBuilder. Iffield.Info.Namecorresponds to the response alias butFieldNameStringreturns the underlying field name (or vice versa), aliased fields will fail to resolve subgraph IDs, and downstream usage metrics will missSubgraphIDsfor those paths.To be safe, consider:
- Verifying with a test query that uses aliases on nested fields and ensuring
GetArgumentUsageInfo/GetInputUsageInfostill produce non-emptySubgraphIDs.- If needed, normalizing both sides to the same notion of “response key” (e.g., always alias/response name) when pushing onto
pathStack/pathBuilder.key()and when populatingfieldMap.Also applies to: 375-429, 430-457
77-106: Argument null/implicit-null tracking is good, but Path loses full field ancestryThe argument visitor wiring (enclosing-type stack, provided-arguments stack, and
trackImplicitNullArguments) correctly distinguishes explicitnullfrom “not provided” and skips introspection, which matches the intended breaking-change detection semantics.However, for each
ArgumentUsageInfoyou currently set:Path: []string{string(fieldName), string(argName)},even though
pathBuildertracks the full field path. The proto comment forArgumentUsageInfo.Pathsays it should be “the path to the field in the request document but without the root type”, so{fieldName, argName}drops all ancestor segments and makes different nesting contexts indistinguishable byPathalone.A small change would align metrics with the documented semantics:
- fieldName := a.operation.FieldNameBytes(anc.Ref) + fieldName := a.operation.FieldNameBytes(anc.Ref) // ... - a.usage = append(a.usage, &graphqlmetrics.ArgumentUsageInfo{ - Path: []string{string(fieldName), string(argName)}, + fullPath := a.pathBuilder.copy() + fullPath = append(fullPath, string(argName)) + + a.usage = append(a.usage, &graphqlmetrics.ArgumentUsageInfo{ + Path: fullPath, TypeName: string(enclosingTypeName), NamedType: string(typeName), SubgraphIDs: subgraphIDs, IsNull: isNull, })This preserves the existing type information while making
Pathfully representative of the location in the operation.Also applies to: 463-475, 476-499, 501-547, 549-629
842-889: Deduplication logic is correct but may become a hotspot on very large operations
appendUniqueUsagedoes an O(n) scan overt.usageusinginfoEqualsfor each append. For typical request sizes this is fine, but for very large operations with many variables/inputs this can become quadratic overall.Not urgent, but if you see this show up in profiles, consider maintaining a small key map (e.g. hash of
NamedType,TypeName,IsNull,Path,EnumValues,SubgraphIDs) to short‑circuit the linear scan.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/pkg/graphqlschemausage/schemausage.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
🧬 Code graph analysis (1)
router/pkg/graphqlschemausage/schemausage.go (2)
router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (6)
ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)shared/src/router-config/builder.ts (1)
Input(39-46)
⏰ 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). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/pkg/graphqlschemausage/schemausage.go (3)
1-44: High-level architecture and public API shape look solidThe top-of-file documentation and the three public entry points (type/field, argument, and input usage) clearly explain the responsibilities and data flows between AST, plan, and variables, which should make this module easier to evolve and test. No issues from a design/API perspective here.
Also applies to: 61-76
225-257: Shared helpers (pathBuilder, nullValueDetector) are cleanly structuredThe path stack abstraction and the centralized null handling (with variable remapping) are straightforward and will help keep the visitors focused on traversal logic. I don’t see functional problems in these helpers.
Also applies to: 262-307
938-992: Implicit input-type argument tracking looks correct and mirrors argument null tracking
collectImplicitArgumentInputUsageandimplicitArgumentInputCollectormirror the argument visitor’s logic: they maintain enclosing type and provided-argument stacks, skip introspection, and only emitInputUsageInfofor arguments whose types are input objects and that are not provided in the operation. This matches the stated goal of catching implicitly-null input-type arguments for breaking-change detection. No issues spotted here.Also applies to: 1008-1092
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
229-235: Potential bug:isNullfilter ignored whenisInputorisArgumentis true.The
else ifchain meansisNullis only added to the query when bothisInputandisArgumentare falsy. However, manyInspectorSchemaChangeobjects set bothisInput: trueandisNull: true/false(e.g., lines 480-481, 491, 511-512). With this logic, theisNullfilter will be ignored in those cases.If
isNullshould be applied alongsideisInputorisArgument, refactor to use independent conditions:if (change.isInput) { where.push(`IsInput = true`); - } else if (change.isArgument) { + } + if (change.isArgument) { where.push(`IsArgument = true`); - } else if (change.isNull !== undefined) { + } + if (change.isNull !== undefined) { where.push(`IsNull = ${change.isNull}`); }
♻️ Duplicate comments (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
515-517: Error message references wrong variable.The error message references
FieldTypeChangeCategory(the enum type) instead ofinputFieldTypeChangeCategory(the variable containing the actual value).default: { - throw new Error(`Unsupported input field type change category: ${FieldTypeChangeCategory}`); + throw new Error(`Unsupported input field type change category: ${inputFieldTypeChangeCategory}`); }
🧹 Nitpick comments (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
159-163: Fallback silently misclassifies edge cases.The fallback returns
OPTIONAL_TO_REQUIRED_SAMEfor any unhandled combination (e.g.,required→optionalof the same type). While the comment notes this "shouldn't happen in breaking changes," silently returning a wrong category could mask bugs or produce incorrect query filters.Consider throwing an error for truly unexpected cases to fail fast:
} else { - // Edge case: same type, from required, to optional (shouldn't happen in breaking changes) - // Fallback to same type becoming required - return FieldTypeChangeCategory.OPTIONAL_TO_REQUIRED_SAME; + // Edge case: same type and no requiredness change, or required→optional (non-breaking) + throw new Error( + `Unexpected type change pattern: '${fromType}' → '${toType}' (same=${sameType}, from=${fromRequired}, to=${toRequired})` + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts(8 hunks)controlplane/src/core/services/SchemaUsageTrafficInspector.ts(8 hunks)graphqlmetrics/migrations/20251120223951_recreate_gql_schema_usage_5m_90d_mv_with_is_null.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- graphqlmetrics/migrations/20251120223951_recreate_gql_schema_usage_5m_90d_mv_with_is_null.sql
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
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.
🧬 Code graph analysis (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts (2)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
InspectorSchemaChange(166-175)controlplane/src/core/composition/composition.ts (1)
buildSchema(67-69)
⏰ 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: build-router
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (7)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (3)
6-27: Well-documented enum with clear categorization.The
FieldTypeChangeCategoryenum with inline examples clearly explains each type-change scenario, making the code self-documenting.
82-95: Parsing logic is correct and well-documented.The regex pattern correctly extracts the argument type from the message format, and the required check is straightforward.
114-158: Type change categorization logic is sound.The function correctly handles both message formats (input fields and arguments) and properly categorizes changes based on requiredness and type differences.
controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts (4)
62-108: Good test coverage for argument removal scenarios.Tests correctly verify both required and optional argument removal behaviors, ensuring
isArgumentandisNullflags are properly set for optional removals.
110-210: Comprehensive tests for argument type change categories.All four
FieldTypeChangeCategoryscenarios are covered with appropriate assertions forpath,fieldName,isArgument, andisNullproperties.
291-387: Good coverage of input field type change scenarios.Tests verify all four type change categories for input fields with correct expectations for
path,typeName,fieldName,isInput, andisNullproperties.
476-493: Test helper function correctly maps changes to inspector format.The
getBreakingChangeshelper properly usestoInspectorChangeand filters null results.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/pkg/graphqlschemausage/schemausage.go (1)
917-963: Nil-variables handling in processVariableDefinition resolves the prior panic riskThe guard:
- Treats
variables == nilthe same as “variable not provided”.- Still emits an
IsNull=trueusage for input-object variables (based on type information), keyed by the normalized variable name for subgraph lookup.This directly addresses the earlier concern about
variables.Get(...)panicking and matches the behavior asserted inTestNilVariablesHandlingandTestInputUsageWithEmptyVariables.
🧹 Nitpick comments (1)
router/pkg/graphqlschemausage/schemausage_test.go (1)
2791-2996: Optional: consider adding a focused case for null elements inside lists of input objectsYou have good coverage for list values being null/empty/missing (e.g.,
tags: nullvs[]), but there’s no explicit test for something like:input Nested { a: ID } input Filter { nestedList: [Nested] } # variables { "filter": { "nestedList": [null, { "a": "x" }] } }Given the documented intent not to track null list elements individually, a small test here could lock in the chosen behavior (either skipping null elements entirely or treating the list as “used but non-null”) and make future refactors safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/pkg/graphqlschemausage/schemausage.go(7 hunks)router/pkg/graphqlschemausage/schemausage_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
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.
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.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
📚 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/pkg/graphqlschemausage/schemausage_test.gorouter/pkg/graphqlschemausage/schemausage.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: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/graphqlschemausage/schemausage.go
🧬 Code graph analysis (2)
router/pkg/graphqlschemausage/schemausage_test.go (2)
router/pkg/graphqlschemausage/schemausage.go (3)
GetArgumentUsageInfo(88-115)GetInputUsageInfo(123-137)TypeFieldUsageInfo(171-179)router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (9)
InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)TypeFieldUsageInfo(476-493)TypeFieldUsageInfo(508-508)TypeFieldUsageInfo(523-525)ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)
router/pkg/graphqlschemausage/schemausage.go (2)
graphqlmetrics/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (6)
ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)shared/src/router-config/builder.ts (1)
Input(39-46)
⏰ 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: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
router/pkg/graphqlschemausage/schemausage_test.go (2)
213-233: Good adaptation of tests to new GetArgumentUsageInfo/GetInputUsageInfo signaturesPassing the merged variables plus the concrete plan into both sync and subscription variants looks correct and keeps the equality assertions meaningful for all three usage types.
568-3101: Thorough coverage of null, implicit-null, list and multi-subgraph scenariosThis suite does a solid job pinning down the semantics you describe in the header comments:
- Explicit vs implicit nulls for inputs and arguments (including deeply nested chains).
- Behavior when variables are missing, present-but-null, or when
variablesis nil.- List fields: null vs empty vs missing, as reflected in
IsNulland the presence/absence of entries.- Multi-subgraph attribution for both shared input objects and shared scalar variables, including merged
SubgraphIDs.- Variable remapping after normalization via
astnormalization.NewVariablesMapper, validating both argument and input usage with and without remapping.I don’t see logical inconsistencies between these expectations and the implementation; the tests should significantly reduce regressions around breaking-change detection signals.
router/pkg/graphqlschemausage/schemausage.go (9)
1-51: Package-level documentation clearly encodes the new semanticsThe header comments do a good job describing the three data sources, the role of subgraph/variable mappings, and the precise null/list semantics, which matches how the tests exercise the code.
83-137: Public API contracts around nil variables and implicit-null tracking look soundExplicitly documenting that
variablesmay be nil, and then:
- Treating nil as “no variables provided” in
GetInputUsageInfo(still emittingIsNull=truerecords for relevant input types).- Defaulting
IsNullto false when variables are nil inGetArgumentUsageInfo.matches both the internal implementation and the dedicated nil-variables tests.
274-319: nullValueDetector correctly supports both direct nulls and remapped variable namesThe split between:
isValueNull(handling inlinenulland variable indirection) andgetOriginalVariableName(usingremapVariablesfor normalized → original name),lines up with the
TestVariableRemappingexpectations for both argument and input null detection.Please double‑check against the current
graphql-go-toolsdocs thatVariableValueNameStringalways returns the normalized variable name afterNewVariablesMapper.NormalizeOperation, as assumed here.
324-404: Subgraph mapping for variables correctly merges IDs per usage site
buildFieldSubgraphIDMapplusvariableSubgraphCollectorandmergeSubgraphIDsgive you:
- Field-path → subgraph IDs from the plan.
- Variable-name → merged subgraph IDs across all argument sites.
This is exactly what the “shared input object/variable across subgraphs” tests assert via merged
SubgraphIDs.The mapping currently keys fields by
field.Info.Nameon the plan side andFieldNameStringon the AST side. It’s worth confirming that both use the same identifier (field name vs alias) ingraphql-go-tools; otherwise aliased fields could fail to resolve subgraph IDs for their variables.
475-640: ArgumentUsage visitor and implicit-null tracking match the test matrixThe combination of:
EnterArgumentrecording provided arg names per field.trackImplicitNullArgumentswalking schema argument definitions and emittingIsNull=truefor those not present in the operation.nullDetector.isValueNullfor inline and variable-based nulls.covers the “null argument”, “implicit null arguments”, and “mixed provided/unprovided arguments” test cases without obvious edge-case gaps.
647-692: inputTypeResolver is a clean, focused helperResolving input object fields into a compact
inputFieldInfoslice simplifies traversal and implicit-null detection, and keepinggetNodeRefhere avoids scattering schema lookups.
697-836: Input traverser correctly implements explicit/implicit-null semantics, including listsKey points that align with tests:
traversestops propagation whenisNullis true, recording only the current node (matching the null-propagation scenarios).- Built-in scalars fall back to a simple usage record when the type isn’t found in the schema.
processField’s list branch now:
- Emits a single
IsNull=truerecord when the list value itself is null.- Iterates non-null lists without attempting per-element null accounting, in line with the documented “no per-element tracking” rule.
- Implicit nulls for missing fields are handled via
trackImplicitNullusing the resolver’s field list.This aligns with
TestNullListHandling,TestInputUsageWithNullVariables,TestInputUsageWithPartialNullFields, and the nested-input tests.
838-912: Deduplication for InputUsageInfo looks correct given current usage patterns
appendUniqueUsage+infoEqualsuse all structural fields (includingIsNullandSubgraphIDs) for equality, which:
- Prevents duplicate records for the same input/path/subgraph combination even when visited multiple times (e.g., list elements).
- Still allows distinct entries when the same input/path is used from different subgraphs, where you prefer the merging to happen earlier at the variable/field mapping level.
This behavior is consistent with how your tests build maps keyed by
Pathand then inspect the mergedSubgraphIDs.
965-1119: Implicit input-type argument usage is captured cleanly for breaking-change detection
collectImplicitArgumentInputUsageandimplicitArgumentInputCollector:
- Mirror the structure of the argument visitor (field/argument stacks, pathBuilder).
- Skip introspection and non-input-object arguments.
- Emit a single
InputUsageInfoper input object type withIsNull=truewhen arguments are not provided.This is exactly what
TestImplicitInputTypeArgumentUsageexpects and complementstrackImplicitNullArgumentson the argument side.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
router/pkg/graphqlschemausage/schemausage.go (2)
428-473: Variable remapping and nil-variable handling in null detection and traversalThe interaction between
nullValueDetectorandprocessVariableDefinitionlooks sound:
- Normalized variable names are remapped back to originals via
remapVariables, so JSON lookup works after minification.isValueNullshort-circuits for literalnulland only callsisVariableNullwhenvariables != nil, avoiding nil dereferences.processVariableDefinitionnow explicitly handlesvariables == nilor missing entries by treating input-object variables as “implicitly null” and emittingInputUsageInfowithIsNull=true, while safely no-oping for other types.Given earlier concerns about panics on nil
variables, this revision closes that hole while also enriching implicit-null coverage. Only suggestion, if you ever reuseisVariableNulldirectly, would be to add a small guard onn.variables == nilinside it for extra defensiveness, but it’s not required with the current call graph.Also applies to: 1139-1185
1082-1133: Potential performance hotspot in linear dedup for large operations
appendUniqueUsagedoes an O(n) scan with a deep equality check overPath,EnumValues, andSubgraphIDsfor each newInputUsageInfo. For typical documents this is fine, but with very large operations or aggregated traffic this can become quadratic.If you expect high cardinality here, consider an optional refactor to use a map keyed by a stable, normalized tuple (e.g.
NamedType|TypeName|IsNull|Path|EnumValues|SubgraphIDs) to get amortized O(1) dedup and keep the slice as just the values.router/pkg/graphqlschemausage/schemausage_test.go (2)
345-432: Extensive coverage is good, but many assertions depend on slice orderingThe test suite gives very thorough coverage of the new behaviors (null propagation, list/null semantics, multi-subgraph attribution, variable remapping, implicit nulls, nil/empty variables). One concern is that many checks use:
assert.Len(...)plus- index-based
assert.JSONEqover the entire slice,which assumes the production order of
fieldUsageInfo,argumentUsageInfo, and especiallyinputUsageInfois stable.If future changes adjust traversal order (e.g., new collectors, different AST walk patterns), these tests could fail even when the data is semantically identical. Where order is not important, consider normalizing before comparison (e.g., sorting by
Path+NamedType) or comparing via maps keyed byPath/NamedTyperather than relying on index positions. You already useassert.ElementsMatchforSubgraphIDs—extending that idea to the outer slices would make the tests more robust.Also applies to: 568-3003
568-1965: Shared test scaffolding could be factored to reduce duplicationMost tests reimplement the same setup sequence (parse schema/operation, merge base schema, normalize, validate, create one or more data source configurations, build a planner, plan the operation). This is perfectly fine functionally, but it does make the file long and a bit harder to maintain.
If this area keeps evolving, you might consider extracting a small helper (e.g., “buildPlanAndDocs(schema, operation)”) or a table-driven harness to DRY up the boilerplate and make the per-scenario intent (null cases, multi-subgraph, remapping, etc.) stand out more clearly. Not urgent, but it would improve readability.
Also applies to: 1970-2598
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/pkg/graphqlschemausage/schemausage.go(7 hunks)router/pkg/graphqlschemausage/schemausage_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
📚 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: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/graphqlschemausage/schemausage.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/pkg/graphqlschemausage/schemausage.gorouter/pkg/graphqlschemausage/schemausage_test.go
🧬 Code graph analysis (2)
router/pkg/graphqlschemausage/schemausage.go (2)
router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (9)
ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)TypeFieldUsageInfo(476-493)TypeFieldUsageInfo(508-508)TypeFieldUsageInfo(523-525)shared/src/router-config/builder.ts (1)
Input(39-46)
router/pkg/graphqlschemausage/schemausage_test.go (2)
router/pkg/graphqlschemausage/schemausage.go (2)
GetArgumentUsageInfo(93-111)GetInputUsageInfo(119-152)router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.go (6)
InputUsageInfo(663-683)InputUsageInfo(698-698)InputUsageInfo(713-715)ArgumentUsageInfo(569-587)ArgumentUsageInfo(602-602)ArgumentUsageInfo(617-619)
⏰ 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). (5)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/pkg/graphqlschemausage/schemausage.go (3)
88-152: Public API wiring and nil-variable semantics look consistent and robust
GetArgumentUsageInfo/GetInputUsageInfocorrectly share thefieldSubgraphMapandnullValueDetector, toleratevariables == nilas documented, and centralize traversal throughrunUnifiedWalkwith dedicated collectors. The follow‑up processing (subgraphMapper,inputTraverser,processVariableDefinition,implicitInputCollector.finalizeUsage) matches the comments about:
- argument null tracking (inline + variable-based),
- input null tracking (explicit + implicit),
- and attribution of both arguments and inputs to subgraph IDs.
I don’t see correctness gaps in this wiring; behavior for
variables == nilin both functions matches the docstrings and is covered by tests likeTestNilVariablesHandling.
981-1053: List/null handling in input traversal matches the documented semanticsThe combination of
processFieldandtraverseimplements the header comments:
- Null list values (
tags: null) result in a single usage withIsNull=trueand the correct path, via thefieldIsNullbranch andtraverse(..., isNull=true).- Empty lists (
tags: []) emit a non-null usage (IsNull=false) so the field is still considered “used” for breaking-change detection.- Non-empty lists iterate elements but produce a single deduped entry at the field level, without marking per-element nulls, which aligns with the “null elements within lists are not individually tracked” note.
TestNullListHandlingand the other input tests appear to exercise these cases. I don’t see remaining inconsistencies with the stated semantics.
959-979: Clarify whether inline literal inputs are intended to contribute toInputUsageInfo
GetInputUsageInfocurrently drives traversal exclusively off JSONvariables(plus implicit input-type arguments), viaprocessVariableDefinitionandinputTraverser. I don’t see any code in this file that walks inline literal argument values.In
TestGetSchemaUsageInfothe expectedInputUsageInfoincludes anEpisodeentry withEnumValues: ["JEDI","EMPIRE"]that appears to correspond to the inlineenumList2: [JEDI, EMPIRE]argument, not a variable. From this file alone it’s not obvious how that entry is produced—unless upstream code is materializing inline argument values into thevariablesJSON before calling this API.Please double-check whether:
- callers always supply a variables payload that already includes coerced inline values, or
- inline argument values are intentionally not supposed to feed into
InputUsageInfo(and the expectations should remain variable-only).If inline literals are meant to be tracked here, we may need an additional traversal path over AST value literals rather than only over
variables.Also applies to: 345-418
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (1)
592-622: Tighten comment wording and clarify the “required argument” assumptionTwo small points here:
- The inline comments still have the typo previously flagged (“recieved”) and slightly awkward phrasing. You can align with the earlier suggested wording:
- // e.g. if the path recieved is 'Query.employee.a', the path should be ['employee'] as its new field or it has changed the type of the argument, we check the usage of the operation. + // e.g. if the path received is 'Query.employee.a', the path should be ['employee'] as it's a new field or it has changed the type of the argument; we check the usage of the operation.Apply that in both the
FieldArgumentAddedand theisRequiredbranch ofFieldArgumentRemoved.
- The comment
// Only when a required argument is addedrelies on upstream filtering to ensure only breakingFieldArgumentAddedchanges reach this branch; the implementation itself doesn’t check requiredness. That’s fine ifschemaChangesis guaranteed to contain only breaking diffs, but if an optional‑argument addition ever slips through, it will be treated as breaking. It’s worth double‑checking that the producer ofSchemaDiffenforces this invariant, or adding a guard/assertion here.
🧹 Nitpick comments (3)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (3)
29-66: Consider recursive handling (or documenting limits) for nested list types
extractBaseTypeonly strips a single layer of brackets, so something like[[Int!]!]!yields a base type of"[Int!]"rather than"Int". If you ever need “base type” to ignore all list nesting inparseTypeChange, either document this one-level behavior or unwrap recursively.A possible recursive refactor (if needed) could look like:
-function extractBaseType(type: string): string { - let base = type.trim(); - - // Remove trailing "!" - if (base.endsWith('!')) { - base = base.slice(0, -1).trim(); - } - - // Handle array types like "[Boolean!]" or "[Boolean!]!" - // Remove outer brackets and inner "!" - if (base.startsWith('[') && base.endsWith(']')) { - base = base.slice(1, -1).trim(); - // Remove inner "!" if present - if (base.endsWith('!')) { - base = base.slice(0, -1).trim(); - } - } - - return base; -} +function extractBaseType(type: string): string { + let base = type.trim(); + + // Strip all trailing "!" first + while (base.endsWith('!')) { + base = base.slice(0, -1).trim(); + } + + // Unwrap nested list brackets and inner "!" markers + while (base.startsWith('[') && base.endsWith(']')) { + base = base.slice(1, -1).trim(); + while (base.endsWith('!')) { + base = base.slice(0, -1).trim(); + } + } + + return base; +}
68-95:parseArgumentRemovalis tightly coupled to the message string formatThe regex and error path look fine, but this function will throw if
SchemaDiff.messageever deviates from"Argument 'name: Type' was removed ...". Since that comes from@graphql-inspector/core, it’d be good to have explicit tests around this message format (and maybe a short comment noting the dependency) so future library upgrades don’t silently break this parsing.
524-533:InputFieldAdded/Removedcurrently assume “all ops using this input type are breaking”For both
InputFieldRemovedandInputFieldAddedyou return a change keyed only by the input type (path: [path[0]],isInput: true,isNull: false), which matches the inline comment “all the ops which use this input type are breaking”. This is a conservative choice and may over‑approximate the actual breakage surface (e.g. fields on the input type that are never used), but it’s consistent with the current inspector’s coarse‑grained behavior.If, in the future, you want to reduce false positives, you could refine this to field‑level paths similar to
InputFieldTypeChangedwith category‑basedisNullhandling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
⏰ 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: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
controlplane/src/core/services/SchemaUsageTrafficInspector.ts (5)
6-27: Enum categorization for type changes looks coherentThe four
FieldTypeChangeCategoryvalues cleanly cover the optional/required × same/different-type matrix and line up with later switch usage; no issues spotted here.
166-175:isNullwiring into inspector changes and ClickHouse filter looks consistentAdding
isNull?: booleantoInspectorSchemaChangeand guarding withif (change.isNull !== undefined) { ... }ensures bothtrueandfalsecases are expressible while omitting the predicate when unset. This is consistent with the existingIsInput/IsArgumentfilters and should compose cleanly with the new nullability‑aware mappings.Also applies to: 237-239
292-311: Stricter enforcement of matchingSchemaCheckChangeActionis a good safety netThrowing when no
schemaCheckActionexists for aSchemaDiff(instead of silently skipping) is a good invariant to enforce; combined with thenullfiltering of non‑inspectable changes, this makes it easier to spot pipeline inconsistencies without dropping inspectable diffs.
473-523:InputFieldTypeChangedcategorization and inspector mapping align with commentsThe four branches for
InputFieldTypeChangedcorrectly mirror the enum categories and the intended semantics:
- Optional→required same type: narrow to the specific input field with
isInput: trueandisNull: true.- Optional→required or required→required different type: treat all usages of the input type (
path: [path[0]],isInput: true,isNull: false) as breaking.- Optional→optional different type: focus on non‑null field usages via
isNull: false.The error branch now correctly references
inputFieldTypeChangeCategory, which matches earlier feedback.
537-590:FieldArgumentTypeChangedmapping looks internally consistent with path semanticsThe argument handling mirrors the input‑field logic:
- Optional→required same type:
isArgument: true,isNull: true,path: path.slice(1)andfieldName: path[2]target calls where the argument is present/null.- Optional→required different and required→required different: same path/fieldName but without
isNull, matching “all ops which have this argument are breaking”.- Optional→optional different: uses
isNull: falseto focus on non‑null argument usages.Assuming
change.pathis always of the form"Type.field.arg", the indexing (path[0]/path[2]/path.slice(1)) lines up correctly.
…493-schema-usage-issues
- Added `meta` field to `SchemaDiff` interface to include structured data from graphql-inspector. - Updated `getSchemaDiff` and `getDiffBetweenGraphs` functions to utilize the new `meta` field for breaking and non-breaking changes. - Refactored type change categorization to use structured metadata instead of parsing messages in `toInspectorChange` function. - Improved type handling in `SchemaUsageTrafficInspector` by incorporating new type change category logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/pkg/graphqlschemausage/schemausage_bench_test.go (1)
286-290: Consider merging input variables for consistency withsetupBenchmark.The
setupBenchmarkfunction (lines 98-102) merges the parsed variables withop.Input.Variables, but this helper does not. While the simple query here may work correctly, inconsistent variable handling between benchmarks could lead to subtle discrepancies if the normalization step populatesop.Input.Variableswith defaults or inline values.// Parse variables vars, err := astjson.Parse(variables) require.NoError(b, err) - return generatedPlan, &op, &def, vars + inputVariables, err := astjson.ParseBytes(op.Input.Variables) + require.NoError(b, err) + + merged, _, err := astjson.MergeValues(vars, inputVariables) + require.NoError(b, err) + + return generatedPlan, &op, &def, merged
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router/debug.config.yaml(1 hunks)router/demo.config.yaml(1 hunks)router/pkg/graphqlschemausage/schemausage_bench_test.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/demo.config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
router/pkg/graphqlschemausage/schemausage_bench_test.go (5)
router/pkg/graphqlschemausage/schemausage.go (3)
GetArgumentUsageInfo(93-111)GetInputUsageInfo(119-152)GetTypeFieldUsageInfo(77-86)router/core/plan_generator.go (1)
NewPlanner(85-96)router-tests/cmd/config-validator/extract_schemas.go (1)
Configuration(15-33)router/core/execution_config_usage.go (1)
DataSource(14-33)router-tests/testenv/testenv.go (1)
Run(105-122)
⏰ 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). (13)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/debug.config.yaml (1)
8-10: Clarify intent: config block is commented but summary indicates it's enabled.The enriched summary states the graphql_metrics configuration block is "added...enabled: true", but the actual code shows it commented out (lines 8–10). This creates a discrepancy—either the comments should be removed to enable the metrics collector for testing, or the summary is misleading about the change's scope.
For a debug config file, commenting is reasonable to let developers opt-in. However, given that this PR introduces metrics propagation (IsNull, SubgraphIDs), the config may need to be active during CI/testing to verify the metrics pipeline. Clarify whether this should be uncommented to validate the new metrics functionality.
router/pkg/graphqlschemausage/schemausage_bench_test.go (2)
111-112: Good practice: resetting timer before measurements.Adding
b.ResetTimer()beforeb.ReportAllocs()correctly excludes setup time from benchmark measurements, ensuring accurate timing results.
293-337: Well-structured scalability benchmark.The parameterized benchmark with varying field counts (10 to 500) is well-designed to identify O(n²) bottlenecks. The use of sub-benchmarks via
b.Run, proper timer reset, and preventing compiler optimization are all correctly implemented.
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
This PR fixes input and argument schema usage tracking:
Add missing subgraphIds: Input and argument schema usage entries were missing subgraphIds. This PR ensures they’re recorded.
Track all inputs/arguments referenced by the operation: Previously, input types, input fields, or arguments that were part of an operation but not used weren’t tracked. That gap made it harder (or impossible) to determine which operations would break when an input or argument changed. This PR now tracks all inputs/arguments of the operation, regardless of whether they are used, improving change-impact detection and breakage analysis.
Checklist