ash-project / ash

A declarative, extensible framework for building Elixir applications.
https://www.ash-hq.org
MIT License
1.62k stars 219 forks source link

Crashes when declaring a many_to_many relationship that declares a join_relationship on a has_many to allow for filtering through the join resource. #446

Closed simpers closed 1 year ago

simpers commented 2 years ago

Describe the bug I want to make it possible to filter a many_to_many by the through resource's values, as it is not just a simple composite key. It also has some state. After discussing on Discord we arrived at the solution below that ought to have worked, but I am getting some bugs.

  1. The original many_to_many discussion did not mention the requirement of declaring any source or destination attributes, but it doesn't compile without them. So this might be a bug if they indeed shouldn't be required when declaring a join_relationship.
  2. If I add the source and destination attributes (commented out in the code below), it compiles but crashes in run-time as I try to load aggregates that rely on these relationships.
  3. If I remove the loading of the aggregates, it instead crashes in Postgrex when attempting to load the data as per the last chunk of logs.

@zachdaniel thinks there are several bugs involved in this.

To Reproduce After discussing on the Discord I have arrived at the following code to easily produce filtered versions of a many_to_many relationship:

    # all_values() -> [:accepted, :declined, :maybe, :invited]
    for state <- Lpe.InvitationState.all_values() do
      has_many :"#{state}_guest_invites", EventGuest do
        private? true
        filter [state: state]
      end

      many_to_many :"#{state}_guests", User do
        through EventGuest

        join_relationship :"#{state}_guest_invites"

        # source_attribute_on_join_resource :event_id
        # destination_attribute_on_join_resource :user_id
      end
    end

If the lines are commented – as above – then it doesn't compile. The many_to_many requires it:

(Spark.Error.DslError) [Lpe.Event]
 relationships -> many_to_many -> invited_guests:
  required option :source_attribute_on_join_resource not found, received options: [:join_relationship, :through, :name, :destination]

With the lines, I get:

1) test createEvent mutation a user can create a basic event (LpeWeb.Gql.CreateEventTest)
     test/lpe_web/gql/mutations/create_event_test.exs:40
     ** (ArgumentError) errors were found at the given arguments:

       * 1st argument: not an already existing atom

     stacktrace:
       :erlang.binary_to_existing_atom("accepted_guests_join_assoc", :utf8)
       (ash_postgres 1.1.1) lib/join.ex:498: AshPostgres.Join.do_join_relationship/6
       (ash_postgres 1.1.1) lib/join.ex:66: anonymous fn/5 in AshPostgres.Join.join_all_relationships/5
       (elixir 1.14.1) lib/enum.ex:4751: Enumerable.List.reduce/3
       (elixir 1.14.1) lib/enum.ex:2514: Enum.reduce_while/3
       (ash_postgres 1.1.1) lib/aggregate.ex:45: anonymous fn/4 in AshPostgres.Aggregate.add_aggregates/4
       (elixir 1.14.1) lib/enum.ex:4751: Enumerable.List.reduce/3
       (elixir 1.14.1) lib/enum.ex:2514: Enum.reduce_while/3
       (ash_postgres 1.1.1) lib/aggregate.ex:15: AshPostgres.Aggregate.add_aggregates/4
       (ash 2.4.2) lib/ash/query/aggregate.ex:342: anonymous fn/6 in Ash.Query.Aggregate.value_request/8
       (ash 2.4.2) lib/ash/engine/engine.ex:441: anonymous fn/2 in Ash.Engine.run_iteration/1
       (ash 2.4.2) lib/ash/engine/engine.ex:461: anonymous fn/3 in Ash.Engine.async/2
       (elixir 1.14.1) lib/task/supervised.ex:89: Task.Supervised.invoke_mfa/2
       (elixir 1.14.1) lib/task/supervised.ex:34: Task.Supervised.reply/4
       (stdlib 4.1) proc_lib.erl:240: :proc_lib.init_p_do_apply/3

Temporarily commented out the loading of those aggregates and tried again, and then I got:

1) test createEvent mutation a user can create a basic event (LpeWeb.Gql.CreateEventTest)
     test/lpe_web/gql/mutations/create_event_test.exs:40
     ** (Postgrex.Error) ERROR 42703 (undefined_column) column su0.state does not exist

         query: SELECT DISTINCT s1."id", s1."inserted_at", s1."updated_at", s1."deleted_at", s1."email_confirmed_at", s1."active", s1."first_name", s1."last_name", s1."username", s1."username_code", s1."email", s1."password_hash", s1."invited_by_id", s1."__lateral_join_source__" FROM "events" AS e0 INNER JOIN LATERAL (SELECT su0."id" AS "id", su0."inserted_at" AS "inserted_at", su0."updated_at" AS "updated_at", su0."deleted_at" AS "deleted_at", su0."email_confirmed_at" AS "email_confirmed_at", su0."active" AS "active", su0."first_name" AS "first_name", su0."last_name" AS "last_name", su0."username" AS "username", su0."username_code" AS "username_code", su0."email" AS "email", su0."password_hash" AS "password_hash", su0."invited_by_id" AS "invited_by_id", se1."event_id" AS "__lateral_join_source__" FROM "public"."users" AS su0 INNER JOIN "public"."event_hosts" AS se1 ON (su0."state"::varchar = $1::varchar) AND (se1."user_id" = su0."id") WHERE ((su0."deleted_at"::timestamp IS NULL) = $2) AND (se1."event_id" = e0."id")) AS s1 ON TRUE WHERE (e0."id" = ANY($3))

         hint: Perhaps you meant to reference the column "se1.state"

Expected behavior It shouldn't crash and depending on the conclusion of # 1 in the first section of this issue, it should or shouldn't be necessary to declare the source and destination attributes.

Runtime

zachdaniel commented 1 year ago

Alight @simpers I've pushed something up to ash_postgres to at least fix this partially. Can you try that out and see how it goes? Thanks for your patience with me, I'm wrapping up my vacation and getting back on top of everything :)

zachdaniel commented 1 year ago

You'd want to try out ash_postgres main

simpers commented 1 year ago

Will give this a go at some point today, @zachdaniel! :)

simpers commented 1 year ago

Sorry for the late report. I tried it on the day but I wanted to run some tests with the old version as well to compare the error messages, but I haven't had enough time to reproduce the different results. This is however the error I get now on both master and v1.1.3 of AshPostgres

I seem to be getting this now:

  1) test createEvent mutation a user can create a basic event (LpeWeb.Gql.CreateEventTest)
     test/lpe_web/gql/mutations/create_event_test.exs:40
     ** (RuntimeError) Error while building reference: accepted_host_invites.state
     stacktrace:
       (ash_postgres 1.1.3) lib/expr.ex:779: AshPostgres.Expr.do_dynamic_expr/5
       (ash_postgres 1.1.3) lib/expr.ex:391: AshPostgres.Expr.do_dynamic_expr/5
       (ash_postgres 1.1.3) lib/join.ex:179: AshPostgres.Join.do_relationship_filter/6
       (ash_postgres 1.1.3) lib/join.ex:140: AshPostgres.Join.maybe_get_resource_query/5
       (ash_postgres 1.1.3) lib/join.ex:560: AshPostgres.Join.do_join_relationship/6
       (ash_postgres 1.1.3) lib/join.ex:66: anonymous fn/5 in AshPostgres.Join.join_all_relationships/5
       (elixir 1.14.1) lib/enum.ex:4751: Enumerable.List.reduce/3
       (elixir 1.14.1) lib/enum.ex:2514: Enum.reduce_while/3
       (ash_postgres 1.1.3) lib/aggregate.ex:45: anonymous fn/4 in AshPostgres.Aggregate.add_aggregates/4
       (elixir 1.14.1) lib/enum.ex:4751: Enumerable.List.reduce/3
       (elixir 1.14.1) lib/enum.ex:2514: Enum.reduce_while/3
       (ash_postgres 1.1.3) lib/aggregate.ex:15: AshPostgres.Aggregate.add_aggregates/4
       (ash 2.4.18) lib/ash/query/aggregate.ex:342: anonymous fn/6 in Ash.Query.Aggregate.value_request/8
       (ash 2.4.18) lib/ash/engine/engine.ex:460: anonymous fn/2 in Ash.Engine.run_iteration/1
       (ash 2.4.18) lib/ash/engine/engine.ex:480: anonymous fn/3 in Ash.Engine.async/2
       (elixir 1.14.1) lib/task/supervised.ex:89: Task.Supervised.invoke_mfa/2
       (elixir 1.14.1) lib/task/supervised.ex:34: Task.Supervised.reply/4
       (stdlib 4.1) proc_lib.erl:240: :proc_lib.init_p_do_apply/3
zachdaniel commented 1 year ago

Got it reproduced, thanks :)

On Fri, Dec 02, 2022 at 4:06 AM, Simon Bergström < @.*** > wrote:

Sorry for the late report. I tried it on the day but I wanted to run some tests with the old version as well to compare the error messages, but I haven't had enough time to reproduce the different results. This is however the error I get now on both master and v1.1.3 of AshPostgres

I seem to be getting this now:

1) test createEvent mutation a user can create a basic event (LpeWeb.Gql.CreateEventTest)

test/lpe_web/gql/mutations/create_event_test.exs:40 ** (RuntimeError) Error while building reference: accepted_host_invites.state stacktrace: (ash_postgres 1.1.3) lib/expr.ex:779: AshPostgres.Expr.do_dynamic_expr/5 (ash_postgres 1.1.3) lib/expr.ex:391: AshPostgres.Expr.do_dynamic_expr/5

(ash_postgres 1.1.3) lib/join.ex:179: AshPostgres.Join.do_relationship_filter/6 (ash_postgres 1.1.3) lib/join.ex:140: AshPostgres.Join.maybe_get_resource_query/5

(ash_postgres 1.1.3) lib/join.ex:560: AshPostgres.Join.do_join_relationship/6 (ash_postgres 1.1.3) lib/join.ex:66: anonymous fn/5 in AshPostgres.Join.join_all_relationships/5 (elixir 1.14.1) lib/enum.ex:4751: Enumerable.List.reduce/3 (elixir 1.14.1) lib/enum.ex:2514: Enum.reduce_while/3 (ash_postgres 1.1.3) lib/aggregate.ex:45: anonymous fn/4 in AshPostgres.Aggregate.add_aggregates/4 (elixir 1.14.1) lib/enum.ex:4751: Enumerable.List.reduce/3 (elixir 1.14.1) lib/enum.ex:2514: Enum.reduce_while/3 (ash_postgres 1.1.3) lib/aggregate.ex:15: AshPostgres.Aggregate.add_aggregates/4 (ash 2.4.18) lib/ash/query/aggregate.ex:342: anonymous fn/6 in Ash.Query.Aggregate.value_request/8 (ash 2.4.18) lib/ash/engine/engine.ex:460: anonymous fn/2 in Ash.Engine.run_iteration/1 (ash 2.4.18) lib/ash/engine/engine.ex:480: anonymous fn/3 in Ash.Engine.async/2 (elixir 1.14.1) lib/task/supervised.ex:89: Task.Supervised.invoke_mfa/2 (elixir 1.14.1) lib/task/supervised.ex:34: Task.Supervised.reply/4 (stdlib 4.1) proc_lib.erl:240: :proc_lib.init_p_do_apply/3

— Reply to this email directly, view it on GitHub ( https://github.com/ash-project/ash/issues/446#issuecomment-1334955696 ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/ABLVBY2NPO73WEMXOB7MRK3WLG33DANCNFSM6AAAAAAR4LIMR4 ). You are receiving this because you were mentioned. Message ID: <ash-project/ash/issues/446/1334955696 @ github. com>

zachdaniel commented 1 year ago

This one was tough! My reproduction test now passes, so if you don't mind trying out ash_postgres main and ash main (need them both), that would be great 🥳

simpers commented 1 year ago

Perfect! I have just now cleared my repo out a little so that the next time I sat down I'd be able to get a simple git diff of what I add so I can myself keep track of what's what hah. It's so early in on this project that I just keep making these huges changes haha.

Testing this first thing tomorrow. Making a Todoist for it too! 🥳

simpers commented 1 year ago

It seems my test has passed now, so I'll just make sure to double check that my test is sane hhaha

But so far, so good! 👍🏽 🌟

zachdaniel commented 1 year ago

🎉🎉🎉