Merge pull request #27 from jhgg/no-fragment-cycles

Implement NoFragmentCycles validation (also tests)

Author: dittos

graphql/core/validation/rules.py

@@ -2,7 +2,7 @@
 from ..error import GraphQLError
 from ..type.definition import is_composite_type, is_input_type, is_leaf_type, GraphQLNonNull
 from ..language import ast
-from ..language.visitor import Visitor
+from ..language.visitor import Visitor, visit
 from ..language.printer import print_ast
 
 
@@ -239,7 +239,74 @@ class PossibleFragmentSpreads(ValidationRule):
 
 
 class NoFragmentCycles(ValidationRule):
-    pass
+    def __init__(self, context):
+        super(NoFragmentCycles, self).__init__(context)
+        self.spreads_in_fragment = {
+            node.name.value: self.gather_spreads(node)
+            for node in context.get_ast().definitions
+            if isinstance(node, ast.FragmentDefinition)
+        }
+        self.known_to_lead_to_cycle = set()
+
+    def enter_FragmentDefinition(self, node, *args):
+        errors = []
+        initial_name = node.name.value
+        spread_path = []
+
+        # This will convert the ast.FragmentDefinition to something that we can add
+        # to a set. Otherwise we get a `unhashable type: dict` error.
+        # This makes it so that we can define a way to uniquely identify a FragmentDefinition
+        # within a set.
+        fragment_node_to_hashable = lambda fs: (fs.loc['start'], fs.loc['end'], fs.name.value)
+
+        def detect_cycle_recursive(fragment_name):
+            spread_nodes = self.spreads_in_fragment[fragment_name]
+
+            for spread_node in spread_nodes:
+                if fragment_node_to_hashable(spread_node) in self.known_to_lead_to_cycle:
+                    continue
+
+                if spread_node.name.value == initial_name:
+                    cycle_path = spread_path + [spread_node]
+                    self.known_to_lead_to_cycle |= set(map(fragment_node_to_hashable, cycle_path))
+
+                    errors.append(GraphQLError(
+                        self.cycle_error_message(initial_name, [s.name.value for s in spread_path]),
+                        cycle_path
+                    ))
+                    continue
+
+                if any(spread is spread_node for spread in spread_path):
+                    continue
+
+                spread_path.append(spread_node)
+                detect_cycle_recursive(spread_node.name.value)
+                spread_path.pop()
+
+        detect_cycle_recursive(initial_name)
+        if errors:
+            return errors
+
+    @staticmethod
+    def cycle_error_message(fragment_name, spread_names):
+        via = ' via {}'.format(', '.join(spread_names)) if spread_names else ''
+        return 'Cannot spread fragment "{}" within itself{}.'.format(fragment_name, via)
+
+    @classmethod
+    def gather_spreads(cls, node):
+        visitor = cls.CollectFragmentSpreadNodesVisitor()
+        visit(node, visitor)
+        return visitor.collect_fragment_spread_nodes()
+
+    class CollectFragmentSpreadNodesVisitor(Visitor):
+        def __init__(self):
+            self.spread_nodes = []
+
+        def enter_FragmentSpread(self, node, *args):
+            self.spread_nodes.append(node)
+
+        def collect_fragment_spread_nodes(self):
+            return self.spread_nodes
 
 
 class NoUndefinedVariables(ValidationRule):

tests/core_validation/test_no_fragment_cycles.py

@@ -0,0 +1,134 @@
+from graphql.core.language.location import SourceLocation as L
+from graphql.core.validation.rules import NoFragmentCycles
+from utils import expect_passes_rule, expect_fails_rule
+
+
+def cycle_error_message(fragment_name, spread_names, *locations):
+    return {
+        'message': NoFragmentCycles.cycle_error_message(fragment_name, spread_names),
+        'locations': list(locations)
+    }
+
+
+def test_single_reference_is_valid():
+    expect_passes_rule(NoFragmentCycles, '''
+    fragment fragA on Dog { ...fragB }
+    fragment fragB on Dog { name }
+    ''')
+
+
+def test_spreading_twice_is_not_circular():
+    expect_passes_rule(NoFragmentCycles, '''
+    fragment fragA on Dog { ...fragB, ...fragB }
+    fragment fragB on Dog { name }
+    ''')
+
+
+def test_spreading_twice_indirectly_is_not_circular():
+    expect_passes_rule(NoFragmentCycles, '''
+      fragment fragA on Dog { ...fragB, ...fragC }
+      fragment fragB on Dog { ...fragC }
+      fragment fragC on Dog { name }
+    ''')
+
+
+def test_double_spread_within_abstract_types():
+    expect_passes_rule(NoFragmentCycles, '''
+      fragment nameFragment on Pet {
+        ... on Dog { name }
+        ... on Cat { name }
+      }
+      fragment spreadsInAnon on Pet {
+        ... on Dog { ...nameFragment }
+        ... on Cat { ...nameFragment }
+      }
+    ''')
+
+
+def test_spreading_recursively_within_field_fails():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Human { relatives { ...fragA } },
+    ''', [
+        cycle_error_message('fragA', [], L(2, 43))
+    ])
+
+
+def test_no_spreading_itself_directly():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Dog { ...fragA }
+    ''', [
+        cycle_error_message('fragA', [], L(2, 29))
+    ])
+
+
+def test_no_spreading_itself_directly_within_inline_fragment():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Pet {
+        ... on Dog {
+            ...fragA
+        }
+    }
+    ''', [
+        cycle_error_message('fragA', [], L(4, 13))
+    ])
+
+
+def test_no_spreading_itself_indirectly():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Dog { ...fragB }
+    fragment fragB on Dog { ...fragA }
+    ''', [
+        cycle_error_message('fragA', ['fragB'], L(2, 29), L(3, 29))
+    ])
+
+
+def test_no_spreading_itself_indirectly_reports_opposite_order():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragB on Dog { ...fragA }
+    fragment fragA on Dog { ...fragB }
+    ''', [
+        cycle_error_message('fragB', ['fragA'], L(2, 29), L(3, 29))
+    ])
+
+
+def test_no_spreading_itself_indirectly_within_inline_fragment():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Pet {
+        ... on Dog {
+            ...fragB
+        }
+    }
+    fragment fragB on Pet {
+        ... on Dog {
+            ...fragA
+        }
+    }
+    ''', [
+        cycle_error_message('fragA', ['fragB'], L(4, 13), L(9, 13))
+    ])
+
+
+def test_no_spreading_itself_deeply():
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Dog { ...fragB }
+    fragment fragB on Dog { ...fragC }
+    fragment fragC on Dog { ...fragO }
+    fragment fragX on Dog { ...fragY }
+    fragment fragY on Dog { ...fragZ }
+    fragment fragZ on Dog { ...fragO }
+    fragment fragO on Dog { ...fragA, ...fragX }
+    ''', [
+        cycle_error_message('fragA', ['fragB', 'fragC', 'fragO'], L(2, 29), L(3, 29), L(4, 29), L(8, 29)),
+        cycle_error_message('fragX', ['fragY', 'fragZ', 'fragO'], L(5, 29), L(6, 29), L(7, 29), L(8, 39))
+    ])
+
+
+def test_no_spreading_itself_deeply_two_paths():  # -- new rule
+    expect_fails_rule(NoFragmentCycles, '''
+    fragment fragA on Dog { ...fragB, ...fragC }
+    fragment fragB on Dog { ...fragA }
+    fragment fragC on Dog { ...fragA }
+    ''', [
+        cycle_error_message('fragA', ['fragB'], L(2, 29), L(3, 29)),
+        cycle_error_message('fragA', ['fragC'], L(2, 39), L(4, 29))
+    ])