fix: terminate overlapping-fields rule on cyclic fragment spreads

[mennatnaga]

The overlapping-fields validator infinitely recurses on cyclic fragment spreads.

How to reproduce

  fragment A on Dog { ...B }                                                                             
  fragment B on Dog { ...A }                                                                             
  { ...A }                                                                                               

Before this patch: runtime: goroutine stack exceeds 1000000000-byte limit; fatal error: stack overflow inside collectConflictsBetweenFieldsAndFragment at rules_overlapping_fields_can_be_merged.go:209.

After: validation returns Cannot spread fragment "A" within itself via B. from NoFragmentCyclesRule.

Root cause

collectConflictsBetweenFieldsAndFragment recurses through nested spreads without memoization. Its sibling collectConflictsBetweenFragments already has the equivalent guard via rule.comparedSet; this one was missing it.

Changes

• New fieldsAndFragmentSet memo keyed by (*fieldsAndFragmentNames, fragmentName, areMutuallyExclusive), same Has/Add shape as the existing pairSet.
• Wired into the rule constructor and struct alongside comparedSet and cacheMap.
• Function-entry check skips repeat triples.
• Added a fieldsInfo == fieldsInfo2 guard so a fragment is not compared against itself.

Test

TestValidate_OverlappingFieldsCanBeMerged_CyclicFragmentTerminates runs the rule inside a goroutine with a 2s deadline. Fails on the unpatched code (the goroutine never finishes), passes after the fix.
Existing OverlappingFields* tests still pass.

Closes

#742

Summary by CodeRabbit

• Bug Fixes

  • Validation no longer hangs on documents with cyclic fragment spreads; overlapping-fields checks terminate reliably and run faster thanks to per-pass memoization.

• Tests

  • Added a test ensuring validation completes for cyclic fragment structures.

[coveralls]

coverage: 92.286% (+0.002%) from 92.284% — mennatnaga:cycle-guard into graphql-go:master

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6f6d50e0-9124-453a-8a6f-fc1c08da562b

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae8dab and 526d0f9.

📒 Files selected for processing (2)

• rules_overlapping_fields_can_be_merged.go
• rules_overlapping_fields_can_be_merged_test.go

---

📝 Walkthrough

Walkthrough

Adds per-validation-pass memoization to OverlappingFieldsCanBeMergedRule to avoid re-walking fragment spreads for identical (fieldsInfo, fragmentName, areMutuallyExclusive) triples; updates collectConflictsBetweenFieldsAndFragment to consult the memo before recursing and adds a test ensuring termination on cyclic fragment spreads.

Changes

Cyclic Fragment Recursion Prevention

Layer / File(s)	Summary
Memoization data structure and initialization rules_overlapping_fields_can_be_merged.go	New comparedFieldsAndFragmentSet field on the rule struct; fieldsAndFragmentSet type with Has and Add methods to track (fieldsInfo, fragmentName, areMutuallyExclusive) triples; initialized per validation pass and wired into the rule instance.
Cyclic recursion prevention in field-fragment comparison rules_overlapping_fields_can_be_merged.go	collectConflictsBetweenFieldsAndFragment checks memoization and returns early when a triple was already compared; otherwise records the triple then looks up the fragment and collects referenced fields.
Cyclic fragment termination verification rules_overlapping_fields_can_be_merged_test.go	Import time and add TestValidate_OverlappingFieldsCanBeMerged_CyclicFragmentTerminates which runs validation on mutually recursive fragments asynchronously and asserts completion within 2 seconds.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• OverlappingFieldsCanBeMergedRule stack-overflows on cyclic fragment spreads #742 — This PR implements per-validation-pass memoization in collectConflictsBetweenFieldsAndFragment, addressing the infinite recursion described in that issue.

Poem

A rabbit hops through fragments, keeps a careful memo,
Triples tucked away so recursion won't grow.
Cycles unwind where spreads once would spin,
With cached little notes, the validator wins. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding memoization to the overlapping-fields validator to prevent infinite recursion on cyclic fragment spreads.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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)

rules_overlapping_fields_can_be_merged.go (1)

221-233: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the original field set when recursing into nested spreads.

Step E here should continue comparing the caller’s fieldsInfo against deeper fragment spreads, but Line 233 switches the left-hand side to fieldsInfo2. That misses conflicts like { x: a, ...A }, fragment A on T { ...B }, fragment B on T { x: b }, because the second step compares A vs B instead of the original selection set vs B.

Suggested fix

 	for _, fragmentName2 := range fieldsInfo2.fragmentNames {
-		conflicts = rule.collectConflictsBetweenFieldsAndFragment(conflicts, areMutuallyExclusive, fieldsInfo2, fragmentName2)
+		conflicts = rule.collectConflictsBetweenFieldsAndFragment(conflicts, areMutuallyExclusive, fieldsInfo, fragmentName2)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rules_overlapping_fields_can_be_merged.go` around lines 221 - 233, The
recursion into nested fragment spreads uses the wrong left-hand side: when
iterating fragmentName2 from fieldsInfo2.fragmentNames you must continue
comparing the original caller's fieldsInfo against the deeper fragment, not
fieldsInfo2; change the call to
rule.collectConflictsBetweenFieldsAndFragment(conflicts, areMutuallyExclusive,
fieldsInfo, fragmentName2) so the original fieldsInfo is preserved while
recursing through fragmentNames (leave collectConflictsBetween and the loop
intact).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rules_overlapping_fields_can_be_merged_test.go`:
- Around line 909-922: The test currently calls testutil.ExpectPassesRule inside
a spawned goroutine (using the done channel and select), which can invoke
t.Fatal from a non-test goroutine; instead, run no assertions in the worker
goroutine — have it execute the rule logic and send back a result (e.g. an error
or boolean) over a result channel; then in the main test goroutine (after the
select or receiving from the channel) call testutil.ExpectPassesRule or
t.Fatal/t.Fatalf based on that result. Update the goroutine to only perform
non-fatal work and send a result, and move any calls to
testutil.ExpectPassesRule/t.Fatal into the main test goroutine that owns t; keep
references to the done channel, the goroutine, the select/time.After timeout,
and the ExpectPassesRule call so you modify the correct code paths.

---

Outside diff comments:
In `@rules_overlapping_fields_can_be_merged.go`:
- Around line 221-233: The recursion into nested fragment spreads uses the wrong
left-hand side: when iterating fragmentName2 from fieldsInfo2.fragmentNames you
must continue comparing the original caller's fieldsInfo against the deeper
fragment, not fieldsInfo2; change the call to
rule.collectConflictsBetweenFieldsAndFragment(conflicts, areMutuallyExclusive,
fieldsInfo, fragmentName2) so the original fieldsInfo is preserved while
recursing through fragmentNames (leave collectConflictsBetween and the loop
intact).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 31faf011-3642-4851-b2cc-77d633878143

📥 Commits

Reviewing files that changed from the base of the PR and between d5f8f3c and 5ae8dab.

📒 Files selected for processing (2)

• rules_overlapping_fields_can_be_merged.go
• rules_overlapping_fields_can_be_merged_test.go