Catch OverlappingFieldsCanBeMergedRule violations with nested fragments

Replicates graphql/graphql-js@e52ed9a6

Author: Cito

src/graphql/validation/rules/overlapping_fields_can_be_merged.py

@@ -58,9 +58,10 @@ class OverlappingFieldsCanBeMergedRule(ValidationRule):
 
     def __init__(self, context: ValidationContext) -> None:
         super().__init__(context)
-        # A memoization for when two fragments are compared "between" each other for
-        # conflicts. Two fragments may be compared many times, so memoizing this can
-        # dramatically improve the performance of this validator.
+        # A memoization for when fields and a fragment or two fragments are compared
+        # "between" each other for conflicts. Comparisons may be made many times, so
+        # memoizing this can dramatically improve the performance of this validator.
+        self.compared_fields_and_fragment_pairs = OrderedPairSet()
         self.compared_fragment_pairs = PairSet()
 
         # A cache for the "field map" and list of fragment names found in any given
@@ -72,6 +73,7 @@ def enter_selection_set(self, selection_set: SelectionSetNode, *_args: Any) -> N
         conflicts = find_conflicts_within_selection_set(
             self.context,
             self.cached_fields_and_fragment_names,
+            self.compared_fields_and_fragment_pairs,
             self.compared_fragment_pairs,
             self.context.get_parent_type(),
             selection_set,
@@ -155,6 +157,7 @@ def enter_selection_set(self, selection_set: SelectionSetNode, *_args: Any) -> N
 def find_conflicts_within_selection_set(
     context: ValidationContext,
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     parent_type: GraphQLNamedType | None,
     selection_set: SelectionSetNode,
@@ -178,6 +181,7 @@ def find_conflicts_within_selection_set(
         context,
         conflicts,
         cached_fields_and_fragment_names,
+        compared_fields_and_fragment_pairs,
         compared_fragment_pairs,
         field_map,
     )
@@ -190,6 +194,7 @@ def find_conflicts_within_selection_set(
                 context,
                 conflicts,
                 cached_fields_and_fragment_names,
+                compared_fields_and_fragment_pairs,
                 compared_fragment_pairs,
                 False,
                 field_map,
@@ -204,6 +209,7 @@ def find_conflicts_within_selection_set(
                     context,
                     conflicts,
                     cached_fields_and_fragment_names,
+                    compared_fields_and_fragment_pairs,
                     compared_fragment_pairs,
                     False,
                     fragment_name,
@@ -217,6 +223,7 @@ def collect_conflicts_between_fields_and_fragment(
     context: ValidationContext,
     conflicts: list[Conflict],
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     are_mutually_exclusive: bool,
     field_map: NodeAndDefCollection,
@@ -227,6 +234,16 @@ def collect_conflicts_between_fields_and_fragment(
     Collect all conflicts found between a set of fields and a fragment reference
     including via spreading in any nested fragments.
     """
+    # Memoize so the fields and fragments are not compared for conflicts more
+    # than once.
+    if compared_fields_and_fragment_pairs.has(
+        field_map, fragment_name, are_mutually_exclusive
+    ):
+        return
+    compared_fields_and_fragment_pairs.add(
+        field_map, fragment_name, are_mutually_exclusive
+    )
+
     fragment = context.get_fragment(fragment_name)
     if not fragment:
         return
@@ -245,6 +262,7 @@ def collect_conflicts_between_fields_and_fragment(
         context,
         conflicts,
         cached_fields_and_fragment_names,
+        compared_fields_and_fragment_pairs,
         compared_fragment_pairs,
         are_mutually_exclusive,
         field_map,
@@ -254,19 +272,11 @@ def collect_conflicts_between_fields_and_fragment(
     # (E) Then collect any conflicts between the provided collection of fields and any
     # fragment names found in the given fragment.
     for referenced_fragment_name in referenced_fragment_names:
-        # Memoize so two fragments are not compared for conflicts more than once.
-        if compared_fragment_pairs.has(
-            referenced_fragment_name, fragment_name, are_mutually_exclusive
-        ):
-            continue
-        compared_fragment_pairs.add(
-            referenced_fragment_name, fragment_name, are_mutually_exclusive
-        )
-
         collect_conflicts_between_fields_and_fragment(
             context,
             conflicts,
             cached_fields_and_fragment_names,
+            compared_fields_and_fragment_pairs,
             compared_fragment_pairs,
             are_mutually_exclusive,
             field_map,
@@ -278,6 +288,7 @@ def collect_conflicts_between_fragments(
     context: ValidationContext,
     conflicts: list[Conflict],
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     are_mutually_exclusive: bool,
     fragment_name1: str,
@@ -318,6 +329,7 @@ def collect_conflicts_between_fragments(
         context,
         conflicts,
         cached_fields_and_fragment_names,
+        compared_fields_and_fragment_pairs,
         compared_fragment_pairs,
         are_mutually_exclusive,
         field_map1,
@@ -331,6 +343,7 @@ def collect_conflicts_between_fragments(
             context,
             conflicts,
             cached_fields_and_fragment_names,
+            compared_fields_and_fragment_pairs,
             compared_fragment_pairs,
             are_mutually_exclusive,
             fragment_name1,
@@ -344,6 +357,7 @@ def collect_conflicts_between_fragments(
             context,
             conflicts,
             cached_fields_and_fragment_names,
+            compared_fields_and_fragment_pairs,
             compared_fragment_pairs,
             are_mutually_exclusive,
             referenced_fragment_name1,
@@ -354,6 +368,7 @@ def collect_conflicts_between_fragments(
 def find_conflicts_between_sub_selection_sets(
     context: ValidationContext,
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     are_mutually_exclusive: bool,
     parent_type1: GraphQLNamedType | None,
@@ -381,6 +396,7 @@ def find_conflicts_between_sub_selection_sets(
         context,
         conflicts,
         cached_fields_and_fragment_names,
+        compared_fields_and_fragment_pairs,
         compared_fragment_pairs,
         are_mutually_exclusive,
         field_map1,
@@ -395,6 +411,7 @@ def find_conflicts_between_sub_selection_sets(
                 context,
                 conflicts,
                 cached_fields_and_fragment_names,
+                compared_fields_and_fragment_pairs,
                 compared_fragment_pairs,
                 are_mutually_exclusive,
                 field_map1,
@@ -409,6 +426,7 @@ def find_conflicts_between_sub_selection_sets(
                 context,
                 conflicts,
                 cached_fields_and_fragment_names,
+                compared_fields_and_fragment_pairs,
                 compared_fragment_pairs,
                 are_mutually_exclusive,
                 field_map2,
@@ -424,6 +442,7 @@ def find_conflicts_between_sub_selection_sets(
                 context,
                 conflicts,
                 cached_fields_and_fragment_names,
+                compared_fields_and_fragment_pairs,
                 compared_fragment_pairs,
                 are_mutually_exclusive,
                 fragment_name1,
@@ -437,6 +456,7 @@ def collect_conflicts_within(
     context: ValidationContext,
     conflicts: list[Conflict],
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     field_map: NodeAndDefCollection,
 ) -> None:
@@ -455,6 +475,7 @@ def collect_conflicts_within(
                     conflict = find_conflict(
                         context,
                         cached_fields_and_fragment_names,
+                        compared_fields_and_fragment_pairs,
                         compared_fragment_pairs,
                         # within one collection is never mutually exclusive
                         False,
@@ -470,6 +491,7 @@ def collect_conflicts_between(
     context: ValidationContext,
     conflicts: list[Conflict],
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     parent_fields_are_mutually_exclusive: bool,
     field_map1: NodeAndDefCollection,
@@ -495,6 +517,7 @@ def collect_conflicts_between(
                     conflict = find_conflict(
                         context,
                         cached_fields_and_fragment_names,
+                        compared_fields_and_fragment_pairs,
                         compared_fragment_pairs,
                         parent_fields_are_mutually_exclusive,
                         response_name,
@@ -508,6 +531,7 @@ def collect_conflicts_between(
 def find_conflict(
     context: ValidationContext,
     cached_fields_and_fragment_names: dict,
+    compared_fields_and_fragment_pairs: OrderedPairSet,
     compared_fragment_pairs: PairSet,
     parent_fields_are_mutually_exclusive: bool,
     response_name: str,
@@ -578,6 +602,7 @@ def find_conflict(
         conflicts = find_conflicts_between_sub_selection_sets(
             context,
             cached_fields_and_fragment_names,
+            compared_fields_and_fragment_pairs,
             compared_fragment_pairs,
             are_mutually_exclusive,
             get_named_type(type1),
@@ -773,6 +798,43 @@ def subfield_conflicts(
     return None  # no conflict
 
 
+class OrderedPairSet:
+    """Ordered pair set
+
+    A way to keep track of pairs of things where the ordering of the pair matters.
+
+    Provides a third argument for has/add to allow flagging the pair as weakly or
+    strongly present within the collection.
+
+    The first element is matched by object identity (its ``id``), since field maps
+    are unhashable mappings that are kept alive for the duration of the validation.
+    """
+
+    __slots__ = ("_data",)
+
+    _data: dict[int, dict[str, bool]]
+
+    def __init__(self) -> None:
+        self._data = {}
+
+    def has(self, a: NodeAndDefCollection, b: str, weakly_present: bool) -> bool:
+        map_ = self._data.get(id(a))
+        if map_ is None:
+            return False
+        result = map_.get(b)
+        if result is None:
+            return False
+
+        return True if weakly_present else weakly_present == result
+
+    def add(self, a: NodeAndDefCollection, b: str, weakly_present: bool) -> None:
+        map_ = self._data.get(id(a))
+        if map_ is None:
+            self._data[id(a)] = {b: weakly_present}
+        else:
+            map_[b] = weakly_present
+
+
 class PairSet:
     """Pair set
 

tests/validation/test_overlapping_fields_can_be_merged.py

@@ -686,6 +686,35 @@ def reports_deep_conflict_in_nested_fragments():
             ],
         )
 
+    def reports_deep_conflict_after_nested_fragments():
+        assert_errors(
+            """
+            fragment F on T {
+              ...G
+            }
+            fragment G on T {
+              ...H
+            }
+            fragment H on T {
+              x: a
+            }
+            {
+              x: b
+              ...F
+            }
+            """,
+            [
+                {
+                    "message": "Fields 'x' conflict"
+                    " because 'b' and 'a' are different fields."
+                    " Use different aliases on the fields"
+                    " to fetch both if this was intentional.",
+                    "locations": [(12, 15), (9, 15)],
+                    "path": None,
+                }
+            ],
+        )
+
     def ignores_unknown_fragments():
         assert_valid(
             """
@@ -1302,3 +1331,31 @@ def finds_invalid_case_even_with_immediately_recursive_fragment():
                 }
             ],
         )
+
+    def does_not_infinite_loop_on_recursive_fragments_separated_by_fields():
+        assert_valid(
+            """
+            {
+              ...fragA
+              ...fragB
+            }
+
+            fragment fragA on T {
+              x {
+                ...fragA
+                x {
+                  ...fragA
+                }
+              }
+            }
+
+            fragment fragB on T {
+              x {
+                ...fragB
+                x {
+                  ...fragB
+                }
+              }
+            }
+            """
+        )