Fix duplicate validation errors for reused fragments (#9754)

Author: glen-84

src/HotChocolate/Core/src/Authorization/AuthorizeValidationVisitor.cs

@@ -137,7 +137,6 @@ protected override ISyntaxVisitorAction VisitChildren(
         if (context.Fragments.TryEnter(node, out var fragment))
         {
             var result = Visit(fragment, node, context);
-            context.Fragments.Leave(fragment);
 
             if (result.IsBreak())
             {

src/HotChocolate/Core/src/Validation/DocumentValidatorContext.cs

@@ -289,6 +289,11 @@ public bool TryEnter(FragmentSpreadNode spread, [NotNullWhen(true)] out Fragment
             return false;
         }
 
+        // Removes the fragment name from _visited so that sibling spreads of the same
+        // fragment can re-enter and re-walk the body. Callers that want per-spread
+        // re-walks (e.g. CostAnalyzer) call this on leave. The base validation walker
+        // intentionally does NOT call Leave so each fragment is walked at most once per
+        // operation by default, which is what most validation rules want.
         public void Leave(FragmentSpreadNode spread)
             => _visited.Remove(spread.Name.Value);
 

src/HotChocolate/Core/src/Validation/DocumentValidatorVisitor.cs

@@ -64,7 +64,6 @@ protected override ISyntaxVisitorAction VisitChildren(
         if (context.Fragments.TryEnter(node, out var fragment))
         {
             var result = Visit(fragment, node, context);
-            context.Fragments.Leave(fragment);
 
             if (result.IsBreak())
             {

src/HotChocolate/Core/src/Validation/Extensions/ErrorBuilderExtensions.cs

@@ -37,9 +37,15 @@ public static ErrorBuilder SetFragmentName(
         this ErrorBuilder errorBuilder,
         ISyntaxNode node)
     {
-        if (node.Kind == SyntaxKind.FragmentDefinition)
+        switch (node.Kind)
         {
-            errorBuilder.SetExtension("fragment", ((FragmentDefinitionNode)node).Name.Value);
+            case SyntaxKind.FragmentDefinition:
+                errorBuilder.SetExtension("fragment", ((FragmentDefinitionNode)node).Name.Value);
+                break;
+
+            case SyntaxKind.FragmentSpread:
+                errorBuilder.SetExtension("fragment", ((FragmentSpreadNode)node).Name.Value);
+                break;
         }
         return errorBuilder;
     }

src/HotChocolate/Core/src/Validation/Rules/DirectiveVisitor.cs

@@ -171,11 +171,12 @@ private static void ValidateDirectives<T>(
                 context.ReportError(context.DirectiveMustBeUniqueInLocation(directive));
             }
 
-            // Defer And Stream Directive Labels Are Unique
-            // The spec iterates over every directive in the document. Because the
-            // document walker descends into fragment definitions once per spread,
-            // the same lexical @defer/@stream directive may be visited multiple
-            // times. Track the processed directive nodes so each is counted once.
+            // Defer And Stream Directive Labels Are Unique.
+            // The rule is document-scoped: a label must be unique across all @defer and
+            // @stream directives in the document. The same lexical directive can be
+            // reached from multiple operations (each operation re-walks its spread fragments
+            // independently), so we must track the directive nodes already counted to avoid
+            // colliding a label with itself when revisited.
             if (node.Kind is Field or InlineFragment or FragmentSpread
                 && (directive.Name.Value.Equals(DirectiveNames.Defer.Name, StringComparison.Ordinal)
                 || directive.Name.Value.Equals(DirectiveNames.Stream.Name, StringComparison.Ordinal))

src/HotChocolate/Core/src/Validation/Rules/FragmentVisitor.cs

@@ -151,10 +151,6 @@ protected override ISyntaxVisitorAction Enter(
         {
             if (type.IsCompositeType())
             {
-                ValidateFragmentSpreadIsPossible(
-                    node, context,
-                    context.Types.Peek().NamedType(),
-                    type);
                 context.Types.Push(type);
                 return Continue;
             }
@@ -171,7 +167,6 @@ protected override ISyntaxVisitorAction Leave(
         FragmentDefinitionNode node,
         DocumentValidatorContext context)
     {
-        context.Fragments.Leave(node);
         return base.Leave(node, context);
     }
 
@@ -214,6 +209,18 @@ protected override ISyntaxVisitorAction Enter(
             {
                 context.ReportError(context.FragmentCycleDetected(node));
             }
+
+            if (context.Schema.Types.TryGetType<IOutputTypeDefinition>(
+                fragment.TypeCondition.Name.Value,
+                out var typeCondition)
+                && typeCondition.IsCompositeType())
+            {
+                ValidateFragmentSpreadIsPossible(
+                    node,
+                    context,
+                    context.Types.Peek().NamedType(),
+                    typeCondition);
+            }
         }
         else
         {

src/HotChocolate/Core/test/Validation.Tests/AllVariableUsagesAreAllowedRuleTests.cs

@@ -361,6 +361,28 @@ query Query($intArg: Int) {
         );
     }
 
+    // The rule must fire once per lexical variable usage, not once per fragment spread.
+    [Fact]
+    public void IntNullableToIntWithinReusedFragment()
+    {
+        ExpectErrors(
+            """
+            fragment nonNullIntArgFieldFrag on Arguments {
+              nonNullIntArgField(intArg: $intArg)
+            }
+
+            query Query($intArg: Int) {
+              arguments {
+                ...nonNullIntArgFieldFrag
+                ...nonNullIntArgFieldFrag
+              }
+            }
+            """,
+            t => Assert.Equal(
+                "The variable `intArg` is not compatible with the type of the current location.",
+                t.Message));
+    }
+
     [Fact]
     public void IntNullableToIntWithinNestedFragment()
     {

src/HotChocolate/Core/test/Validation.Tests/ArgumentNamesRuleTests.cs

@@ -65,6 +65,29 @@ fragment invalidArgName on Dog {
                 "The argument `dogCommand` is required.", t.Message));
     }
 
+    // The rule must fire once per lexical argument, not once per fragment spread.
+    [Fact]
+    public void InvalidFieldArgNameInReusedFragment()
+    {
+        ExpectErrors(
+            """
+            query {
+              dog {
+                ... invalidArgName
+                ... invalidArgName
+              }
+            }
+
+            fragment invalidArgName on Dog {
+              doesKnowCommand(command: CLEAN_UP_HOUSE)
+            }
+            """,
+            t => Assert.Equal(
+                "The argument `command` does not exist.", t.Message),
+            t => Assert.Equal(
+                "The argument `dogCommand` is required.", t.Message));
+    }
+
     [Fact]
     public void InvalidDirectiveArgName()
     {

src/HotChocolate/Core/test/Validation.Tests/FragmentSpreadIsPossibleRuleTests.cs

@@ -469,6 +469,32 @@ ...on Intelligent { iq }
             """);
     }
 
+    // When a fragment is reused under multiple parent types, the spread-is-possible
+    // check must run for each spread's parent, not only the first one walked. This
+    // case is order-dependent: a compatible spread first then an incompatible one.
+    [Fact]
+    public void FragmentReusedAcrossCompatibleThenIncompatibleParent()
+    {
+        ExpectErrors(
+            """
+            {
+              dog {
+                ...dogNameFragment
+              }
+              human {
+                ...dogNameFragment
+              }
+            }
+
+            fragment dogNameFragment on Dog {
+              name
+            }
+            """,
+            t => Assert.Equal(
+                "The parent type does not match the type condition on the fragment.",
+                t.Message));
+    }
+
     [Fact]
     public void InterfaceIntoNonOverlappingUnion()
     {

src/HotChocolate/Core/test/Validation.Tests/FragmentsOnCompositeTypesRuleTests.cs

@@ -165,6 +165,28 @@ fragment fragOnScalar on Int {
                 t.Message));
     }
 
+    // The rule must fire once per lexical fragment definition, not once per spread.
+    [Fact]
+    public void Fragment_On_Scalar_Is_Invalid_When_Reused()
+    {
+        ExpectErrors(
+            """
+            {
+              dog {
+                ... fragOnScalar
+                ... fragOnScalar
+              }
+            }
+
+            fragment fragOnScalar on Int {
+              something
+            }
+            """,
+            t => Assert.Equal(
+                "Fragments can only be declared on unions, interfaces, and objects.",
+                t.Message));
+    }
+
     [Fact]
     public void InlineFragment_On_Scalar_Is_Invalid()
     {