From 28f26f62ddf6d90414a09ae144fcb30636719e45 Mon Sep 17 00:00:00 2001 From: Matt Baker Date: Tue, 5 Sep 2023 14:46:46 -0700 Subject: [PATCH 1/5] Add test demonstrating odd behavior --- .../absinthe/phase/execution/non_null_test.exs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/absinthe/phase/execution/non_null_test.exs b/test/absinthe/phase/execution/non_null_test.exs index f65d401b03..2369777cd4 100644 --- a/test/absinthe/phase/execution/non_null_test.exs +++ b/test/absinthe/phase/execution/non_null_test.exs @@ -96,6 +96,13 @@ 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 + @desc """ A field declared to be non null. @@ -217,6 +224,17 @@ defmodule Absinthe.Phase.Document.Execution.NonNullTest do end describe "lists" do + test "a field that is a non-null list of a non-null list of a nullable value will return a null value" do + doc = """ + { + nonNullListOfNonNullListOfNullable + } + """ + + assert {:ok, %{data: %{"nonNullListOfNonNullListOfNullable" => [["ok", nil]]}}} == + Absinthe.run(doc, Schema) + end + test "list of nullable things returns an error when child has a null violation" do doc = """ { From 3e5fafc747386249fba616bc8f94da5c71ffd20b Mon Sep 17 00:00:00 2001 From: Matt Baker Date: Fri, 8 Sep 2023 18:37:40 -0700 Subject: [PATCH 2/5] more tests --- .../phase/execution/non_null_test.exs | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/test/absinthe/phase/execution/non_null_test.exs b/test/absinthe/phase/execution/non_null_test.exs index 2369777cd4..c14d401007 100644 --- a/test/absinthe/phase/execution/non_null_test.exs +++ b/test/absinthe/phase/execution/non_null_test.exs @@ -103,6 +103,22 @@ defmodule Absinthe.Phase.Document.Execution.NonNullTest do 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. @@ -224,7 +240,7 @@ defmodule Absinthe.Phase.Document.Execution.NonNullTest do end describe "lists" do - test "a field that is a non-null list of a non-null list of a nullable value will return a null value" do + test "non-null list of non-null list of nullable value returns null value" do doc = """ { nonNullListOfNonNullListOfNullable @@ -235,6 +251,40 @@ defmodule Absinthe.Phase.Document.Execution.NonNullTest do 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 = """ { From 006b3135e3be8c9fff138b431d80217bf6612947 Mon Sep 17 00:00:00 2001 From: Matt Baker Date: Fri, 8 Sep 2023 18:38:11 -0700 Subject: [PATCH 3/5] traverse deeply nested lists when searching for nullable violations --- .../phase/document/execution/resolution.ex | 64 ++++++++++--------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/lib/absinthe/phase/document/execution/resolution.ex b/lib/absinthe/phase/document/execution/resolution.ex index f831a5d613..a16a5c6f62 100644 --- a/lib/absinthe/phase/document/execution/resolution.ex +++ b/lib/absinthe/phase/document/execution/resolution.ex @@ -285,30 +285,28 @@ 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: 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 - 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]} + defp maybe_add_non_null_error([], [_ | _] = values, %Type.List{of_type: type}, path) do + Enum.with_index(values) + |> Enum.flat_map(fn {value, index} -> + maybe_add_non_null_error([], value, type, path ++ [index]) end) end - defp maybe_add_non_null_error(errors, _, _) do + defp maybe_add_non_null_error(errors, _, _, _path) do errors end @@ -360,28 +358,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 From 708375ac3ddeea3f37e97fb48a1f8ed330cf284b Mon Sep 17 00:00:00 2001 From: Ben Wilson Date: Sat, 18 Nov 2023 08:21:14 -0500 Subject: [PATCH 4/5] tweak index handling in the path --- lib/absinthe/phase/document/execution/resolution.ex | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/absinthe/phase/document/execution/resolution.ex b/lib/absinthe/phase/document/execution/resolution.ex index a16a5c6f62..f5b01e5c19 100644 --- a/lib/absinthe/phase/document/execution/resolution.ex +++ b/lib/absinthe/phase/document/execution/resolution.ex @@ -292,7 +292,7 @@ defmodule Absinthe.Phase.Document.Execution.Resolution do end defp maybe_add_non_null_error([], nil, %Type.NonNull{}, path) do - [%{message: "Cannot return null for non-nullable field", path: path}] + [%{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 @@ -300,9 +300,10 @@ defmodule Absinthe.Phase.Document.Execution.Resolution do end defp maybe_add_non_null_error([], [_ | _] = values, %Type.List{of_type: type}, path) do - Enum.with_index(values) + values + |> Enum.with_index() |> Enum.flat_map(fn {value, index} -> - maybe_add_non_null_error([], value, type, path ++ [index]) + maybe_add_non_null_error([], value, type, [index | path]) end) end From f69e0d077c812e0a5bb0aeaab24f8541a74afba8 Mon Sep 17 00:00:00 2001 From: Ben Wilson Date: Sat, 18 Nov 2023 08:22:45 -0500 Subject: [PATCH 5/5] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42eb3b6972..8f95305cd2 100644 --- a/CHANGELOG.md +++ b/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