fix: handle fully external, unresolvable entity keys#2568
Merged
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech,
jensneuse and
thisisnithin
as code owners
Contributor
WalkthroughAdds tracking for external edges/field-sets and resolved selection paths across the resolvability graph, propagating external-subgraph names into resolution data and adjusting traversal to short-circuit or record external resolution outcomes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2568 +/- ##
===========================================
- Coverage 63.59% 42.53% -21.06%
===========================================
Files 298 1014 +716
Lines 41920 140682 +98762
Branches 4439 8088 +3649
===========================================
+ Hits 26657 59836 +33179
- Misses 15241 79147 +63906
- Partials 22 1699 +1677
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/tests/v1/resolvability.test.ts`:
- Around line 1821-1823: The test "that an `@external` key can still be a valid
target" destructures success from federateSubgraphsSuccess([jaaa, jaab],
ROUTER_COMPATIBILITY_VERSION_ONE) but never asserts it; update the test to
assert the expected value (e.g., expect(success).toBe(true) or equivalent) so
the test fails on regressions—locate the test by its name and the call to
federateSubgraphsSuccess and add the appropriate assertion against the success
variable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
composition-go/index.global.jscomposition/src/resolvability-graph/graph-nodes.tscomposition/src/resolvability-graph/graph.tscomposition/src/resolvability-graph/node-resolution-data/node-resolution-data.tscomposition/src/resolvability-graph/node-resolution-data/types/params.tscomposition/src/resolvability-graph/types/params.tscomposition/src/resolvability-graph/types/types.tscomposition/src/resolvability-graph/utils/utils.tscomposition/src/resolvability-graph/walker/entity-walker/entity-walker.tscomposition/src/resolvability-graph/walker/entity-walker/types/params.tscomposition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.tscomposition/src/resolvability-graph/walker/root-field-walkers/types/params.tscomposition/src/utils/types.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/utils.tscomposition/tests/v1/resolvability.test.ts
Contributor
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/resolvability-graph/walker/entity-walker/entity-walker.ts (1)
237-249:⚠️ Potential issue | 🟠 MajorMark direct paths as resolved in the non-relative branch.
removeUnresolvablePathsdeletesselectionPath(Line 239) but never adds it toresolvedPaths. BecauseaddUnresolvablePathschecksresolvedPathsfirst (Line 222), this misses the direct-path guard and can re-register a path that was already resolved.💡 Proposed fix
removeUnresolvablePaths({ selectionPath, removeDescendantPaths }: RemoveUnresolvablePathsParams) { if (!this.relativeOriginPaths) { this.subgraphNameByUnresolvablePath.delete(selectionPath); + this.resolvedPaths.add(selectionPath); if (removeDescendantPaths) { for (const unresolvablePath of this.subgraphNameByUnresolvablePath.keys()) { if (unresolvablePath.startsWith(selectionPath)) { this.subgraphNameByUnresolvablePath.delete(unresolvablePath); this.resolvedPaths.add(unresolvablePath); } } } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/resolvability-graph/walker/entity-walker/entity-walker.ts` around lines 237 - 249, In removeUnresolvablePaths (entity-walker.ts) the non-relative branch deletes selectionPath from subgraphNameByUnresolvablePath but never marks it in resolvedPaths, so addUnresolvablePaths can later re-add it; fix by adding selectionPath to this.resolvedPaths right after deleting it in the !this.relativeOriginPaths branch (keep the existing removeDescendantPaths loop behavior that also adds descendant unresolvablePath entries to resolvedPaths).
🧹 Nitpick comments (1)
composition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.ts (1)
29-35: Consider extracting duplicated edge precheck logic.The inaccessible/external/leaf/revisit prechecks are now repeated in both
visitEdgeandvisitSharedEdge. A shared helper would reduce drift risk as this logic evolves.Also applies to: 127-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.ts` around lines 29 - 35, The precheck logic for edges (inaccessible, external, leaf, revisit) is duplicated between visitEdge and visitSharedEdge; extract this into a shared helper (e.g., create a function like precheckEdge or evaluateEdgePreconditions) that accepts the same inputs used by visitEdge/visitSharedEdge (edge, selectionPath, and any visited-tracking context) and returns the VisitNodeResult-like precheck outcome (visited/areDescendantsResolved/isExternal/shouldSkip). Replace the duplicated blocks in visitEdge and visitSharedEdge to call this new helper first and early-return when the helper indicates a precheck result, keeping existing function signatures (visitEdge, visitSharedEdge, VisitEdgeParams, VisitNodeResult) intact so callers and types remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@composition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.ts`:
- Around line 127-133: visitSharedEdge now returns isExternal but that flag is
dropped by visitSharedConcreteNode and propagateSharedVisitedField; update
visitSharedConcreteNode to forward the isExternal value from visitSharedEdge
into its VisitNodeResult and modify propagateSharedVisitedField to accept and
handle an isExternal branch (e.g., when isExternal is true, mark the returned
visited/areDescendantsResolved appropriately and set an external flag on the
propagated result) so external-subgraph tracking is preserved during shared
root-field traversal; adjust all call sites in visitSharedConcreteNode (and the
similar blocks around lines 192-200 and 273-299) to propagate the new isExternal
field through.
---
Outside diff comments:
In `@composition/src/resolvability-graph/walker/entity-walker/entity-walker.ts`:
- Around line 237-249: In removeUnresolvablePaths (entity-walker.ts) the
non-relative branch deletes selectionPath from subgraphNameByUnresolvablePath
but never marks it in resolvedPaths, so addUnresolvablePaths can later re-add
it; fix by adding selectionPath to this.resolvedPaths right after deleting it in
the !this.relativeOriginPaths branch (keep the existing removeDescendantPaths
loop behavior that also adds descendant unresolvablePath entries to
resolvedPaths).
---
Nitpick comments:
In
`@composition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.ts`:
- Around line 29-35: The precheck logic for edges (inaccessible, external, leaf,
revisit) is duplicated between visitEdge and visitSharedEdge; extract this into
a shared helper (e.g., create a function like precheckEdge or
evaluateEdgePreconditions) that accepts the same inputs used by
visitEdge/visitSharedEdge (edge, selectionPath, and any visited-tracking
context) and returns the VisitNodeResult-like precheck outcome
(visited/areDescendantsResolved/isExternal/shouldSkip). Replace the duplicated
blocks in visitEdge and visitSharedEdge to call this new helper first and
early-return when the helper indicates a precheck result, keeping existing
function signatures (visitEdge, visitSharedEdge, VisitEdgeParams,
VisitNodeResult) intact so callers and types remain consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composition-go/index.global.jscomposition/src/resolvability-graph/walker/entity-walker/entity-walker.tscomposition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.tscomposition/tests/v1/resolvability.test.ts
composition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.ts
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Chores
Tests
Checklist