Fix non-null-list-of-non-null error when root data is nil #1259
Conversation
There was a problem hiding this comment.
Hey @tamanugi thanks for taking a crack at fixing this!
This fix may be useful on its own, but it is not testing the scenario that the issue outlines. At a core level, the issue is about fields with return type non_null(list_of(:string)) but you are testing fields with return type non_null(list_of(non_null(:string))).
You need a test that uses field return type non_null(list_of(:string)) and exhibits the error message in the issue, that is then fixed by your changes.
Your change DOES fix a crash, which is great! This might be the same root cause as the crash described in the issue but since the return types don't match I'm not sure.
Please add in this test and we'll be good to go!
|
Hi @benwilson512 , Thanks for review ✨ You are right, the test should have been for I have fixed the test 👍 |
|
Hey @tamanugi thanks for your continued effort. I'm going to push some changes up I think that better distinguish between I'm not entirely convinced that this does the right thing for lists of nullable that return |
| test "list of non null things works when child is null" do | ||
| doc = """ | ||
| { | ||
| nonNullListOfNonNull(makeNull: true) { __typename } | ||
| nonNullListOfNonNull(makeChildNull: true) { __typename } |
There was a problem hiding this comment.
I don't see why this test is modified.
There was a problem hiding this comment.
OH I see the semantics of make_null was changed.
|
OK I see what you were doing now. In the future as a bit of advice for open source contributions please add some comments to the PR to explain why you're changing existing tests. I get why you did it now because we need to distinguish between making the whole field |
|
Right and the thing is if you're changing the argument name then you should change EVERY test that uses that argument because now they're testing different things. |
| @@ -285,7 +285,21 @@ defmodule Absinthe.Phase.Document.Execution.Resolution do | |||
| |> propagate_null_trimming | |||
| end | |||
|
|
|||
| defp maybe_add_non_null_error([], values, %Type.NonNull{of_type: %Type.List{}}) do | |||
| defp maybe_add_non_null_error([], nil, %Type.NonNull{}) do | |||
There was a problem hiding this comment.
Moved this up as this the most fundamental check, best to do it first.
| maybe_add_non_null_error([], values, type) | ||
| end | ||
|
|
||
| defp maybe_add_non_null_error([], values, %Type.List{of_type: %Type.NonNull{}}) |
There was a problem hiding this comment.
Before, this clause would happen for any non_null(list_of( regardless of wether the inner type was non_null or not. Now we check the inner type properly.
|
@benwilson512 |
|
Thanks for your contribution! |
Thanks for awesome library ✨
This PR is fix #1256