Postgres Queries have wrong where clause

barnabasj
2023-06-22

barnabasj:

Recently we some read actions do not return the correct data, the Queries that are logged have an obviously wrong where clause

SELECT c0."id", c0."first_name", c0."last_name", c0."title", c0."legal_entity_name", c0."is_legal_entity" FROM "contact" AS c0 WHERE (false) []

The clause should be c0."id" = ? with the id in the parameter list

This is the resource and we query the current_user graphql query

defmodule Demo.Auth.Resources.User do
  @moduledoc """
  The Logged in User
  """
  use Demo.AuditedResource,
    data_layer: AshPostgres.DataLayer,
    extensions: [AshGraphql.Resource]

  attributes do
    uuid_primary_key :id, generated?: false

    attribute :email, :string
    attribute :email_confirmed_at, :utc_datetime

    attribute :contact_id, :uuid

    attribute :features, Demo.Auth.Resources.User.Features

    #  attribute :roles, {:array, :atom}

    create_timestamp :inserted_at
    update_timestamp :updated_at
  end

  rbac do
    bypass(:admin)
    role(:user, [:id, :email, :email_confirmed_at, :contact, :contact_id, :features])
  end

  audit do
    actors?(false)
    timestamps?(false)
  end

  graphql do
    type :user

    queries do
      read_one :user, :current_user
    end
  end

  relationships do
    belongs_to :contact, Demo.CustomerService.Resources.Contact do
      filterable? false
      api Demo.CustomerService.Api
    end
  end

  actions do
    defaults [:read, :create, :update]

    read :current_user do
      get? true
    end
  end

  policies do
    policy always() do
      forbid_unless(actor_present())

      authorize_if(expr(id == type(^actor(:id), Ash.Type.UUID)))

      forbid_if(always())
    end
  end

  postgres do
    repo Demo.Repo
    table "user"
  end
end

We could also observe this behaviour when loading a has_one relationship

Versions:

ash: 06329b97cf531b6b585630638028233a48a7fa0b ash_graphql: c70e7dec7dac1aac7fd40a67b51a6d61d67f9d41 ash_postgres: 9e31f905861c8f97bb2b54fd8604eb362391e675

barnabasj:

In the case of the user we found that there is a problem with the embedded resource, if the attribute features is NULL in postgres the api returns null as the user, if there is a value in the column it returns the user. We can also get the user with the null in features if we exclude it in the graphql Query.

On the relationship side, we only get the relationship if we have also included the foreign key in the graphql query.

zachdaniel:

šŸ¤” so that soudns like maybe two bugs

zachdaniel:

hmmmmm….

zachdaniel:

have you reproduced this in code?

zachdaniel:

skipping ash_graphql I mean

zachdaniel:

ohhhhhhh

barnabasj:

not without it no

zachdaniel:

I think I see at least one problem, maybe

zachdaniel:

one sec

zachdaniel:

got one of these reproduced in test

zachdaniel:

the nil attribute

zachdaniel:

okay can you try ash main

barnabasj:

on it

barnabasj:

looks good after quick check

barnabasj:

I’m still unable to reproduce the relationship one in a test, still trying though šŸ˜‰

zachdaniel:

wait

zachdaniel:

maybe I don’t understand

zachdaniel:

are you saying that there is still an issue?

barnabasj:

yes, the second problem is still there.

let me describe it again:

~~If I query the api like this

offer {
  id
  owner {
    name
  }
}

I get this back

{
  "offer": {
    "id" "offer-id"
    "owner": null
  }
}

but if I do

offer {
  id
  owner_id
  owner {
    name
  }
}

i get this back

{
  "offer": {
    "id" "offer-id"
    "owner": {
      "name": "Max"
    }
  }
}

zachdaniel:

okay, will look into it.

barnabasj:

Thanks

barnabasj:

I checked the Query from the FE again and it seems that it has to do something with fragments

barnabasj:

This is the actual query that the frontend sent.

query Offer {
  offer {
    id
    ...Owner
  }
}

fragment Owner on Offer {
    id
    owner {
      name
    }
  }

with the fragment inlined it works

or adding the ownerId in the fragment also works

zachdaniel:

okay, thanks for the info

zachdaniel:

i can probably fix it with that

barnabasj:

amazing

zachdaniel:

hmm….actually still trying to reproduce this

zachdaniel:

    resp =
      """
      query Comments {
        listComments {
          ...CommentPost
        }
      }

      fragment CommentPost on Comment {
        post {
          text
        }
      }
      """
      |> Absinthe.run(AshGraphql.Test.Schema)

    assert {:ok, result} = resp

    refute Map.has_key?(result, :errors)
    post_id = post.id

    assert %{data: %{"listComments" => [%{"post" => %{"text" => "foo"}}]}} = result

zachdaniel:

oh there is a difference

zachdaniel:

  test "reading relationships works with fragments, without selecting the id field" do
    post =
      AshGraphql.Test.Post
      |> Ash.Changeset.for_create(:create, text: "foo", published: true)
      |> AshGraphql.Test.Api.create!()

    AshGraphql.Test.Comment
    |> Ash.Changeset.for_create(:create, %{text: "stuff"})
    |> Ash.Changeset.force_change_attribute(:post_id, post.id)
    |> AshGraphql.Test.Api.create!()

    resp =
      """
      query PostLibrary($published: Boolean) {
        postLibrary(published: $published) {
          text
          ...PostFragment
        }
      }

      fragment PostFragment on Post {
        comments{
          text
        }
      }
      """
      |> Absinthe.run(AshGraphql.Test.Schema,
        variables: %{
          "published" => true
        }
      )

    assert {:ok, result} = resp

    refute Map.has_key?(result, :errors)

    assert %{data: %{"postLibrary" => [%{"text" => "foo", "comments" => [%{"text" => "stuff"}]}]}} =
             result
  end

zachdaniel:

That also works for me currently

zachdaniel:

Is there anything special about order/owner/the relationship between them I should know about?

barnabasj:

Not sure, I checked a couple other fragments we have in the fe, we do have other fragments with relationships that work. But the other ones i saw were all has_many relationships, this one was a has_one. I tried switching has_one with belongs_to because i thought I might have it configured wrong, but that didn’t change anything.

I thought that maybe it would only trigger the error with the Postgres DataLayer because I assumed that the ETS DataLayer might always return all the fields instead of only the selected ones. So i tried using AshPostgres in the test from AshGraphql and add the fragment but I was unable to reproduce there too.

barnabasj:

I also had the relationship declared only on the first resource offer -> owner, I then added the reverse declartion too, but I that didn’t change anything either.

barnabasj:

The resources are in different AshApi/Registries

barnabasj:

I inspected the query https://github.com/ash-project/ash_postgres/blob/9e31f905861c8f97bb2b54fd8604eb362391e675/lib/data_layer.ex#L606 and in the were clause you can see that there is no Id in the array. But I didn’t quite figure out how ash got there^^

[
  query: #Ecto.Query<from e0 in Demo.HumanResources.Resources.Employee,
   as: 0,
   where: type(as(0).id, {:parameterized, Ash.Type.StringWrapper.EctoType, []}) in [],
   select: merge(struct(e0, [:id, :email, :name, :metadata, :gender]), %{
  phone:
    type(
      type(
        fragment("(? #>> ?)", type(as(0).metadata, {:parameterized, Ash.Type.Map.EctoType, []}), [
          "phone"
        ]),
        {:parameterized, Ash.Type.StringWrapper.EctoType, allow_empty?: false, trim?: true}
      ),
      {:parameterized, Ash.Type.StringWrapper.EctoType, allow_empty?: false, trim?: true}
    ),
  picture:
    type(
      fragment(
        "ash_elixir_or(?, ?)",
        type(
          fragment(
            "(? #>> ?)",
            type(as(0).metadata, {:parameterized, Ash.Type.Map.EctoType, []}),
            ["picture"]
          ),
          {:parameterized, Ash.Type.StringWrapper.EctoType, allow_empty?: false, trim?: true}
        ),
        type(as(0).picture_fallback, {:parameterized, Ash.Type.StringWrapper.EctoType, []})
      ),
      {:parameterized, Ash.Type.StringWrapper.EctoType, allow_empty?: false, trim?: true}
    )
})>,
  resource: Demo.HumanResources.Resources.Employee
]

zachdaniel:

šŸ¤” šŸ¤”

barnabasj:

So inside AshGrapqhl https://github.com/ash-project/ash_graphql/blob/c70e7dec7dac1aac7fd40a67b51a6d61d67f9d41/lib/graphql/resolver.ex#L1901

we get the fragment name as identifier.

field_or_relationship: [resource: JdlEngine.Offer.Resources.Offer, identifier: ā€œOwnerā€] field_or_relationship: nil

barnabasj:

I was now able to reproduce the issue and add a test to ash grapqhl https://github.com/ash-project/ash_graphql/pull/80

zachdaniel:

Amazing as always šŸ™‚

zachdaniel:

I’ll address this today.

barnabasj:

Thank you

zachdaniel:

okay, got a fix incoming

zachdaniel:

Should be fixed in main 🄳

barnabasj:

šŸš€ will probably check it out tomorrow

barnabasj:

Queries look good on our end too.

During testing I also tried updating ash to the latest commit on main, but I’m getting compile errors

** (CompileError) lib/demo/inventory/resources/search.ex:78: undefined function calculation/1 (there is no such import)
    (ash 2.11.0-rc.0) expanding macro: Ash.Resource.Dsl.Calculations.Calculate.calculate/3

zachdaniel:

Interesting…update spark as well

zachdaniel:

Ill push a fix to a new RC

zachdaniel:

oh, interesting

zachdaniel:

it looks like it does depend on the latest spark…

zachdaniel:

oh, I bet you’re doing this

calculate :foo, :bar do
  calculation ...
end

zachdaniel:

which was actually a bug that it was supported

zachdaniel:

we’ve actually kinda figured that out, realized we can support it easily. I’ll push rc.1 that supports that syntax and then you can upgrade

barnabasj:

so I just need to put the calc after the type instead of the do block right?

barnabasj:

after you wrote this i remember i had a similar problem once, where i had the option specified at the top and inside the do block and I was wondering why it was not working because the one in the do block overwrote the outer one.

zachdaniel:

It should work without it inside the do block now

zachdaniel:

Using rc 1