fix: propagate @openfed__requireFetchReasons on Interfaces #2256
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughAdds composite output node kinds and guards, permits REQUIRE_FETCH_REASONS on INTERFACE, centralizes field-inheritable directive handling, converts many string-keyed maps/sets to typed aliases (SubgraphName/TypeName/FieldName/DirectiveName), initializes requireFetchReasons tracking in federation nodes, and updates tests/fixtures to match renamed fields. 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 |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
composition/src/schema-building/types.ts (1)
97-118: Standardize per-subgraph collections on SubgraphName
Replace allMap<string, …>andSet<string>in per-subgraph fields withMap<SubgraphName, …>andSet<SubgraphName>in:
- composition/src/schema-building/types.ts (FieldData.externalFieldDataBySubgraphName, ScalarDefinitionData.configureDescriptionDataBySubgraphName, EntityData.keyFieldSetDatasBySubgraphName, EntityInterfaceFederationData.interfaceObjectSubgraphs)
- composition/src/federation/types.ts (MutualParentDefinitionData.configureDescriptionDataBySubgraphName)
- all instantiations across normalization, federation‐utils, schema-building utils, v1 utils and tests (e.g.
new Map<string,…>()→new Map<SubgraphName,…>(),new Set<string>()→new Set<SubgraphName>()).
This ensures consistent typing for all per-subgraph maps/sets.composition/src/v1/normalization/normalization-factory.ts (1)
606-631: Fix sticky parent-state: reset per-parent flags before scanning a composite type’s directivesdoesParentRequireFetchReasons (new) and the existing isParentObjectExternal/isParentObjectShareable are not reset per parent. Once set true on a previous type, they will leak into subsequent types without their own directives, causing incorrect field injections and unvalidated external markings.
Reset them at the start of extractDirectives for composite output types.
Apply this diff:
extractDirectives( node: | TypeDefinitionNode | TypeExtensionNode | FieldDefinitionNode | InputValueDefinitionNode | EnumValueDefinitionNode | SchemaNode, directivesByDirectiveName: Map<string, Array<ConstDirectiveNode>>, ) { + // Reset per-parent flags when visiting a new composite output type node + if (isCompositeOutputNodeKind(node.kind)) { + this.doesParentRequireFetchReasons = false; + this.isParentObjectExternal = false; + this.isParentObjectShareable = false; + } if (!node.directives) { return directivesByDirectiveName; }
🧹 Nitpick comments (6)
composition/src/v1/utils/constants.ts (1)
675-679: Fix comment: use INTERFACE (not INTERFACE_DEFINITION).The code correctly uses INTERFACE; the comment should match.
Apply:
-// directive @openfed__requireFetchReasons repeatable on FIELD_DEFINITION | INTERFACE_DEFINITION | OBJECT +// directive @openfed__requireFetchReasons repeatable on FIELD_DEFINITION | INTERFACE | OBJECTcomposition/src/v1/normalization/params.ts (1)
10-15: Good addition; consider Readonly for params.*Parameters represent inputs; ReadonlyMap/ReadonlySet/readonly arrays can reduce accidental mutation.
Example:
export type HandleFieldInheritableDirectivesParams = { readonly directivesByDirectiveName: ReadonlyMap<DirectiveName, readonly ConstDirectiveNode[]>; readonly fieldName: FieldName; readonly inheritedDirectiveNames: ReadonlySet<DirectiveName>; readonly parentData: CompositeOutputData; };composition/src/v1/federation/federation-factory.ts (1)
1141-1143: Confirm empty-set initialization for requireFetchReasonsFieldNamescopyParentDefinitionData usually copies sets from sourceData; initializing an empty Set here deviates and can drop pre-populated values. If the source already carries interface-level requireFetchReasons (from normalization), consider copying instead.
- requireFetchReasonsFieldNames: new Set<FieldName>(), + requireFetchReasonsFieldNames: new Set<FieldName>(sourceData.requireFetchReasonsFieldNames),If you adopt this, mirror the same for the OBJECT case for consistency. Please confirm the intended semantics (reset vs. copy).
composition/src/v1/utils/string-constants.ts (1)
63-68: Make COMPOSITE_OUTPUT_NODE_KINDS immutableUse ReadonlySet to prevent accidental mutation and better convey intent.
-export const COMPOSITE_OUTPUT_NODE_KINDS = new Set<Kind>([ +export const COMPOSITE_OUTPUT_NODE_KINDS: ReadonlySet<Kind> = new Set<Kind>([ Kind.INTERFACE_TYPE_DEFINITION, Kind.INTERFACE_TYPE_EXTENSION, Kind.OBJECT_TYPE_DEFINITION, Kind.OBJECT_TYPE_EXTENSION, ]);composition/src/v1/utils/utils.ts (1)
36-38: Use type-only import for CompositeOutputNodeKindAvoids unnecessary runtime import in ESM builds.
-import { CompositeOutputNodeKind } from '../../ast/utils'; +import type { CompositeOutputNodeKind } from '../../ast/utils';composition/src/v1/normalization/normalization-factory.ts (1)
627-631: Gating looks good; consider minor cleanupThe isCompositeOutputNodeKind check here is correct to narrow parent-scope logic. With the per-parent reset added above, this block is robust.
Nit: hoist a const isComposite = isCompositeOutputNodeKind(node.kind) at the top of the function and reuse to avoid recompute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
composition/src/ast/utils.ts(1 hunks)composition/src/schema-building/types.ts(13 hunks)composition/src/v1/federation/federation-factory.ts(1 hunks)composition/src/v1/normalization/directive-definition-data.ts(1 hunks)composition/src/v1/normalization/normalization-factory.ts(13 hunks)composition/src/v1/normalization/params.ts(1 hunks)composition/src/v1/normalization/walkers.ts(5 hunks)composition/src/v1/utils/constants.ts(1 hunks)composition/src/v1/utils/string-constants.ts(2 hunks)composition/src/v1/utils/utils.ts(2 hunks)composition/tests/v1/directives/require-fetch-reasons.test.ts(29 hunks)composition/tests/v1/utils/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
composition/src/v1/federation/federation-factory.ts (2)
composition/src/resolvability-graph/types/types.ts (1)
FieldName(7-7)composition/src/types/types.ts (1)
FieldName(5-5)
composition/src/v1/normalization/directive-definition-data.ts (1)
composition/src/utils/string-constants.ts (3)
FIELD_DEFINITION_UPPER(54-54)INTERFACE_UPPER(76-76)OBJECT_UPPER(105-105)
composition/src/v1/normalization/params.ts (3)
composition/src/v1/federation/params.ts (1)
ValidateOneOfDirectiveParams(5-9)composition/src/schema-building/types.ts (2)
InputObjectDefinitionData(120-132)CompositeOutputData(252-252)composition/src/types/types.ts (1)
DirectiveName(3-3)
composition/tests/v1/directives/require-fetch-reasons.test.ts (1)
composition/src/utils/string-constants.ts (1)
INTERFACE(75-75)
composition/src/schema-building/types.ts (1)
composition/src/types/types.ts (2)
SubgraphName(9-9)FieldName(5-5)
composition/src/v1/utils/constants.ts (2)
composition/src/ast/utils.ts (1)
stringArrayToNameNodeArray(84-90)composition/src/utils/string-constants.ts (3)
FIELD_DEFINITION_UPPER(54-54)INTERFACE_UPPER(76-76)OBJECT_UPPER(105-105)
composition/src/v1/utils/utils.ts (2)
composition/src/ast/utils.ts (1)
CompositeOutputNodeKind(295-295)composition/src/v1/utils/string-constants.ts (1)
COMPOSITE_OUTPUT_NODE_KINDS(63-68)
composition/src/v1/normalization/normalization-factory.ts (6)
composition/src/resolvability-graph/types/types.ts (2)
SubgraphName(13-13)FieldName(7-7)composition/src/v1/normalization/params.ts (1)
HandleFieldInheritableDirectivesParams(10-15)composition/src/utils/string-constants.ts (3)
REQUIRE_FETCH_REASONS(120-120)EXTERNAL(46-46)SHAREABLE(135-135)composition/src/utils/utils.ts (1)
generateSimpleDirective(161-166)composition/src/v1/utils/utils.ts (2)
isObjectDefinitionData(446-451)isCompositeOutputNodeKind(442-444)composition/src/schema-building/utils.ts (1)
isTypeRequired(124-126)
⏰ 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_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
composition/src/v1/normalization/directive-definition-data.ts (1)
652-652: Extending locations to INTERFACE looks correct.Matches other directives (e.g., KEY, REQUIRES_SCOPES) and aligns with the PR goal.
Please confirm normalization/walkers now read interface-level openfed__requireFetchReasons and propagate to fields as intended.
composition/tests/v1/utils/utils.ts (1)
47-55: Test schema update aligns with behavior change.Including INTERFACE here is correct.
composition/src/ast/utils.ts (1)
293-296: New kind aliases are clear and useful.Good public typing for composite output nodes.
composition/src/v1/utils/utils.ts (1)
442-444: Type guard LGTMSimple, correct set-membership check over COMPOSITE_OUTPUT_NODE_KINDS.
composition/src/v1/normalization/walkers.ts (3)
364-369: Centralizing field-inheritable directives: verify parityGood refactor. Please verify the new handler preserves prior behaviors:
- Inheritance of @Shareable and @external for both interface/object parents
- Handling/rejection rules for incompatible combinations
- Setting/propagating @openfed__requireFetchReasons to parentData.requireFetchReasonsFieldNames
485-501: Resetting doesParentRequireFetchReasons on Interface leaves is correctPrevents state leakage between interfaces and their fields/extensions.
523-526: Resetting doesParentRequireFetchReasons on Object leaves is correctConsistent with interface handling; avoids cross-parent contamination.
Also applies to: 549-552
composition/tests/v1/directives/require-fetch-reasons.test.ts (1)
29-32: Approve tests: INTERFACE export confirmed
INTERFACE is re-exported by composition/src/index.ts, so the test’s import from '../../../src' will succeed.composition/src/v1/normalization/normalization-factory.ts (5)
380-381: Import updates look correctBringing HandleFieldInheritableDirectivesParams from ./params aligns with the new handler. No issues.
410-410: New parent-level flag is fine; relies on proper resetAdding doesParentRequireFetchReasons is good but must be reset per parent (see separate comment).
Ensure tests cover: parent A (with @openfed__requireFetchReasons) followed by parent B (without it). Fields on B must not inherit the directive.
1284-1286: Interface/object tracking + config emission LGTM
- New InterfaceDefinitionData.requireFetchReasonsFieldNames Set and subsequent emission to configurationData are correct.
- Also fine for Object types and final config copy.
Confirm downstream consumers of ConfigurationData read requireFetchReasonsFieldNames and updated tests assert interface-level behavior.
Also applies to: 1364-1365, 3636-3638
1037-1039: Type-narrowing to Set/FieldName is consistentThe generics tightening across data models improves type safety. No functional concerns.
Also applies to: 1177-1177, 1234-1236, 1452-1453, 443-443, 82-85
577-604: No action – handler invocation present
The call tohandleFieldInheritableDirectivesis incomposition/src/v1/normalization/walkers.ts(around line 363) for every composite output field (objects and interfaces) beforeaddFieldDataByNode.Likely an incorrect or invalid review comment.
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/normalization/directive-definition-data.ts (1)
111-119: RenameargumentTypeNodeByArgumentNametoargumentTypeNodeByNameinExtractArgumentDataResultand its usages.
Incomposition/src/v1/normalization/types.tsandnormalization-factory.ts, update the type property and local variables toargumentTypeNodeByNamefor consistency with directive data.
♻️ Duplicate comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
577-604: Address the past review feedback: mark REQUIRE_FETCH_REASONS as referenced when injected.When
handleFieldInheritableDirectivesinjects@openfed__requireFetchReasonsinto a field (line 584), the directive is not added toreferencedDirectiveNames. Downstream at lines 3491-3493, the schema only includesREQUIRE_FETCH_REASONS_DEFINITIONifreferencedDirectiveNamescontains it. This means the injected directive won't have its definition in the emitted SDL, which will break schema validation.This issue was flagged in a previous review but has not been addressed yet.
Apply this diff to mark the directive as referenced when injecting:
handleFieldInheritableDirectives({ directivesByDirectiveName, fieldName, inheritedDirectiveNames, parentData, }: HandleFieldInheritableDirectivesParams) { if (this.doesParentRequireFetchReasons && !directivesByDirectiveName.has(REQUIRE_FETCH_REASONS)) { directivesByDirectiveName.set(REQUIRE_FETCH_REASONS, [generateSimpleDirective(REQUIRE_FETCH_REASONS)]); + this.referencedDirectiveNames.add(REQUIRE_FETCH_REASONS); inheritedDirectiveNames.add(REQUIRE_FETCH_REASONS); } if (this.doesParentRequireFetchReasons || directivesByDirectiveName.has(REQUIRE_FETCH_REASONS)) { + this.referencedDirectiveNames.add(REQUIRE_FETCH_REASONS); parentData.requireFetchReasonsFieldNames.add(fieldName); }
🧹 Nitpick comments (1)
composition/src/v1/federation/federation-factory.ts (1)
1099-1101: Copy enum values using enumValueDataByName.Naming mismatch: helper is still copyEnumValueDataByValueName while handling ByName maps. Consider renaming for clarity in a follow-up.
- copyEnumValueDataByValueName( + copyEnumValueDataByName( sourceData.enumValueDataByName, sourceData.isInaccessible, ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
composition/src/schema-building/types.ts(7 hunks)composition/src/schema-building/utils.ts(12 hunks)composition/src/v1/federation/federation-factory.ts(14 hunks)composition/src/v1/federation/utils.ts(2 hunks)composition/src/v1/normalization/directive-definition-data.ts(28 hunks)composition/src/v1/normalization/normalization-factory.ts(21 hunks)composition/src/v1/normalization/walkers.ts(6 hunks)composition/src/v1/utils/constants.ts(1 hunks)composition/src/v1/utils/string-constants.ts(2 hunks)composition/src/v1/utils/utils.ts(5 hunks)composition/tests/v1/directives/interface-object.test.ts(3 hunks)composition/tests/v1/entity-interface.test.ts(1 hunks)composition/tests/v1/types/enums.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
composition/src/v1/normalization/walkers.ts (1)
composition/src/errors/errors.ts (1)
duplicateEnumValueDefinitionError(120-122)
composition/src/v1/utils/constants.ts (2)
composition/src/ast/utils.ts (1)
stringArrayToNameNodeArray(84-90)composition/src/utils/string-constants.ts (3)
FIELD_DEFINITION_UPPER(54-54)INTERFACE_UPPER(76-76)OBJECT_UPPER(105-105)
composition/src/v1/utils/utils.ts (2)
composition/src/ast/utils.ts (1)
CompositeOutputNodeKind(295-295)composition/src/v1/utils/string-constants.ts (1)
COMPOSITE_OUTPUT_NODE_KINDS(63-68)
composition/src/v1/federation/utils.ts (3)
composition/src/types/types.ts (2)
TypeName(11-11)SubgraphName(9-9)composition/src/schema-building/types.ts (4)
AuthorizationData(309-315)EntityData(268-275)EntityInterfaceFederationData(322-330)ParentDefinitionData(242-248)composition/src/subgraph/types.ts (1)
InternalSubgraph(24-38)
composition/src/v1/normalization/normalization-factory.ts (7)
composition/src/types/types.ts (2)
SubgraphName(9-9)FieldName(5-5)composition/src/v1/normalization/params.ts (1)
HandleFieldInheritableDirectivesParams(10-15)composition/src/utils/string-constants.ts (3)
REQUIRE_FETCH_REASONS(120-120)EXTERNAL(46-46)SHAREABLE(135-135)composition/src/utils/utils.ts (1)
generateSimpleDirective(161-166)composition/src/v1/utils/utils.ts (3)
isObjectDefinitionData(446-451)isCompositeOutputNodeKind(442-444)isObjectNodeKind(438-440)composition/src/schema-building/utils.ts (2)
isTypeRequired(124-126)childMapToValueArray(233-247)composition/src/schema-building/types.ts (1)
EnumValueData(73-85)
composition/src/v1/federation/federation-factory.ts (4)
composition/src/schema-building/types.ts (1)
EnumValueData(73-85)composition/src/resolvability-graph/types/types.ts (1)
FieldName(7-7)composition/src/types/types.ts (1)
FieldName(5-5)composition/src/schema-building/ast.ts (1)
getTypeNodeNamedTypeName(281-290)
composition/src/v1/normalization/directive-definition-data.ts (3)
composition/src/schema-building/types.ts (2)
ArgumentData(30-34)DirectiveDefinitionData(41-50)composition/src/utils/string-constants.ts (4)
FIELD_DEFINITION_UPPER(54-54)INTERFACE_UPPER(76-76)OBJECT_UPPER(105-105)REQUIRE_FETCH_REASONS(120-120)composition/src/v1/utils/constants.ts (1)
REQUIRE_FETCH_REASONS_DEFINITION(676-681)
composition/src/schema-building/types.ts (2)
composition/src/types/types.ts (4)
SubgraphName(9-9)DirectiveName(3-3)TypeName(11-11)FieldName(5-5)composition/src/v1/normalization/types.ts (1)
KeyFieldSetData(14-19)
composition/src/schema-building/utils.ts (5)
composition/src/schema-building/types.ts (2)
PersistedDirectivesData(202-207)CompositeOutputData(252-252)composition/src/types/types.ts (4)
DirectiveName(3-3)TypeName(11-11)FieldName(5-5)SubgraphName(9-9)composition/src/v1/normalization/normalization-factory.ts (2)
isTypeNameRootType(489-491)getRenamedRootTypeName(1294-1307)composition/src/utils/string-constants.ts (1)
ROOT_TYPE_NAMES(170-170)composition/src/utils/types.ts (1)
InvalidRequiredInputValueData(33-37)
⏰ 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: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
🔇 Additional comments (22)
composition/tests/v1/types/enums.test.ts (1)
645-657: LGTM! Field rename aligns with typed-key refactor.The update from
enumValueDataByValueNametoenumValueDataByNameis consistent with the broader refactor introducing strongly-typed keys (SubgraphName, TypeName, FieldName, DirectiveName) across the codebase. The test logic remains unchanged—only the property accessor is updated.composition/src/schema-building/types.ts (3)
28-28: LGTM! Import enables typed-key refactor.The import of strongly-typed aliases (DirectiveName, FieldName, SubgraphName, TypeName) is essential for the typed-key refactor throughout this file. This improves type safety and clarity across the public API surface.
169-169: LGTM! New field supports interface-level fetch reasons tracking.The addition of
requireFetchReasonsFieldNames: Set<FieldName>to InterfaceDefinitionData directly supports the PR objective of propagating@openfed__requireFetchReasonson Interfaces. This mirrors the existing field in ObjectDefinitionData (line 188), maintaining consistency across composite output types.
327-327: LGTM! Rename clarifies intent and aligns with typed conventions.The rename from
interfaceObjectSubgraphstointerfaceObjectSubgraphNameswith typeSet<SubgraphName>improves clarity (it's a set of names, not full subgraph objects) and aligns with other typed-key naming conventions in the codebase (e.g.,subgraphNames,concreteTypeNames).composition/src/schema-building/utils.ts (1)
83-792: LGTM! Function signatures consistently updated with typed aliases.All function signature updates throughout this file correctly adopt the typed aliases (DirectiveName, FieldName, SubgraphName, TypeName) for Map/Set keys, replacing primitive string keys. The changes maintain existing logic while improving type safety. The iteration over
argumentDataByName(line 528) is consistent with the field rename in PersistedDirectiveDefinitionData.composition/tests/v1/directives/interface-object.test.ts (1)
777-777: LGTM! Test expectations updated for field rename.The updates from
interfaceObjectSubgraphstointerfaceObjectSubgraphNamesin test expectations are consistent with the field rename in EntityInterfaceFederationData (composition/src/schema-building/types.ts, line 327). The test logic remains unchanged—only the expected field name is updated.Also applies to: 870-870, 905-905
composition/src/v1/utils/string-constants.ts (1)
28-28: LGTM! New constant provides useful abstraction for composite output nodes.The new
COMPOSITE_OUTPUT_NODE_KINDSconstant groups interface and object definition/extension kinds, which is useful for type guards and kind checking throughout the codebase. The use ofReadonlySetensures immutability. This aligns well with theCompositeOutputDataunion type (InterfaceDefinitionData | ObjectDefinitionData) in the type system.Also applies to: 63-68
composition/src/v1/normalization/normalization-factory.ts (3)
619-640: LGTM! Composite-output directive handling is correctly gated.The addition of composite-output kind checks correctly broadens
@openfed__requireFetchReasonssupport to interfaces while maintaining object-specific handling for@externaland@shareable. The gating logic ensures:
- Only composite output types (objects and interfaces) can set
doesParentRequireFetchReasons- Only objects inherit
@externaland@shareable
443-443: LGTM! Type aliases improve code clarity.The replacement of raw
stringtypes with typed aliases (SubgraphName,FieldName) improves type documentation and makes the codebase more maintainable. Since these are type aliases (percomposition/src/types/types.ts), there's no runtime impact.Also applies to: 1039-1039, 1179-1179, 1236-1237, 1286-1287, 1366-1366, 1454-1454
410-410: doesParentRequireFetchReasons reset confirmed State is reset inwalkers.tsleave() handlers for each parent node, no further action required.composition/tests/v1/entity-interface.test.ts (1)
175-176: Rename aligns with public API (interfaceObjectSubgraphNames).Expectation matches the new field name used across federation data. Looks good.
composition/src/v1/utils/constants.ts (1)
675-681: Enable @openfed__requireFetchReasons on INTERFACE.Directive locations now include INTERFACE. This matches the PR intent and related tests.
composition/src/v1/utils/utils.ts (2)
1-2: Type-only imports and new kind guards look good.
- type imports reduce runtime footprint.
- isObjectNodeKind and isCompositeOutputNodeKind are correct and idiomatic.
Also applies to: 36-38, 438-445
151-152: No stale references tointerfaceObjectSubgraphsAll occurrences use
interfaceObjectSubgraphNames; the old identifier isn’t present in the codebase.composition/src/v1/normalization/walkers.ts (3)
485-486: Reset doesParentRequireFetchReasons on leave hooks.State reset for Interface/Object leave hooks prevents leakage across parents. Good.
Also applies to: 497-498, 524-526, 550-552
291-296: Switch to enumValueDataByName map verified. No remaining enumValueDataByValueName usages found.
363-368: Field-inheritable directives handler covers interface-level requireFetchReasons
The normalization-factory sets doesParentRequireFetchReasons for interfaces and handleFieldInheritableDirectives adds and tracks @openfed__requireFetchReasons on fields, with the flag reset in InterfaceTypeDefinition.leave.composition/src/v1/federation/federation-factory.ts (4)
597-615: Enum values merge updated to enumValueDataByName.Upsert path correctly targets the new map; propagations intact.
1141-1161: Initialize requireFetchReasonsFieldNames on federated nodes.Placeholder Set is fine and clearly annotated as unused for federated nodes.
1213-1226: Handle interface-object subgraph shape via interfaceObjectSubgraphNames.Promotion of OBJECT defs to INTERFACE when sourced from interface-object subgraphs is correct and keyed to subgraph membership.
Also applies to: 1232-1234
1607-1612: Iterate interfaceObjectSubgraphNames when wiring interface-objects.Loop updated to new field; downstream config propagation remains consistent.
composition/src/v1/normalization/directive-definition-data.ts (1)
649-657: Broaden REQUIRE_FETCH_REASONS to INTERFACE in normalization metadata.Locations set aligns with constants.ts; normalization should now accept INTERFACE placements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
577-604: Add injected directive to referencedDirectiveNames.When
handleFieldInheritableDirectivesinjects@openfed__requireFetchReasons(lines 583–586), it does not add the directive toreferencedDirectiveNames. Downstream (line 3491), the schema only emitsREQUIRE_FETCH_REASONS_DEFINITIONif the directive is inreferencedDirectiveNames. If the directive appears only via injection and not in the original AST, its definition will be missing from the output.Apply this diff to ensure the definition is emitted:
if (this.doesParentRequireFetchReasons && !directivesByDirectiveName.has(REQUIRE_FETCH_REASONS)) { directivesByDirectiveName.set(REQUIRE_FETCH_REASONS, [generateSimpleDirective(REQUIRE_FETCH_REASONS)]); + this.referencedDirectiveNames.add(REQUIRE_FETCH_REASONS); inheritedDirectiveNames.add(REQUIRE_FETCH_REASONS); }Defensively, also add it when the directive already exists on the field:
if (this.doesParentRequireFetchReasons || directivesByDirectiveName.has(REQUIRE_FETCH_REASONS)) { + this.referencedDirectiveNames.add(REQUIRE_FETCH_REASONS); parentData.requireFetchReasonsFieldNames.add(fieldName); }
🧹 Nitpick comments (1)
composition/src/v1/federation/federation-factory.ts (1)
1138-1139: Clarify the purpose of initializing unused fields.The
requireFetchReasonsFieldNamesfield is initialized with an emptySet<FieldName>()and marked with a comment stating it's "not required for the federated node." This could be confusing for future maintainers.Consider either:
- Adding more context to the comment explaining why it must be initialized (e.g., "required by type definition" or "used in other contexts")
- Making the field optional if it's truly not needed in this context
Based on learnings.
Also applies to: 1156-1157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composition/src/v1/federation/federation-factory.ts(15 hunks)composition/src/v1/normalization/normalization-factory.ts(24 hunks)composition/src/v1/normalization/types.ts(1 hunks)composition/src/v1/utils/utils.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
composition/src/v1/normalization/types.ts (1)
composition/src/schema-building/types.ts (1)
ArgumentData(30-34)
composition/src/v1/normalization/normalization-factory.ts (7)
composition/src/types/types.ts (2)
SubgraphName(9-9)FieldName(5-5)composition/src/v1/normalization/params.ts (1)
HandleFieldInheritableDirectivesParams(10-15)composition/src/utils/string-constants.ts (3)
REQUIRE_FETCH_REASONS(120-120)EXTERNAL(46-46)SHAREABLE(135-135)composition/src/utils/utils.ts (1)
generateSimpleDirective(161-166)composition/src/v1/utils/utils.ts (3)
isObjectDefinitionData(447-452)isCompositeOutputNodeKind(443-445)isObjectNodeKind(439-441)composition/src/schema-building/types.ts (2)
ArgumentData(30-34)EnumValueData(73-85)composition/src/schema-building/utils.ts (1)
childMapToValueArray(233-247)
composition/src/v1/utils/utils.ts (5)
composition/src/types/types.ts (2)
SubgraphName(9-9)TypeName(11-11)composition/src/schema-building/types.ts (1)
EntityData(268-275)composition/src/v1/normalization/types.ts (1)
KeyFieldSetData(14-19)composition/src/ast/utils.ts (1)
CompositeOutputNodeKind(295-295)composition/src/v1/utils/string-constants.ts (1)
COMPOSITE_OUTPUT_NODE_KINDS(63-68)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/schema-building/types.ts (1)
EnumValueData(73-85)composition/src/resolvability-graph/types/types.ts (1)
FieldName(7-7)composition/src/schema-building/ast.ts (1)
getTypeNodeNamedTypeName(281-290)
⏰ 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_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
🔇 Additional comments (12)
composition/src/v1/utils/utils.ts (4)
1-2: LGTM! Type-only imports improve build output.Using type-only imports for
DocumentNodeandFieldConfigurationis correct since these are only used in type annotations and should not generate runtime imports.
36-38: LGTM! Typed aliases improve type safety.The addition of
SubgraphNameandTypeNametyped aliases, along withCompositeOutputNodeKindandCOMPOSITE_OUTPUT_NODE_KINDS, supports the PR's objective of improving type safety across the codebase.
439-445: Rename complete; new type guard implementation is correct.
152-169: Rename is consistent across codebase
No occurrences ofinterfaceObjectSubgraphsremain;interfaceObjectSubgraphNamesis applied uniformly.composition/src/v1/normalization/types.ts (1)
37-37: LGTM! More concise naming.The rename from
argumentTypeNodeByArgumentNametoargumentTypeNodeByNameremoves redundancy and aligns with the naming pattern used elsewhere in the codebase (e.g.,enumValueDataByName).composition/src/v1/federation/federation-factory.ts (4)
597-614: LGTM! Consistent naming improvements.The parameter rename from
enumValueDataByValueNametoenumValueDataByNameremoves redundancy and aligns with similar naming patterns likeargumentDataByName. The function correctly handles the renamed parameter.
1057-1071: LGTM! Function rename matches parameter updates.The function rename from
copyEnumValueDataByValueNametocopyEnumValueDataByNameis consistent with the parameter renamings and improves code clarity.
1219-1230: LGTM! Consistent naming improvements.The rename from
interfaceObjectSubgraphstointerfaceObjectSubgraphNamesis consistent with the changes incomposition/src/v1/utils/utils.tsand improves naming clarity.
1342-1369: LGTM! Consistent parameter naming.The rename from
argumentDataByArgumentNametoargumentDataByNamethroughout theupsertPersistedDirectiveDefinitionDatamethod removes redundancy and aligns with the naming pattern used for similar maps.composition/src/v1/normalization/normalization-factory.ts (3)
82-84: LGTM! Type safety and naming improvements.The imported type guards (
isCompositeOutputNodeKind,isObjectDefinitionData,isObjectNodeKind) enable proper differentiation between composite output types (interfaces and objects) and objects specifically. The renaming ofdoesParentObjectRequireFetchReasonstodoesParentRequireFetchReasonsaccurately reflects that the directive now propagates to interfaces as well. TheSubgraphNametyping improves type safety.Also applies to: 380-380, 410-410, 443-443
619-640: LGTM! Correctly differentiates composite vs. object-only directives.The logic properly detects
REQUIRE_FETCH_REASONSon all composite output types (interfaces and objects) while restrictingEXTERNALandSHAREABLEdetection to objects only. This aligns with the federation specification and enables correct propagation inhandleFieldInheritableDirectives.
1286-1287: LGTM! Properly tracks and emits requireFetchReasonsFieldNames.The field set is correctly initialized on both interface and object definition data, then transferred to the configuration data when non-empty. This completes the feature implementation for tracking fields with
@openfed__requireFetchReasons.Also applies to: 1364-1364, 3638-3640
Summary by CodeRabbit
New Features
Refactor
Tests
Checklist