absinthe-graphql / absinthe

The GraphQL toolkit for Elixir
http://absinthe-graphql.org
Other
4.3k stars 527 forks source link

Incorrect resolution of inner fields for union types with fields with same name and type #875

Closed mpoeter closed 4 years ago

mpoeter commented 4 years ago

Environment

We have a union type where some objects have a field with the same name of the same type - and that type is again an (inner) object. For different (union type) objects, we request different fields of this inner object. When returning a list of such union types, the fields for these inner objects should be returned as requested. But what actually happens is that the list of fields for the inner type depends on the first object of the list. It is rather complicated to describe the scenario, so I have created a minimal example with a unit test to reproduce this issue:

defmodule Bug do
  defmodule Data1 do
    defstruct [
      inner: %{ id: "id", field: "field" }
    ]
  end

  defmodule Data2 do
    defstruct [
      inner: %{ id: "id", field: "field" }
    ]
  end

  defmodule Schema do
    use Absinthe.Schema

    union :data do
      types [:data1, :data2]
      resolve_type fn
        %Bug.Data1{}, _ -> :data1
        %Bug.Data2{}, _ -> :data2
      end
    end

    object :inner do
      field :id, non_null(:id)
      field :field, non_null(:string)
    end

    object :data1 do
      field :inner, non_null(:inner)
    end

    object :data2 do
      field :inner, non_null(:inner)
    end

    query do
      field :data, list_of(non_null(:data)) do
        resolve fn _, _ ->
          {:ok, [ %Bug.Data1{}, %Bug.Data2{} ]}
        end
      end

      field :data_reverse, list_of(non_null(:data)) do
        resolve fn _, _ ->
          {:ok, [ %Bug.Data2{}, %Bug.Data1{} ]}
        end
      end
    end
  end
end
defmodule BugTest do
  use ExUnit.Case
  doctest Bug

  test "fetch data" do
    query = """
      {
        data {
          ... on Data1 { __typename, inner { id } }
          ... on Data2 { __typename, inner { field } }
        }
      }
    """
    assert {:ok, %{data: %{"data" => data}}} = Absinthe.run(query, Bug.Schema)
    assert [
      %{"__typename" => "Data1", "inner" => %{"id" => "id"}},
      %{"__typename" => "Data2", "inner" => %{"field" => "field"}}
    ] == data
  end

  test "fetch data_reverse" do
    query = """
      {
        data_reverse {
          ... on Data1 { __typename, inner { id } }
          ... on Data2 { __typename, inner { field } }
        }
      }
    """
    assert {:ok, %{data: %{"data_reverse" => data}}} = Absinthe.run(query, Bug.Schema)
    assert [
      %{"__typename" => "Data2", "inner" => %{"field" => "field"}},
      %{"__typename" => "Data1", "inner" => %{"id" => "id"}}
    ] == data
  end
end

These are the test results:

  1) test fetch data_reverse (BugTest)
     test/bug_test.exs:21
     Assertion with == failed
     code:  assert [%{"__typename" => "Data2", "inner" => %{"field" => "field"}}, %{"__typename" => "Data1", "inner" => %{"id" => "id"}}] == data
     left:  [%{"__typename" => "Data2", "inner" => %{"field" => "field"}}, %{"__typename" => "Data1", "inner" => %{"id" => "id"}}]
     right: [%{"__typename" => "Data2", "inner" => %{"field" => "field"}}, %{"__typename" => "Data1", "inner" => %{"field" => "field"}}]
     stacktrace:
       test/bug_test.exs:31: (test)

  2) test fetch data (BugTest)
     test/bug_test.exs:5
     Assertion with == failed
     code:  assert [%{"__typename" => "Data1", "inner" => %{"id" => "id"}}, %{"__typename" => "Data2", "inner" => %{"field" => "field"}}] == data
     left:  [%{"__typename" => "Data1", "inner" => %{"id" => "id"}}, %{"__typename" => "Data2", "inner" => %{"field" => "field"}}]
     right: [%{"__typename" => "Data1", "inner" => %{"id" => "id"}}, %{"__typename" => "Data2", "inner" => %{"id" => "id"}}]
     stacktrace:
       test/bug_test.exs:15: (test)

It does however work if the fields have different names (e.g. inner and inner2), or different types.

FWIW: the error seems to be fixed in 1.5.0-rc3, but I have not found similar issue, so it might be worth adding a test to avoid regressions.

benwilson512 commented 4 years ago

Hi @mpoeter this could definitely be a good test case to add to the 1.5 release to avoid regressions. With 1.5 in RC, we aren't planning on tackling bugs like this any future 1.4 releases.

mpoeter commented 4 years ago

Make sense, especially since there is an easy workaround (always request the superset of required fields on the inner type). Feel free to close this anytime you like.

binaryseed commented 4 years ago

PRs welcome for tests!