feat: implement openfed__requireFetchReasons#2170
Conversation
WalkthroughAdds a repeatable directive openfed__requireFetchReasons, threads its definition and propagation through normalization and schema-building, records per-object fields that require fetch reasons, tightens string types to FieldName/TypeName across composition and config, updates protobuf/TS models, and adds tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (16)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
ysmolski
left a comment
ysmolski
left a comment
There was a problem hiding this comment.
It looks good. I have integrated engine and router to use this change. Thank you!
ysmolski
requested review from
JivusAyrus,
Noroth,
StarpTech,
jensneuse and
wilsonrivera
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
proto/wg/cosmo/node/v1/node.proto (1)
140-145: Documentprotected_field_namesin the proto and regenerate all clients/serversPlease add a concise comment in
proto/wg/cosmo/node/v1/node.protodescribing the semantics ofprotected_field_names, then run your protobuf generation pipeline to update all affected artifacts (Go, JS/TS, Connect code). The field is already in use across tests and generated code, so we need to ensure the new comment makes its way into every client.• File to update:
proto/wg/cosmo/node/v1/node.proto(line 144, before field 4)• Steps:
- Add the following comment above
protected_field_namesin the proto.- Re-run
protoc(and any codegen scripts) to regenerate:
- Go:
router/gen/proto/wg/cosmo/node/v1/node.pb.go- TS:
connect/src/wg/cosmo/node/v1/node_pb.ts- Any other generated clients/servers.
- Commit the updated generated files.
Diff to apply in
proto/wg/cosmo/node/v1/node.proto:message TypeField { string type_name = 1; repeated string field_names = 2; repeated string external_field_names = 3; + // Fields on this type marked with @openfed__protected. + // List is empty if no fields are protected. repeated string protected_field_names = 4; }composition/src/v1/normalization/normalization-factory.ts (2)
3417-3450: protectedFieldNames never populated; configuration remains empty.You copy parentData.protectedFieldNames into configuration, but nothing adds field names to that Set. Populate it while iterating fields.
for (const [fieldName, fieldData] of parentData.fieldDataByName) { if (!isObject && fieldData.externalFieldDataBySubgraphName.get(this.subgraphName)?.isDefinedExternal) { externalInterfaceFieldNames.push(fieldName); } + // Track protected fields on object types + if (isObject && fieldData.directivesByDirectiveName.has(PROTECTED)) { + (parentData as ObjectDefinitionData).protectedFieldNames.add(fieldName as FieldName); + } // Arguments can only be fully validated once all parents types are known this.validateArguments(fieldData, parentData.kind);
587-596: Sticky inheritance flags cause cross-type leakage. Reset for each parent object.isParentObjectProtected (and peers) are never reset; a protected object will incorrectly mark subsequent objects’ fields as protected. Reset at object entry in extractDirectives.
) { - if (!node.directives) { + if (!node.directives) { return directivesByDirectiveName; } + // Reset inheritable flags per parent object + if (isNodeKindObject(node.kind)) { + this.isParentObjectExternal = false; + this.isParentObjectProtected = false; + this.isParentObjectShareable = false; + } for (const directiveNode of node.directives) {
🧹 Nitpick comments (6)
composition/src/router-configuration/types.ts (1)
84-95: Consider deduping protectedFieldNames to avoid drift.
ConfigurationData mixes Set (fieldNames, externalFieldNames) with Array (protectedFieldNames). Keep Array if preferred, but ensure consumers dedupe to avoid duplicates in emitted configs. I’ve proposed a small downstream change where it’s consumed.Proposed consumer change (see shared/src/router-config/graphql-configuration.ts lines 132-134).
shared/src/router-config/graphql-configuration.ts (1)
132-134: Deduplicate and stabilize ordering of protectedFieldNames.
Helps produce deterministic configs and avoids accidental duplicates from upstream.Apply this diff:
- if (data.protectedFieldNames && data.protectedFieldNames.length > 0) { - typeField.protectedFieldNames = [...data.protectedFieldNames]; - } + if (data.protectedFieldNames && data.protectedFieldNames.length > 0) { + const protectedFieldNames = Array.from(new Set(data.protectedFieldNames)).sort(); + typeField.protectedFieldNames = protectedFieldNames; + }composition/src/v1/normalization/walkers.ts (1)
366-374: Add protected-field tracking and update the comment to reflect PROTECTED.Logic is correct: apply when parent is object and either parent is protected or field is protected. Update the comment to avoid future confusion.
Suggested in-range comment tweak:
-// Add parent-level shareable and external to the field extraction and repeatable validation +// Add parent-level shareable, external, and protected to the field extraction and repeatable validationcomposition/tests/v1/directives/protected.test.ts (2)
254-338: Add a regression guard to ensure PROTECTED definition is emitted exactly once.To prevent accidental duplicate directive-definition emission, add an assertion that counts a single occurrence of "directive @openfed__protected" in each subgraph SDL.
I can add a small helper to assert “occursExactlyOnce(schemaSDL, 'directive @openfed__protected')”.
341-349: Use the project parse wrapper for consistency and noLocation defaults.Prefer importing parse from composition/src/ast/utils to align with other tests and avoid location noise.
-import { parse } from 'graphql'; +import { parse } from '../../../src';composition/src/v1/normalization/normalization-factory.ts (1)
3479-3482: Optional: ensure deterministic ordering of protectedFieldNames.Sorting yields stable output across environments and avoids order flakiness if Set insertion varies.
- if (isObject && parentData.protectedFieldNames.size > 0) { - configurationData.protectedFieldNames = [...parentData.protectedFieldNames]; - } + if (isObject && parentData.protectedFieldNames.size > 0) { + configurationData.protectedFieldNames = [...parentData.protectedFieldNames].sort(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (19)
composition/src/router-configuration/types.ts(2 hunks)composition/src/router-configuration/utils.ts(2 hunks)composition/src/schema-building/types.ts(1 hunks)composition/src/schema-building/utils.ts(1 hunks)composition/src/subgraph/types.ts(2 hunks)composition/src/utils/string-constants.ts(2 hunks)composition/src/v1/federation/federation-factory.ts(2 hunks)composition/src/v1/normalization/directive-definition-data.ts(3 hunks)composition/src/v1/normalization/normalization-factory.ts(11 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/src/v1/normalization/walkers.ts(5 hunks)composition/src/v1/utils/constants.ts(3 hunks)composition/src/v1/utils/utils.ts(0 hunks)composition/tests/v1/directives/protected.test.ts(1 hunks)composition/tests/v1/directives/shareable.test.ts(0 hunks)composition/tests/v1/utils/utils.ts(1 hunks)connect/src/wg/cosmo/node/v1/node_pb.ts(2 hunks)proto/wg/cosmo/node/v1/node.proto(1 hunks)shared/src/router-config/graphql-configuration.ts(1 hunks)
💤 Files with no reviewable changes (2)
- composition/src/v1/utils/utils.ts
- composition/tests/v1/directives/shareable.test.ts
🧰 Additional context used
🧬 Code graph analysis (13)
composition/src/schema-building/utils.ts (1)
composition/src/schema-building/types.ts (2)
ParentDefinitionData(240-246)InterfaceDefinitionData(156-170)
composition/src/subgraph/types.ts (3)
composition/src/types/types.ts (1)
TypeName(9-9)composition/src/router-configuration/types.ts (1)
ConfigurationData(83-95)composition/src/schema-building/types.ts (1)
ParentDefinitionData(240-246)
composition/src/schema-building/types.ts (1)
composition/src/types/types.ts (1)
FieldName(3-3)
composition/tests/v1/directives/protected.test.ts (8)
composition/tests/utils/utils.ts (3)
federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)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 (3)
versionOneRouterDefinitions(106-106)schemaQueryDefinition(84-87)baseDirectiveDefinitionsWithProtected(37-45)composition/src/types/types.ts (2)
TypeName(9-9)FieldName(3-3)composition/src/router-configuration/types.ts (1)
ConfigurationData(83-95)composition/src/utils/string-constants.ts (1)
QUERY(110-110)composition/src/subgraph/types.ts (1)
Subgraph(11-15)composition/src/ast/utils.ts (1)
parse(272-274)
shared/src/router-config/graphql-configuration.ts (1)
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
TypeField(1085-1135)
composition/src/router-configuration/utils.ts (1)
composition/src/types/types.ts (1)
FieldName(3-3)
composition/src/v1/federation/federation-factory.ts (1)
composition/src/types/types.ts (1)
FieldName(3-3)
composition/src/v1/normalization/walkers.ts (2)
composition/src/schema-building/utils.ts (1)
isInterfaceDefinitionData(261-263)composition/src/utils/string-constants.ts (2)
EXTERNAL(42-42)PROTECTED(106-106)
composition/src/v1/normalization/directive-definition-data.ts (3)
composition/src/schema-building/types.ts (2)
DirectiveDefinitionData(41-50)ArgumentData(30-34)composition/src/utils/string-constants.ts (3)
FIELD_DEFINITION_UPPER(50-50)OBJECT_UPPER(99-99)PROTECTED(106-106)composition/src/v1/utils/constants.ts (1)
PROTECTED_DEFINITION(663-668)
composition/src/router-configuration/types.ts (1)
composition/src/types/types.ts (2)
FieldName(3-3)TypeName(9-9)
composition/src/v1/normalization/utils.ts (2)
composition/src/utils/string-constants.ts (1)
PROTECTED(106-106)composition/src/v1/normalization/directive-definition-data.ts (1)
PROTECTED_DEFINITION_DATA(444-452)
composition/src/v1/normalization/normalization-factory.ts (4)
composition/src/utils/string-constants.ts (2)
PROTECTED(106-106)SHAREABLE(128-128)composition/src/utils/utils.ts (1)
generateSimpleDirective(159-164)composition/src/types/types.ts (1)
FieldName(3-3)composition/src/v1/utils/constants.ts (1)
PROTECTED_DEFINITION(663-668)
composition/src/v1/utils/constants.ts (2)
composition/src/utils/string-constants.ts (3)
PROTECTED(106-106)FIELD_DEFINITION_UPPER(50-50)OBJECT_UPPER(99-99)composition/src/ast/utils.ts (2)
stringArrayToNameNodeArray(84-90)stringToNameNode(77-82)
⏰ 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). (2)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
🔇 Additional comments (28)
composition/src/schema-building/types.ts (1)
185-185: No changes needed for protectedFieldNames initialization and mergingI ran the suggested checks and confirmed that:
- Both the federation and normalization factories initialize
protectedFieldNamesas a newSet<FieldName>(normalization-factory.ts:1302; federation-factory.ts:1135).- Merge operations correctly union the sets via
.add(...)in walkers (walkers.ts:372) and by spreading into a new array (configurationData.protectedFieldNames = [...parentData.protectedFieldNames]) in the normalization factory (normalization-factory.ts:3480).All constructors and mergers properly handle
protectedFieldNames, so there’s no accidental data loss to address here.composition/src/router-configuration/utils.ts (2)
2-2: Type alignment LGTMImporting FieldName tightens typing without runtime impact.
16-16: Set typing LGTMfieldNames: new Set() aligns with updated ConfigurationData.
composition/src/utils/string-constants.ts (2)
106-106: New directive constant PROTECTED LGTMName aligns with directive usage elsewhere.
169-169: PROTECTED fully integrated — LGTMAll references to PROTECTED are present and correctly wired through the inheritance mechanism:
- The INHERITABLE_DIRECTIVE_NAMES set in composition/src/utils/string-constants.ts now includes PROTECTED, and this is the same set consumed by the v1 normalization factory to filter out inherited directives.
- PROTECTED is declared in the v1 constants (composition/src/v1/utils/constants.ts) and has its corresponding PROTECTED_DEFINITION and PROTECTED_DEFINITION_DATA in directive-definition-data.ts, ensuring it’s recognized as a first-class directive.
- The normalization pipeline (walkers.ts and normalization-factory.ts) both reference PROTECTED when tracking parent protection state, auto-injecting the directive where needed, and adding its definition in generated output.
No further changes required. Approved.
composition/src/schema-building/utils.ts (1)
261-263: Verification complete: Type guard relocation and imports are correct
- No occurrences of the old predicate
isParentDataInterfaceTyperemain in composition/src.- In composition/src/v1/normalization/walkers.ts,
isInterfaceDefinitionDatais imported from../../schema-building/utils(lines 24–28).composition/src/subgraph/types.ts (2)
9-9: Importing TypeName is appropriate.
Aligns this module with the new keyed-by-TypeName maps.
18-21: Switch to Map<TypeName, …> looks good.
TypeName aliases string, so this is type-safe and non-breaking.composition/src/router-configuration/types.ts (2)
1-1: Stronger typing via FieldName/TypeName import is correct.
Improves clarity without runtime impact.
77-81: RequiredFieldConfiguration.fieldName → FieldName is fine.
Alias to string; call sites should already type-check.shared/src/router-config/graphql-configuration.ts (1)
128-128: Constructor arg order change is harmless.
Object-literal init ignores order; usage is correct.connect/src/wg/cosmo/node/v1/node_pb.ts (2)
1117-1118: FieldList updated with tag 4 — backward compatible.
No wire conflicts; older readers ignore unknown field.
1101-1105: Verified generated field:protectedFieldNames
Theprotected_field_names = 4;declaration innode.protoand the generation marker innode_pb.tsconfirm this is an auto-generated field. No manual edits are needed.•
node.proto(line 144):repeated string protected_field_names = 4;
•connect/src/wg/cosmo/node/v1/node_pb.ts(line 1):// @generated by protoc-gen-es v1.10.0 with parameter "target=ts"composition/src/v1/federation/federation-factory.ts (1)
234-234: Importing FieldName is fine.
Prepares for protectedFieldNames typing.composition/src/v1/utils/constants.ts (1)
101-102: Import of PROTECTED constant looks correct.Matches downstream usages and directive definition.
composition/src/v1/normalization/directive-definition-data.ts (1)
444-452: Directive definition data for PROTECTED is consistent.No args, repeatable, FIELD_DEFINITION | OBJECT, and wired to PROTECTED_DEFINITION.
composition/tests/v1/utils/utils.ts (1)
37-45: Test fixture for base definitions with PROTECTED is accurate.Locations and repeatable match the implementation.
composition/src/v1/normalization/walkers.ts (3)
528-530: Resetting isParentObjectProtected on leave is correct.Prevents bleed-over between object scopes.
Also applies to: 554-556
26-27: Switch to isInterfaceDefinitionData is appropriate.Keeps interface parents excluded from object-only inheritance paths.
42-43: Import of PROTECTED constant aligns with new usage.All good.
composition/src/v1/normalization/utils.ts (1)
50-51: Wiring PROTECTED into directive-definition catalog is correct.Ensures validation and extraction treat it as first-class.
Also applies to: 74-75, 409-410
composition/tests/v1/directives/protected.test.ts (3)
18-49: Solid coverage for field-level propagation.Assertions for router SDL and per-subgraph ConfigurationData look correct.
271-291: Great: explicit check of subgraph SDL with baseDirectiveDefinitionsWithProtected.This precisely validates inclusion of the PROTECTED directive definition when referenced.
292-337: Map/Set expectations are clear and deterministic.Using Set for fieldNames and Array for protectedFieldNames matches router-configuration types.
composition/src/v1/normalization/normalization-factory.ts (4)
3344-3346: Keep single-source emission here.Centralizing PROTECTED_DEFINITION emission in normalize() is correct.
1261-1264: Good: safer nullish coalescing when extracting parent directives.Prevents accidental fallback when an empty Map is valid.
566-573: Inheritance of PROTECTED to fields is correct.Adding the directive when the parent object is protected aligns with the intended semantics.
361-362: Type import used appropriately.Using FieldName for the per-object Set is consistent with downstream types.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
composition/src/v1/normalization/normalization-factory.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
composition/src/v1/normalization/normalization-factory.ts (4)
composition/src/utils/string-constants.ts (1)
PROTECTED(106-106)composition/src/utils/utils.ts (1)
generateSimpleDirective(159-164)composition/src/types/types.ts (1)
FieldName(3-3)composition/src/v1/utils/constants.ts (1)
PROTECTED_DEFINITION(663-668)
⏰ 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). (16)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
composition/src/v1/normalization/normalization-factory.ts (7)
3329-3333: Gated LINK directive emission: LGTM.Reintroducing LINK_DEFINITION/IMPORT/PURPOSE only when LINK is referenced is consistent with prior patterns.
1263-1264: Nullish coalescing for parent directives: LGTM.Safer than || for existing empty maps.
67-68: Imports wired correctly.PROTECTED_DEFINITION, PROTECTED, and FieldName imports align with their usages.
Also applies to: 307-308, 361-362
3340-3343: ✅ Single PROTECTED_DEFINITION emission verifiedRipgrep search for
definitions.push(PROTECTED_DEFINITION)returns exactly one match:
- composition/src/v1/normalization/normalization-factory.ts:3341
No other occurrences found, so there are no duplicate directive definitions.
566-573: Resolved:INHERITABLE_DIRECTIVE_NAMESalready includesPROTECTED.The grep output confirms that in
composition/src/utils/string-constants.tsline 169,INHERITABLE_DIRECTIVE_NAMESis defined as:export const INHERITABLE_DIRECTIVE_NAMES = new Set<string>([EXTERNAL, PROTECTED, SHAREABLE]);Since
PROTECTEDis already present, no changes are needed.
3475-3477: Config propagation verified – TheprotectedFieldNamesarray is correctly consumed downstream in the router configuration:• In
shared/src/router-config/graphql-configuration.ts(lines 132–134), the code checks fordata.protectedFieldNames.length > 0and spreads it intotypeField.protectedFieldNames: string[].
• The generated protobuf definitions (.pb.tsand Go code) exposeprotectedFieldNamesasstring[].No further action needed.
1302-1303: Protected fields are populated end-to-end
- In the normalization walker (composition/src/v1/normalization/walkers.ts:372), protected fields are added to the parent’s set when
@protectedis present:
parentData.protectedFieldNames.add(fieldName);- The factory then spreads these into the configuration object (normalization-factory.ts), ensuring the set is carried forward.
- Finally, in the GraphQL router config (shared/src/router-config/graphql-configuration.ts:133), the protection set is applied to each field:
typeField.protectedFieldNames = [...data.protectedFieldNames];All population steps—from directive detection to final configuration—are in place.
|
@Aenimus should we review this PR as it is or some additional work needs to be done here? |
Aenimus
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
proto/wg/cosmo/node/v1/node.proto (1)
140-145: Additive proto change looks good; add brief comment for clarity.Adding require_fetch_reasons_field_names = 4 is backward-compatible and aligns with generated TS. Consider documenting intended semantics on this field for future readers.
composition/tests/v1/directives/require-fetch-reasons.test.ts (1)
18-339: Solid positive-path coverage; add a couple of negative tests.Consider adding:
- misuse on unsupported locations (e.g., INTERFACE/INPUT_OBJECT) → expect composition to fail with invalid location.
- repeatable application on the same field (twice) → expect no duplication in config (still one field entry).
Low effort, increases guardrail coverage.
I can draft these tests if you want.
composition/src/v1/normalization/normalization-factory.ts (1)
3475-3477: Deterministic ordering for requireFetchReasonsFieldNames.For stable configs across runs, sort before assigning.
Apply:
- if (isObject && parentData.requireFetchReasonsFieldNames.size > 0) { - configurationData.requireFetchReasonsFieldNames = [...parentData.requireFetchReasonsFieldNames]; + if (isObject && parentData.requireFetchReasonsFieldNames.size > 0) { + configurationData.requireFetchReasonsFieldNames = [...parentData.requireFetchReasonsFieldNames].sort(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (14)
composition/src/router-configuration/types.ts(2 hunks)composition/src/schema-building/types.ts(1 hunks)composition/src/utils/string-constants.ts(2 hunks)composition/src/v1/federation/federation-factory.ts(2 hunks)composition/src/v1/normalization/directive-definition-data.ts(5 hunks)composition/src/v1/normalization/normalization-factory.ts(10 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/src/v1/normalization/walkers.ts(5 hunks)composition/src/v1/utils/constants.ts(5 hunks)composition/tests/v1/directives/require-fetch-reasons.test.ts(1 hunks)composition/tests/v1/utils/utils.ts(1 hunks)connect/src/wg/cosmo/node/v1/node_pb.ts(4 hunks)proto/wg/cosmo/node/v1/node.proto(1 hunks)shared/src/router-config/graphql-configuration.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- composition/src/utils/string-constants.ts
- composition/src/v1/federation/federation-factory.ts
- composition/src/router-configuration/types.ts
- composition/tests/v1/utils/utils.ts
- composition/src/schema-building/types.ts
🧰 Additional context used
🧬 Code graph analysis (7)
composition/src/v1/utils/constants.ts (2)
composition/src/utils/string-constants.ts (3)
REQUIRE_FETCH_REASONS(106-106)FIELD_DEFINITION_UPPER(50-50)OBJECT_UPPER(99-99)composition/src/ast/utils.ts (2)
stringArrayToNameNodeArray(84-90)stringToNameNode(77-82)
composition/src/v1/normalization/utils.ts (2)
composition/src/utils/string-constants.ts (1)
REQUIRE_FETCH_REASONS(106-106)composition/src/v1/normalization/directive-definition-data.ts (1)
REQUIRE_FETCH_REASONS_DEFINITION_DATA(444-452)
shared/src/router-config/graphql-configuration.ts (1)
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
TypeField(1092-1142)
composition/src/v1/normalization/normalization-factory.ts (4)
composition/src/utils/string-constants.ts (5)
REQUIRE_FETCH_REASONS(106-106)SHAREABLE(128-128)LINK(76-76)CONFIGURE_DESCRIPTION(13-13)CONFIGURE_CHILD_DESCRIPTIONS(14-14)composition/src/utils/utils.ts (1)
generateSimpleDirective(159-164)composition/src/types/types.ts (1)
FieldName(3-3)composition/src/v1/utils/constants.ts (6)
LINK_DEFINITION(609-642)LINK_IMPORT_DEFINITION(586-589)LINK_PURPOSE_DEFINITION(591-606)CONFIGURE_DESCRIPTION_DEFINITION(918-956)CONFIGURE_CHILD_DESCRIPTIONS_DEFINITION(963-983)REQUIRE_FETCH_REASONS_DEFINITION(663-668)
composition/src/v1/normalization/directive-definition-data.ts (3)
composition/src/schema-building/types.ts (2)
DirectiveDefinitionData(41-50)ArgumentData(30-34)composition/src/utils/string-constants.ts (3)
FIELD_DEFINITION_UPPER(50-50)OBJECT_UPPER(99-99)REQUIRE_FETCH_REASONS(106-106)composition/src/v1/utils/constants.ts (1)
REQUIRE_FETCH_REASONS_DEFINITION(663-668)
composition/src/v1/normalization/walkers.ts (2)
composition/src/schema-building/utils.ts (1)
isInterfaceDefinitionData(261-263)composition/src/utils/string-constants.ts (2)
EXTERNAL(42-42)REQUIRE_FETCH_REASONS(106-106)
composition/tests/v1/directives/require-fetch-reasons.test.ts (7)
composition/tests/utils/utils.ts (3)
federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)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 (3)
versionOneRouterDefinitions(106-106)schemaQueryDefinition(84-87)baseDirectiveDefinitionsWithRequireFetchReasons(37-45)composition/src/types/types.ts (2)
TypeName(9-9)FieldName(3-3)composition/src/router-configuration/types.ts (1)
ConfigurationData(83-95)composition/src/utils/string-constants.ts (1)
QUERY(110-110)composition/src/ast/utils.ts (1)
parse(272-274)
🪛 GitHub Actions: wgc CI
connect/src/wg/cosmo/node/v1/node_pb.ts
[error] 3-18: Git diff check failed. Differences detected in connect/src/wg/cosmo/node/v1/node_pb.ts (exit code 1). Command: 'git diff --no-ext-diff --exit-code'.
🪛 GitHub Actions: Composition CI
connect/src/wg/cosmo/node/v1/node_pb.ts
[error] 3-9: Command 'git diff --no-ext-diff --exit-code' failed (exit code 1) due to unstaged changes in connect/src/wg/cosmo/node/v1/node_pb.ts (top import block).
[error] 2783-2789: Command 'git diff --no-ext-diff --exit-code' failed (exit code 1) due to unstaged changes in connect/src/wg/cosmo/node/v1/node_pb.ts (GraphQLSubscriptionConfiguration docblock).
🪛 GitHub Actions: Shared CI
connect/src/wg/cosmo/node/v1/node_pb.ts
[error] 3-11: git diff --no-ext-diff --exit-code failed: exit code 1 due to differences in the working tree (connect/src/wg/cosmo/node/v1/node_pb.ts).
⏰ 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: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (20)
shared/src/router-config/graphql-configuration.ts (2)
30-30: Import change is fine.Including PROVIDER_TYPE_REDIS here is consistent with usage below.
126-135: Remove unnecessary Set guard
requireFetchReasonsFieldNamesis defined asFieldName[]inrouter-configuration/types.ts(line 93), so it can never be aSetand the existing.lengthcheck and spread are safe.Likely an incorrect or invalid review comment.
connect/src/wg/cosmo/node/v1/node_pb.ts (1)
1108-1112: TypeField: generated property and field descriptor match proto.requireFetchReasonsFieldNames and tag no. 4 align with the proto. No issues.
Also applies to: 1121-1125
composition/src/v1/utils/constants.ts (3)
23-25: New string constants imported: OK.
40-42: EDFS Redis imports: OK.
76-77: Import of REQUIRE_FETCH_REASONS: OK.composition/src/v1/normalization/walkers.ts (4)
25-26: Updated type guard import: OK.
43-44: Import directive name constant: OK.
366-374: Correctly track fields requiring fetch reasons.Adding the field name when the parent object or field is annotated is the right spot; limited to non-interface parents. Looks good.
If not already, ensure ObjectDefinitionData initializes requireFetchReasonsFieldNames = new Set() in upsertObjectDataByNode.
528-529: State reset on object leaves: OK.Clearing doesParentObjectRequireFetchReasons avoids leakage across objects.
Also applies to: 554-555
composition/src/v1/normalization/directive-definition-data.ts (2)
23-23: Wiring in REQUIRE_FETCH_REASONS definition — looks good.Import added correctly and consistent with usage below.
444-452: Directive data for @openfed__requireFetchReasons — correct shape and locations.
- repeatable: true
- locations: FIELD_DEFINITION, OBJECT
- no args
All aligned with the constant definition.
composition/src/v1/normalization/utils.ts (1)
409-410: Include REQUIRE_FETCH_REASONS in the initialization map.Good addition; ensures normalization recognizes and validates the directive.
composition/tests/v1/directives/require-fetch-reasons.test.ts (1)
341-485: Subgraph fixtures are clear and target the propagation matrix well.Nice spread of object-level, extension-level, and mixed cases across multiple subgraphs.
composition/src/v1/normalization/normalization-factory.ts (6)
566-574: Correct inheritance: object-level @openfed__requireFetchReasons applied to fields.This mirrors existing EXTERNAL/SHAREABLE inheritance logic.
1299-1306: Track field names on the parent object — good.requireFetchReasonsFieldNames: Set() is the right container for later emission.
3330-3333: Emitting LINK only when referenced — good optimization.*This avoids unnecessary SDL noise.
3341-3342: Conditionally emit REQUIRE_FETCH_REASONS_DEFINITION — correct.Keeps SDL minimal while enabling the directive when used.
1299-1306: Resolved: walkers.ts includesparentData.requireFetchReasonsFieldNames.add(fieldName)(L371–372), ensuring annotated or inherited fields are recorded.
3341-3342: No action needed:INHERITABLE_DIRECTIVE_NAMESalready includesREQUIRE_FETCH_REASONS(composition/src/utils/string-constants.ts:169).
4 participants
Updates ENG-7769
Summary by CodeRabbit
New Features
Refactor
Tests
Checklist