Merge pull request #87 from freshtonic/refactor/executor

Visitor and executor refactorings

Author: Josh Price

lib/graphql/execution/executor.ex

@@ -45,8 +45,10 @@ defmodule GraphQL.Execution.Executor do
       :mutation     ->
         {context, result} = execute_fields_serially(context, type, root_value, fields)
         {:ok, result, context.errors}
-      :subscription -> {:error, "Subscriptions not currently supported"}
-      _             -> {:error, "Can only execute queries, mutations and subscriptions"}
+      :subscription ->
+        {:error, "Subscriptions not currently supported"}
+      _             ->
+        {:error, "Can only execute queries, mutations and subscriptions"}
     end
   end
 
@@ -107,15 +109,10 @@ defmodule GraphQL.Execution.Executor do
     end
   end
 
-  @spec execute_fields(ExecutionContext.t, atom | Map, any, any) :: {ExecutionContext.t, map}
-  defp execute_fields(context, parent_type, source_value, fields) when is_atom(parent_type) do
-    execute_fields(context, apply(parent_type, :type, []), source_value, fields)
-  end
-
   @spec execute_fields(ExecutionContext.t, atom | Map, any, any) :: {ExecutionContext.t, map}
   defp execute_fields(context, parent_type, source_value, fields) do
     Enum.reduce fields, {context, %{}}, fn({field_name_ast, field_asts}, {context, results}) ->
-      case resolve_field(context, parent_type, source_value, field_asts) do
+      case resolve_field(context, unwrap_type(parent_type), source_value, field_asts) do
         {context, :undefined} -> {context, results}
         {context, value} -> {context, Map.put(results, field_name_ast.value, value)}
       end
@@ -130,8 +127,7 @@ defmodule GraphQL.Execution.Executor do
 
   defp resolve_field(context, parent_type, source, field_asts) do
     field_ast = hd(field_asts)
-    # FIXME: possible memory leak with atoms
-    field_name = String.to_atom(field_ast.name.value)
+    field_name = String.to_existing_atom(field_ast.name.value)
 
     if field_def = field_definition(parent_type, field_name) do
       return_type = field_def.type
@@ -156,7 +152,7 @@ defmodule GraphQL.Execution.Executor do
 
       case FieldResolver.resolve(field_def, source, args, info) do
         {:ok, result} ->
-          complete_value_catching_error(context, return_type, field_asts, info, result)
+          complete_value(return_type, context, field_asts, info, result)
         {:error, message} ->
           {ExecutionContext.report_error(context, message), nil}
       end
@@ -165,65 +161,50 @@ defmodule GraphQL.Execution.Executor do
     end
   end
 
-  @spec complete_value_catching_error(ExecutionContext.t, any, GraphQL.Document.t, any, map) :: {ExecutionContext.t, map | nil}
-  defp complete_value_catching_error(context, return_type, field_asts, info, result) do
-    # TODO lots of error checking
-    complete_value(context, return_type, field_asts, info, result)
-  end
-
-  @spec complete_value(ExecutionContext.t, any, any, any, nil) :: {ExecutionContext.t, nil}
-  defp complete_value(context, _, _, _, nil), do: {context, nil}
+  @spec complete_value(any, ExecutionContext.t, any, any, nil) :: {ExecutionContext.t, nil}
+  defp complete_value(_, context, _, _, nil), do: {context, nil}
 
-  @spec complete_value(ExecutionContext.t, %ObjectType{}, GraphQL.Document.t, any, map) :: {ExecutionContext.t, map}
-  defp complete_value(context, %ObjectType{} = return_type, field_asts, _info, result) do
+  @spec complete_value(%ObjectType{}, ExecutionContext.t, GraphQL.Document.t, any, map) :: {ExecutionContext.t, map}
+  defp complete_value(%ObjectType{} = return_type, context, field_asts, _info, result) do
     {context, sub_field_asts} = collect_sub_fields(context, return_type, field_asts)
     execute_fields(context, return_type, result, sub_field_asts.fields)
   end
 
-  defp complete_value(context, %NonNull{ofType: inner_type}, field_asts, info, result) when is_atom(inner_type) do
-    complete_value(context, %NonNull{ofType: apply(inner_type, :type, [])}, field_asts, info, result)
-  end
-
-  @spec complete_value(ExecutionContext.t, %NonNull{}, GraphQL.Document.t, any, any) :: {ExecutionContext.t, map}
-  defp complete_value(context, %NonNull{ofType: inner_type}, field_asts, info, result) do
+  @spec complete_value(%NonNull{}, ExecutionContext.t, GraphQL.Document.t, any, any) :: {ExecutionContext.t, map}
+  defp complete_value(%NonNull{ofType: inner_type}, context, field_asts, info, result) do
     # TODO: Null Checking
-    complete_value(context, inner_type, field_asts, info, result)
+    complete_value(unwrap_type(inner_type), context, field_asts, info, result)
   end
 
-  @spec complete_value(ExecutionContext.t, %Interface{}, GraphQL.Document.t, any, any) :: {ExecutionContext.t, map}
-  defp complete_value(context, %Interface{} = return_type, field_asts, info, result) do
+  @spec complete_value(%Interface{}, ExecutionContext.t, GraphQL.Document.t, any, any) :: {ExecutionContext.t, map}
+  defp complete_value(%Interface{} = return_type, context, field_asts, info, result) do
     runtime_type = AbstractType.get_object_type(return_type, result, info.schema)
     {context, sub_field_asts} = collect_sub_fields(context, runtime_type, field_asts)
     execute_fields(context, runtime_type, result, sub_field_asts.fields)
   end
 
-  @spec complete_value(ExecutionContext.t, %Union{}, GraphQL.Document.t, any, any) :: {ExecutionContext.t, map}
-  defp complete_value(context, %Union{} = return_type, field_asts, info, result) do
+  @spec complete_value(%Union{}, ExecutionContext.t, GraphQL.Document.t, any, any) :: {ExecutionContext.t, map}
+  defp complete_value(%Union{} = return_type, context, field_asts, info, result) do
     runtime_type = AbstractType.get_object_type(return_type, result, info.schema)
     {context, sub_field_asts} = collect_sub_fields(context, runtime_type, field_asts)
     execute_fields(context, runtime_type, result, sub_field_asts.fields)
   end
 
-  defp complete_value(context, %List{ofType: list_type}, field_asts, info, result) when is_atom(list_type) do
-    complete_value(context, %List{ofType: apply(list_type, :type, [])}, field_asts, info, result)
-  end
-
-  @spec complete_value(ExecutionContext.t, %List{}, GraphQL.Document.t, any, any) :: map
-  defp complete_value(context, %List{ofType: list_type}, field_asts, info, result) do
+  @spec complete_value(%List{}, ExecutionContext.t, GraphQL.Document.t, any, any) :: map
+  defp complete_value(%List{ofType: list_type}, context, field_asts, info, result) do
     {context, result} = Enum.reduce result, {context, []}, fn(item, {context, acc}) ->
-      {context, value} = complete_value_catching_error(context, list_type, field_asts, info, item)
+      {context, value} = complete_value(unwrap_type(list_type), context, field_asts, info, item)
       {context, [value] ++ acc}
     end
     {context, Enum.reverse(result)}
   end
 
-  defp complete_value(context, return_type, field_asts, info, result) when is_atom(return_type) do
-    type = apply(return_type, :type, [])
-    complete_value(context, type, field_asts, info, result)
+  defp complete_value(return_type, context, field_asts, info, result) when is_atom(return_type) do
+    complete_value(unwrap_type(return_type), context, field_asts, info, result)
   end
 
-  defp complete_value(context, return_type, _field_asts, _info, result) do
-    {context, GraphQL.Types.serialize(return_type, result)}
+  defp complete_value(return_type, context, _field_asts, _info, result) do
+    {context, GraphQL.Types.serialize(unwrap_type(return_type), result)}
   end
 
   defp collect_sub_fields(context, return_type, field_asts) do
@@ -311,7 +292,7 @@ defmodule GraphQL.Execution.Executor do
   def value_from_ast(nil, _, _), do: nil # remove once NonNull is actually done..
 
   def value_from_ast(value_ast, type, variable_values) when is_atom(type) do
-    value_from_ast(value_ast, apply(type, :type, []), variable_values)
+    value_from_ast(value_ast, type.type, variable_values)
   end
 
   def value_from_ast(value_ast, type, _) do
@@ -355,4 +336,7 @@ defmodule GraphQL.Execution.Executor do
       true -> runtime_type.name == typed_condition.name
     end
   end
+
+  defp unwrap_type(type) when is_atom(type), do: type.type
+  defp unwrap_type(type), do: type
 end

lib/graphql/execution/field_resolver.ex

@@ -3,14 +3,20 @@ defmodule GraphQL.Execution.FieldResolver do
   alias GraphQL.Execution.Resolvable
 
   def resolve(field_def, source, args, info) do
-    Resolvable.resolve(Map.get(field_def, :resolve), deref_source(source), args, info)
+    Resolvable.resolve(
+      Map.get(field_def, :resolve),
+      unwrap_type(source),
+      args,
+      info
+    )
   end
 
-  defp deref_source(nil), do: nil
-  defp deref_source(source) when is_atom(source) do
-    apply(source, :type, [])
+  # FIXME: this `unwrap_type` logic is duplicated in a few places.
+  defp unwrap_type(nil), do: nil
+  defp unwrap_type(source) when is_atom(source) do
+    source.type
   end
-  defp deref_source(source), do: source
+  defp unwrap_type(source), do: source
 end
 
 

lib/graphql/execution/resolvable.ex

@@ -20,9 +20,16 @@ alias GraphQL.Execution.ResolveWrapper
 defimpl GraphQL.Execution.Resolvable, for: Function do
   def resolve(fun, source, args, info) do
     ResolveWrapper.wrap fn() ->
-      fun.(source, args, info)
+      case arity(fun) do
+        0 -> fun.()
+        1 -> fun.(source)
+        2 -> fun.(source, args)
+        3 -> fun.(source, args, info)
+      end
     end
   end
+
+  defp arity(fun), do: :erlang.fun_info(fun)[:arity]
 end
 
 defimpl GraphQL.Execution.Resolvable, for: Tuple  do

lib/graphql/lang/ast/parallel_visitor.ex

@@ -11,9 +11,11 @@ defmodule GraphQL.Lang.AST.ParallelVisitor do
   This code based on the graphql-js *visitInParallel* function.
   """
 
-  alias GraphQL.Lang.AST.Visitor
-  alias GraphQL.Lang.AST.InitialisingVisitor
-  alias GraphQL.Lang.AST.PostprocessingVisitor
+  alias GraphQL.Lang.AST.{
+    Visitor,
+    InitialisingVisitor,
+    PostprocessingVisitor
+  }
 
   defstruct visitors: []
 

lib/graphql/lang/ast/type_info_visitor.ex

@@ -94,7 +94,6 @@ defmodule GraphQL.Lang.AST.TypeInfoVisitor do
           type = case node.operation do
             :query -> accumulator[:type_info].schema.query
             :mutation -> accumulator[:type_info].schema.mutation
-            # TODO 'subscription' -> schema.subscription
             _ -> raise "node operation #{node.operation} not handled"
           end
           stack_push(:type_stack, type)

lib/graphql/type/introspection.ex

@@ -35,24 +35,24 @@ defmodule GraphQL.Type.Introspection do
           types: %{
             description: "A list of all types supported by this server.",
             type: %NonNull{ofType: %List{ofType: %NonNull{ofType: Type}}},
-            resolve: fn(schema, _, _) ->
+            resolve: fn(schema) ->
               Map.values(schema.type_cache)
             end
           },
           queryType: %{
             description: "The type that query operations will be rooted at.",
             type: %NonNull{ofType: Type},
-            resolve: fn(%{query: query}, _, _) -> query end
+            resolve: fn(%{query: query}) -> query end
           },
           mutationType: %{
             description: "If this server supports mutation, the type that mutation operations will be rooted at.",
             type: Type,
-            resolve: fn(%{mutation: mutation}, _, _) -> mutation end
+            resolve: fn(%{mutation: mutation}) -> mutation end
           },
           subscriptionType: %{
             description: "If this server support subscription, the type that subscription operations will be rooted at.",
             type: Type,
-            resolve: nil #fn(%{subscription: subscription}, _, _) -> subscription end
+            resolve: nil #fn(%{subscription: subscription}, _, _,_) -> subscription end
           },
           directives: %{
             description: "A list of all directives supported by this server.",
@@ -112,7 +112,7 @@ defmodule GraphQL.Type.Introspection do
         fields: %{
           kind: %{
             type: %NonNull{ofType: TypeKind},
-            resolve: fn(schema, _, _) ->
+            resolve: fn(schema) ->
               case schema do
                 %ObjectType{} -> "OBJECT"
                 %Interface{} -> "INTERFACE"
@@ -137,22 +137,22 @@ defmodule GraphQL.Type.Introspection do
             type: %List{ofType: %NonNull{ofType: Field}},
             args: %{includeDeprecated: %{type: %Boolean{}, defaultValue: false}},
             resolve: fn
-              (%ObjectType{} = schema, _, _) ->
+              (%ObjectType{} = schema) ->
                 thunk_fields = CompositeType.get_fields(schema)
                 Enum.map(thunk_fields, fn({n, v}) -> Map.put(v, :name, n) end)
                 # |> filter_deprecated
-              (%Interface{} = schema, _, _) ->
+              (%Interface{} = schema) ->
                 thunk_fields = CompositeType.get_fields(schema)
                 Enum.map(thunk_fields, fn({n, v}) -> Map.put(v, :name, n) end)
-              (_, _, _) -> nil
+              (_) -> nil
             end
           },
           interfaces: %{
             type: %List{ofType: %NonNull{ofType: Type}},
             resolve: fn
-              (%ObjectType{} = schema, _, _) ->
+              (%ObjectType{} = schema) ->
                 schema.interfaces
-              (_, _, _) -> nil
+              (_) -> nil
             end
           },
           possibleTypes: %{
@@ -169,22 +169,22 @@ defmodule GraphQL.Type.Introspection do
             type: %List{ofType: %NonNull{ofType: EnumValue}},
             args: %{includeDeprecated: %{type: %Boolean{}, defaultValue: false}},
             resolve: fn
-              (%GraphQL.Type.Enum{} = schema, _, _) -> schema.values
-              (_, _, _) -> nil
+              (%GraphQL.Type.Enum{} = schema) -> schema.values
+              (_) -> nil
             end
           },
           inputFields: %{
             type: %List{ofType: %NonNull{ofType: InputValue}},
             resolve: fn
-              (%GraphQL.Type.Input{} = type, _args, _info) ->
+              (%GraphQL.Type.Input{} = type) ->
                 fields = type.fields
                 Enum.map(Map.keys(fields), fn(key) ->
                   %{
                     name: key,
                     type: fields[key].type
                   }
                 end)
-              (_, _, _) -> nil
+              (_) -> nil
             end
           },
           ofType: %{type: Type}
@@ -251,9 +251,9 @@ defmodule GraphQL.Type.Introspection do
           args: %{
             type: %NonNull{ofType: %List{ofType: %NonNull{ofType: InputValue}}},
             resolve: fn
-              (%{args: _args} = schema, _, _) ->
+              (%{args: _args} = schema) ->
                 Enum.map(schema.args, fn({name, v}) -> Map.put(v, :name, name) end)
-              (_, _, _) ->  []
+              (_) ->  []
             end
           },
           type: %{type: %NonNull{ofType: Type}},

lib/graphql/validation/validator.ex

@@ -25,7 +25,7 @@ defmodule GraphQL.Validation.Validator do
   Short circuits the validations entirely when there are no rules specified.
   This is useful for examining the performance impact of validations.
   """
-  def validate_with_rules(schema, document, rules) when length(rules) == 0, do: :ok
+  def validate_with_rules(_schema, _document, rules) when length(rules) == 0, do: :ok
 
   @doc """
   For performance testing with a single rule, the overhead of the ParallelVisitor

test/graphql/execution/executor_blog_schema_test.exs

@@ -58,7 +58,7 @@ defmodule GraphQL.Execution.Executor.ExecutorBlogSchemaTest do
             height: %{type: %Int{}}
           },
           type: image,
-          resolve: fn(o, %{width: w, height: h}, _) -> o.pic.(w, h) end
+          resolve: fn(o, %{width: w, height: h}) -> o.pic.(w, h) end
         },
         recentArticle: nil
       }
@@ -86,11 +86,11 @@ defmodule GraphQL.Execution.Executor.ExecutorBlogSchemaTest do
         article: %{
           type: article,
           args: %{id: %{type: %ID{}}},
-          resolve: fn(_, %{id: id}, _) -> make_article(id) end
+          resolve: fn(_, %{id: id}) -> make_article(id) end
         },
         feed: %{
           type: %List{ofType: article},
-          resolve: fn(_, _, _) -> for id <- 1..2, do: make_article(id) end
+          resolve: fn() -> for id <- 1..2, do: make_article(id) end
         }
       }
     }