chore: change how schema node is propagated #2338
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughRenames directive-tracking maps and parameters from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Areas needing extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-08T20:57:07.946ZApplied to files:
🧬 Code graph analysis (3)composition/src/federation/types.ts (2)
composition/src/subgraph/types.ts (1)
composition/src/schema-building/utils.ts (3)
⏰ 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)
🔇 Additional comments (17)
Comment |
Router-nonroot 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
🧹 Nitpick comments (4)
composition/src/subgraph/types.ts (1)
1-8: Imports and schemaNode typing look correct.Top-level graphql imports are preferred and the union
SchemaDefinitionNode | SchemaExtensionNodeis appropriate. Minor consistency nit:InternalSubgraph.nameis astringwhileSubgraph.nameisSubgraphName. Consider aligning for type clarity.export type InternalSubgraph = { @@ - name: string; + name: SubgraphName;Also, double-check there are no lingering
from 'graphql/index'imports elsewhere.Also applies to: 30-31, 47-47
composition/tests/v1/types/objects.test.ts (1)
242-246: Stylistic: place test on a new line after the block comment.
*/ test('...')on the same line is valid but harms readability. Movetest(to the next line.Also applies to: 258-262
composition/src/federation/types.ts (1)
64-69: Use domain-specific key types in maps.Prefer
DirectiveNameandSubgraphNameover rawstringfor stronger intent and consistency with other types.export type MutualParentDefinitionData = { - configureDescriptionDataBySubgraphName: Map<string, ConfigureDescriptionData>; - directivesByName: Map<string, ConstDirectiveNode[]>; + configureDescriptionDataBySubgraphName: Map<SubgraphName, ConfigureDescriptionData>; + directivesByName: Map<DirectiveName, ConstDirectiveNode[]>; extensionType: ExtensionType; name: string; persistedDirectivesData: PersistedDirectivesData; description?: StringValueNode; };composition/src/v1/normalization/walkers.ts (1)
206-207: Rename looks good; consider typing directive maps by DirectiveName.The migration to
directivesByNameis consistent. Optional: initialize withnew Map<DirectiveName, ConstDirectiveNode[]>()(and importDirectiveName) to encode intent and catch key-type mistakes early.Also applies to: 211-212, 271-272, 333-347, 351-354
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
composition/src/federation/types.ts(1 hunks)composition/src/schema-building/types.ts(10 hunks)composition/src/schema-building/utils.ts(11 hunks)composition/src/subgraph/types.ts(1 hunks)composition/src/v1/federation/federation-factory.ts(23 hunks)composition/src/v1/normalization/normalization-factory.ts(38 hunks)composition/src/v1/normalization/params.ts(1 hunks)composition/src/v1/normalization/walkers.ts(3 hunks)composition/tests/v1/federation-factory.test.ts(0 hunks)composition/tests/v1/normalization.test.ts(4 hunks)composition/tests/v1/types/objects.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- composition/tests/v1/federation-factory.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/types/objects.test.tscomposition/tests/v1/normalization.test.tscomposition/src/v1/normalization/normalization-factory.ts
🧬 Code graph analysis (7)
composition/src/v1/normalization/params.ts (1)
composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/src/v1/normalization/walkers.ts (2)
composition/src/schema-building/types.ts (1)
InputValueData(134-155)composition/src/utils/string-constants.ts (2)
PROVIDES(115-115)REQUIRES(123-123)
composition/src/schema-building/types.ts (1)
composition/src/types/types.ts (2)
DirectiveName(3-3)SubgraphName(9-9)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/utils/string-constants.ts (3)
SUBSCRIPTION_FILTER(147-147)ONE_OF(104-104)INACCESSIBLE(67-67)composition/src/utils/utils.ts (3)
copyArrayValueMap(247-253)getValueOrDefault(153-161)generateSimpleDirective(171-176)composition/src/schema-building/utils.ts (1)
newPersistedDirectivesData(86-93)
composition/tests/v1/normalization.test.ts (6)
composition/tests/utils/utils.ts (3)
normalizeSubgraphSuccess(36-46)schemaToSortedNormalizedString(23-25)normalizeString(19-21)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/tests/v1/utils/utils.ts (1)
KEY_DIRECTIVE(67-69)composition/src/ast/utils.ts (2)
stringToNamedTypeNode(100-105)parse(272-274)composition/src/utils/string-constants.ts (1)
QUERY(117-117)composition/src/subgraph/types.ts (1)
Subgraph(18-22)
composition/src/v1/normalization/normalization-factory.ts (5)
composition/src/utils/string-constants.ts (10)
INACCESSIBLE(67-67)REQUIRE_FETCH_REASONS(122-122)EXTERNAL(48-48)SHAREABLE(137-137)EXTENDS(49-49)CONFIGURE_DESCRIPTION(16-16)KEY(80-80)INTERFACE_OBJECT(79-79)ONE_OF(104-104)SCHEMA(128-128)composition/src/v1/normalization/params.ts (1)
HandleFieldInheritableDirectivesParams(10-15)composition/src/schema-building/types.ts (7)
FieldData(97-118)ObjectDefinitionData(174-191)EnumValueData(73-85)InputValueData(134-155)EnumDefinitionData(58-71)SchemaData(221-227)UnionDefinitionData(229-240)composition/src/schema-building/utils.ts (1)
isNodeExternalOrShareable(100-123)composition/src/ast/utils.ts (1)
stringToNamedTypeNode(100-105)
composition/src/schema-building/utils.ts (3)
composition/src/types/types.ts (1)
DirectiveName(3-3)composition/src/utils/string-constants.ts (5)
EXTERNAL(48-48)SHAREABLE(137-137)AUTHENTICATED(8-8)REQUIRES_SCOPES(124-124)INACCESSIBLE(67-67)composition/src/utils/utils.ts (1)
generateSimpleDirective(171-176)
⏰ 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: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- 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: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
🔇 Additional comments (4)
composition/src/v1/normalization/params.ts (1)
11-11: Rename applied; verify call sites.
directivesByNamematches the project-wide change. Please ensure all invocations ofhandleFieldInheritableDirectivesnow passdirectivesByName.composition/src/v1/federation/federation-factory.ts (1)
339-343: Directive propagation refactor LGTM.Map-based persisted-directive extraction/flattening is consistent with the rename and preserves behavior (non-repeatable handling, tag/deprecated, semanticNonNull). No blocking issues spotted.
To be safe, scan the repo for any remaining
directivesByDirectiveNameorOperationTypeNode.usages and oldgraphql/indeximports.Also applies to: 651-652, 685-686, 980-989, 999-1011, 1059-1067, 1076-1101, 1818-1821
composition/src/schema-building/types.ts (1)
61-61: Type surface updated consistently.All definition data now expose
directivesByName: Map<DirectiveName, Array<ConstDirectiveNode>>. This aligns types across normalization/federation. Looks good.Also applies to: 76-76, 100-100, 122-122, 136-136, 159-160, 176-176, 204-205, 211-212, 222-223, 231-232
composition/tests/v1/normalization.test.ts (1)
63-63: Ignore this review comment—OperationTypeNode is a valid runtime value in graphql v16.9.0.The composition package uses
"graphql": "^16.9.0", and starting in graphql-js v16 (16.0.0), OperationTypeNode was exported as a runtime value (an enum-like object with members QUERY, MUTATION, SUBSCRIPTION). The current code usingOperationTypeNode.QUERYis correct and does not yieldundefinedat runtime. No changes are required.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Refactor
New Features
Tests
Chores
Checklist