chore: expose schema node in FederationSuccess#2330
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughReplaces string-based name aliases with typed aliases (SubgraphName, TypeName, FieldName), adds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composition/src/v1/federation/federation-factory.ts (1)
3189-3197: Inconsistency:schemaNodemissing from contract federation result.The
buildFederationContractResultmethod constructssubgraphConfigBySubgraphNameentries (lines 3191-3197) without including theschemaNodefield, unlikebuildFederationResult(lines 2897-2903). This creates an inconsistency where the schema node is exposed for regular federation but not for contract federation.Consider updating this section to include
schemaNode:for (const subgraph of this.internalSubgraphBySubgraphName.values()) { subgraphConfigBySubgraphName.set(subgraph.name, { configurationDataByTypeName: subgraph.configurationDataByTypeName, directiveDefinitionByName: subgraph.directiveDefinitionByName, isVersionTwo: subgraph.isVersionTwo, parentDefinitionDataByTypeName: subgraph.parentDefinitionDataByTypeName, schema: subgraph.schema, + schemaNode: subgraph.schemaNode, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composition/src/subgraph/types.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(2 hunks)composition/src/v1/normalization/normalization-factory.ts(2 hunks)composition/tests/v1/federation-factory.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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-09-08T20:57:07.946Z
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.
Applied to files:
composition/src/subgraph/types.tscomposition/tests/v1/federation-factory.test.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/normalization/normalization-factory.ts
📚 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:
composition/src/subgraph/types.tscomposition/src/v1/federation/federation-factory.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
composition/src/v1/federation/federation-factory.ts
🧬 Code graph analysis (4)
composition/src/subgraph/types.ts (3)
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
Subgraph(249-293)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
Subgraph(430-438)Subgraph(453-453)Subgraph(468-470)router/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
Subgraph(430-438)Subgraph(453-453)Subgraph(468-470)
composition/tests/v1/federation-factory.test.ts (3)
composition/tests/utils/utils.ts (1)
federateSubgraphsSuccess(58-71)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/ast/utils.ts (1)
stringToNamedTypeNode(100-105)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/resolvability-graph/types/types.ts (1)
SubgraphName(13-13)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/subgraph/types.ts (2)
InternalSubgraph(27-43)SubgraphConfig(18-25)
composition/src/v1/normalization/normalization-factory.ts (3)
composition/src/resolvability-graph/types/types.ts (3)
TypeName(15-15)SubgraphName(13-13)FieldName(7-7)composition/src/schema-building/types.ts (3)
AuthorizationData(309-315)EntityData(268-275)ParentDefinitionData(242-248)composition/src/subgraph/types.ts (1)
InternalSubgraph(27-43)
⏰ 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: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
composition/src/subgraph/types.ts (3)
9-10: LGTM! Clean import additions for schema node support.The new imports properly support the schema node fields being added to the subgraph types.
14-14: LGTM! Type refinement improves consistency.Using
SubgraphNameinstead ofstringprovides better type clarity throughout the codebase.
24-24: LGTM! Schema node fields properly added.The optional
schemaNodefields are appropriately typed and will allow propagation of schema definition/extension information through the federation pipeline.Also applies to: 41-41
composition/tests/v1/federation-factory.test.ts (2)
16-17: LGTM! Test imports are appropriate.The imported utilities are needed for the new schema node assertions.
1028-1068: LGTM! Test coverage validates schema node propagation.The new test assertions properly verify that:
subgraphConfigBySubgraphNameis returned from federation- The
schemaNodefield is present and correctly structured- The schema definition includes the expected query operation type
composition/src/v1/federation/federation-factory.ts (2)
271-271: LGTM! Type refinement is consistent with subgraph types.The change from
Map<string, InternalSubgraph>toMap<SubgraphName, InternalSubgraph>aligns with the type updates in the subgraph types file.
2887-2904: LGTM! Schema node properly propagated in federation result.The changes correctly:
- Update the map type to use
SubgraphNamekeys- Destructure
schemaNodefrom internal subgraphs- Include
schemaNodein the subgraph config entriesThis ensures the schema node information is available in the federation success result.
composition/src/v1/normalization/normalization-factory.ts (1)
3886-3903: LGTM! Type aliases and schemaNode propagation correctly implemented.The typed aliases for
overriddenFieldNamesByParentTypeName(line 3896) provide better semantic clarity, and theschemaNodepropagation (line 3901) correctly threads the schema node from normalization results into the internal subgraph structure, aligning with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
composition/src/v1/normalization/normalization-factory.ts (1)
3896-3896: Consistency nit: align InternalSubgraph override map typing.You initialize overriddenFieldNamesByParentTypeName as Map<TypeName, Set> here, while InternalSubgraph currently declares Map<string, Set>. Consider updating the type in composition/src/subgraph/types.ts to Map<TypeName, Set> to avoid drift.
composition/src/v1/federation/federation-factory.ts (2)
2887-2904: Expose schemaNode in subgraph config — LGTM.Correctly threads name/schema/schemaNode and typed maps into subgraphConfigBySubgraphName.
If desired, extract this population into a small helper to reuse below and avoid duplication.
3190-3206: <!-- [scratchpad_start]
[task verify_review_comment_federation_factory]
[observations]
- Base path (lines ~2887-2904) and contract path (lines ~3189-3207) both:
- Create
subgraphConfigBySubgraphName: Map<SubgraphName, SubgraphConfig>()- Loop over
this.internalSubgraphBySubgraphName.values()- Destructure identical properties including schemaNode
- Set into map with identical 6-property object
- Base path uses shorthand:
schemaNode,at end- Contract path uses verbose:
schemaNode: schemaNode,- "as above" refers to base path pattern that appears ~300 lines earlier
- SubgraphConfig type properly includes
schemaNode?: SchemaDefinitionNode | SchemaExtensionNode;- No downstream usage of .schemaNode from these config objects found
[analysis]
- Code is functionally correct; no issues or bugs detected
- schemaNode properly exposed in both paths
- True duplication: identical 42-line block repeated (Map creation + loop + object construction)
- Optional refactor suggestion valid: extract shared helper to reduce duplication
- Review intent: approve with optional improvement suggestion
- This is NOT a critical issue; code works as-is
[pending]
None - verification complete[actions]
Generate final rewritten comment with tag[done]
- Verified both paths handle schemaNode identically
- Confirmed no critical issues
- Identified duplication opportunity (optional)
- Review is approval with optional suggestion
</scratchpad_end] -->Consider extracting a shared helper to reduce duplication between base and contract paths.
Both code paths populate
subgraphConfigBySubgraphNameidentically—the same loop and object structure repeat at lines ~2887–2904 and ~3189–3207. Extract this into a private helper method to DRY the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
composition/src/v1/federation/federation-factory.ts(3 hunks)composition/src/v1/normalization/normalization-factory.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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-09-08T20:57:07.946Z
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.
Applied to files:
composition/src/v1/federation/federation-factory.tscomposition/src/v1/normalization/normalization-factory.ts
📚 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:
composition/src/v1/federation/federation-factory.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
composition/src/v1/federation/federation-factory.ts
🧬 Code graph analysis (2)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/resolvability-graph/types/types.ts (1)
SubgraphName(13-13)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/subgraph/types.ts (2)
InternalSubgraph(27-43)SubgraphConfig(18-25)
composition/src/v1/normalization/normalization-factory.ts (3)
composition/src/resolvability-graph/types/types.ts (3)
TypeName(15-15)SubgraphName(13-13)FieldName(7-7)composition/src/schema-building/types.ts (3)
AuthorizationData(309-315)EntityData(268-275)ParentDefinitionData(242-248)composition/src/subgraph/types.ts (1)
InternalSubgraph(27-43)
⏰ 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_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
composition/src/v1/normalization/normalization-factory.ts (4)
3807-3810: Good: stronger typed maps for aggregated state.Switch to Map<TypeName/...> improves intent without behavior change. LGTM.
3814-3818: Resolved: correct type for error message list.invalidNameErrorMessages now string[], which matches its contents. Thanks for fixing.
3820-3820: Good: typed fieldCoords map.Map<TypeName, Set> aligns with downstream consumers.
3901-3902: Test coverage gap confirmed; SchemaExtensionNode (directives-only) path lacks test assertions.The normalization-factory.ts code at lines 3486-3505 handles both SchemaDefinitionNode (when operationTypes exist) and SchemaExtensionNode (directives-only, lines 3496-3499). The test suite covers only the SchemaDefinitionNode case (federation-factory.test.ts line 1057). No tests validate the SchemaExtensionNode path.
Note: Lines 3500-3504 in the source file contain a TODO comment flagging that pushing the SchemaExtensionNode to definitions "currently breaks engine"—verify whether this test gap is intentional due to engine limitations or requires implementation.
composition/src/v1/federation/federation-factory.ts (1)
271-271: Good: key type tightened to SubgraphName.Improves clarity; no behavioral change. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
composition/src/v1/federation/federation-factory.ts (1)
2887-2905: Consider extracting duplicated subgraph config building logic.The logic for building
subgraphConfigBySubgraphNameis duplicated betweenbuildFederationResult(lines 2887-2905) andbuildFederationContractResult(lines 3189-3207).Consider extracting this into a helper method:
private buildSubgraphConfigMap(): Map<SubgraphName, SubgraphConfig> { const subgraphConfigBySubgraphName = new Map<SubgraphName, SubgraphConfig>(); for (const { configurationDataByTypeName, directiveDefinitionByName, isVersionTwo, name, parentDefinitionDataByTypeName, schema, schemaNode, } of this.internalSubgraphBySubgraphName.values()) { subgraphConfigBySubgraphName.set(name, { configurationDataByTypeName, directiveDefinitionByName, isVersionTwo, parentDefinitionDataByTypeName, schema, schemaNode, }); } return subgraphConfigBySubgraphName; }Then call it in both methods:
const subgraphConfigBySubgraphName = this.buildSubgraphConfigMap();Also applies to: 3189-3207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
composition/src/subgraph/types.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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-09-08T20:57:07.946Z
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.
Applied to files:
composition/src/subgraph/types.tscomposition/src/v1/federation/federation-factory.ts
📚 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:
composition/src/v1/federation/federation-factory.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
composition/src/v1/federation/federation-factory.ts
🧬 Code graph analysis (2)
composition/src/subgraph/types.ts (5)
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
Subgraph(249-293)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
Subgraph(430-438)Subgraph(453-453)Subgraph(468-470)router/gen/proto/wg/cosmo/node/v1/node.pb.go (3)
Subgraph(430-438)Subgraph(453-453)Subgraph(468-470)composition/src/schema-building/types.ts (2)
ParentDefinitionData(242-248)PersistedDirectiveDefinitionData(193-200)composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/resolvability-graph/types/types.ts (1)
SubgraphName(13-13)composition/src/types/types.ts (1)
SubgraphName(9-9)composition/src/subgraph/types.ts (2)
InternalSubgraph(27-43)SubgraphConfig(18-25)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
🔇 Additional comments (5)
composition/src/subgraph/types.ts (3)
9-10: LGTM! Clean import additions for type safety.The new imports support the typed aliases (SubgraphName, TypeName, FieldName) and the schema node feature (SchemaDefinitionNode, SchemaExtensionNode), aligning with the PR objective.
12-25: LGTM! Type improvements and schema node exposure.The changes correctly:
- Replace plain
stringwithSubgraphNametype alias for better semantic typing- Add optional
schemaNodefield toSubgraphConfig, enabling downstream consumers to access schema definition nodesThe optional nature of
schemaNodeis appropriate since not all schemas explicitly define a schema node.
27-43: LGTM! Consistent type refinements across InternalSubgraph.The changes introduce:
TypeNamefor map keys inkeyFieldNamesByParentTypeName(line 34)FieldNamefor set values inoverriddenFieldNamesByParentTypeName(line 37)- Optional
schemaNodefield (line 41)These refinements improve type safety and consistency with the rest of the codebase while exposing the schema node as intended by the PR.
composition/src/v1/federation/federation-factory.ts (2)
271-271: LGTM! Type refinement for better semantic clarity.Changing the map key type from
stringtoSubgraphNameimproves type safety and aligns with the typed aliases introduced incomposition/src/subgraph/types.ts.
2887-2905: LGTM! Clean destructuring pattern exposes schema node.The changes correctly:
- Use typed
Map<SubgraphName, SubgraphConfig>instead ofMap<string, SubgraphConfig>- Destructure fields from internal subgraphs for clarity
- Include
schemaNodein the returnedSubgraphConfigobjects (line 2903), fulfilling the PR objective- Use the extracted
namefield as the map keyThe destructuring pattern makes the field extraction explicit and improves code readability.
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Checklist