Merge pull request #1275 from mattbaker/non-null-list-non-null-list-n…

…ullable

Traverse deeply nested lists when searching for null violations

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased
 
 - Bugfix: [Handle non_null(list_of(:thing)) with null list elements properly](https://github.com/absinthe-graphql/absinthe/pull/1259)
+- Bugfix: [More non null result handling improvements](https://github.com/absinthe-graphql/absinthe/pull/1275)
 
 ## 1.7.4
 

lib/absinthe/phase/document/execution/resolution.ex

@@ -285,30 +285,29 @@ defmodule Absinthe.Phase.Document.Execution.Resolution do
     |> propagate_null_trimming
   end
 
-  defp maybe_add_non_null_error([], nil, %Type.NonNull{}) do
+  defp maybe_add_non_null_error(errors, value, type, path \\ [])
+
+  defp maybe_add_non_null_error([], nil, %Type.NonNull{}, []) do
     ["Cannot return null for non-nullable field"]
   end
 
-  # Unwrap the non null so we can check again for the possible case of a non null
-  # inside of a list. We want that clause to handle both
-  # - `non_null(list_of(non_null(:thing)))`
-  # - `list_of(non_null(:thing))`
-  # Thus the single layer of unwrapping here.
-  defp maybe_add_non_null_error([], values, %Type.NonNull{of_type: type}) do
-    maybe_add_non_null_error([], values, type)
+  defp maybe_add_non_null_error([], nil, %Type.NonNull{}, path) do
+    [%{message: "Cannot return null for non-nullable field", path: Enum.reverse(path)}]
+  end
+
+  defp maybe_add_non_null_error([], value, %Type.NonNull{of_type: %Type.List{} = type}, path) do
+    maybe_add_non_null_error([], value, type, path)
   end
 
-  defp maybe_add_non_null_error([], values, %Type.List{of_type: %Type.NonNull{}})
-       when is_list(values) do
+  defp maybe_add_non_null_error([], [_ | _] = values, %Type.List{of_type: type}, path) do
     values
     |> Enum.with_index()
-    |> Enum.filter(&is_nil(elem(&1, 0)))
-    |> Enum.map(fn {_value, index} ->
-      %{message: "Cannot return null for non-nullable field", path: [index]}
+    |> Enum.flat_map(fn {value, index} ->
+      maybe_add_non_null_error([], value, type, [index | path])
     end)
   end
 
-  defp maybe_add_non_null_error(errors, _, _) do
+  defp maybe_add_non_null_error(errors, _, _, _path) do
     errors
   end
 
@@ -360,28 +359,36 @@ defmodule Absinthe.Phase.Document.Execution.Resolution do
     false
   end
 
+  defp non_null_list_violation?(%{values: values}) do
+    Enum.find(values, &non_null_list_violation?/1)
+  end
+
   # FIXME: Not super happy with this lookup process.
   # Also it would be nice if we could use the same function as above.
-  defp non_null_list_violation?(%{
-         value: nil,
-         emitter: %{schema_node: %{type: %Type.List{of_type: %Type.NonNull{}}}}
-       }) do
-    true
+  defp non_null_list_violation?(%{value: nil, emitter: %{schema_node: %{type: type}}}) do
+    !null_allowed_in_list?(type)
   end
 
-  defp non_null_list_violation?(%{
-         value: nil,
-         emitter: %{
-           schema_node: %{type: %Type.NonNull{of_type: %Type.List{of_type: %Type.NonNull{}}}}
-         }
-       }) do
-    true
+  defp non_null_list_violation?(_) do
+    false
   end
 
-  defp non_null_list_violation?(_) do
+  defp null_allowed_in_list?(%Type.List{of_type: wrapped_type}) do
+    null_allowed_in_list?(wrapped_type)
+  end
+
+  defp null_allowed_in_list?(%Type.NonNull{of_type: %Type.List{of_type: wrapped_type}}) do
+    null_allowed_in_list?(wrapped_type)
+  end
+
+  defp null_allowed_in_list?(%Type.NonNull{of_type: _wrapped_type}) do
     false
   end
 
+  defp null_allowed_in_list?(_type) do
+    true
+  end
+
   defp add_errors(result, errors, fun) do
     Enum.reduce(errors, result, fun)
   end

test/absinthe/phase/execution/non_null_test.exs

@@ -96,6 +96,29 @@ defmodule Absinthe.Phase.Document.Execution.NonNullTest do
         resolve &things_resolver/3
       end
 
+      field :non_null_list_of_non_null_list_of_nullable,
+            non_null(list_of(non_null(list_of(:string)))) do
+        resolve fn _, _ ->
+          {:ok, [["ok", nil]]}
+        end
+      end
+
+      field :deeply_nested_non_nullable_list_of_nullable,
+            non_null(list_of(non_null(list_of(non_null(list_of(non_null(list_of(:string)))))))) do
+        resolve fn _, _ ->
+          {:ok, [[[["ok", nil]]]]}
+        end
+      end
+
+      field :deeply_nested_non_nullable_list_of_non_nullable,
+            non_null(
+              list_of(non_null(list_of(non_null(list_of(non_null(list_of(non_null(:string))))))))
+            ) do
+        resolve fn _, _ ->
+          {:ok, [[[["1", nil, "3"], ["4", nil, "6"]]]]}
+        end
+      end
+
       @desc """
       A field declared to be non null.
 
@@ -217,6 +240,51 @@ defmodule Absinthe.Phase.Document.Execution.NonNullTest do
   end
 
   describe "lists" do
+    test "non-null list of non-null list of nullable value returns null value" do
+      doc = """
+      {
+        nonNullListOfNonNullListOfNullable
+      }
+      """
+
+      assert {:ok, %{data: %{"nonNullListOfNonNullListOfNullable" => [["ok", nil]]}}} ==
+               Absinthe.run(doc, Schema)
+    end
+
+    test "deeply nested nullable value inside non-nullable lists can be null" do
+      doc = """
+      {
+        deeplyNestedNonNullableListOfNullable
+      }
+      """
+
+      assert {:ok, %{data: %{"deeplyNestedNonNullableListOfNullable" => [[[["ok", nil]]]]}}} ==
+               Absinthe.run(doc, Schema)
+    end
+
+    test "deeply nested non-nullable value inside non-nullable lists cannot be null" do
+      doc = """
+      {
+        deeplyNestedNonNullableListOfNonNullable
+      }
+      """
+
+      errors = [
+        %{
+          locations: [%{column: 3, line: 2}],
+          message: "Cannot return null for non-nullable field",
+          path: ["deeplyNestedNonNullableListOfNonNullable", 0, 0, 0, 1]
+        },
+        %{
+          locations: [%{column: 3, line: 2}],
+          message: "Cannot return null for non-nullable field",
+          path: ["deeplyNestedNonNullableListOfNonNullable", 0, 0, 1, 1]
+        }
+      ]
+
+      assert {:ok, %{data: nil, errors: errors}} == Absinthe.run(doc, Schema)
+    end
+
     test "list of nullable things returns an error when child has a null violation" do
       doc = """
       {