feat: expose subgraph defined directives#2287
Conversation
wilsonrivera
requested review from
Aenimus,
JivusAyrus and
StarpTech
as code owners
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThis pull request adds directive definition mapping capabilities across the normalization and subgraph composition layers. It introduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: Changes span five files across type definitions, constants, and factory implementations with heterogeneous modifications (type signature changes, field additions, map key type migrations, and propagation logic). The public API surface expands significantly with stricter type constraints on directive maps. While individual changes follow consistent patterns, each layer requires independent reasoning to verify correctness of propagation and type alignment. Test coverage additions validate the changes but require verification of assertion completeness. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
3448-3491: Keep exposed directiveDefinitionByDirectiveName in sync with all injected definitionsYou push several directive definitions into definitions (EDFS-related, subscription filter, configure*, link, oneOf, requireFetchReasons, semanticNonNull), but not all are added to directiveDefinitionByDirectiveName. For consumers relying on the exposed map, this drifts from the actual SDL produced.
Suggestion: whenever a directive definition is pushed into definitions, also upsert it into directiveDefinitionByDirectiveName.
Apply the following additions near the existing pushes:
@@ for (const directiveName of this.edfsDirectiveReferences) { const directiveDefinition = EVENT_DRIVEN_DIRECTIVE_DEFINITIONS_BY_DIRECTIVE_NAME.get(directiveName); if (!directiveDefinition) { // should never happen this.errors.push(invalidEdfsDirectiveName(directiveName)); continue; } definitions.push(directiveDefinition); + this.directiveDefinitionByDirectiveName.set(directiveDefinition.name.value, directiveDefinition); } @@ if (this.edfsDirectiveReferences.size > 0 && this.referencedDirectiveNames.has(SUBSCRIPTION_FILTER)) { definitions.push(SUBSCRIPTION_FILTER_DEFINITION); definitions.push(SUBSCRIPTION_FILTER_CONDITION_DEFINITION); definitions.push(SUBSCRIPTION_FIELD_CONDITION_DEFINITION); definitions.push(SUBSCRIPTION_FILTER_VALUE_DEFINITION); + this.directiveDefinitionByDirectiveName.set(SUBSCRIPTION_FILTER, SUBSCRIPTION_FILTER_DEFINITION); + this.directiveDefinitionByDirectiveName.set(SUBSCRIPTION_FILTER_CONDITION, SUBSCRIPTION_FILTER_CONDITION_DEFINITION); + this.directiveDefinitionByDirectiveName.set(SUBSCRIPTION_FIELD_CONDITION, SUBSCRIPTION_FIELD_CONDITION_DEFINITION); + this.directiveDefinitionByDirectiveName.set(SUBSCRIPTION_FILTER_VALUE, SUBSCRIPTION_FILTER_VALUE_DEFINITION); } @@ if (this.referencedDirectiveNames.has(CONFIGURE_DESCRIPTION)) { definitions.push(CONFIGURE_DESCRIPTION_DEFINITION); + this.directiveDefinitionByDirectiveName.set(CONFIGURE_DESCRIPTION, CONFIGURE_DESCRIPTION_DEFINITION); } @@ if (this.referencedDirectiveNames.has(CONFIGURE_CHILD_DESCRIPTIONS)) { definitions.push(CONFIGURE_CHILD_DESCRIPTIONS_DEFINITION); + this.directiveDefinitionByDirectiveName.set(CONFIGURE_CHILD_DESCRIPTIONS, CONFIGURE_CHILD_DESCRIPTIONS_DEFINITION); } @@ if (this.referencedDirectiveNames.has(LINK)) { definitions.push(LINK_DEFINITION); definitions.push(LINK_IMPORT_DEFINITION); definitions.push(LINK_PURPOSE_DEFINITION); + this.directiveDefinitionByDirectiveName.set(LINK, LINK_DEFINITION); + this.directiveDefinitionByDirectiveName.set(LINK_IMPORT, LINK_IMPORT_DEFINITION); + this.directiveDefinitionByDirectiveName.set(LINK_PURPOSE, LINK_PURPOSE_DEFINITION); } @@ if (this.referencedDirectiveNames.has(ONE_OF)) { definitions.push(ONE_OF_DEFINITION); + this.directiveDefinitionByDirectiveName.set(ONE_OF, ONE_OF_DEFINITION); } @@ if (this.referencedDirectiveNames.has(REQUIRE_FETCH_REASONS)) { definitions.push(REQUIRE_FETCH_REASONS_DEFINITION); + this.directiveDefinitionByDirectiveName.set(REQUIRE_FETCH_REASONS, REQUIRE_FETCH_REASONS_DEFINITION); } @@ if (this.referencedDirectiveNames.has(SEMANTIC_NON_NULL)) { definitions.push(SEMANTIC_NON_NULL_DEFINITION); + this.directiveDefinitionByDirectiveName.set(SEMANTIC_NON_NULL, SEMANTIC_NON_NULL_DEFINITION); }Also applies to: 3576-3579
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
composition/src/normalization/types.ts(3 hunks)composition/src/subgraph/types.ts(3 hunks)composition/src/v1/federation/federation-factory.ts(2 hunks)composition/src/v1/normalization/normalization-factory.ts(4 hunks)composition/src/v1/utils/constants.ts(4 hunks)composition/tests/v1/federation-factory.test.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
composition/src/subgraph/types.ts (2)
composition/src/types/types.ts (1)
DirectiveName(3-3)composition/src/schema-building/types.ts (1)
PersistedDirectiveDefinitionData(193-200)
composition/src/normalization/types.ts (1)
composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/src/v1/utils/constants.ts (1)
composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/tests/v1/federation-factory.test.ts (3)
composition/tests/utils/utils.ts (2)
federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/v1/utils/constants.ts (2)
BASE_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME(481-497)V2_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME(864-873)
composition/src/v1/normalization/normalization-factory.ts (1)
composition/src/types/types.ts (1)
DirectiveName(3-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). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
composition/src/v1/federation/federation-factory.ts (2)
2820-2829: LGTM! Directive definitions propagated correctly.The addition of
directiveDefinitionByDirectiveNameto the subgraph configuration is correct and properly propagates the directive definitions from the internal subgraph representation to the public API.
3112-3121: LGTM! Contract flow also propagates directive definitions.Good consistency—the contract federation flow also correctly propagates
directiveDefinitionByDirectiveName, ensuring directive definitions are available in both normal and contract federation results.composition/src/subgraph/types.ts (1)
1-1: LGTM! Public API expanded to expose directive definitions.The changes consistently add
directiveDefinitionByDirectiveNameto both the internal (InternalSubgraph) and public (SubgraphConfig) subgraph types. The type refinement on line 37 fromMap<string, ...>toMap<DirectiveName, ...>improves semantic clarity.Note:
SubgraphConfigis a public type, so adding a required field expands the API surface. This is appropriate for exposing subgraph-defined directives to consumers.Also applies to: 9-9, 19-19, 29-29, 37-37
composition/src/v1/utils/constants.ts (1)
106-106: LGTM! Type refinements improve semantic clarity.The changes refine directive-related types from generic
string/Map<string, ...>/Set<string>to the more specificDirectiveNametype alias. This improves code readability and type safety by making the intent explicit—these are directive names, not arbitrary strings.Since
DirectiveNameis a type alias forstring(as shown in the relevant snippets), there are no runtime behavior changes, only compile-time type improvements.Also applies to: 481-497, 499-529, 864-873
composition/src/normalization/types.ts (1)
2-2: LGTM! Initialization verified.✅ Verified that
directiveDefinitionByDirectiveNameis properly initialized in the sole NormalizationSuccess return statement at composition/src/v1/normalization/normalization-factory.ts:3757. The field is correctly set tothis.directiveDefinitionByDirectiveNameand appears with all other required fields.composition/src/v1/normalization/normalization-factory.ts (4)
379-381: LGTM: type import addedImporting DirectiveName aligns types across the file.
406-410: LGTM: key type strengthened to DirectiveNameMap<DirectiveName, DirectiveDefinitionNode> keeps semantics while clarifying intent.
3574-3580: LGTM: expose directiveDefinitionByDirectiveName on NormalizationResultThis is the right place to surface it to callers.
3858-3863: LGTM: propagate directives map into InternalSubgraphThis enables per-subgraph directive introspection later in composition.
composition/tests/v1/federation-factory.test.ts (3)
2-3: LGTM: import public directive definition mapsUsing exported BASE_/V2_ maps is appropriate for assertions.
Also applies to: 16-18
527-531: LGTM: destructure to access subgraph configsMakes the test clearer and avoids repeated property access.
1149-1151: LGTM: custom directive for subgraph-bAdding @A verifies exposure of user-defined directives.
Router image scan passed✅ No security vulnerabilities found in image: |
2 participants
Summary by CodeRabbit
New Features
Refactor
Checklist