fix: propagate @external interface object fields#2145
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech and
jensneuse
as code owners
WalkthroughTracks per-subgraph defined and required concrete types for entity interfaces, renames Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 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). (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
…d-propagate-external
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: 2
📜 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 (5)
composition/src/errors/errors.ts(2 hunks)composition/src/utils/types.ts(1 hunks)composition/src/v1/federation/federation-factory.ts(5 hunks)composition/tests/v1/directives/interface-object.test.ts(3 hunks)composition/tests/v1/entity-interface.test.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
composition/src/errors/errors.ts (1)
composition/src/utils/utils.ts (1)
getEntriesNotInHashSet(31-39)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/types/types.ts (2)
SubgraphName(7-7)TypeName(9-9)composition/src/utils/utils.ts (2)
getValueOrDefault(141-149)addIterableValuesToSet(56-60)composition/src/utils/types.ts (1)
InvalidEntityInterface(39-43)
composition/tests/v1/directives/interface-object.test.ts (6)
composition/tests/utils/utils.ts (4)
federateSubgraphsSuccess(59-72)schemaToSortedNormalizedString(27-29)normalizeString(19-21)federateSubgraphsFailure(49-57)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/errors/errors.ts (1)
undefinedEntityInterfaceImplementationsError(845-877)composition/src/utils/types.ts (1)
InvalidEntityInterface(39-43)composition/src/schema-building/types.ts (3)
EntityInterfaceFederationData(319-327)SimpleFieldData(274-277)EntityInterfaceSubgraphData(279-287)composition/src/ast/utils.ts (1)
parse(272-274)
composition/tests/v1/entity-interface.test.ts (6)
composition/tests/utils/utils.ts (1)
federateSubgraphsFailure(49-57)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/errors/errors.ts (1)
undefinedEntityInterfaceImplementationsError(845-877)composition/src/utils/types.ts (1)
InvalidEntityInterface(39-43)composition/src/utils/string-constants.ts (1)
INTERFACE(71-71)composition/src/schema-building/types.ts (1)
EntityInterfaceFederationData(319-327)
⏰ 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). (8)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
composition/src/utils/types.ts (1)
40-43: Type shape change aligns with per-subgraph semanticsReplacing
concreteTypeNameswithdefinedConcreteTypeNamesandrequiredConcreteTypeNamesmakes the error payload precise and actionable per subgraph. This matches downstream usages in errors and federation-factory. LGTM.composition/src/errors/errors.ts (1)
868-874: Correct switch-over to definedConcreteTypeNamesUsing
definedConcreteTypeNamesin disparity computation is consistent with the new InvalidEntityInterface payload. This ensures we report exactly the implementations absent from a subgraph's explicit or propagated (via interface objects) definitions.composition/src/v1/federation/federation-factory.ts (3)
3071-3080: Good: Track concrete types defined per subgraph (incl. interface-object propagation)Building
definedConcreteTypeNamesBySubgraphNameand aggregating both the subgraph’s own concrete types and those implied via@interfaceObjectensures later validation can account for “provided” implementations, not just local definitions.
3082-3091: Good: InvalidEntityInterface now carries defined vs required setsPopulating
definedConcreteTypeNamesandrequiredConcreteTypeNamesmakes the downstream error useful and aligns with type changes. The resolvable gate prevents false positives whenresolvable: false.
1660-1686: Propagating @external and @Shareable from Interface Object to implementations is correctSetting
isShareableBySubgraphName, adding the subgraph tosubgraphNames, and copyingexternalFieldDataBySubgraphNameonly when defined avoids inadvertently marking fields as external. This matches the goal: allow interface-object-provided externals to satisfy interface contracts on concrete types.composition/tests/v1/entity-interface.test.ts (1)
146-183: Tests align with new error payload and INTERFACE keySwitching to destructured
{ errors }, using the publicINTERFACEsymbol, and assertingdefinedConcreteTypeNames/requiredConcreteTypeNamesmakes the test robust to the new API. Looks good.composition/tests/v1/directives/interface-object.test.ts (3)
4-21: Import shape updates reflect public API evolutionBringing in
EntityInterfaceFederationData,EntityInterfaceSubgraphData,InvalidEntityInterface,SimpleFieldData,INTERFACE, and the error factory ensures tests remain tightly coupled to the exported API surface. Good coverage.
684-716: Excellent: Validates propagation of @external fields from Interface Object to implementations (entity-interface chain)This scenario ensures fields required by
EntityInterfaceB(via@interfaceObject) propagate to the concreteEntityand satisfy bothEntityInterfaceAandEntityInterfaceB. This is a high-value regression test for the fix.
752-785: Error-path test correctly asserts per-subgraph defined vs required implementationsVerifying
undefinedEntityInterfaceImplementationsErrorwithdefinedConcreteTypeNamesandrequiredConcreteTypeNamesconfirms the new error semantics—particularly when no Interface Object in the subgraph provides the missing implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
composition/src/v1/federation/federation-factory.ts (1)
3113-3132: Bug: .intersection is not a function on Set — runtime failureNative Set in JS/TS doesn’t implement intersection. This line will throw. Compute the intersection via filtering instead.
Apply this diff:
- const definedTypeNames = invalidInterface.requiredConcreteTypeNames.intersection(validTypeNames); + const definedTypeNames = new Set<TypeName>( + [...invalidInterface.requiredConcreteTypeNames].filter((t) => validTypeNames.has(t)), + );Optionally verify no other usages of Set.prototype.intersection remain:
#!/bin/bash # Find any remaining uses of Set.prototype.intersection in the repo with context rg -nPU '\.intersection\s*\(' -C3
🧹 Nitpick comments (1)
composition/src/v1/federation/federation-factory.ts (1)
1676-1686: Avoid overwriting shareability to false when not explicitly setWhen propagating @Shareable from the Interface Object field to an existing concrete field, defaulting missing entries to false can inadvertently downgrade previously true values for that subgraph. Only set the value when the source map explicitly has the subgraph entry.
Apply this diff:
- const isShareable = fieldData.isShareableBySubgraphName.get(subgraphName) ?? false; - existingFieldData.isShareableBySubgraphName.set(subgraphName, isShareable); + if (fieldData.isShareableBySubgraphName.has(subgraphName)) { + existingFieldData.isShareableBySubgraphName.set( + subgraphName, + fieldData.isShareableBySubgraphName.get(subgraphName)!, + ); + } existingFieldData.subgraphNames.add(subgraphName); const externalData = fieldData.externalFieldDataBySubgraphName.get(subgraphName); if (!externalData) { continue; } existingFieldData.externalFieldDataBySubgraphName.set(subgraphName, { ...externalData });
📜 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 (2)
composition/src/errors/errors.ts(2 hunks)composition/src/v1/federation/federation-factory.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composition/src/errors/errors.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
composition/src/v1/federation/federation-factory.ts (4)
composition/src/types/types.ts (2)
SubgraphName(7-7)TypeName(9-9)controlplane/src/core/util.ts (1)
getValueOrDefault(391-399)composition/src/utils/utils.ts (2)
getValueOrDefault(141-149)addIterableValuesToSet(56-60)composition/src/utils/types.ts (1)
InvalidEntityInterface(39-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: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- 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 (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
composition/src/v1/federation/federation-factory.ts (3)
233-234: Import additions align with new per-subgraph trackingImporting SubgraphName and TypeName is consistent with the new per-subgraph type tracking introduced later in the file.
1620-1620: Comment clarification LGTMThe updated comment improves intent clarity around propagating Interface Object subgraph locations to implementing concrete types.
3070-3095: Per-subgraph defined concrete types accumulation looks soundTracking definedConcreteTypeNamesBySubgraphName and populating it while iterating per-interface/per-subgraph data is consistent with the later cross-subgraph validation pass. The InvalidEntityInterface payload updates (definedConcreteTypeNames/requiredConcreteTypeNames) align with the type changes.
…d-propagate-external
Aenimus
deleted the
david/eng-7924-interface-object-fields-should-propagate-external
branch
3 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Checklist