Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Traverse deeply nested lists when searching for null violations #1275

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 34 additions & 27 deletions lib/absinthe/phase/document/execution/resolution.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
68 changes: 68 additions & 0 deletions test/absinthe/phase/execution/non_null_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should be redundant when test "deeply nested nullable value inside non-nullable lists can be null" exists below, but the latter actually passes in master for the same reason test "deeply nested non-nullable value inside non-nullable lists cannot be null" fails (I think).

So I ended up keeping this test just for my own sanity.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't the issue I saw when I started digging into this, but this test also fails on master. I think it's in the same theme of needing to traverse through multiple layers of lists/non-nulls.

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 = """
{
Expand Down
Loading