Skip to content

fix: handle internal graph edge case with shared root field and nested entities#2298

Merged
Aenimus merged 5 commits intomainfrom
wilson/eng-8391-handle-error-when-subgraph-cannot-escape-subgraph-due-to
Oct 23, 2025
Merged

fix: handle internal graph edge case with shared root field and nested entities#2298
Aenimus merged 5 commits intomainfrom
wilson/eng-8391-handle-error-when-subgraph-cannot-escape-subgraph-due-to

Conversation

@wilsonrivera
Copy link
Copy Markdown
Contributor

@wilsonrivera wilsonrivera commented Oct 23, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved resilience when linking resources so missing root paths no longer cause errors; cross-linking is performed only when the root exists.
  • Tests

    • Expanded coverage with scenarios that validate errors for unresolvable fields when shared root fields coexist with unreachable nested entities across multiple subgraphs.

Checklist

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 23, 2025

Walkthrough

Replaced a strict lookup that threw on missing root resolution data with optional safe access in the resolvability graph; cross-linking between entity and root resolution data now only happens when the root path exists. Added a new test scenario and three subgraph fixtures exercising field resolvability and unreachable/inaccessible key fields.

Changes

Cohort / File(s) Summary
Resolution data access
composition/src/resolvability-graph/graph.ts
Replaced a strict getOrThrowError access on walker.resDataByPath with a safe optional .get(fullPath) lookup; cross-linking between entityResData and rootResData is performed only if the root entry exists, preventing an exception when absent.
Test coverage for resolvability
composition/tests/v1/resolvability.test.ts
Added a new test case validating unresolvable fields when a shared root query field coexists with unreachable nested entities; introduced three new subgraph fixtures haaa, haab, and haac modeling inaccessible key fields and relationships, and expecting four resolvability errors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Reasoning: Small, focused logic change in core graph code plus new multi-subgraph test fixtures. Review requires checking conditional linking semantics and verifying test coverage and expectations.

Possibly related PRs

  • fix: internal graph bugs #2240: Modifies the same composition/src/resolvability-graph/graph.ts access patterns and root/entity resolution data linking; likely directly related.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: handle edge case with shared root field and nested entities" is directly related to the changeset. The code modifications replace a strict error-throwing approach with safe optional access that gates cross-linking behind path existence, addressing an edge case with shared root fields. The test additions validate error handling when shared root query fields coexist with unreachable nested entities. The title is specific and concise, clearly conveying that this is a fix for an edge case involving shared root field and nested entity handling, which aligns with the main technical changes implemented.
✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 23, 2025

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-9f7be6e62d6253a9fc7566d3cc03c627f0a3fbf0-nonroot

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 609cf13 and 8c97021.

📒 Files selected for processing (2)
  • composition/src/resolvability-graph/graph.ts (1 hunks)
  • composition/tests/v1/resolvability.test.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
composition/tests/v1/resolvability.test.ts (4)
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/subgraph/types.ts (1)
  • Subgraph (11-15)
composition/src/ast/utils.ts (1)
  • parse (272-274)
⏰ 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). (12)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_push_image
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: image_scan
  • 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
🔇 Additional comments (2)
composition/src/resolvability-graph/graph.ts (1)

301-306: LGTM! Defensive change appropriately handles missing root resolution data.

The change from getOrThrowError to optional .get() access with existence gating prevents crashes when root path data is unavailable due to misconfigured @key fields across subgraphs. Cross-linking now only occurs when both entity and root resolution data exist.

composition/tests/v1/resolvability.test.ts (1)

3391-3461: LGTM! Subgraph definitions correctly model the error scenario.

The three subgraphs properly test resolvability errors arising from:

  • Inaccessible key fields (idB: ScalarID! @inaccessible) in haaa and haab
  • Mismatched key fields between subgraphs (haaa/haab use idB, haac uses idA)
  • Complex nested relationships through Connection/Edge types

This setup validates that the graph handles cases where entities cannot jump between subgraphs due to incorrect @key configurations.

Comment thread composition/tests/v1/resolvability.test.ts Outdated
Comment thread composition/tests/v1/resolvability.test.ts Outdated
Comment thread composition/tests/v1/resolvability.test.ts Outdated
Comment thread composition/tests/v1/resolvability.test.ts
Comment thread composition/src/resolvability-graph/graph.ts Outdated
Copy link
Copy Markdown
Member

@Aenimus Aenimus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Aenimus Aenimus changed the title feat: handle error when subgraph cannot escape subgraph due to different @key fields fix: handle edge case with shared root field and nested entities Oct 23, 2025
@Aenimus Aenimus changed the title fix: handle edge case with shared root field and nested entities fix: handle internal graph edge case with shared root field and nested entities Oct 23, 2025
@Aenimus Aenimus enabled auto-merge (squash) October 23, 2025 17:24
@Aenimus Aenimus merged commit 3a50788 into main Oct 23, 2025
45 of 46 checks passed
@Aenimus Aenimus deleted the wilson/eng-8391-handle-error-when-subgraph-cannot-escape-subgraph-due-to branch October 23, 2025 17:24
asoorm pushed a commit that referenced this pull request Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants