Traverse deeply nested lists when searching for null violations #1275
Conversation
mattbaker
changed the title
| Absinthe.run(doc, Schema) | ||
| end | ||
|
|
||
| test "deeply nested non-nullable value inside non-nullable lists cannot be null" do |
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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.
|
@benwilson512: Full transparency — this is a complex file and I don't know if what I'm doing is the right thing here, it could definitely benefit from your eyes! It seems like, one way or another, this boils down to the fact that a sufficiently deep wrapping of |
|
@benwilson512 - Curious if you have any thoughts, but I'm also sure you're busy and I know this is probably more than a surface fix! I could try to set some time aside this month to dive deeper and build some more familiarity myself and maybe emerge with a bit more confidence too, if you think you won't have time to take a look. It's also not super urgent for us. |
| 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]) |
There was a problem hiding this comment.
Appending to this list here will be O(n^2) and the lists could potentially be quite large. I'm working to refactor this.
There was a problem hiding this comment.
Oh very good catch, I breezed right past that!
|
Not a problem at all! It was an odd edge case that was on our "to do" list but wasn't urgent at all. Thanks so much! |
Attempts to fix #1274 by traversing through layers of non-null/list wrappers when looking for non-null constraint violations.