elixir-ecto / ecto_sql

SQL-based adapters for Ecto and database migrations
https://hexdocs.pm/ecto_sql
Apache License 2.0
578 stars 312 forks source link

subquery inside coalesce produces invalid MySQL query syntax #642

Closed beerlington closed 1 month ago

beerlington commented 1 month ago

Elixir version

Elixir 1.15.4 (compiled with Erlang/OTP 25)

Database and Version

MySQL 8.2

Ecto Versions

Ecto 3.12.4

Database Adapter and Versions (postgrex, myxql, etc)

myxql 0.7.1

Current behavior

This is an edge case but I thought it would be worth reporting. When trying to use a subquery inside of coalesce, Ecto produces an invalid SQL query.

Here's a simplified example of what I'm doing:

defmodule MyApp.Events do
  import Ecto.Query

  defmacrop tickets_sold_count do
    quote do
      subquery(
        Ticket
        |> where([t], t.event_id == parent_as(:event).id)
        |> select([t], %{count: count(t.id)})
      )
    end
  end

  def list_events_stats(event_id) do
    Event
    |> from(as: :event)
    |> where([e], e.id == ^event_id)
    |> select(
      [e],
      %{
        id: e.id,
        tickets_sold_count: coalesce(tickets_sold_count(), 0)
      }
    )
    |> Repo.all()
  end
end

It will produce an SQL query that looks like this:

SELECT
  e0.`id`,
  coalesce(
    SELECT
      COUNT(st0.`id`) AS `count`
    FROM
      `tickets` AS st0
    WHERE
      (st0.`event_id` = e0.`id`),
      0
  )
FROM
  `events` AS e0
WHERE
  (e0.`id` = '123')

This is invalid because the subquery inside coalesce needs to be wrapped in parentheses.

I can easily fix the code by moving coalesce inside the subquery, but wanted to report it because Ecto generates valid SQL when using the postgrex adapter instead. It correctly wraps the subquery in an extra set of parentheses.

Expected behavior

It should product an SQL query that looks like this instead:

SELECT
  e0.`id`,
  coalesce(
    (
      SELECT
        COUNT(st0.`id`) AS `count`
      FROM
        `tickets` AS st0
      WHERE
        (st0.`event_id` = e0.`id`)
    ),
    0
  )
FROM
  `events` AS e0
WHERE
  (e0.`id` = '123')
greg-rychlewski commented 1 month ago

Thanks for the report. This is definitely a bug.

I traced it to this line: https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/myxql/connection.ex#L829. Since we use top_level_expr instead of expr it doesn't add the parenthesis. I believe this is a recent change that was made so to avoid things like exists((..subquery)). Though from my testing I don't see that is an issue:

mysql> select exists((select 1));
+--------------------+
| exists((select 1)) |
+--------------------+
|                  1 |
+--------------------+
1 row in set (0.00 sec)

mysql> select 1 order by  exists((select 1));
+---+
| 1 |
+---+
| 1 |
+---+
1 row in set (0.01 sec)

mysql> select 1 group by exists((select 1)) order by exists((select 1));
+---+
| 1 |
+---+
| 1 |
+---+
1 row in set (0.01 sec)

I think the most likely fix is we should change that line to use expr. But will test some more