Failure while raising QueryError in assert_subquery_fields!

[rbino]

Elixir version

1.16.2

Database and Version

PostgreSQL 15.4

Ecto Versions

master, commit a8cd6ac

Database Adapter and Versions (postgrex, myxql, etc)

Postgrex 0.17.5

Current behavior

I was investigating a failure while working on a new feature in Ash. The code failed on a rather obscure point (a FunctionClauseError deep in Code.Normalizer). Ecto.Query.Planner was removing its portion of the stacktrace so I've started digging to find the cause of the error.

The error bubbles up from the call to Macro.to_string/1 here.

These are the arguments passed to the function (printed with IO.inspect(binding()):

[
  expr: {:%{}, [],
   [
     aggregates: {:%{}, [],
      [
        aggregate_0: {:type, [],
         [
           {:coalesce, [],
            [
              {:type, [],
               [
                 {{:., [], [{:as, [], [1]}, :aggregate_0]}, [], []},
                 {:parameterized, Ash.Type.Integer.EctoType, []}
               ]},
              {:type, [],
               [{:^, [], [0]}, {:parameterized, Ash.Type.Integer.EctoType, []}]}
            ]},
           {:parameterized, Ash.Type.Integer.EctoType, []}
         ]}
      ]}
   ]},
  pairs: [
    aggregates: {:%{}, [],
     [
       aggregate_0: {:type, [],
        [
          {:coalesce, [],
           [
             {:type, [],
              [
                {{:., [], [{:as, [], [1]}, :aggregate_0]}, [], []},
                {:parameterized, Ash.Type.Integer.EctoType, []}
              ]},
             {:type, [],
              [{:^, [], [0]}, {:parameterized, Ash.Type.Integer.EctoType, []}]}
           ]},
          {:parameterized, Ash.Type.Integer.EctoType, []}
        ]}
     ]}
  ],
  query: #Ecto.Query<from p0 in AshPostgres.Test.Post, as: 0,
   left_lateral_join: u1 in subquery(from u0 in AshPostgres.Test.User,
  as: 0,
  join: p1 in AshPostgres.Test.PostFollower,
  on: p1.follower_id == u0.id,
  where: type(
  type(as(0).is_active, {:parameterized, Ash.Type.Boolean.EctoType, []}) ==
    type(^true, {:parameterized, Ash.Type.Boolean.EctoType, []}),
  {:parameterized, Ash.Type.Boolean.EctoType, []}
),
  where: parent_as(0).id == p1.post_id,
  group_by: [p1.post_id],
  distinct: [asc: p1.post_id],
  select: %{
  post_id: map(p1, [:post_id]).post_id,
  aggregate_0:
    type(
      coalesce(
        count(type(as(0).id, {:parameterized, Ash.Type.UUID.EctoType, []})),
        type(^0, {:parameterized, Ash.Type.Integer.EctoType, []})
      ),
      {:parameterized, Ash.Type.Integer.EctoType, []}
    )
}),
   on: true,
   where: type(as(0).type, {:parameterized, Ash.Type.Atom.EctoType, []}) ==
  type(^"sponsored", {:parameterized, Ash.Type.Atom.EctoType, []}),
   where: p0.author_id == parent_as(0).id,
   windows: [order: [order_by: [asc: as(0).id]]], order_by: [asc: as(0).id],
   limit: ^2,
   select: merge(
  merge(
    struct(p0, [
      :id,
      :title,
      :datetime,
      :score,
      :public,
      :category,
      :type,
      :price,
      :decimal,
      :status,
      :status_enum,
      :status_enum_no_cast,
      :point,
      :composite_point,
      :stuff,
      :list_of_stuff,
      :uniq_one,
      :uniq_two,
      :uniq_custom_one,
      :uniq_custom_two,
      :list_containing_nils,
      :created_at,
      :updated_at,
      :organization_id,
      :author_id
    ]),
    %{
      aggregates: %{
        aggregate_0:
          type(
            coalesce(
              type(as(1).aggregate_0, {:parameterized, Ash.Type.Integer.EctoType, []}),
              type(^0, {:parameterized, Ash.Type.Integer.EctoType, []})
            ),
            {:parameterized, Ash.Type.Integer.EctoType, []}
          )
      }
    }
  ),
  %{__order__: over(row_number(), :order)}
)>
]

This is the original stacktrace (without the filtering from Ecto.Query.Planner:

    (elixir 1.16.2) lib/access.ex:307: Access.get(Ash.Type.Integer.EctoType, :line, nil)
    (elixir 1.16.2) lib/code/normalizer.ex:569: Code.Normalizer.patch_meta_line/2
    (elixir 1.16.2) lib/code/normalizer.ex:333: Code.Normalizer.normalize_call/2
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.16.2) lib/code/normalizer.ex:371: Code.Normalizer.normalize_call/2
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.16.2) lib/code/normalizer.ex:371: Code.Normalizer.normalize_call/2
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.16.2) lib/code/normalizer.ex:371: Code.Normalizer.normalize_call/2
    (elixir 1.16.2) lib/code/normalizer.ex:511: Code.Normalizer.normalize_kw_args/3
    (elixir 1.16.2) lib/code/normalizer.ex:445: Code.Normalizer.normalize_map_args/2
    (elixir 1.16.2) lib/code/normalizer.ex:176: Code.Normalizer.do_normalize/2
    (elixir 1.16.2) lib/code/normalizer.ex:511: Code.Normalizer.normalize_kw_args/3
    (elixir 1.16.2) lib/code/normalizer.ex:445: Code.Normalizer.normalize_map_args/2
    (elixir 1.16.2) lib/code/normalizer.ex:176: Code.Normalizer.do_normalize/2
    (elixir 1.16.2) lib/code.ex:1413: Code.quoted_to_algebra/2
    (elixir 1.16.2) lib/macro.ex:1138: Macro.to_string/1
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:560: anonymous fn/3 in Ecto.Query.Planner.assert_subquery_fields!/3
    (elixir 1.16.2) lib/enum.ex:987: Enum."-each/2-lists^foreach/1-0-"/2
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:492: Ecto.Query.Planner.subquery_select/3
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:470: Ecto.Query.Planner.subquery_select/3
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:469: Ecto.Query.Planner.subquery_select/3
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:451: Ecto.Query.Planner.rewrite_subquery_select_expr/2
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:378: Ecto.Query.Planner.normalize_subquery_select/3
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:354: Ecto.Query.Planner.plan_subquery/6
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:297: Ecto.Query.Planner.plan_source/4
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:694: Ecto.Query.Planner.plan_joins/9
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:262: Ecto.Query.Planner.plan_sources/3
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:229: Ecto.Query.Planner.plan/4
    (ecto 3.12.0-dev) lib/ecto/query/planner.ex:139: Ecto.Query.Planner.query/5
    (ecto 3.12.0-dev) lib/ecto/adapter/queryable.ex:112: Ecto.Adapter.Queryable.prepare_query/3
    (ecto_sql 3.11.1) lib/ecto/adapters/sql.ex:340: Ecto.Adapters.SQL.to_sql/3
    (ash_postgres 2.0.2) lib/data_layer.ex:899: AshPostgres.DataLayer.run_query_with_lateral_join/4
    (ash 3.0.2) lib/ash/actions/read/read.ex:2273: Ash.Actions.Read.run_query/4
    (ash 3.0.2) lib/ash/actions/read/read.ex:444: anonymous fn/5 in Ash.Actions.Read.do_read/4
    (ash 3.0.2) lib/ash/process_helpers.ex:38: anonymous fn/4 in Ash.ProcessHelpers.async/2
    (elixir 1.16.2) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
    (elixir 1.16.2) lib/task/supervised.ex:36: Task.Supervised.reply/4

Expected behavior

Not sure if this is a problem in Macro.to_string or maybe in the original expression. The expected behavior is that raising an error should not to trigger another error obfuscating the original one (possibly even with malformed expressions).

[greg-rychlewski]

Thanks for the report @rbino. Are you able to provide a query and Repo function call that generates the issue? That will be easier for us to play with and investigate.

[rbino]

I've pushed a branch to help reproduce the problem.

Run a postgres locally:

docker run -it --net=host --rm -e POSTGRES_PASSWORD=postgres -e POSTGRES_USER=postgres postgres

Then use the repo:

git clone https://github.com/rbino/ash_postgres.git -b relationship-count-ecto-debug
cd ash_postgres
mix deps.get
mix test.reset
mix test test/load_test.exs:788

This runs only the specific test that was triggering the error

[greg-rychlewski]

Thanks for the repo, it was very helpful.

I'm not familiar with Ash but from my debugging it seems like you are running a subquery where this is part of the select statement:

%{
      aggregates: %{
        aggregate_0:
          type(
            coalesce(
              type(as(1).aggregate_0, {:parameterized, Ash.Type.Integer.EctoType, []}),
              type(^0, {:parameterized, Ash.Type.Integer.EctoType, []})
            ),
            {:parameterized, Ash.Type.Integer.EctoType, []}
          )
      }
    }

For subqueries Ecto doesn't allow the values to be a map, in this case the value of aggregates is a map.

It seems there is something not working nice with Macro.to_string as well, otherwise you would have got this error

atoms, structs, maps, lists, tuples and sources are not allowed as map values in subquery ....

This might be something @josevalim needs to take a look at in elixir. For reference, this is the expression causing the issue:

{:%{}, [],
 [
   aggregates: {:%{}, [],
    [
      aggregate_0: {:type, [],
       [
         {:coalesce, [],
          [
            {:type, [],
             [
               {{:., [], [{:as, [], [1]}, :aggregate_0]}, [], []},
               {:parameterized, Ash.Type.Integer.EctoType, []}
             ]},
            {:type, [],
             [{:^, [], [0]}, {:parameterized, Ash.Type.Integer.EctoType, []}]}
          ]},
         {:parameterized, Ash.Type.Integer.EctoType, []}
       ]}
    ]}
 ]}

[josevalim]

{:parameterized, Ash.Type.Integer.EctoType, []} is not valid Elixir AST. I need to understand if Ecto is the one generating it (by forgetting to escape things) or Ash. We can likely check it by doing type(^expr, f.parameterized_type) and we need to make sure that we either Macro.escape (or change the internal representation to {:parameterized, {mod, args}}).

[zachdaniel]

We do some pretty crazy stuff under the hood in our query builder to build these kinds of things. It makes sense that this would not be possible. With the exception (possibly) of the error message, I don't think that this is something that needs to (or could be) fixed in ecto. What we'll need to do is flatten those out with some indicator we can use to expand them again, like __aggregates__aggregate_0.

As for generating the type, I'm pretty sure that we do that always with type(value, ^parameterized_type). There are places we abuse the query builder and construct the AST directly, and would not expect to get any help on that front. Places where we do that represent contributions we should make back to Ecto to make things that we need to do possible.

[josevalim]

Digging deeper this is certainly an Ecto bug. We cannot call Macro.to_string/1 to expressions, but everywhere assumes that's the case. We need to either not do that (and introduce something like Inspect.Ecto.Query.to_string or make parameterized types valid AST by making it a two element tuple. I am more inclined to go with the latter.

Thoughts @greg-rychlewski?

[zachdaniel]

I'm cool with that change, just want to keep in mind that we do manually construct {:parameterized, type, options}. I don't know if that structure is considered a public API, but if it is then this would constitute a breaking change. If not, then we'll figure something out. If that format changes, pretty much any query built by Ash would fail, so it would be a pretty obvious incompatibility to our users, and I can find a way to make sure that we aren't manually making that, but calling appropriate callback functions on types or something along those lines.

[greg-rychlewski]

I was going to say I prefer making it valid AST because I believe that's what we try to do for all the other query expressions.

Zach's message made me wonder where people might be using this though...the only public place I could find related to this is the function Ecto.ParameterizedType.init/2 which will return that form. Though we don't explicitly say it will be the 3-tuple.

If I'm not missing anything about public APIs then I would prefer making it valid AST. It's consistent with the rest of the query handling and it just works™.

[zachdaniel]

I can get ahead of it in advance and make sure that we only produce these values w/ Ecto.Parameterized.init/2 in that case. Some users will inevitably hit an issue if they upgrade ecto before they upgrade Ash, but ultimately that comes down to me since I relied on something that wasn't a public structure :) And most users would expect to upgrade Ash if they are upgrading other packages it uses too.

[acalejos]

I'm wondering if this will be in the next patch release? I know libraries such as Instructor rely on the previous version (3-tuple), so would this be considered a breaking change / be a minor release? Or is this considered a private API and thus the change could happen in a patch?

Curious because my schema validation library Flint also matches on this.

Thanks!

[zachdaniel]

Hasn't it already been released? AFAIK this is considered a private API and was changed in a minor release.

[zachdaniel]

Looks like I'm wrong and it hasn't been released :)

[acalejos]

Oh maybe it has been. I've been working from main for Flint since Im depending on the :defaults_to_struct option for embedded schemas so I've been using it, but I assumed it wasn't on Hex yet.

EDIT: Just saw your response right after I sent this comment

[josevalim]

It will be there on v3.12. Probably happening sooner than later, let me start with a CHANGELOG.

[acalejos]

It will be there on v3.12. Probably happening sooner than later, let me start with a CHANGELOG.

Appreciate the reply!