Skip to content

Fix duplicate validation errors for reused fragments#9754

Merged
glen-84 merged 3 commits into
mainfrom
validation-fragment-reentry-dedupe
May 21, 2026
Merged

Fix duplicate validation errors for reused fragments#9754
glen-84 merged 3 commits into
mainfrom
validation-fragment-reentry-dedupe

Conversation

@glen-84
Copy link
Copy Markdown
Member

@glen-84 glen-84 commented May 20, 2026

Summary

  • Scope fragment visit tracking to the enclosing operation in DocumentValidatorContext.FragmentContext: each fragment is walked at most once per operation by default. Fixes a class of false-positive validation errors that previously multiplied with the number of times a fragment was spread (@defer/@stream label uniqueness, argument names, variable usage, input fields, fragment spread possibility, subscription root-field directives).
  • Move the Fragment Spread Is Possible check from FragmentDefinitionNode.Enter to FragmentSpreadNode.Enter so parent-type compatibility is evaluated per spread; required because fragment definitions are now visited at most once per operation. As a side benefit, error locations now point to the offending spread instead of the fragment definition.
  • Extend SetFragmentName to read from FragmentSpreadNode so the fragment error extension is preserved when fragment-related errors fire from spread context.
  • Keep CostAnalyzer's context.Fragments.Leave(fragment) call so it opts back into per-spread re-walks. Cost analysis collects fields into each spread's outer selection set, so each spread legitimately needs to re-walk the fragment body. The Leave API is now a documented opt-in for consumers that need per-spread semantics.
  • Migration guide entry under "Noteworthy changes" describing the new operation-scoped default and the opt-in mechanism.

Test plan

  • Added 6 regression tests covering the rules that previously misfired on reused fragments: subscription root-field skip/include directives, argument names, variable usage compatibility, input object field names, fragments on composite types, and fragment spread possibility (including the order-dependent case where a fragment compatible with one parent is then spread under an incompatible parent).
  • Validation suite green on net8.0, net9.0, and net10.0 (624 passed, 1 skipped).
  • HotChocolate.Authorization.Tests (32), HotChocolate.CostAnalysis.Tests (61), and HotChocolate.Utilities.Introspection.Tests (16) all pass; introspection was the suite that caught the CostAnalyzer regression in CI.
  • Updated 14 snapshots whose error locations or counts changed. Most are pure location shifts (errors now point to the spread, not the fragment definition).

Note on the cycle-detection snapshot change

FragmentSpreadsMustNotFormCyclesRuleTests.DoesNotInfiniteLoopOnTransitivelyRecursiveFragment previously asserted 3 cycle errors for the query

{ dogOrHuman { ...fragA ...fragB ...fragC } }
fragment fragA on Human { name, ...fragB }
fragment fragB on Human { name, ...fragC }
fragment fragC on Human { name, ...fragA }

The fragments form a single cycle A → B → C → A. Before this change, the validator re-walked each fragment from each starting spread, so the same cycle was detected and reported three times (once per entry point). With operation-scoped visit tracking, a fragment is walked at most once per operation; the cycle is detected once on the first walk, and the sibling spreads no longer trigger redundant re-walks. The cycle is still detected and the test's stated intent ("does not infinite loop") is preserved; only the count differs. This aligns with how typical implementations (e.g. graphql-js) report each distinct cycle once.

Copilot AI review requested due to automatic review settings May 20, 2026 15:12
@github-actions github-actions Bot added 📚 documentation This issue is about working on our documentation. 🌶️ hot chocolate labels May 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts HotChocolate’s GraphQL document validation traversal to avoid emitting duplicate validation errors when the same fragment is spread multiple times, by tracking fragment visits per operation and relocating fragment-spread-specific checks to spread context.

Changes:

  • Make fragment visit tracking operation-scoped (each fragment walked at most once per operation) and deprecate DocumentValidatorContext.FragmentContext.Leave(...) as no-op.
  • Move “Fragment Spread Is Possible” validation to FragmentSpreadNode so reused fragments are validated against each spread’s parent type and error locations point to the spread.
  • Extend fragment error extensions to preserve "fragment" when errors originate from fragment spread context; add regression tests and update snapshots accordingly.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.

Show a summary per file
File Description
website/src/docs/hotchocolate/v16/migrating/migrate-from-15-to-16.md Documents the FragmentContext.Leave(...) deprecation and the new operation-scoped fragment visitation semantics.
src/HotChocolate/CostAnalysis/src/CostAnalysis/CostAnalyzer.cs Removes internal Fragments.Leave(...) usage to match new operation-scoped fragment tracking.
src/HotChocolate/Core/src/Authorization/AuthorizeValidationVisitor.cs Removes internal Fragments.Leave(...) usage to match new operation-scoped fragment tracking.
src/HotChocolate/Core/src/Validation/DocumentValidatorContext.cs Makes FragmentContext.Leave(...) obsolete no-ops; preserves operation-scoped visited tracking via Reset().
src/HotChocolate/Core/src/Validation/DocumentValidatorVisitor.cs Stops calling Fragments.Leave(...) after visiting a fragment definition from a spread.
src/HotChocolate/Core/src/Validation/Extensions/ErrorBuilderExtensions.cs Allows SetFragmentName to read fragment name from FragmentSpreadNode as well as FragmentDefinitionNode.
src/HotChocolate/Core/src/Validation/Rules/DirectiveVisitor.cs Updates rationale/comments for @defer/@stream label de-duping across traversal entry points.
src/HotChocolate/Core/src/Validation/Rules/FragmentVisitor.cs Moves spread-is-possible validation to fragment spread enter; removes fragment leave tracking.
src/HotChocolate/Core/test/Validation.Tests/AllVariableUsagesAreAllowedRuleTests.cs Adds regression test ensuring variable-usage errors don’t duplicate across repeated fragment spreads.
src/HotChocolate/Core/test/Validation.Tests/ArgumentNamesRuleTests.cs Adds regression test ensuring argument-name errors don’t duplicate across repeated fragment spreads.
src/HotChocolate/Core/test/Validation.Tests/FragmentSpreadIsPossibleRuleTests.cs Adds regression test ensuring spread-is-possible is evaluated per spread parent type (order-dependent case).
src/HotChocolate/Core/test/Validation.Tests/FragmentsOnCompositeTypesRuleTests.cs Adds regression test ensuring fragment-on-non-composite error is not duplicated by repeated spreads.
src/HotChocolate/Core/test/Validation.Tests/InputObjectFieldNamesRuleTests.cs Adds regression test ensuring invalid input-field errors don’t duplicate across repeated fragment spreads.
src/HotChocolate/Core/test/Validation.Tests/SubscriptionSingleRootFieldRuleTests.cs Adds regression test ensuring subscription root-field directive errors don’t duplicate across repeated fragment spreads.
src/HotChocolate/Core/test/Validation.Tests/snapshots/AllVariableUsagesAreAllowedRuleTests.IntNullableToIntWithinReusedFragment.snap New snapshot for the added variable-usage regression test.
src/HotChocolate/Core/test/Validation.Tests/snapshots/ArgumentNamesRuleTests.InvalidFieldArgNameInReusedFragment.snap New snapshot for the added argument-names regression test.
src/HotChocolate/Core/test/Validation.Tests/snapshots/DocumentValidatorTests.Ensure_Recursive_Fragments_Fail_2.snap Updates snapshot due to fragment-spread-is-possible error location/count changes.
src/HotChocolate/Core/test/Validation.Tests/snapshots/DocumentValidatorTests.FragmentDoesNotMatchType.snap Updates snapshot due to fragment-related error location shifting to spread context.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadsMustNotFormCyclesRuleTests.DoesNotInfiniteLoopOnTransitivelyRecursiveFragment.snap Updates snapshot to reflect cycle errors no longer duplicating per spread entry point.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.DifferentObjectIntoObject.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.FragmentDoesNotMatchType.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.FragmentReusedAcrossCompatibleThenIncompatibleParent.snap New snapshot for the added per-spread parent compatibility regression test.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.InterfaceIntoNonImplementingObject.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.InterfaceIntoNonOverlappingInterface.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.InterfaceIntoNonOverlappingUnion.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.ObjectIntoNotContainingUnion.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.ObjectIntoNotImplementingInterface.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.UnionIntoNonOverlappingInterface.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.UnionIntoNonOverlappingUnion.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentSpreadIsPossibleRuleTests.UnionIntoNotContainedObject.snap Updates snapshot error location to point at the offending spread.
src/HotChocolate/Core/test/Validation.Tests/snapshots/FragmentsOnCompositeTypesRuleTests.Fragment_On_Scalar_Is_Invalid_When_Reused.snap New snapshot for the added fragments-on-composite-types regression test.
src/HotChocolate/Core/test/Validation.Tests/snapshots/InputObjectFieldNamesRuleTests.InvalidInputObjectFieldInReusedFragment.snap New snapshot for the added input-object-field regression test.
src/HotChocolate/Core/test/Validation.Tests/snapshots/SubscriptionSingleRootFieldRuleTests.DisallowedSkipDirectiveOnRootFieldWithinReusedFragment.snap New snapshot for the added subscription root-field directive regression test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@glen-84 glen-84 marked this pull request as draft May 20, 2026 16:09
@glen-84 glen-84 marked this pull request as ready for review May 20, 2026 16:21
@glen-84 glen-84 merged commit 9bb4064 into main May 21, 2026
272 of 276 checks passed
@glen-84 glen-84 deleted the validation-fragment-reentry-dedupe branch May 21, 2026 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📚 documentation This issue is about working on our documentation. 🌶️ hot chocolate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants