Merge pull request #81 from freshtonic/validations/no-fragment-cycles

NoFragmentCycles validation

Author: Josh Price

lib/graphql/util/stack.ex

@@ -25,4 +25,6 @@ defmodule GraphQL.Util.Stack do
       [] -> nil
     end
   end
+  
+  def length(stack), do: Kernel.length(stack.elements)
 end

lib/graphql/validation/rules.ex

@@ -2,12 +2,11 @@
 defmodule GraphQL.Validation.Rules do
 
   # All of the known validation rules.
-  # TODO: it would be great if the rules auto-registered themselves.
-  # A task for a rainy day.
   @rules [
     %GraphQL.Validation.Rules.UniqueOperationNames{},
     %GraphQL.Validation.Rules.FieldsOnCorrectType{},
-    %GraphQL.Validation.Rules.ProvidedNonNullArguments{}
+    %GraphQL.Validation.Rules.ProvidedNonNullArguments{},
+    %GraphQL.Validation.Rules.NoFragmentCycles{}
   ]
 
   def all(), do: @rules

lib/graphql/validation/rules/no_fragment_cycles.ex

@@ -0,0 +1,123 @@
+defmodule GraphQL.Validation.Rules.NoFragmentCycles do
+
+  alias GraphQL.Lang.AST.{Visitor, InitialisingVisitor}
+  alias GraphQL.Util.Stack
+  import GraphQL.Validation
+
+  defstruct name: "NoFragmentCycles"
+
+  defimpl InitialisingVisitor do
+    def init(_visitor, acc) do
+      Map.merge(acc, %{
+        visited_fragments: %{},
+        spread_path: %Stack{},
+        spread_path_indices: %{}
+      })
+    end
+  end
+
+  defimpl Visitor do
+    def enter(_visitor, %{kind: :FragmentDefinition} = node, acc) do
+      if !visited?(acc, node) do
+        {:continue, detect_cycles(acc, node)}
+      else
+        {:continue, acc}
+      end
+    end
+
+    def enter(_visitor, _node, acc) do
+      {:continue, acc}
+    end
+
+    def leave(_visitor, _node, acc) do
+      {:continue, acc}
+    end
+
+    defp detect_cycles(acc, fragment_def) do
+      acc
+      |> mark_visited(fragment_def)
+      |> detect_cycles_via_spread_nodes(fragment_def, spread_nodes_of_fragment(fragment_def))
+    end
+
+    defp detect_cycles_via_spread_nodes(acc, _, []), do: acc
+    defp detect_cycles_via_spread_nodes(acc, fragment_def, spread_nodes) do
+      frag_name = fragment_def.name.value
+      acc = %{ acc | spread_path_indices:
+        Map.merge(acc[:spread_path_indices], %{frag_name => Stack.length(acc[:spread_path])})}
+
+      acc = process_spread_nodes(acc, spread_nodes)
+
+      %{ acc | spread_path_indices: Map.delete(acc[:spread_path_indices], frag_name)}
+    end
+
+    defp process_spread_nodes(acc, []), do: acc
+    defp process_spread_nodes(acc, [spread_node|rest]) do
+      spread_name = spread_node.name.value
+      cycle_index = Map.get(acc[:spread_path_indices], spread_name, nil)
+      process_spread_nodes(process_one_node(acc, spread_node, cycle_index), rest)
+    end
+
+    defp process_one_node(acc, spread_node, cycle_index) when is_integer(cycle_index) do
+      cycle_path = Enum.slice(
+        Enum.reverse(acc[:spread_path].elements),
+        cycle_index,
+        Stack.length(acc[:spread_path])
+      )
+      report_error(acc, cycle_error_message(spread_node.name.value, Enum.map(cycle_path, fn(s) -> s.name.value end)))
+    end
+
+    defp process_one_node(acc, spread_node, _) do
+      acc = %{ acc | spread_path: Stack.push(acc[:spread_path], spread_node)} 
+      if !visited?(acc, spread_node) do
+        spread_fragment = get_fragment(acc[:document], spread_node.name.value)
+        if spread_fragment do
+          acc = detect_cycles(acc, spread_fragment)
+        end
+      end
+      %{ acc | spread_path: Stack.pop(acc[:spread_path])} 
+    end
+
+    defp visited?(acc, node) do
+      Map.has_key?(acc[:visited_fragments], node.name.value)
+    end
+
+    defp mark_visited(acc, node) do
+      %{acc | visited_fragments:
+        Map.merge(acc[:visited_fragments], %{node.name.value => true})}
+    end
+
+    defp cycle_error_message(frag_name, spread_names) do
+      via = case length(spread_names) do
+        0 -> ""
+        _ -> " via #{Enum.join(spread_names, ", ")}"
+      end
+      "Cannot spread fragment #{frag_name} within itself#{via}."
+    end
+
+    defp get_fragment(document, fragment_name) do
+      Enum.find(document.definitions, fn(definition) ->
+        definition.kind == :FragmentDefinition && definition.name.value == fragment_name
+      end)
+    end
+
+    defp spread_nodes_of_fragment(fragment_def) do
+      spread_nodes_of_selection_sets([fragment_def.selectionSet])
+    end
+
+    defp spread_nodes_of_selection_sets([]), do: []
+    defp spread_nodes_of_selection_sets([selection_set|rest]) do
+      spread_nodes_of_selections(selection_set.selections) ++ spread_nodes_of_selection_sets(rest)
+    end
+
+    defp spread_nodes_of_selections([]), do: []
+    defp spread_nodes_of_selections([%{kind: :FragmentSpread} = selection|rest]) do
+      [selection] ++ spread_nodes_of_selections(rest)
+    end
+    defp spread_nodes_of_selections([%{selectionSet: selection_set}|rest]) do
+      spread_nodes_of_selection_sets([selection_set]) ++ spread_nodes_of_selections(rest)
+    end
+    defp spread_nodes_of_selections([_|rest]) do
+      spread_nodes_of_selections(rest)
+    end
+  end
+end

lib/graphql/validation/validator.ex

@@ -28,6 +28,7 @@ defmodule GraphQL.Validation.Validator do
     ])
     result = Reducer.reduce(document, validation_pipeline, %{
       type_info: %TypeInfo{schema: schema},
+      document: document,
       validation_errors: []
     })
     errors = result[:validation_errors]

test/graphql/validation/rules/no_fragment_cycles_test.exs

@@ -0,0 +1,202 @@
+
+Code.require_file "../../../support/validations.exs", __DIR__
+
+defmodule GraphQL.Validation.Rules.NoFragmentCyclesTest do
+  use ExUnit.Case, async: true
+
+  import ValidationsSupport
+
+  alias GraphQL.Validation.Rules.NoFragmentCycles, as: Rule
+
+  test "single reference is valid" do
+    assert_passes_rule(
+      """ 
+      fragment fragA on Dog { ...fragB }
+      fragment fragB on Dog { name }
+      """,
+      %Rule{}
+    )
+  end
+
+  test "spreading twice is not circular" do
+    assert_passes_rule(
+      """ 
+      fragment fragA on Dog { ...fragB, ...fragB }
+      fragment fragB on Dog { name }
+      """,
+      %Rule{}
+    )
+  end
+
+  test "spreading twice indirectly is not circular" do
+    assert_passes_rule(
+      """ 
+      fragment fragA on Dog { ...fragB, ...fragC }
+      fragment fragB on Dog { ...fragC }
+      fragment fragC on Dog { name }
+      """,
+      %Rule{}
+    )
+  end
+
+  test "double spread within abstract types" do
+    assert_passes_rule(
+      """ 
+      fragment nameFragment on Pet {
+        ... on Dog { name }
+        ... on Cat { name }
+      }
+
+      fragment spreadsInAnon on Pet {
+        ... on Dog { ...nameFragment }
+        ... on Cat { ...nameFragment }
+      }
+      """,
+      %Rule{}
+    )
+  end
+
+  test "does not false positive on unknown fragment" do
+    assert_passes_rule(
+      """ 
+      fragment nameFragment on Pet {
+        ...UnknownFragment
+      }
+      """,
+      %Rule{}
+    )
+  end
+
+  test "spreading recursively within field fails" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Human { relatives { ...fragA } }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself."]
+    )
+  end
+
+  test "no spreading itself directly" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Dog { ...fragA }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself."]
+    )
+  end
+
+  test "no spreading itself directly within inline fragment" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Pet {
+        ... on Dog {
+          ...fragA
+        }
+      }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself."]
+    )
+  end
+
+  test "no spreading itself indirectly" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Dog { ...fragB }
+      fragment fragB on Dog { ...fragA }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself via fragB."]
+    )
+  end
+
+  test "no spreading itself indirectly reports opposite order" do
+    assert_fails_rule(
+      """ 
+      fragment fragB on Dog { ...fragA }
+      fragment fragA on Dog { ...fragB }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragB within itself via fragA."]
+    )
+  end
+
+  test "no spreading itself indirectly within inline fragment" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Pet {
+        ... on Dog {
+          ...fragB
+        }
+      }
+      fragment fragB on Pet {
+        ... on Dog {
+          ...fragA
+        }
+      }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself via fragB."]
+    )
+  end
+
+  test "no spreading itself deeply" do
+    assert_fails_rule(
+      """ 
+      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 { ...fragP }
+      fragment fragP on Dog { ...fragA, ...fragX }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself via fragB, fragC, fragO, fragP.",
+       "Cannot spread fragment fragO within itself via fragP, fragX, fragY, fragZ."]
+    )
+  end
+
+  test "no spreading itself deeply two paths" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Dog { ...fragB, ...fragC }
+      fragment fragB on Dog { ...fragA }
+      fragment fragC on Dog { ...fragA }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself via fragB.",
+       "Cannot spread fragment fragA within itself via fragC."]
+    )
+  end
+
+  test "no spreading itself deeply two paths -- alt traverse order" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Dog { ...fragC }
+      fragment fragB on Dog { ...fragC }
+      fragment fragC on Dog { ...fragA, ...fragB }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragA within itself via fragC.",
+       "Cannot spread fragment fragC within itself via fragB."]
+    )
+  end
+
+  test "no spreading itself deeply and immediately" do
+    assert_fails_rule(
+      """ 
+      fragment fragA on Dog { ...fragB }
+      fragment fragB on Dog { ...fragB, ...fragC }
+      fragment fragC on Dog { ...fragA, ...fragB }
+      """,
+      %Rule{},
+      ["Cannot spread fragment fragB within itself.",
+       "Cannot spread fragment fragA within itself via fragB, fragC.",
+       "Cannot spread fragment fragB within itself via fragC."]
+    )
+  end
+end