Skip to content

fix: check max depth for every depth a fragment is spread at - #780

Merged
pavelnikolov merged 1 commit into
graph-gophers:mainfrom
mwhooker:fix/max-depth-repeated-fragments
Oct 7, 2026
Merged

pavelnikolov merged 1 commit into
graph-gophers:mainfrom
mwhooker:fix/max-depth-repeated-fragments

Conversation

@mwhooker

@mwhooker mwhooker commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

validateMaxDepth guards against fragment cycles with a single visited set for the whole operation. Once a fragment has been checked, every later spread of it is skipped, including spreads at a greater depth, where its fields are deeper. A query can exceed MaxDepth by spreading a fragment shallowly before spreading it deeply, or reach any depth with a chain of fragments that are each spread once near the top first.

This keys visited by fragment and depth instead. A spread at a depth that has already been checked is still skipped, so cycles still terminate, and depth never exceeds maxDepth+1 because fields beyond the limit are not descended into, so each fragment is walked at most maxDepth+1 times.

The existing spreadAtDifferentDepths case covered this scenario but passed only because its query also failed type validation (PossibleFragmentSpreadsRule, FieldsOnCorrectTypeRule); no MaxDepthExceeded error was produced. It now uses a valid query and asserts MaxDepthExceeded, and fragmentChainSpreadShallowFirst covers the chain. Both fail on main.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The depth-aware traversal is correct, bounded, and adequately covered by regression tests.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes max-depth validation for fragments reused at different nesting depths.

Changes:

  • Tracks visited fragments by definition and depth.
  • Adds valid regression tests for direct and chained fragment spreads.
File Description
internal/​validation/​validation.go Corrects fragment depth traversal and cycle handling.
internal/​validation/​validate_max_depth_test.go Adds targeted regression coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@pavelnikolov
pavelnikolov enabled auto-merge October 7, 2026 21:17
@pavelnikolov
pavelnikolov disabled auto-merge October 7, 2026 21:20
@pavelnikolov
pavelnikolov added this pull request to the merge queue Oct 7, 2026
Merged via the queue into graph-gophers:main with commit c645bee Oct 7, 2026
3 checks passed
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.

3 participants